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

[CA1416] Targeted build attributes can be overriden in cross platform assemblies #5117

Merged
merged 5 commits into from
May 27, 2021

Conversation

buyaa-n
Copy link
Contributor

@buyaa-n buyaa-n commented May 19, 2021

Fixes #5023

Allow overriding assembly-level attribute in targeted builds of cross-platform assemblies

By the design, child APIs should not extend parent support. SupportedOSPlatform attributes illustrate that the API/assembly is only reachable for that platform and it should not have any API supported on other platforms inside it. We have planned to warn if it happens by dotnet/runtime#43971, but this scenario is happening in runtime for targeted cross-platform builds. Ideally, all platform-specific implementation should be separated from cross-platform src. Though that causing code duplication at some level, so most developers would not like to apply strict separation of code.

Even though it is an expected scenario by design it is unfortunate to not get any warning about this inconsistency or possible reference of incompatible APIs within the same assembly

Initially, we decided to add a special case [SupportedOSPlatform("CrossPlatofrm")] attribute for targeted builds of cross-platform assembly to allow child member attributes to override the parent. For distinguishing real platform-specific assemblies from cross-platform assembly with targeted builds. But using SupportedOSPlatform attribute for handling this is causing side effects in further analysis and needs more tweak to get rid of it.

So i decided to use MSBuild property <CompilerVisibleProperty Include="CrossPlatform" /> instead, which we will add into targeted builds of cross platform assembly.

(We might change "CrossPlatform" into "PlatformNeutral" or something like that)

@jeffhandley
Copy link
Member

/cc @terrajobst

Copy link
Member

@jeffhandley jeffhandley left a comment

Choose a reason for hiding this comment

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

Looks good to me. And I like this approach much better than the magic string platform name!

@codecov
Copy link

codecov bot commented May 19, 2021

Codecov Report

Merging #5117 (f43ebbf) into release/6.0.1xx (4bda814) will increase coverage by 0.01%.
The diff coverage is 100.00%.

❗ Current head f43ebbf differs from pull request most recent head a4dc1f1. Consider uploading reports for the commit a4dc1f1 to get more accurate results

@@                 Coverage Diff                 @@
##           release/6.0.1xx    #5117      +/-   ##
===================================================
+ Coverage            95.57%   95.59%   +0.01%     
===================================================
  Files                 1212     1226      +14     
  Lines               277225   280903    +3678     
  Branches             16737    16872     +135     
===================================================
+ Hits                264971   268528    +3557     
- Misses               10020    10096      +76     
- Partials              2234     2279      +45     

@mavasani mavasani self-assigned this May 20, 2021
@buyaa-n buyaa-n force-pushed the targeted-cross-platofrm branch from 1ab0251 to 77c6a67 Compare May 26, 2021 17:06
@buyaa-n buyaa-n enabled auto-merge (squash) May 26, 2021 21:25
@buyaa-n
Copy link
Contributor Author

buyaa-n commented May 27, 2021

@mavasani @jmarolf I could not understand why the Spanish tests are failing, seems it is not sorting the platform names at all but do not know how could that happen:

Expected: Se puede acceder a este sitio de llamada en todas las plataformas. "Test.SupportedOnWindows10LinuxMacOS14()" solo se admite en 'linux', "macOS/OSX" 14.0 y versiones posteriores, "windows" 10.0 y versiones posteriores.
Actual:   Se puede acceder a este sitio de llamada en todas las plataformas. "Test.SupportedOnWindows10LinuxMacOS14()" solo se admite en "macOS/OSX" 14.0 y versiones posteriores, "windows" 10.0 y versiones posteriores, 'linux'.
``

@buyaa-n buyaa-n closed this May 27, 2021
auto-merge was automatically disabled May 27, 2021 16:11

Pull request was closed

@buyaa-n buyaa-n reopened this May 27, 2021
@buyaa-n buyaa-n enabled auto-merge (squash) May 27, 2021 16:37
@buyaa-n buyaa-n disabled auto-merge May 27, 2021 17:28
@buyaa-n buyaa-n force-pushed the targeted-cross-platofrm branch from f43ebbf to a4dc1f1 Compare May 27, 2021 19:29
@buyaa-n buyaa-n enabled auto-merge (squash) May 27, 2021 19:33
@buyaa-n buyaa-n merged commit 5078028 into dotnet:release/6.0.1xx May 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants