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

Update scripts to set distro RID and properties to use it as TargetRid #17185

Merged
merged 2 commits into from
Aug 15, 2023

Conversation

elinor-fung
Copy link
Member

@elinor-fung elinor-fung commented Aug 15, 2023

This updates installer and source-build to call shared scripts for determining the distro RID and use that value as the TargetRid.

cc @dsplaisted @tmds

Fixes dotnet/source-build#3578

@mthalman
Copy link
Member

It's not clear to me what the behavior change here is. With source-build today, the TargetRid property does get set to the distro's RID. For example, in our CI based on CentOS Stream 8, the TargetRid property gets set to centos.8-x64. What is the effect of these changes? How would they impact the behavior differently?

@elinor-fung
Copy link
Member Author

This is in response to one of the changes we're making this release around RIDs and moving the runtime away from being distro-aware. RuntimeInformation.RuntimeIdentifier will return the RID for which the runtime was built (for example, a portable build for linux would return linux- instead of a computed, distro-specific value):

The intent here is no behaviour change.

@@ -159,6 +159,8 @@
</PropertyGroup>

<PropertyGroup>
<!-- Use current machine distro RID if set. Otherwise, fall back to RuntimeInformation.RuntimeIdentifier -->
<TargetRid Condition="'$(TargetRid)' == ''">$(__DistroRid)</TargetRid>
<TargetRid Condition="'$(TargetRid)' == ''">$([System.Runtime.InteropServices.RuntimeInformation]::RuntimeIdentifier)</TargetRid>

<TargetOS Condition="'$(TargetOS)' == '' AND $([MSBuild]::IsOSPlatform('WINDOWS'))">Windows_NT</TargetOS>
Copy link
Member

Choose a reason for hiding this comment

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

At line 171, the PortableRid is computed, should this be updated to default to the __PortableTargetOS? Also should the AdditionalRuntimeIdentified get set when PortableBuild == false? I thought that is required part of fixing dotnet/source-build#3578.

Copy link
Member Author

Choose a reason for hiding this comment

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

At line 171, the PortableRid is computed, should this be updated to default to the __PortableTargetOS?

That makes sense - will update.

Also should the AdditionalRuntimeIdentified get set when PortableBuild == false? I thought that is required part of fixing dotnet/source-build#3578.

I was going to do that separately, since that is in response to a different RID-related change (dotnet/sdk#34279). Also, I believe it needs to happen in the sdk repo rather than this one.

Copy link
Member Author

Choose a reason for hiding this comment

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

dotnet/source-build#3584 is the issue tracking the need for AdditionalRuntimeIdentifier.

@elinor-fung elinor-fung merged commit 23922bc into dotnet:main Aug 15, 2023
18 checks passed
@elinor-fung elinor-fung deleted the set-distro-rid branch August 15, 2023 20:20
@mthalman
Copy link
Member

@elinor-fung - Could you please backport this to the rc1 branch?

@mthalman
Copy link
Member

This also needs to be backported to the 8.0.1xx branch. The main branch is .NET 9 at this point.

@elinor-fung
Copy link
Member Author

release/8.0.1xx-rc1: #17202
release/8.0.1xx: #17203

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.

TargetRid default needs to be updated
3 participants