-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Pin the implicit package reference for NetStandard.Library to 2.0.0 #1603
Conversation
…hen the TFM is NETStandard2.0.
@@ -54,6 +54,9 @@ Copyright (c) .NET Foundation. All rights reserved. | |||
<!-- If targeting the same release that is bundled with the .NET Core SDK, use the bundled package version provided by Microsoft.NETCoreSdk.BundledVersions.props --> | |||
<NETStandardImplicitPackageVersion Condition="'$(NETStandardImplicitPackageVersion)' =='' And '$(_TargetFrameworkVersionWithoutV)' == '$(BundledNETStandardTargetFrameworkVersion)'">$(BundledNETStandardPackageVersion)</NETStandardImplicitPackageVersion> | |||
|
|||
<!-- If targeting .NET Standard 2.0.x, use version 2.0.0 of the package. --> | |||
<NETStandardImplicitPackageVersion Condition="'$(NETStandardImplicitPackageVersion)' =='' And '$(_TargetFrameworkVersionWithoutV)' >= '2.0' And '$(_TargetFrameworkVersionWithoutV)' < '2.1'">2.0.0</NETStandardImplicitPackageVersion> | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should go before the other clause and just match on == 2.0. There should be symmetry between this and the .NET Core app case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, hang on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like the intent was to change the final clause to 2.0.0 now. See the comment on line 62 before/65 after. "Currently this is the same as the previous clause, but when we have a stable 2.0 package this should change."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think @nguerrera is right, and the logic we want is:
- If targeting bundled target framework version of .NET Standard, then use the bundled version of the package
- If targeting a version of .NET Standard less than 2.0, then use version 1.6.1 of the package.
- Otherwise, use the latest stable version of the package (currently 2.0.0)
This means that when there's eventually a .NET Standard 2.1 and a corresponding stable package, we would reference package version 2.1.0 even when targeting .NET Standard 2.0. The reason we don't do that for 1.x is because the reference ends up getting exposed as a NuGet dependency when you pack the project, but we stopped doing that for 2.0.
@ericstj @weshaggard does the logic I've outlined sound right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. It makes sense. I will change the default number to 2.0.0.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason not to use 2.0.1
? dotnet/standard#492.
With out it, it will never be used on mono (no bundled version), but it only contains an UWP fix and an xbuild workaround. But dotnet/standard#510 will be important for perf improvements.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dasMulli Good point, it should probably be 2.0.1 for now, and updated again once there is a package with dotnet/standard#510
…the 2.0.0 runtime separately.
@@ -648,7 +648,7 @@ private void TestInvalidTargetFramework(string testName, string targetFramework, | |||
|
|||
[Theory] | |||
[InlineData("netcoreapp2.2")] | |||
[InlineData("netstandard2.1")] | |||
[InlineData("netstandard2.3")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what was this about?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like a bug, there's still no netstandard2.1 TFM. I think it's because the bundled versions aren't right.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Filled #1632
Fix analyzer reference loading
Pin the implicit package reference for NetStandard.Library to 2.0.0 when the TFM is NETStandard2.0.
Fixes https://github.com/dotnet/cli/issues/7691
@dotnet/dotnet-cli