System.Linq.Async takes a dependency on Microsoft.Bcl.AsyncInterfaces on target frameworks where it should not #1941
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…that it will take the dependency on explicitly specified target frameworks rather than a negation of frameworks, which fails as soon as new target frameworks are added.
danielcweber
added a commit
to Gremlinq/ExRam.Gremlinq
that referenced
this pull request
May 19, 2023
… due to a bug (dotnet/reactive#1941) and the utilized operators are rather trivial to re-implement here. The dll is over 1MB.
saul
reviewed
May 19, 2023
|
|
||
| <ItemGroup> | ||
| <PackageReference Condition="'$(TargetFramework)' != 'netcoreapp3.1' and '$(TargetFramework)' != 'netstandard2.1' " Include="Microsoft.Bcl.AsyncInterfaces" Version="6.0.0" /> | ||
| <PackageReference Condition="'$(TargetFramework)' == 'net48' or '$(TargetFramework)' == 'netstandard2.0' " Include="Microsoft.Bcl.AsyncInterfaces" Version="6.0.0" /> |
There was a problem hiding this comment.
If we only want to include this package reference on non-.NET Core targets, the idiomatic condition would be:
Suggested change
| <PackageReference Condition="'$(TargetFramework)' == 'net48' or '$(TargetFramework)' == 'netstandard2.0' " Include="Microsoft.Bcl.AsyncInterfaces" Version="6.0.0" /> | |
| <PackageReference Condition=" '$(TargetFrameworkIdentifier)' != '.NETCoreApp' " Include="Microsoft.Bcl.AsyncInterfaces" Version="6.0.0" /> |
Note that net5.0, net6.0, etc fall under .NETCoreApp too.
See some examples of this condition in the dotnet/runtime repo.
Collaborator
Author
There was a problem hiding this comment.
That'll give us the reference on .netstandard 2.1 as well which we do not want:
danielcweber
added a commit
to Gremlinq/ExRam.Gremlinq
that referenced
this pull request
May 19, 2023
… due to a bug (dotnet/reactive#1941) and the utilized operators are rather trivial to re-implement here. The dll is over 1MB.
Collaborator
Author
Collaborator
Author
|
Fixed by #1719. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Bugfix
A change in a2410b2 added net6.0, bumped to net48 and dropped netcoreapp3.1 for System.Linq.Async. As a consequence, the condition on which a reference to Microsoft.Bcl.AsyncInterfaces is taken proves no longer useful. This results in the package reference being present on net6.0 (and beyond) when it should not be.
This commit rewrites the condition.
Fixes #1834.