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

Enable the Platform Compatibility Analyzer and address warnings it introduces #41354

Closed
jeffhandley opened this issue Aug 25, 2020 · 8 comments
Closed
Assignees
Milestone

Comments

@jeffhandley
Copy link
Member

We intend to merge the Platform Compatibility Analyzer into the .NET 5.0 RC1 SDK. When the updated version of the NetAnalyzers package is taken into the dotnet/runtime repo that contains this analyzer, it will introduce build warnings. Those build warnings surface because:

  • The Runtime repo applies [SupportedOSPlatform] and [UnsupportedOSPlatform] attributes to many APIs now
  • We began applying those attributes before having the analyzer merged in because we wanted to have those attributes in place for the RC1 release
  • There are cases, specifically around our multi-targeting, where we will need to either apply further annotations or utilize guard methods around calls to the platform-specific APIs

Because these warnings will occur, we are going to disable the analyzer in the dotnet/runtime repo before merging it into the SDK. Before we ship .NET 5.0 RC2 though, we want to re-enable the analyzer and address all of the warnings introduced.

See #41209 for a draft PR where we updated the infrastructure to allow MSBuild properties for applying assembly-wide attributes. We should include that infrastructure work as part of this issue, including the detail of producing a build error if an assembly attempts to define both Supported and Unsupported platforms at the assembly-level.

@jeffhandley jeffhandley added this to the 5.0.0 milestone Aug 25, 2020
@jeffhandley jeffhandley self-assigned this Aug 25, 2020
@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added the untriaged New issue has not been triaged by the area owner label Aug 25, 2020
@marek-safar
Copy link
Contributor

It'd be fine to enable the analyzer for master branch only and we'd backport fixes if there would be any.

@buyaa-n
Copy link
Member

buyaa-n commented Aug 25, 2020

Overall 41 warnings found with local build, all looks valid warnings and should be suppressed if window targets are appropriately produced the SupportedOSPlatform attribute local warnings.txt

Overall 48 warnings found with -allconfigurations build 22 of them repeated, so its 26 build warnings for real. Somehow warnings in Microsoft.Extensions.Logging.EventLog.csproj and System.IO.Ports.csproj are missing from -allconfigurations build. Results attached warnings with allconfiguration.txt

@ericstj ericstj removed the untriaged New issue has not been triaged by the area owner label Aug 26, 2020
@jeffhandley
Copy link
Member Author

@buyaa-n can you post instructions on how to recreate these lists so that if someone else picks this issue up, they'll be able to reproduce the list and test their work?

@buyaa-n
Copy link
Member

buyaa-n commented Aug 27, 2020

Sure:

  1. build roslyn-analyzer repo (release/dotnet5-rc1 branch) in release mode and copy produced dlls into the corresponding nuget package consumed by runtime repo instructions here (or just ping me for the roslyn-analyzers dlls)
  2. command used for producing the errors with local configuration: build -subset libs -runtimeConfiguration release /p:TreatWarningsAsErrors=false , command used for producing errors in allconfig: build -subset libs -allconfiguration /p:TreatWarningsAsErrors=false

@jeffhandley
Copy link
Member Author

jeffhandley commented Aug 27, 2020

To resolve this, we should:

  1. Use a local build of the analyzer and locally enable it to resolve all of the issues in runtime, and submit a PR for those fixes into master, while leaving the analyzer disabled
  2. Port the PR from step 1 into the release/5.0-rc2 branch (with an Ask Mode approval request)
  3. Take in an official build of the NetAnalyzers package and enable the analyzer in master; this will not be ported into release/5.0

@ericstj
Copy link
Member

ericstj commented Sep 8, 2020

@jeffhandley is this work still planned for the 5.0 codebase? Should it be marked blocking?

@jeffhandley
Copy link
Member Author

Update to the plan above based on the results of #41760: /cc @ericstj

  • We will not port any of the changes into 5.0
    • None of the changes affected any API surface area
    • Therefore there's no reason to port the changes into 5.0
    • That means we don't need to chunk the work up into multiple PRs
  • We will then use a single PR (Enabling CA1416: Validate platform compatibility #41760) to complete the work in 6.0
    1. Enable the analyzer
    2. Update the analyzer build
    3. Resolve all of the issues in runtime

@jeffhandley jeffhandley modified the milestones: 5.0.0, 6.0.0 Sep 8, 2020
@jeffhandley
Copy link
Member Author

This was resolved for 6.0.0 (master) with #41760. We will not port any of this into the 5.0 branches since no public API surface area was affected.

@ghost ghost locked as resolved and limited conversation to collaborators Dec 7, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants