-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Remove .NET Standard 2.1 Targeting Pack from installers #50354
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
Conversation
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.
Thanks for your offline explanation. I am going to approve to help get a build so we can validate further, but it would be best to have a review from someone who understands this better.
Is this considered a breaking change or because this SDK will not get used in VS 16.3, then it's not really something we're concerned about anymore?
@@ -212,13 +200,6 @@ | |||
<DownloadFileName>$(DownloadedNetCoreAppTargetingPackInstallerFileName)</DownloadFileName> | |||
</BundledInstallerComponent> | |||
|
|||
<!-- TODO: Should we somehow obtain a .NET Standard ARM64 package? --> |
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.
Hey, that's exciting, I love that we can remove a TODO
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.
NET Standard 2.1 was created as part of .NET Core 3.0, long before we had support for ARM64 installers.
@@ -30,7 +30,6 @@ | |||
<ToolsetBrandName>Microsoft .NET Toolset $(Version)</ToolsetBrandName> | |||
<SharedFrameworkBrandName>Microsoft .NET Runtime $(MicrosoftNETCoreAppRuntimePackageVersion)</SharedFrameworkBrandName> | |||
<NetCoreAppTargetingPackBrandName>Microsoft .NET Targeting Pack $(MicrosoftNETCoreAppRefPackageVersion)</NetCoreAppTargetingPackBrandName> | |||
<NetStandardTargetingPackBrandName>Microsoft .NET Standard 2.1 Targeting Pack $(NETStandardLibraryRefPackageVersion)</NetStandardTargetingPackBrandName> |
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 wonder if we can get rid of the .ttf and .otf signing now?
Line 37 in fc373ee
<!-- .ttf and .otf files come in from some older aspnetcore packages (e.g. 2.1). |
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.
That is unrelated to this change. The signing props comment is just a general reference that likely came from Arcade or was copied over from the base copy in the Arcade repo.
@@ -30,7 +30,6 @@ | |||
<ToolsetBrandName>Microsoft .NET Toolset $(Version)</ToolsetBrandName> | |||
<SharedFrameworkBrandName>Microsoft .NET Runtime $(MicrosoftNETCoreAppRuntimePackageVersion)</SharedFrameworkBrandName> | |||
<NetCoreAppTargetingPackBrandName>Microsoft .NET Targeting Pack $(MicrosoftNETCoreAppRefPackageVersion)</NetCoreAppTargetingPackBrandName> | |||
<NetStandardTargetingPackBrandName>Microsoft .NET Standard 2.1 Targeting Pack $(NETStandardLibraryRefPackageVersion)</NetStandardTargetingPackBrandName> |
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.
Line 143 in fc373ee
<NETStandardLibraryRefPackageVersion>2.1.0</NETStandardLibraryRefPackageVersion> |
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.
Let me check. We do need some information when generating the bundled versions to know what the latest version of the actual reference pack is when we go online to retrieve it.
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.
Thanks, np and it's not necessarily needed for this PR, but seems like a nice to have if doable.
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.
ok, as expected, this is used when generating the bundled versions file we include with the SDK, so that should stay where it is
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.
we can actually remove this one since we're already setting the property in Versions.props: #50533
No, not a breaking change. 16.3 is the VS version where .NET Core 3.0.100 shipped. The .NET Standard 2.1 Targeting Pack has shipped in every SDK since then (3.1, 5.0, 6.0, 7.0, 8.0, and 9.0). It can be a bit confusing because it was produced as part of 3.0, but also not really part of 3.0 like the runtime. |
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.
Why this file change? Did this file get modified in your local build?
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.
Hmm, let me check, that seems suspect. My changes should only impact props/targets, not any .md files
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.
This file gets touched by a build task in src/Microsoft.CodeAnalysis.NetAnalyzers. Maybe that's why.
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've tried resetting it, but I can't make it go away at all. this doesn't make sense.
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.
Should be fixed in the last commit
Forward flow into main isn't configured yet via https://github.com/dotnet/sdk/blob/main/github-merge-flow.jsonc, cc @marcpopMSFT |
Description
Remove the .NET Standard Targeting Pack MSI and remove it from the .deb, .tar, pkg, and .zip binaries as well. A separate internal PR will be created to remove this from the 10.0.1xx SDK in Visual Studio.
Microsoft.NetCoreSdk.BundledVersions.props
and that file continues to ship.Testing
Verified a subset of packages (tar, zip, pkg) to confirm its removed. Will require an additional test pass once there's an official build. Some manual verification on Windows Sandbox checks out well.