Skip to content

Conversation

@tmds
Copy link
Member

@tmds tmds commented Oct 26, 2021

Fixes #12515.

@tmds
Copy link
Member Author

tmds commented Oct 26, 2021

@dsplaisted ptal.

Copy link
Member

@dsplaisted dsplaisted left a comment

Choose a reason for hiding this comment

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

LGTM. I think we should merge this into 6.0.2xx once we've created a branch for that.

@dsplaisted
Copy link
Member

@tmds I've created a release/6.0.2xx branch. Could you rebase this PR onto that? Thanks!

@tmds
Copy link
Member Author

tmds commented Oct 27, 2021

@tmds I've created a release/6.0.2xx branch.

@dsplaisted I don't see the branch.

@dsplaisted
Copy link
Member

@tmds Sorry, I forgot this PR was for dotnet/installer. I created the 6.0.2xx branch for dotnet/sdk, but not yet for dotnet/installer.

@dsplaisted
Copy link
Member

@tmds OK, the 6.0.2xx branch has now been created in dotnet/installer. It's not ready to merge PRs into yet, but you can go ahead and rebase on top of it if you like.

@tmds tmds changed the base branch from main to release/6.0.2xx October 28, 2021 11:35
@tmds tmds requested a review from a team as a code owner October 28, 2021 11:35
@tmds tmds changed the base branch from release/6.0.2xx to main October 28, 2021 11:35
@tmds tmds force-pushed the portable_rid_default branch from b79fab8 to 676335b Compare October 28, 2021 11:37
@tmds tmds changed the base branch from main to release/6.0.2xx October 28, 2021 11:37
@tmds
Copy link
Member Author

tmds commented Oct 28, 2021

@dsplaisted I've rebased against 6.0.2xx.

cc @omajid

@tmds
Copy link
Member Author

tmds commented Nov 2, 2021

@dsplaisted with the help of @omajid I built an rpm with this patch.

Unfortunately, it doesn't seem to work. Microsoft.NETCoreSdk.BundledVersions.props has:

<NETCoreSdkPortableRuntimeIdentifier>rhel.8-x64</NETCoreSdkPortableRuntimeIdentifier>

I was expecting to see linux-x64 here.

Do you have an idea where things might go wrong?

@tmds
Copy link
Member Author

tmds commented Nov 2, 2021

Do you have an idea where things might go wrong?

I guess OSName gets derived from the TargetRid, and I should be using HostOSName instead?

<OverrideTargetRid>$(TargetRid)</OverrideTargetRid>
<OverrideTargetRid Condition="'$(TargetOS)' == 'OSX'">osx-x64</OverrideTargetRid>
<OSNameOverride>$(OverrideTargetRid.Substring(0, $(OverrideTargetRid.IndexOf("-"))))</OSNameOverride>
<RuntimeArg>--runtime-id $(OverrideTargetRid)</RuntimeArg>
<RuntimeArg Condition="'$(TargetOS)' == 'Linux'">--runtime-id $(TargetRid)</RuntimeArg>
<BuildCommandArgs>$(StandardSourceBuildArgs)</BuildCommandArgs>
<BuildCommandArgs>$(BuildCommandArgs) $(RuntimeArg)</BuildCommandArgs>
<!--
Setting NETCoreAppMaximumVersion to a high version so that the sdk doesn't complain if we're restoring/publishing for a higher version than the sdk.
See https://github.com/dotnet/sdk/issues/1512#issuecomment-377082883
-->
<BuildCommandArgs>$(BuildCommandArgs) /p:NETCoreAppMaximumVersion=99.9</BuildCommandArgs>
<BuildCommandArgs>$(BuildCommandArgs) /p:OSName=$(OSNameOverride)</BuildCommandArgs>

@tmds
Copy link
Member Author

tmds commented Nov 2, 2021

I guess OSName gets derived from the TargetRid.

The build log confirms this. I'll update the PR.

@tmds tmds force-pushed the portable_rid_default branch from f092ef0 to 8922220 Compare November 3, 2021 14:13
@tmds
Copy link
Member Author

tmds commented Nov 4, 2021

I'll update the PR.

The PR now works as expected.

@tmds
Copy link
Member Author

tmds commented Nov 10, 2021

@dsplaisted can this be merged now?

@dsplaisted dsplaisted merged commit f835fd6 into dotnet:release/6.0.2xx Nov 10, 2021
@dsplaisted
Copy link
Member

@tmds Thanks for the ping, I've merged this

ayakael added a commit to ayakael/installer that referenced this pull request Sep 13, 2022
marcpopMSFT added a commit that referenced this pull request Oct 11, 2022
…6.0.1xx

[release/6.0.1xx] GetRuntimeInformation.targets: determine PortableProductMonikerRid based on OSName and Architecture. (backports #12516)
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.

NETCoreSdkPortableRuntimeIdentifier is not the portable rid on source-built SDK

3 participants