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

[release/8.0.1xx] Detect correctly multitargeted apps for trim/AOT/single-file warnings #35851

Merged
merged 2 commits into from
Oct 6, 2023

Conversation

sbomer
Copy link
Member

@sbomer sbomer commented Oct 4, 2023

Backport of #35767.

Customer Impact

In .NET 8 we made a breaking change that introduces a warning when setting IsTrimmable or related properties in a .NET Standard library. The warning was introduced because we changed the behavior (to not reference the trim analyzers, and to not mark the assembly with [assembly: AssemblyMetadat("IsTrimmable", "True")].

The intended way to fix this in a library was by multi-targeting the library to include a TFM supported by trimming, and to set IsTrimmable only on the supported TFMs. We got feedback (see #35528) that the warning was a painful break for multitargeted libraries, and that the warning shouldn't be produced in the first place for libraries following our guidance.

This change addresses it by silencing the warning for correctly multitargeted projects. The warning is silenced only when the multitargeting set includes a low enough supported TFM to ensure that the IsTrimmable assets will be consumed (instead of non-trimmable .NET Standard assets) in any supported app that references the library.

Testing

Existing unit tests passed without changes. Added tests to validate the behavior for various multitargeted projects. Added tests to ensure that the changes don't break the behavior of projects that use custom TargetFramework values as aliases for TargetFrameworkIdentifier and TargetFrameworkVersion.

Risk

Low to medium. This change reduces overall impact of the breaking change by limiting the scope of the warning, but adds complexity to the behavior. The complexity is unlikely to be surfaced to the developer, but is observable (it would be confusing to any developer who tried to understand the specific circumstances under which the warning was produced).

Original description

The warnings introduced by
#34077 and described in
https://learn.microsoft.com/en-us/dotnet/core/compatibility/sdk/8.0/trimming-unsupported-targetframework
are unnecessary noise for projects that multitarget to include a
TargetFramework that is compatible with
trimming/AOT/single-file, and is a low enough version to ensure
that it will be picked over any other TFM's assets when the
library is consumed in a trimmed/AOT'd/single-file app.

This fixes the issue by detecting correctly multi-targeted libraries and suppressing the warnings in these cases.

Note that prior to the .NET 8 SDK, netstandard libraries would still get the IsTrimmable attribute embedded in the assembly when setting IsTrimmable, and could use trim analysis despite incomplete warnings due to unannotated ref assemblies. With the .NET 8 SDK this is no longer the case. Even for correctly multi-targeted libraries, the netstandard output assembly will no longer contain the IsTrimmable attribute. We debated whether it was worth warning in this case, but decided that the negative impact of the warning on library developers following the "golden path" was too large to justify keeping this as a warning.

This solution addresses a correctness problem that we wanted to avoid (the correctness problem: allowing an inadvertently non-"trimmable" netstandard asset to be consumed in a trimmed app that uses a supported TFM).

It's still possible to get into that situation in an app that explicitly references the netstandard assembly, or that targets an EOL TFM (causing it to consume the netstandard asset from a library that multitargets netstandard2.0;net6.0 for example).

Fixes #35528

…dotnet#35767)

The warnings introduced by
 dotnet#34077 and described in
 https://learn.microsoft.com/en-us/dotnet/core/compatibility/sdk/8.0/trimming-unsupported-targetframework
 are unnecessary noise for projects that multitarget to include a
 TargetFramework that is compatible with
 trimming/AOT/single-file, and is a low enough version to ensure
 that it will be picked over any other TFM's assets when the
 library is consumed in a trimmed/AOT'd/single-file app.

This fixes the issue by detecting correctly multi-targeted
libraries and suppressing the warnings in these cases.

Note that prior to the .NET 8 SDK, netstandard libraries would
still get the `IsTrimmable` attribute embedded in the assembly
when setting `IsTrimmable`, and could use trim analysis despite
incomplete warnings due to unannotated ref assemblies. With the
.NET 8 SDK this is no longer the case. Even for correctly
multi-targeted libraries, the netstandard output assembly will no
longer contain the `IsTrimmable` attribute. We debated whether it
was worth warning in this case, but decided that the negative
impact of the warning on library developers following the "golden
path" was too large to justify keeping this as a warning.

This solution addresses a correctness problem that we wanted to
avoid (the correctness problem: allowing an inadvertently
non-"trimmable" netstandard asset to be consumed in a trimmed app
that uses a supported TFM).

It's still possible to get into that situation in an app that
explicitly references the netstandard assembly, or that targets
an EOL TFM (causing it to consume the netstandard asset from a
library that multitargets `netstandard2.0;net6.0` for example).

Fixes dotnet#35528
@sbomer sbomer requested review from agocke and vitek-karas October 4, 2023 16:59
@sbomer sbomer requested review from AntonLapounov and a team as code owners October 4, 2023 16:59
@dotnet-issue-labeler dotnet-issue-labeler bot added Area-ILLink untriaged Request triage from a team member labels Oct 4, 2023
@sbomer
Copy link
Member Author

sbomer commented Oct 5, 2023

@marcpopMSFT this one is also ready to go I think. We got approval from tactics in email. Is it ok to apply the Servicing-approved label myself and merge it? (It touches the same localized messages as #35595).

@marcpopMSFT
Copy link
Member

If you got approval from email, you can switch the label to servicing approved and merge. We trust folks for now to be honest about their approval and as long as no one abuses this, we'll stay that way.

@sbomer sbomer merged commit 5d0c38b into dotnet:release/8.0.1xx Oct 6, 2023
@leecow leecow removed the untriaged Request triage from a team member label Nov 16, 2023
@leecow leecow added this to the 8.0.1xx milestone Nov 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants