Add separate warnings for settings that depend on illink pack #34077
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
- Don't publish single-file test for unsupported net472 TFM - Expect single-file to fail adding ILLink pack reference for unupported TFMs - Adjust TFM to check single-file error behavior when publishing library
ghost
added
Also fix some issues with existing tests
This was referenced Jul 21, 2023
PublishSingleFile without EnableSingleFileAnalyzer doesn't require the ILLink pack. Also add some comments, clean up some logic.
vitek-karas
approved these changes
Jul 27, 2023
- net7.0 for AOT - Add IsTargetFrameworkCompatible to message for single-file analysis
sbomer
added a commit
that referenced
this pull request
Oct 3, 2023
…#35767) 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
sbomer
added a commit
to sbomer/sdk
that referenced
this pull request
Oct 4, 2023
…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
added a commit
that referenced
this pull request
Oct 5, 2023
Up to .NET 7, the analyzers versioned with the SDK (so you would always get the latest analyzers). This allowed using the analyzers (and related options which enable the analyzers) even on older TFMs. #32045 changed to versioning the analyzers with the target framework, but still allowed use of the analyzers on target frameworks where the .NET 7 ILLink pack was made available for backwards compatibility. This change removes such backwards compat by treating the analyzers as unsupported for target frameworks before the framework was annotated with the relevant attributes. This increases the scope of the warning added in #29441 and #34077 to also warn for these unsupported TFMs. The first commit adds testcases to show the old behavior. The second commit implements the new behavior and shows the changes to these testcases. PublishTrimmed/PublishSingleFile/PublishAot used to enable the corresponding analyzers. With this change, we now need to avoid failing PublishTrimmed/PublishSingleFile for older TFMs that supported these publish options, but don't support the analyzers. This change handles that by only defaulting the `Enable*Analyzer` settings based on the publish settings for TFMs where the analyzers are known to be supported.
sbomer
added a commit
that referenced
this pull request
Oct 6, 2023
…ngle-file warnings (#35851) 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This splits the error added in #33041 into separate warnings/errors specific to the various options that depend on the ILLink pack. These warnings/errors are produced when those options are set for an unsupported target framework.
The PublishTrimmed error text reuses the message that used to be produced as a warning when targeting < netcoreapp3.0. This error is produced only when PublishAot is not set (there's a more specific error if it is).
Previously, we would produce errors when attempting to use the analyzers on unsupported TFMs. This has been changed to a warning in the case of a library (for example IsTrimmable, rather than PublishTrimmed). See comments in ProcessFrameworkReferences for details.
This also fixes an issue where ProcessFrameworkReferences was not running for netcoreapp2.0 because there were no FrameworkReferences defined for that TFM. This adds an extra condition to let ProcessFrameworkReferences run if any settings depend on the ILLink pack, so that it can produce proper warnings/errors.
Fixes dotnet/linker#3175