Skip to content

Resolve unknown platform name warnings - #50193

Merged
buyaa-n merged 11 commits into
dotnet:mainfrom
buyaa-n:resolve_invalid_platforms
Mar 31, 2021
Merged

Resolve unknown platform name warnings#50193
buyaa-n merged 11 commits into
dotnet:mainfrom
buyaa-n:resolve_invalid_platforms

Conversation

@buyaa-n

@buyaa-n buyaa-n commented Mar 24, 2021

Copy link
Copy Markdown
Contributor

Fixes #49323, Related to #45851

Now we have a new analyzer "Referring to unknown platform names should result in warnings" which validate platform name and version of SupportedOSPlatform, UnupportedOSPlatform attributes parameter and warn accordingly. By running the analyzer in runtime found dozens of warnings mostly on attributes generated in AssemblyInfo.cs file added by the target platforms, see #49323 for more details. This PR:

  1. updates analyzer version to dogfood that analyzer
  2. fixes the unknows platform names by adding them into MSBuild SupportedPlatforms list
  3. Restrict UnsupportedOSPlatform("browser") attribute on .net framework, add name "browser" into MSBuild SupportedPlatforms list for netstandard and netcoreapp buiilds
  4. Generate version less attributes by TFM target as some platforms not expected to have versions
  5. Add [UnsupportedOSPlatform("browser")] attribute only for NetCoreApp

@ghost

ghost commented Mar 24, 2021

Copy link
Copy Markdown

Tagging subscribers to this area: @tarekgh, @tommcdon, @pjanotti
See info in area-owners.md if you want to be subscribed.

Issue Details

Fixes #49323, Related to #45851

Now we have a new analyzer "Referring to unknown platform names should result in warnings" which validate platform name and version of SupportedOSPlatform, UnupportedOSPlatform attributes parameter and warn accordingly. By running the analyzer in runtime found dozens of warnings mostly on attributes generated in AssemblyInfo.cs file added by the target platforms, see #49323 for more details. This PR:

  1. updates analyzer version to dogfood that analyzer
  2. fixes the unknows platform names by adding them into MSBuild SupportedPlatforms list
  3. Restrict UnsupportedOSPlatform("browser") attribute on .net framework, add name "browser" into MSBuild SupportedPlatforms list for netstandard and netcoreapp buiilds
  4. Generate version less attributes by TFM target as some platforms not expected to have versions
  5. Add [UnsupportedOSPlatform("browser")] attribute only for NetCoreApp
Author: buyaa-n
Assignees: -
Labels:

area-System.Diagnostics.Tracing
Milestone: -

@ghost

ghost commented Mar 24, 2021

Copy link
Copy Markdown

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

Issue Details

Fixes #49323, Related to #45851

Now we have a new analyzer "Referring to unknown platform names should result in warnings" which validate platform name and version of SupportedOSPlatform, UnupportedOSPlatform attributes parameter and warn accordingly. By running the analyzer in runtime found dozens of warnings mostly on attributes generated in AssemblyInfo.cs file added by the target platforms, see #49323 for more details. This PR:

  1. updates analyzer version to dogfood that analyzer
  2. fixes the unknows platform names by adding them into MSBuild SupportedPlatforms list
  3. Restrict UnsupportedOSPlatform("browser") attribute on .net framework, add name "browser" into MSBuild SupportedPlatforms list for netstandard and netcoreapp buiilds
  4. Generate version less attributes by TFM target as some platforms not expected to have versions
  5. Add [UnsupportedOSPlatform("browser")] attribute only for NetCoreApp
Author: buyaa-n
Assignees: -
Labels:

area-Infrastructure-libraries, area-System.Diagnostics.Tracing
Milestone: -

@buyaa-n buyaa-n mentioned this pull request Mar 24, 2021
Closed
jeffhandley
jeffhandley reviewed Mar 24, 2021
View reviewed changes
Comment thread eng/targetframeworksuffix.props
Comment thread eng/versioning.targets
@buyaa-n
buyaa-n force-pushed the resolve_invalid_platforms branch from 2131107 to 479d0ba Compare March 25, 2021 04:33
safern
safern reviewed Mar 25, 2021
View reviewed changes
Comment thread eng/versioning.targets Outdated
jeffhandley
jeffhandley approved these changes Mar 26, 2021
View reviewed changes

@jeffhandley jeffhandley left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I have a request for a new comment to be added; otherwise, approved.

Comment thread eng/targetframeworksuffix.props
Comment thread eng/versioning.targets Outdated
jeffhandley
jeffhandley reviewed Mar 26, 2021
View reviewed changes
Comment thread eng/versioning.targets Outdated
@buyaa-n
buyaa-n requested a review from safern March 29, 2021 22:20
carlossanlop
carlossanlop approved these changes Mar 30, 2021
View reviewed changes

@carlossanlop carlossanlop left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM, but I'd like @safern / @ViktorHofer / @Anipik to help confirm the infra changes are fine.

Comment thread eng/versioning.targets
Comment thread eng/versioning.targets
safern
safern reviewed Mar 31, 2021
View reviewed changes
Comment thread eng/versioning.targets Outdated
@buyaa-n
buyaa-n merged commit 09f075f into dotnet:main Mar 31, 2021
@buyaa-n
buyaa-n deleted the resolve_invalid_platforms branch April 25, 2021 20:12
@karelz karelz mentioned this pull request May 11, 2021
Closed
@karelz karelz added this to the 6.0.0 milestone May 20, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Jun 19, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@marek-safar marek-safar marek-safar left review comments
@safern safern safern approved these changes
@jeffhandley jeffhandley jeffhandley approved these changes
@ViktorHofer ViktorHofer Awaiting requested review from ViktorHofer
@Anipik Anipik Awaiting requested review from Anipik
+1 more reviewer
@carlossanlop carlossanlop carlossanlop approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area-Infrastructure-libraries

Projects

None yet

Milestone

6.0.0

Development

Successfully merging this pull request may close these issues.

Platform names correctness analyzer fails for OSPlatform attributes added in AssemblyInfo.cs file

6 participants

@buyaa-n @marek-safar @jeffhandley @carlossanlop @safern @karelz