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

Refactor the platform support attribute generation in the build #51450

Merged
merged 1 commit into from
Apr 20, 2021
Merged

Refactor the platform support attribute generation in the build #51450

merged 1 commit into from
Apr 20, 2021

Conversation

jeffhandley
Copy link
Member

@jeffhandley jeffhandley commented Apr 17, 2021

Refactor the SupportedOSPlatform/UnsupportedOSPlatform attribute generation in the build to make it more consistent with fewer moving parts.

With #49261, we resurrected the idea of having an MSBuild property for <SupportedOSPlatforms> which was entertained in #41209 but not merged. This is a follow-up PR that further utilizes that new property and simplifies the logic overall while improving consistency of the attributes across the configurations.

Here are some concepts important to note:

  1. The SDK automatically produces [SupportedOSPlatform] attributes for recognized platforms, such as net6.0-windows and net6.0-ios
  2. The SDK recognizes <SupportedOSPlatformVersion> as a property to override the version number it uses on the attributes, with 0.0 meaning to omit the version
  3. The Platform Compatibility Analyzer (CA1416) consumes the <SupportedPlatform> item to get the list of platform names the build recognizes
    • The SDK includes a default set of platforms in that item
    • Newer platforms (such as ios) are not included by default
    • This item supports custom platform names too, such as Solaris
  4. Some cross-platform builds (net6.0) need to have assembly-level [SupportedOSPlatform] and [UnsupportedOSPlatform] attributes to indicate library-wide support
    • The SDK does not have a mechanism for specifying those attributes via MSBuild properties/items
    • We have introduced our own <SupportedOSPlatforms> and <UnsupportedOSPlatforms> properties to achieve that
  5. The SupportedOSPlatformAttribute and UnsupportedOSPlatformAttribute types were introduced in net5.0, but we have places where we annotate libraries/APIs with the attributes in netstandard builds
    • The CA1416 analyzer recognizes and supports the attributes being defined within a library as internal for downlevel support
    • We have an <IncludePlatformAttributes>true</IncludePlatformAttributes> property that can be specified to include those types (as internal) in the project

In this PR, the logic for our build is refactored to:

  • Replace the <IsWindowsSpecific>true</IsWindowsSpecific> property that was introduced in mark Windows-specific APIs as such #39265 with <SupportedOSPlatforms>windows</SupportedOSPlatforms>
  • Always specify <SupportedOSPlatformVersion>0.0</SupportedOSPlatformVersion> to produce versionless attributes
    • This was previously not applied to Windows builds, resulting in the production of Windows7.0 attributes
    • We had concluded for Windows that we should use the simple windows platform name without a version
    • Because of this mismatch, we had some that were windows and some that were Windows7.0; we even had some getting both
  • For any platform specified in <SupportedOSPlatforms> or <UnsupportedOSPlatforms>, we now explicitly add the platform to <SupportedPlatform> to ensure it's a recognized platform name (for custom targets not recognized by the SDK)
  • Infer the effect of <IncludePlatformAttributes>true</IncludePlatformAttributes> when any <SupportedOSPlatform> or ` value exists and the target is downlevel from net5.0

Testing the refactor and evaluating our attribute production consistency was annoying, so I crafted a utility that can scan the artifacts folder and build a report of all libraries' generated attributes. This utility looks in the generated AssemblyInfo.cs files for each library and reports on the src and ref builds' attributes for SupportedOSPlatform and UnsupportedOSPlatform. I produced a copy of that report from main before this PR and then produced a copy of that report from after PR's changes.

The net effects are:

  1. Several Windows-specific builds were getting [SupportedOSPlatform("Windows7.0")] produced; those are now [SupportedOSPlatform("windows")] to be consistent throughout
  2. Several Windows-specific builds were getting both "Windows7.0" and "windows" produced; this is now reduced to just "Windows" (side effect of the above change)
  3. Several .NET Framework targeted builds were getting the attributes produced, but we had not intended to produce them for .NET Framework builds; those are now removed
  4. Some netstandard platform-specific builds were getting the attributes produced, but that wasn't intentional (only the cross-platform build was intended to have them)
    • If we allow platform-specific builds to get the attributes applied, it can lead to conflicts with the [SupportedOSPlatform] attribute applied by the SDK itself
    • For example, for a library that is unsupported on browser, the Windows-specific build would end up with [SupportedOSPlatform("Windows")] and [UnsupportedOSPlatform("browser")], which is not desired. The result should just be [UnsupportedOSPlatform("browser")]

@ghost
Copy link

ghost commented Apr 17, 2021

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

Issue Details

Refactor the SupportedOSPlatform/UnsupportedOSPlatform attribute generation in the build to make it more consistent with fewer moving parts.

With #49261, we resurrected the idea of having an MSBuild property for <SupportedOSPlatforms> which was entertained in #41209 but not merged. This is a follow-up PR that further utilizes that new property and simplifies the logic overall while improving consistency of the attributes across the configurations.

Here are some concepts important to note:

  1. The SDK automatically produces [SupportedOSPlatform] attributes for recognized platforms, such as net6.0-windows and net6.0-ios
  2. The SDK recognizes <SupportedOSPlatformVersion> as a property to override the version number it uses on the attributes, with 0.0 meaning to omit the version
  3. The Platform Compatibility Analyzer (CA1416) consumes the <SupportedPlatform> item to get the list of platform names the build recognizes
    • The SDK includes a default set of platforms in that item
    • Newer platforms (such as ios) are not included by default
    • This item supports custom platform names too, such as Solaris
  4. Some cross-platform builds (net6.0) need to have assembly-level [SupportedOSPlatform] and [UnsupportedOSPlatform] attributes to indicate library-wide support
    • The SDK does not have a mechanism for specifying those attributes via MSBuild properties/items
    • We have introduced our own <SupportedOSPlatforms> and <UnsupportedOSPlatforms> properties to achieve that
  5. The SupportedOSPlatformAttribute and UnsupportedOSPlatformAttribute types were introduced in net5.0, but we have places where we annotate libraries/APIs with the attributes in netstandard builds
    • The CA1416 analyzer recognizes and supports the attributes being defined within a library as internal for downlevel support
    • We have an <IncludePlatformAttributes>true</IncludePlatformAttributes> property that can be specified to include those types (as internal) in the project

In this PR, the logic for our build is refactored to:

  • Replace the <IsWindowsSpecific>true</IsWindowsSpecific> property that was introduced in mark Windows-specific APIs as such #39265 with <SupportedOSPlatforms>windows</SupportedOSPlatforms>
  • Always specify `0.0 to produce versionless attributes
    • This was previously not applied to Windows builds, resulting in the production of Windows7.0 attributes
    • We had concluded for Windows that we should use the simple windows platform name without a version
    • Because of this mismatch, we had some that were windows and some that were Windows7.0; we even had some getting both
  • For any platform specified in <SupportedOSPlatforms> or <UnsupportedOSPlatforms>, we now explicitly add the platform to <SupportedPlatform> to ensure it's a recognized platform name (for custom targets not recognized by the SDK)
  • Infer the effect of <IncludePlatformAttributes>true</IncludePlatformAttributes> when any <SupportedOSPlatform> or ` value exists

Testing the refactor and evaluating our attribute production consistency was annoying, so I crafted a utility that can scan the artifacts folder and build a report of all libraries' generated attributes. This utility looks in the generated AssemblyInfo.cs files for each library and reports on the src and ref builds' attributes for SupportedOSPlatform and UnsupportedOSPlatform. I produced a copy of that report from main before this PR and then produced a copy of that report from after PR's changes.

The net effects are:

  1. Several Windows-specific builds were getting [SupportedOSPlatform("Windows7.0")] produced; those are now [SupportedOSPlatform("windows")] to be consistent throughout
  2. Several Windows-specific builds were getting both "Windows7.0" and "windows" produced; this is now reduced to just "Windows" (side effect of the above change)
  3. Several .NET Framework targeted builds were getting the attributes produced, but we had not intended to produce them for .NET Framework builds; those are now removed
  4. Some netstandard platform-specific builds were getting the attributes produced, but that wasn't intentional (only the cross-platform build was intended to have them)
    • If we allow platform-specific builds to get the attributes applied, it can lead to conflicts with the [SupportedOSPlatform] attribute applied by the SDK itself
    • For example, for a library that is unsupported on browser, the Windows-specific build would end up with [SupportedOSPlatform("Windows")] and [UnsupportedOSPlatform("browser")], which is not desired. The result should just be [UnsupportedOSPlatform("browser")]
Author: jeffhandley
Assignees: -
Labels:

area-Infrastructure-libraries

Milestone: -

@ViktorHofer
Copy link
Member

@jeffhandley excellent summary of the current state and what's being changed by this PR 👍

@ViktorHofer
Copy link
Member

The SDK does not have a mechanism for specifying those attributes via MSBuild properties/items
We have introduced our own and properties to achieve that

Would there be value in moving these properties into the SDK in case other products want or need to do something similar?

@jeffhandley
Copy link
Member Author

Thanks, @ViktorHofer! I do think we'll eventually want the SDK to have built-in support for using properties/items to produce the attributes. I don't think what we have here is quite ready for baking in though. I'd like to see us work through the remaining 6.0 work of annotating libraries for the new platforms so we can see if any other scenarios surface that we'd need to account for. I also think moving to an Item would be better than a property, but I didn't want to try to include that in this refactor.

Copy link
Contributor

@buyaa-n buyaa-n left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@jeffhandley jeffhandley merged commit c7e02f3 into dotnet:main Apr 20, 2021
@jeffhandley jeffhandley deleted the jeffhandley/use-supportedosplatforms branch April 20, 2021 04:52
@@ -232,7 +232,10 @@
<!-- Adds System.Runtime.Versioning*Platform* annotation attributes to < .NET 5 builds -->
<Choose>
<When Condition="'$(IncludePlatformAttributes)' == 'false'" />
<When Condition="('$(IncludePlatformAttributes)' == 'true' or '$(IsWindowsSpecific)' == 'true') and ('$(TargetFrameworkIdentifier)' != '.NETCoreApp' or $([MSBuild]::VersionLessThan($(TargetFrameworkVersion), '5.0')))">
<When Condition="('$(IncludePlatformAttributes)' == 'true' or '$(SupportedOSPlatforms)' != '' or '$(UnsupportedOSPlatforms)' != '') and ('$(TargetFrameworkIdentifier)' != '.NETCoreApp' or $([MSBuild]::VersionLessThan($(TargetFrameworkVersion), '5.0')))">
Copy link
Member

Choose a reason for hiding this comment

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

Nit: a simpler way to do this would've been setting IncludePlatformAttributes to true if it was empty and any of SupportedOSPlatforms or UnsupportedOSPlatforms is not empty.

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Yeah exactly... that simplifies this hard to read condition.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks. PR incoming.

Copy link
Member

@safern safern left a comment

Choose a reason for hiding this comment

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

LGTM

@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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants