Use SupportedOSPlatforms property to generate assembly attributes #41209

jeffhandley · 2020-08-22T12:54:16Z

In preparation for taking in the Platform Compatibility Analyzer, we need to address build errors that will be introduced by the analyzer due to our [SupportedOSPlatform] and [UnsupportedOSPlatform] annotations.

This Draft PR uses an MSBuild-based approach similar to what was put in place for <UnsupportedOSPlatforms> such that assembly-level attributes can be generated based on the platforms listed in that property; a <SupportedOSPlatforms> property is introduced that works the same way. This approach will prevent supported and unsupported combinations in assembly attributes.

Dotnet-GitSync-Bot · 2020-08-22T12:54:19Z

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

… not allow supported and unsupported combinations

jeffhandley · 2020-08-24T05:47:51Z

Here are the current build errors that occur when using a private build from dotnet/roslyn-analyzers#3917.

Build FAILED.

System\Net\Sockets\SocketAsyncEventArgs.Windows.cs(1222,60): error CA1416: 'ThreadPoolBoundHandle.GetNativeOverlappedState(NativeOverlapped*)' is supported on 'windows' [C:\Users\jeffhand\git\dotnet\runtime-master-supportedosplatforms\src\libraries\System.Net.Sockets\src\System.Net.Sockets.csproj]
C:\Users\jeffhand\git\dotnet\runtime-master-supportedosplatforms\src\libraries\Common\src\Interop\Windows\WinSock\SafeNativeOverlapped.cs(66,33): error CA1416: 'ThreadPoolBoundHandle.FreeNativeOverlapped(NativeOverlapped*)' is supported on 'windows' [C:\Users\jeffhand\git\dotnet\runtime-master-supportedosplatforms\src\libraries\System.Net.Sockets\src\System.Net.Sockets.csproj]
C:\Users\jeffhand\git\dotnet\runtime-master-supportedosplatforms\src\libraries\Common\src\System\Net\ContextAwareResult.Windows.cs(16,32): error CA1416: 'WindowsIdentity.GetCurrent()' is supported on 'windows' [C:\Users\jeffhand\git\dotnet\runtime-master-supportedosplatforms\src\libraries\System.Net.Sockets\src\System.Net.Sockets.csproj]
C:\Users\jeffhand\git\dotnet\runtime-master-supportedosplatforms\src\libraries\Common\src\System\Net\ContextAwareResult.Windows.cs(23,17): error CA1416: 'WindowsIdentity.Dispose()' is supported on 'windows' [C:\Users\jeffhand\git\dotnet\runtime-master-supportedosplatforms\src\libraries\System.Net.Sockets\src\System.Net.Sockets.csproj]
System\Net\Sockets\SafeSocketHandle.Windows.cs(108,17): error CA1416: 'ThreadPoolBoundHandle.Dispose()' is supported on 'windows' [C:\Users\jeffhand\git\dotnet\runtime-master-supportedosplatforms\src\libraries\System.Net.Sockets\src\System.Net.Sockets.csproj]
System\Net\Sockets\SafeSocketHandle.Windows.cs(79,74): error CA1416: 'ThreadPoolBoundHandle.Handle' is supported on 'windows' [C:\Users\jeffhand\git\dotnet\runtime-master-supportedosplatforms\src\libraries\System.Net.Sockets\src\System.Net.Sockets.csproj]
System\Net\Sockets\SafeSocketHandle.Windows.cs(54,39): error CA1416: 'ThreadPoolBoundHandle.BindHandle(SafeHandle)' is supported on 'windows' [C:\Users\jeffhand\git\dotnet\runtime-master-supportedosplatforms\src\libraries\System.Net.Sockets\src\System.Net.Sockets.csproj]
System\Net\Sockets\SocketAsyncEventArgs.Windows.cs(84,20): error CA1416: 'ThreadPoolBoundHandle.AllocateNativeOverlapped(PreAllocatedOverlapped)' is supported on 'windows' [C:\Users\jeffhand\git\dotnet\runtime-master-supportedosplatforms\src\libraries\System.Net.Sockets\src\System.Net.Sockets.csproj]
System\Net\Sockets\SocketAsyncEventArgs.Windows.cs(96,13): error CA1416: 'ThreadPoolBoundHandle.FreeNativeOverlapped(NativeOverlapped*)' is supported on 'windows' [C:\Users\jeffhand\git\dotnet\runtime-master-supportedosplatforms\src\libraries\System.Net.Sockets\src\System.Net.Sockets.csproj]
System\Net\Sockets\SocketAsyncEventArgs.Windows.cs(60,43): error CA1416: 'PreAllocatedOverlapped' is supported on 'windows' [C:\Users\jeffhand\git\dotnet\runtime-master-supportedosplatforms\src\libraries\System.Net.Sockets\src\System.Net.Sockets.csproj]
System\Net\Sockets\SocketAsyncEventArgs.Windows.cs(907,17): error CA1416: 'PreAllocatedOverlapped.Dispose()' is supported on 'windows' [C:\Users\jeffhand\git\dotnet\runtime-master-supportedosplatforms\src\libraries\System.Net.Sockets\src\System.Net.Sockets.csproj]
System\Net\Sockets\BaseOverlappedAsyncResult.Windows.cs(53,48): error CA1416: 'ThreadPoolBoundHandle.AllocateNativeOverlapped(IOCompletionCallback, object?, object?)' is supported on 'windows' [C:\Users\jeffhand\git\dotnet\runtime-master-supportedosplatforms\src\libraries\System.Net.Sockets\src\System.Net.Sockets.csproj]
System\Net\Sockets\BaseOverlappedAsyncResult.Windows.cs(62,80): error CA1416: 'ThreadPoolBoundHandle.GetNativeOverlappedState(NativeOverlapped*)' is supported on 'windows' [C:\Users\jeffhand\git\dotnet\runtime-master-supportedosplatforms\src\libraries\System.Net.Sockets\src\System.Net.Sockets.csproj]
    0 Warning(s)
    13 Error(s)

adamsitnik

Very good idea! LGTM 👍

adamsitnik · 2020-08-24T07:33:28Z

src/libraries/Directory.Build.targets

@@ -243,7 +243,7 @@

  <!-- Adds System.Runtime.Versioning*Platform* annotation attributes to < .NET 5 builds -->
  <Choose>
-    <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')))">


very good idea to not always require IncludePlatformAttributes when SupportedOSPlatforms or UnsupportedOSPlatforms are used 👍

You could simplify the assemblies annotated in #40612 and replace

<IncludePlatformAttributes>true</IncludePlatformAttributes> <UnsupportedOSPlatforms>browser</UnsupportedOSPlatforms>

with just

<UnsupportedOSPlatforms>browser</UnsupportedOSPlatforms>

eng/versioning.targets

terrajobst · 2020-08-24T17:00:07Z

eng/versioning.targets

+      <AssemblyAttribute Include="System.Runtime.Versioning.UnsupportedOSPlatform" Condition="'@(_supportedOSPlatforms)' == '' and '@(_unsupportedOSPlatforms)' != ''">
        <_Parameter1>%(_unsupportedOSPlatforms.Identity)</_Parameter1>
      </AssemblyAttribute>


Should we produce a build warning when both $(SupportedOSPlatforms) and $(UnsupportedOSPlatforms) are set? Those are mutual exclusive choices, right?

ghost · 2020-08-24T17:01:07Z

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

ViktorHofer · 2020-08-24T17:11:44Z

@jeffhandley curious, is this runtime specific or should customers be able to set this in their application as well? If the latter this would probably need to go into dotnet/sdk.

jeffhandley · 2020-08-24T17:14:12Z

@jeffhandley curious, is this runtime specific or should customers be able to set this in their application as well? If the latter this would probably need to go into dotnet/sdk.

This is specific to the runtime repo, making it easier for us to apply assembly-wide attributes throughout our repo in various configs, but ensuring we don't produce conflicting [SupportedOSPlatform] and [UnsupportedOSPlatform] attributes. We could consider productizing this in the SDK in the future though.

ViktorHofer · 2020-08-24T17:16:49Z

Thanks Jeff. Let me rephrase. Will the platform compat analyzer, which respects these attributes, analyze non runtime libraries (customer applications) as well? If that's the case then we should probably open an issue in the sdk repo.

jeffhandley · 2020-08-24T17:27:39Z

Ah, thanks for rephrasing.

The analyzer will be part of the SDK through the NetAnalyzers package. dotnet/roslyn-analyzers#3917 will add the analyzer into roslyn-analyzers, and then there will be a step to update the SDK with the latest roslyn-analyzers build.

This PR sets the runtime repo up to be able to mark entire assemblies with [SupportedOSPlatform] attributes to make the assemblies platform-specific where appropriate, so that we can be ready for the intake of that analyzer for our repo.

eerhardt · 2020-08-24T17:38:04Z

I had a similar initial reaction as @ViktorHofer: Should this be an official product feature? If this is generally applicable to our customers, then we should probably add the capability to dotnet/sdk instead of our build infrastructure. However, I can understand if it is too late in 5.0 to get that in now - but maybe something in 6.0?

eerhardt · 2020-08-24T17:38:57Z

src/libraries/Microsoft.Win32.Registry.AccessControl/Directory.Build.props

@@ -2,6 +2,6 @@
  <Import Project="..\Directory.Build.props" />
  <PropertyGroup>
    <StrongNameKeyId>Microsoft</StrongNameKeyId>
-    <IsWindowsSpecific>true</IsWindowsSpecific>
+    <SupportedOSPlatforms>windows</SupportedOSPlatforms>


I thought SupportedOSPlatforms was an Item and not a property?

There are 2 with similar naming:

<SupportedPlatform>, which is an item, and it tells the analyzer which platforms we care to see warnings about, and it's part of the SDK product

<SupportedOSPlatforms>, which is this property, and it results in assembly-level [SupportedOSPlatform] attributes, and it's part of the runtime repo only

These names are so similar they're surely easy to get mixed up. Perhaps a better name for the runtime repo properties that produce the assembly-level attributes would be:

<AssemblySupportedOSPlatformAttributes>

<AssemblyUnsupportedOSPlatformAttributes>

bartonjs · 2020-08-24T17:49:10Z

I had a similar initial reaction as @ViktorHofer: Should this be an official product feature?

My understanding is that anyone building with a platform-specific TFM (e.g. net5.0-win) will have an assembly-level attribute automatically applied (whether that work is done or not, I don't know, but it was discussed). In our repository we build with the platform agnostic TFM (net5.0) for our OS-split assemblies. So I think we're just back-plumbing that SDK feature into our build variant.

Another possibility would be to try building our OS-split libraries with their OS-variant TFM, but I have no idea what impact that would have on our build composition. It's maybe the right answer for 6, but probably risky for 5 (unless it Just Works).

…attributes

eerhardt · 2020-08-25T13:24:51Z

src/libraries/Microsoft.Extensions.Hosting/src/Host.cs

@@ -111,8 +111,11 @@ public static IHostBuilder CreateDefaultBuilder(string[] args)

                if (isWindows)
                {
+#pragma warning disable CA1416 // Platform compat analyzer


Why is this necessary? isWindows is defined above as:

bool isWindows = RuntimeInformation.IsOSPlatform(OSPlatform.Windows);

Can the analyzer not recognize this pattern?

Analyzer can track result save in local, it shouldn't warn

It was warning on this. Remind me; can the analyzer also recognize the old RuntimeInformation.IsOSPlatform() calls like this one? This branch is still intact despite the PR being close. Could you pull the branch down and check this scenario please, @buyaa-n?

can the analyzer also recognize the old RuntimeInformation.IsOSPlatform()

Yes it can

It was warning on this.

That is strange, i added exact same test and it is not warning

This branch is still intact despite the PR being close. Could you pull the branch down and check this scenario please, @buyaa-n?

Sure

Filed an issue for the bug dotnet/roslyn-analyzers#4090

eerhardt · 2020-08-25T13:32:14Z

src/libraries/System.Net.Sockets/src/System/Net/Sockets/TCPListener.cs

@@ -274,7 +275,7 @@ private void CreateNewSocketIfNeeded()
                _serverSocket.ExclusiveAddressUse = true;
            }

-            if (_allowNatTraversal != null)
+            if (_allowNatTraversal != null && OperatingSystem.IsWindows())


Are we concerned about behavior change here at all? Previously someone would get a PNSE during socket creation, if they called TCPListener.AllowNatTraversal(bool allowed). Now it looks like it will no longer PNSE.

safern · 2020-08-25T16:38:26Z

eng/versioning.targets

      <AssemblyAttribute Include="System.Runtime.Versioning.UnsupportedOSPlatform" Condition="'@(_supportedOSPlatforms)' == '' and '@(_unsupportedOSPlatforms)' != ''">
        <_Parameter1>%(_unsupportedOSPlatforms.Identity)</_Parameter1>
      </AssemblyAttribute>
    </ItemGroup>
+    <Warning Condition="'@(_supportedOSPlatforms)' != '' and '@(_unsupportedOSPlatforms)' != ''"


I would just Error. We set warnAsError for the global build, so it is the same, we usually try to error for this kind of stuff as warnings could slip sometimes.

jeffhandley · 2020-08-25T16:41:05Z

We're still iterating on this; I'm going to close the draft PR and then we will open another PR when ready for another review. The feedback here was helpful to steer the direction; thanks!

jeffhandley marked this pull request as draft August 22, 2020 12:54

jeffhandley added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Aug 22, 2020

jeffhandley mentioned this pull request Aug 22, 2020

Compare master-supportedosplatforms to master jeffhandley/supportedosplatforms#2

Closed

Use SupportedOSPlatforms property to generate assembly attributes; do…

c280248

… not allow supported and unsupported combinations

jeffhandley requested review from adamsitnik, eerhardt, terrajobst and bartonjs August 22, 2020 13:23

jeffhandley added 2 commits August 23, 2020 22:21

Undo SupportedOSPlatforms properties, but leave infrastructure in place

a1bb7bf

Undo remaining platform properties that had been added

7e95fd6

adamsitnik approved these changes Aug 24, 2020

View reviewed changes

terrajobst approved these changes Aug 24, 2020

View reviewed changes

terrajobst added the area-Infrastructure-libraries label Aug 24, 2020

eerhardt reviewed Aug 24, 2020

View reviewed changes

This was referenced Aug 24, 2020

Mark APIs as unsupported on Browser WASM #41087

Closed

Platform compatibility analyzer dotnet/roslyn-analyzers#3917

Merged

jeffhandley added 3 commits August 25, 2020 00:38

Rename MSBuild properties for clarity; add a warning for conflicting …

2feacc8

…attributes

Fix some platform compat analyzer warnings

66a0773

Remove inferred SupportedOSPlatform attribute for windows builds

f75b56c

eerhardt reviewed Aug 25, 2020

View reviewed changes

safern reviewed Aug 25, 2020

View reviewed changes

jeffhandley closed this Aug 25, 2020

jeffhandley mentioned this pull request Aug 25, 2020

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

Closed

buyaa-n mentioned this pull request Aug 28, 2020

Platform compat analyzer: flow analysis not working for the arguments of lambda expression dotnet/roslyn-analyzers#4090

Closed

This was referenced Sep 2, 2020

Enabling CA1416: Validate platform compatibility #41760

Merged

Annotate platform-specific implementation assemblies with [SupportedOSPlatform] #41901

Closed

ghost locked as resolved and limited conversation to collaborators Dec 7, 2020

jeffhandley deleted the master-supportedosplatforms branch January 30, 2021 21:39

jeffhandley restored the master-supportedosplatforms branch April 3, 2021 17:19

jeffhandley deleted the master-supportedosplatforms branch April 18, 2021 03:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use SupportedOSPlatforms property to generate assembly attributes #41209

Use SupportedOSPlatforms property to generate assembly attributes #41209

jeffhandley commented Aug 22, 2020 •

edited

Loading

Dotnet-GitSync-Bot commented Aug 22, 2020

jeffhandley commented Aug 24, 2020

adamsitnik left a comment

adamsitnik Aug 24, 2020

terrajobst Aug 24, 2020

jeffhandley Aug 24, 2020

ghost commented Aug 24, 2020

ViktorHofer commented Aug 24, 2020

jeffhandley commented Aug 24, 2020

ViktorHofer commented Aug 24, 2020 •

edited

Loading

jeffhandley commented Aug 24, 2020

eerhardt commented Aug 24, 2020

eerhardt Aug 24, 2020

jeffhandley Aug 24, 2020 •

edited

Loading

bartonjs commented Aug 24, 2020

eerhardt Aug 25, 2020

buyaa-n Aug 25, 2020

jeffhandley Aug 25, 2020

buyaa-n Aug 25, 2020

buyaa-n Aug 28, 2020 •

edited

Loading

eerhardt Aug 25, 2020

safern Aug 25, 2020

jeffhandley commented Aug 25, 2020

Use SupportedOSPlatforms property to generate assembly attributes #41209

Use SupportedOSPlatforms property to generate assembly attributes #41209

Conversation

jeffhandley commented Aug 22, 2020 • edited Loading

Dotnet-GitSync-Bot commented Aug 22, 2020

jeffhandley commented Aug 24, 2020

adamsitnik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Aug 24, 2020

ViktorHofer commented Aug 24, 2020

jeffhandley commented Aug 24, 2020

ViktorHofer commented Aug 24, 2020 • edited Loading

jeffhandley commented Aug 24, 2020

eerhardt commented Aug 24, 2020

Choose a reason for hiding this comment

jeffhandley Aug 24, 2020 • edited Loading

Choose a reason for hiding this comment

bartonjs commented Aug 24, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

buyaa-n Aug 28, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeffhandley commented Aug 25, 2020

jeffhandley commented Aug 22, 2020 •

edited

Loading

ViktorHofer commented Aug 24, 2020 •

edited

Loading

jeffhandley Aug 24, 2020 •

edited

Loading

buyaa-n Aug 28, 2020 •

edited

Loading