-
Notifications
You must be signed in to change notification settings - Fork 437
Add known RID platforms to generated BundledVersions props #16578
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
Conversation
| <_KnownRuntimeIdentiferPlatforms Include="any;freebsd;illumos;linux;linux-bionic;linux-musl;osx;solaris;unix;win" /> | ||
| <_KnownRuntimeIdentiferPlatforms Include="android;aot;browser;ios;iossimulator;maccatalyst;tvos;tvossimulator" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we expect this list to change over time? If the list might change for later .NET versions, it might be better to process KnownFrameworkReference / KnownRuntimePack items for the correct target framework to extract the platforms from the RuntimePackRuntimeIdentifiers metadata.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It could if we add a new base/portable platform. Would it make sense to keep only the ones without packs (for example, unix) here and do the processing of KnownFrameworkReference / KnownRuntimePack for the corresponding target framework in Microsoft.NET.Sdk.targets when needed then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated to process the KnownFrameworkReference/KnownRuntimePack matching $(TargetFramework). Generated text looks like:
<_KnownRuntimeIdentifersForTfm Include="@(KnownRuntimePack->WithMetadataValue('TargetFramework', $(TargetFramework))->Metadata('RuntimePackRuntimeIdentifiers'))" />
<_KnownRuntimeIdentifersForTfm Include="@(KnownFrameworkReference->WithMetadataValue('TargetFramework', $(TargetFramework))->Metadata('RuntimePackRuntimeIdentifiers'))" />
<_KnownRuntimeIdentifersForTfmWithPlatform Include="@(_KnownRuntimeIdentifersForTfm->ClearMetadata()->Distinct())">
<Platform Condition="$([System.String]::new('%(Identity)').LastIndexOf('-')) == -1">%(Identity)</Platform>
<Platform Condition="$([System.String]::new('%(Identity)').LastIndexOf('-')) != -1">$([System.String]::new('%(Identity)').Substring(0, $([System.String]::new('%(Identity)').LastIndexOf('-'))))</Platform>
</_KnownRuntimeIdentifersForTfmWithPlatform>
<_KnownRuntimeIdentiferPlatforms Include="any;aot;freebsd;illumos;solaris;unix" />
<_KnownRuntimeIdentiferPlatforms Include="@(_KnownRuntimeIdentifersForTfmWithPlatform->Metadata('Platform')->ClearMetadata()->Distinct())" Exclude="rhel.6;tizen.4.0.0;tizen.5.0.0" />
Is there a better way to do this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved the processing of the runtime packs to the SDK and kept the ones that don't have associated packs here. There's also the exclusion of the few platforms that have packs that we don't consider portable - since this can generate it only if those aren't the same platform as whatever is being used for a non-portable source-build.
…ntimeIdentifierPlatforms for current target framework
dsplaisted
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good.
| <_ExcludedKnownRuntimeIdentiferPlatforms Include="rhel.6;tizen.4.0.0;tizen.5.0.0" Condition="'$(DotNetBuildFromSource)' != 'true'" /> | ||
| <_ExcludedKnownRuntimeIdentiferPlatforms Include="rhel.6" Condition="'$(DotNetBuildFromSource)' == 'true' and !$(ProductMonikerRid.StartsWith('rhel.6-'))" /> | ||
| <_ExcludedKnownRuntimeIdentiferPlatforms Include="tizen.4.0.0" Condition="'$(DotNetBuildFromSource)' == 'true' and !$(ProductMonikerRid.StartsWith('tizen.4.0.0-'))" /> | ||
| <_ExcludedKnownRuntimeIdentiferPlatforms Include="tizen.5.0.0" Condition="'$(DotNetBuildFromSource)' == 'true' and !$(ProductMonikerRid.StartsWith('tizen.5.0.0-'))" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really understand this part. Is there a reason that the list can't be the same whether we're building from source or not? And should the RID parsing in ProcessFrameworkReferences be removing the versions from the platforms so we don't have to do it here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I basically wanted to exclude those three non-portable RIDs (that we have runtime packs for) except if we are building from source and the non-portable RID (without the architecture) being used by source build matches one of them. We could also do it in the SDK based on NETCoreSdkRuntimeIdentifier, but I figured since we had all the information at generation time I'd add the logic in the generation and keep the actual SDK logic simpler.
I think we need to keep the versions. For example, if it is a non-portable source build for rhel.8, we want rhel.8 to be considered as known (and not warn), but rhel or rhel.9 should not be known (and should warn).
| <WindowsDesktop50RuntimePackRids Include="@(WindowsDesktop31RuntimePackRids);win-arm64" /> | ||
| <WindowsDesktopRuntimePackRids Include="@(WindowsDesktop50RuntimePackRids)" /> | ||
|
|
||
| <_KnownRuntimeIdentiferPlatforms Include="any;aot;freebsd;illumos;solaris;unix" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@elinor-fung first time I see an aot RID, is there an issue/design for this?
Update generated BundledVersions props to include a list of known RID platforms. This will be used by dotnet/sdk#32970.