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/7.0] Limit impact of global NoWarn #3123

Merged
merged 1 commit into from
Jan 18, 2023

Conversation

sbomer
Copy link
Member

@sbomer sbomer commented Nov 17, 2022

Customer Impact

This addresses a reported customer scenario where the global NoWarn breaks projects that use older tools. #3118

Testing

Local testing to confirm that this doesn't break trimmed apps. The change has also flowed to SDK in main, where all of the SDK tested scenarios have been validated.

Risk

Low risk. This doesn't change existing functionality at all - it only avoids setting an unnecessary property in scenarios where it is unused.


Fixes #3118. This limits the impact of the global NoWarn to scenarios where PublishTrimmed is true. It should help with older tools that aren't able to ignore NoWarn settings.

I also looked into making the whole target file import conditional on trim scenarios, but this isn't straightforward:

  • There are comments in there which say that they need to be imported unconditionally:
    <!-- These properties should be set even if PublishTrimmed != true, to allow SDK components to
    set PublishTrimmed in targets which are imported after these targets. -->
    We could probably clean this up since we have already been relying on PublishTrimmed being set early for the analyzer.
  • The targets have logic for PublishTrimmed, IsTrimmable, SuppressTrimAnalysisWarnings, and feature switches, so we would need to have a more complex condition for the import and/or move some of the logic elsewhere.

If the analyzer adds support for redundant suppression detection, the condition will need to be updated. I'm not trying to make the condition future-proof here because the redundant suppression feature isn't officially supported in the first place. If we add support for it, this should come with proper SDK testing.

@sbomer sbomer changed the base branch from main to release/7.0 November 17, 2022 00:03
@sbomer sbomer changed the title Limit impact of global NoWarn [release/7.0] Limit impact of global NoWarn Nov 17, 2022
@marek-safar marek-safar added this to the .NET 7.0.x milestone Nov 20, 2022
@marek-safar
Copy link
Contributor

Could you send tactics request for 7.0 and create PR targeting main.

Copy link
Member

@jeffschwMSFT jeffschwMSFT left a comment

Choose a reason for hiding this comment

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

approved. we will take for consideration in 7.0.x

@agocke
Copy link
Member

agocke commented Dec 16, 2022

@carlossanlop Is this clear to merge for 7.0.3?

@sbomer sbomer merged commit f13ad38 into dotnet:release/7.0 Jan 18, 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.

IL2121 being added needlessly to NoWarn and causing compiler errors on older netfx projects
7 participants