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

Remove NS1.x assets from building and packaging #53283

Merged
merged 3 commits into from
May 26, 2021

Conversation

ViktorHofer
Copy link
Member

These ten projects still built for netstandard1.x. This PR
trims out these assets from both the build which also results
in them not being packaged (as there is no harvesting
mechanism in the repository present anymore).

Suppressing the package warnings for the intentionally dropped
assets and cleaning up conditions in the project file as well.

For more details please see the reasoning in the linked issue.

Contributes to #53282

These ten projects still built for netstandard1.x. This PR
trims out these assets from both the build which also results
in them not being packaged anymore as there is no harvesting
mechanism in the repository anymore.

Suppressing the package warnings for the intentionally dropped
assets and cleaning up conditions in the project file as well.

For more details please see the reasoning in the linked issue.

Contributes to dotnet#53282
@ViktorHofer ViktorHofer added this to the 6.0.0 milestone May 26, 2021
@ViktorHofer ViktorHofer requested review from ericstj and Anipik May 26, 2021 13:20
@ViktorHofer ViktorHofer self-assigned this May 26, 2021
@ghost
Copy link

ghost commented May 26, 2021

Tagging subscribers to this area: @Anipik, @safern, @ViktorHofer
See info in area-owners.md if you want to be subscribed.

Issue Details

These ten projects still built for netstandard1.x. This PR
trims out these assets from both the build which also results
in them not being packaged (as there is no harvesting
mechanism in the repository present anymore).

Suppressing the package warnings for the intentionally dropped
assets and cleaning up conditions in the project file as well.

For more details please see the reasoning in the linked issue.

Contributes to #53282

Author: ViktorHofer
Assignees: ViktorHofer
Labels:

area-Infrastructure-libraries

Milestone: 6.0.0

@dotnet-issue-labeler
Copy link

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@ViktorHofer ViktorHofer added breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. and removed new-api-needs-documentation labels May 26, 2021
@ghost ghost added the needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet label May 26, 2021
@@ -118,10 +118,8 @@ public static partial class ImmutableArray
public System.Collections.Immutable.ImmutableArray<T> Add(T item) { throw null; }
public System.Collections.Immutable.ImmutableArray<T> AddRange(System.Collections.Generic.IEnumerable<T> items) { throw null; }
public System.Collections.Immutable.ImmutableArray<T> AddRange(System.Collections.Immutable.ImmutableArray<T> items) { throw null; }
#if !NETSTANDARD1_0
Copy link
Member

@stephentoub stephentoub May 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

System.Collections.Immutable is widely used, including by Roslyn. Just want to re-confirm there are no known remaining important dependencies on the .NET Standard 1.x assets?

Copy link
Member Author

@ViktorHofer ViktorHofer May 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me provide clarity on the overall communication strategy and then go into specifics for Roslyn.

  1. The change to remove unsupported application tfms (.NETCoreApp < 3.1 and .NETFramework < 4.6.1) and pre netstandard2.0 library assets (.NETStandard < 2.0) was approved by leadership months ago.
  2. The first wave of this effort (removing harvested assets) was presented to Tactics months ago.
  3. In yesterday's meeting I communicated that I'll start with trimming the remaining 25 shipping packages and we agreed on to a) send out a separate mail to Tactics in case people were not present in the meeting b) send out a notice to our first party customers via the breaking change notice alias which includes filing a public breaking change issue.
  4. The docs team will then later convert the breaking change issue into a docs page.

In parallel to submitting these PRs (one for .NETStandard, another one for .NETFramework and the last one for .NETCoreApp) I'm filing the breaking change issue to send out the mail to Tactics and the bcn alias. I plan to do that later this week.

In regards to roslyn as a consumer, I found three code pieces where they still consume the netstandard1.x asset of
System.Collections.Immutable and System.Reflection.Metadata. All three projects will need to react to this change by either a) upgrading the tfms or if that's not possible/desirable b) continue referencing the 5.0.0 package even after the 6.0.0 shipped (only for configurations that are affected by this change). Thanks for the feedback, I'll make sure to include the roslyn team in the breaking change notice and will proactively reach out to them.

Customers who try to upgrade these packages to the 6.0.0 version (or to the preview 6 pre-release versions) and still target netstandard1.x, < net461 or < netcoreapp3.1 in their applications will hit an upgrade error in the NuGet Package Explorer or when trying to restore the newer version of the packages i.e. via dotnet restore. The error will inform customers about which packages don't support the tfms in use anymore.

Please let me know if you have further suggestions regarding the handling and communication of the breaking changes.

Copy link
Member

@stephentoub stephentoub left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks

</PropertyGroup>
<ItemGroup Condition="'$(TargetFramework)' == 'netstandard2.0'">
<ItemGroup Condition="'$(IsPartialFacadeAssembly)' != 'true'">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

curious on why choose IsPartialFacadeAssembly over the TargetFramework ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IsPartialFacadeAssembly is more expressive in this case as it indicates that the partial facade assembly doesn't contain any source where as the non partial facade assembly only contains source and no type forwards.

Copy link
Contributor

@Anipik Anipik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apart from TargetFrameworkIdentifier., everything looks good

@ViktorHofer
Copy link
Member Author

Browser wasm leg is timing out. @akoeplinger @steveisok is that known?

@ViktorHofer ViktorHofer merged commit c95aa3f into dotnet:main May 26, 2021
@ViktorHofer ViktorHofer deleted the NS1xRemoval branch May 26, 2021 19:51
@ghost ghost locked as resolved and limited conversation to collaborators Jun 25, 2021
@ViktorHofer ViktorHofer removed the needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet label Sep 30, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Infrastructure-libraries breaking-change Issue or PR that represents a breaking API or functional change over a prerelease.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants