Skip to content

Update SupportedPlatform heuristic #2

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.

Sign up for GitHub

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

Closed
ViktorHofer wants to merge 1 commit into from
Closed

Update SupportedPlatform heuristic #2

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
114 changes: 89 additions & 25 deletions eng/versioning.targets
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,32 +61,96 @@
</AssemblyAttribute>
</ItemGroup>
</Target>

<PropertyGroup Condition="'$(TargetPlatformIdentifier)' == '' and !$(TargetFrameworks.Contains('$(TargetFramework)-Browser'))">
<CrossPlatformAndHasNoBrowserTarget>true</CrossPlatformAndHasNoBrowserTarget>
</PropertyGroup>

<!-- Enables warnings for Android, iOS, tvOS and macCatalyst for all builds -->

<!-- Keep in sync with Microsoft.NET.SupportedPlatforms.props and OSGroups.json. -->
<ItemGroup>
Copy link

@buyaa-n buyaa-n Jun 24, 2022

Choose a reason for hiding this comment

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

Probably should add condition Condition="'$(IsTestProject)' != 'true'" to avoid noise in test projects

<SupportedPlatform Include="Android" />
<SupportedPlatform Include="iOS" />
<SupportedPlatform Include="tvOS" />
<SupportedPlatform Include="macCatalyst" />
</ItemGroup>

<!-- Enables browser warnings for cross platform or Browser targeted builds -->
<ItemGroup Condition="('$(TargetPlatformIdentifier)' == 'Browser' or '$(CrossPlatformAndHasNoBrowserTarget)' == 'true') and '$(IsTestProject)' != 'true'">
<SupportedPlatform Include="browser"/>
</ItemGroup>

<!-- Add target platforms into MSBuild SupportedPlatform list -->
<ItemGroup Condition="'$(IsTestProject)' != 'true'">
<SupportedPlatform Condition="'$(TargetPlatformIdentifier)' == 'illumos'" Include="illumos" />
<SupportedPlatform Condition="'$(TargetPlatformIdentifier)' == 'Solaris'" Include="Solaris" />
<SupportedPlatform Condition="'$(TargetPlatformIdentifier)' == 'tvOS'" Include="tvOS" />
<SupportedPlatform Condition="'$(TargetPlatformIdentifier)' != '' and
'$(TargetPlatformIdentifier)' != 'Browser' and
'$(TargetPlatformIdentifier)' != 'windows'" Include="Unix" />
<SupportedPlatform Remove="@(SupportedPlatform)" />
<SupportedPlatform Include="Windows"
Condition="'$(TargetPlatformIdentifier)' == 'windows' or
Copy link

@buyaa-n buyaa-n Jun 24, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

NIT: adding Windows into SupportedPlatforms has no effect when targeting windows, though I see we did the same logic for Browser

(
'$(TargetPlatformIdentifier)' == '' and
!$(TargetFrameworks.Contains('$(TargetFramework)-windows'))
)" />
<SupportedPlatform Include="macOS"
Condition="'$(TargetPlatformIdentifier)' == 'OSX' or
'$(TargetPlatformIdentifier)' == 'Unix' or
(
'$(TargetPlatformIdentifier)' == '' and
!$(TargetFrameworks.Contains('$(TargetFramework)-OSX')) and
!$(TargetFrameworks.Contains('$(TargetFramework)-Unix'))
)" />
<SupportedPlatform Include="Linux"
Condition="'$(TargetPlatformIdentifier)' == 'Linux' or
Copy link

@buyaa-n buyaa-n Jun 24, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Same here and all similar cases below: is redundant for Linux target (by assuming that [SupportedOSPlatform("Linux")] attribute will be added to the assembly by MSbuild)

'$(TargetPlatformIdentifier)' == 'Unix' or
(
'$(TargetPlatformIdentifier)' == '' and
!$(TargetFrameworks.Contains('$(TargetFramework)-Linux')) and
!$(TargetFrameworks.Contains('$(TargetFramework)-Unix'))
)" />
<SupportedPlatform Include="Browser"
Condition="'$(TargetPlatformIdentifier)' == 'Browser' or
(
'$(TargetPlatformIdentifier)' == '' and
!$(TargetFrameworks.Contains('$(TargetFramework)-Browser'))
)" />
<SupportedPlatform Include="Android"
Condition="'$(TargetPlatformIdentifier)' == 'Android' or
'$(TargetPlatformIdentifier)' == 'Linux' or
'$(TargetPlatformIdentifier)' == 'Unix' or
(
'$(TargetPlatformIdentifier)' == '' and
!$(TargetFrameworks.Contains('$(TargetFramework)-Android')) and
!$(TargetFrameworks.Contains('$(TargetFramework)-Linux')) and
!$(TargetFrameworks.Contains('$(TargetFramework)-Unix'))
)" />
<SupportedPlatform Include="iOS"
Condition="'$(TargetPlatformIdentifier)' == 'iOS' or
'$(TargetPlatformIdentifier)' == 'Unix' or
(
'$(TargetPlatformIdentifier)' == '' and
!$(TargetFrameworks.Contains('$(TargetFramework)-iOS')) and
!$(TargetFrameworks.Contains('$(TargetFramework)-Unix'))
)" />
<SupportedPlatform Include="tvOS"
Condition="'$(TargetPlatformIdentifier)' == 'tvOS' or
'$(TargetPlatformIdentifier)' == 'Unix' or
(
'$(TargetPlatformIdentifier)' == '' and
!$(TargetFrameworks.Contains('$(TargetFramework)-tvOS')) and
!$(TargetFrameworks.Contains('$(TargetFramework)-Unix'))
)" />
<SupportedPlatform Include="macCatalyst"
Condition="'$(TargetPlatformIdentifier)' == 'macCatalyst' or
'$(TargetPlatformIdentifier)' == 'iOS' or
'$(TargetPlatformIdentifier)' == 'Unix' or
(
'$(TargetPlatformIdentifier)' == '' and
!$(TargetFrameworks.Contains('$(TargetFramework)-macCatalyst')) and
!$(TargetFrameworks.Contains('$(TargetFramework)-iOS')) and
!$(TargetFrameworks.Contains('$(TargetFramework)-Unix'))
)" />
<SupportedPlatform Include="illumos"
Condition="'$(TargetPlatformIdentifier)' == 'illumos' or
'$(TargetPlatformIdentifier)' == 'Unix' or
(
'$(TargetPlatformIdentifier)' == '' and
Copy link

@buyaa-n buyaa-n Jun 24, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

If I remember correctly we did not wanted to include illumos and Solaris for all platform agnostic targets, though I don't remember what was the exact scenario

!$(TargetFrameworks.Contains('$(TargetFramework)-illumos')) and
!$(TargetFrameworks.Contains('$(TargetFramework)-Unix'))
)" />
<SupportedPlatform Include="Solaris"
Condition="'$(TargetPlatformIdentifier)' == 'Solaris' or
'$(TargetPlatformIdentifier)' == 'Unix' or
(
'$(TargetPlatformIdentifier)' == '' and
!$(TargetFrameworks.Contains('$(TargetFramework)-Solaris')) and
!$(TargetFrameworks.Contains('$(TargetFramework)-Unix'))
)" />
<SupportedPlatform Include="Unix"
Condition="'$(TargetPlatformIdentifier)' == 'Unix' or
(
'$(TargetPlatformIdentifier)' == '' and
!$(TargetFrameworks.Contains('$(TargetFramework)-Unix'))
)" />
</ItemGroup>

<!-- Add PlatformNeutralAssembly property for targeted builds of cross platform assemblies -->
Expand Down