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

Use Sdks assembly Level Attributes for supported and unsupported platforms. #44257

Open
Anipik opened this issue Nov 4, 2020 · 11 comments
Open

Comments

@Anipik
Copy link
Contributor

Anipik commented Nov 4, 2020

We are no longer removing the platform from the targetFrameworkString for net5.0 and above, this results in sdk adding
SupportedOSPlatformAttribute & TargetPlatformAttribute to the assemblyInfo file. This conflicts with the attributes that we are adding on the api level.

We should define a repo wide policy going forward.

There is currently no flag in the sdk to disable writing this attribute while writing this assembly level attriibute. asking sdk team to provide a flag will also be helpful here.

cc @ViktorHofer @ericstj @buyaa-n @safern

@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added the untriaged New issue has not been triaged by the area owner label Nov 4, 2020
@Dotnet-GitSync-Bot
Copy link
Collaborator

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

@ViktorHofer
Copy link
Member

We should define a repo wide policy going forward.

Does this even matter outside of libraries?

@ghost
Copy link

ghost commented Nov 4, 2020

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

@ViktorHofer ViktorHofer removed the untriaged New issue has not been triaged by the area owner label Nov 4, 2020
@ViktorHofer ViktorHofer added this to the 6.0.0 milestone Nov 4, 2020
@ViktorHofer
Copy link
Member

ViktorHofer commented Nov 11, 2020

@buyaa-n @jeffhandley what are your thoughts on this? The SDK by default adds the in the top post mentioned attributes. Is there a reason to not have them? Presumably the TargetPlatformAttribute is less of a concern?

@ViktorHofer
Copy link
Member

ViktorHofer commented Nov 11, 2020

Another thought, are we concerned about the assembly metadata difference: SupportedOSPlatform (dotnet/runtime specific and already shipped with 5.0) vs SupportedOSPlatformAttribute (sdk style)?

@buyaa-n
Copy link
Member

buyaa-n commented Nov 19, 2020

The SDK by default adds the in the top post mentioned attributes. Is there a reason to not have them?

The reason for not having them is false positives mentioned in this issue, I have no good way (non-hack like) of handling those, the question if it could happen outside of runtime, if not i thinke its just better to keep disabling the sdk logic cc @terrajobst

Presumably the TargetPlatformAttribute is less of a concern?

I don't think there is a concern, the analyzer not using that attribute at all

@buyaa-n
Copy link
Member

buyaa-n commented Nov 30, 2020

I think we should reenable SupportedOSPlatformAttribute attributes added by sdk, most of the warning are found in Browser build which i think is the only target we should disable/remove adding the attribute. The details how to handle the warnings introduced are commented here. By enabling the sdk attribute back

  • We will have the attribute for windows build from sdk, which we added to suppress warnings on windows build
    <ItemGroup Condition="('$(IsWindowsSpecific)' == 'true' or '$(TargetsWindows)' == 'true') and '$(IsTestProject)' != 'true'">
    <AssemblyAttribute Include="System.Runtime.Versioning.SupportedOSPlatform">
    <_Parameter1>windows</_Parameter1>
    </AssemblyAttribute>
  • We will have the attribute for other build targets like linux, ios etc. For now we have no any APIs attributed with those platforms but they are coming soon, so having corresponding attributes added by MSBuild would help with seeing warning for existing platform inconsistency and suppress warnings for proper usage of platform specific APIs in runtime repo

@buyaa-n
Copy link
Member

buyaa-n commented Jan 7, 2021

@ViktorHofer @Anipik I see the TFM targets platforms other than windows is not really working, for non-runtime env, for example with simple console app i cannot use Browser or ios or Linux, Android platforms as a target. The error message: C:\Program Files\dotnet\sdk\5.0.100\Sdks\Microsoft.NET.Sdk\targets\Microsoft.NET.TargetFrameworkInference.targets(109,5): error NETSDK1139: The target platform identifier iOS was not recognized. How/why it is able to work in runtime?

@buyaa-n
Copy link
Member

buyaa-n commented Jan 7, 2021

Never mind i see the new identifiers added here

<PropertyGroup Condition="'$(TargetFrameworkSuffix)' != '' and '$(TargetFrameworkSuffix)' != 'windows'">
<TargetPlatformIdentifier>$(TargetFrameworkSuffix)</TargetPlatformIdentifier>
<TargetPlatformVersion>0.0</TargetPlatformVersion>
<TargetPlatformMoniker>$(TargetFrameworkSuffix),Version=$(TargetPlatformVersion)</TargetPlatformMoniker>
</PropertyGroup>

@Anipik
Copy link
Contributor Author

Anipik commented Aug 4, 2021

@buyaa-n is this still an issue ?

@buyaa-n
Copy link
Member

buyaa-n commented Aug 4, 2021

There is nothing more left until dotnet/sdk#14836 implemented and using that feature for disabling TFM attribute in test projects

@Anipik Anipik modified the milestones: 6.0.0, 7.0.0 Aug 4, 2021
@Anipik Anipik moved this from 6.0.0 to 7.0.0 in Infrastructure Backlog Aug 4, 2021
@ghost ghost moved this from 7.0.0 to Untriaged in Infrastructure Backlog Aug 4, 2021
@ghost ghost moved this from Untriaged to 7.0.0 in Infrastructure Backlog Aug 4, 2021
@ViktorHofer ViktorHofer modified the milestones: 7.0.0, Future Jul 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

No branches or pull requests

4 participants