Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUG] In Splat/Splat.csproj, TFM in Itemgroup's Condition seems to be unreachable compared to TFM in TargetFrameworks #787

Closed
eriawan opened this issue Oct 8, 2021 · 10 comments · Fixed by #788

Comments

@eriawan
Copy link
Contributor

eriawan commented Oct 8, 2021

Describe the bug
In https://github.com/reactiveui/splat/src/Splat/Splat.csproj, TFM in Itemgroup's Condition seems to be unreachable compared to TFM defined in TargetFrameworks.

Steps To Reproduce
In the code of Splat.csproj, the TFMs defined in TargetFrameworks doesn't specify TFM starts with net46 whereas in the ItemGroup's condition below specify condition to check TFM that starts with net46:

https://github.com/reactiveui/splat/blob/main/src/Splat/Splat.csproj#L3-L15

    <TargetFrameworks>netstandard2.0;net5.0;net6.0</TargetFrameworks>
    <TargetFrameworks Condition=" '$(OS)' == 'Windows_NT' ">$(TargetFrameworks);net461;net472</TargetFrameworks>
    <AssemblyName>Splat</AssemblyName>
    <RootNamespace>Splat</RootNamespace>
    <Authors>.NET Foundation and Contributors</Authors>
    <Description>A library to make things cross-platform that should be</Description>
    <PackageId>Splat</PackageId>
    <NoWarn>$(NoWarn);1591</NoWarn>
    <LangVersion>latest</LangVersion>
    <Nullable>enable</Nullable>
  </PropertyGroup>

  <ItemGroup Condition=" $(TargetFramework.StartsWith('net46')) ">

NOTE:
Step to reproduce I think it's not necessary for this. CMIIW.

Expected behavior
If any TFM starts with net46 is going to be used, then we should add net46, net461 to the TargetFrameworks, otherwise we should use netstandard2.0 as the TFM in the condition of the ItemGroup.
As far as I know, checking for TFM starts with net46 is not necessary, because

Screenshots
If applicable, add screenshots to help explain your problem.

Environment(please complete the following information):
N/A

Additional context
This bug report is focusing on more correctness and the TFM defined in the condition should match the TargetFrameworks defined.

@eriawan
Copy link
Contributor Author

eriawan commented Oct 8, 2021

if this issue is considered as a bug, I would love to submit PR 🙂

@glennawatson
Copy link
Contributor

Not a bug but thanks for reporting. StartsWith is used to allow for easier upgrading of the target framework.

@glennawatson
Copy link
Contributor

Reopening to discuss further. May be an issue with that file after discussing offline.

@glennawatson glennawatson reopened this Oct 8, 2021
@eriawan
Copy link
Contributor Author

eriawan commented Oct 8, 2021

@glennawatson
thanks!

@glennawatson
Copy link
Contributor

I suspect the net framework targets aren't needed in the main splat.csproj since all that was moved to the Splat.Drawing project. Happy to have a PR to remove net461/net472 and removing the conditional.

@eriawan
Copy link
Contributor Author

eriawan commented Oct 8, 2021

ok, I'm going to submit PR in about one hour.

eriawan added a commit to eriawan/splat that referenced this issue Oct 8, 2021
Remove ItemGroup that has conditon with TFM that starts with `net46`in Splat.csproj file. 
Having this condition of TFM net46* is unnecessary, Because defined TargetFrameworks does not contain NET Fx related TFM.
@eriawan
Copy link
Contributor Author

eriawan commented Oct 8, 2021

PR submitted: #788

@eriawan
Copy link
Contributor Author

eriawan commented Oct 8, 2021

@glennawatson

About this:

Happy to have a PR to remove net461/net472 and removing the conditional.

Do you also want to remove unnecessary net461 / net472 usage and conditional too? Currently I focus on Splat.csproj first :)

@eriawan
Copy link
Contributor Author

eriawan commented Oct 8, 2021

@glennawatson

I understand now! Many thanks for the pointer to that net46;net472 ! 👍
I'm updating my PR now.

eriawan added a commit to eriawan/splat that referenced this issue Oct 8, 2021
glennawatson pushed a commit that referenced this issue Oct 8, 2021
…#788)

* Fixes #787
Remove ItemGroup that has conditon with TFM that starts with `net46`in Splat.csproj file. 
Having this condition of TFM net46* is unnecessary, Because defined TargetFrameworks does not contain NET Fx related TFM.

* remove the net46 and net472 as well because these TFMs are in the Splat.Drawing as suggested in issue comment:
 #787 (comment)
@github-actions
Copy link

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 23, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants