BundledVersions: update portable rid logic. #14647
Conversation
|
@am11 @crummel @MichaelSimons @ayakael ptal. This is an alternative to #14637 which also covers source-build. |
|
LGTM. I was targeting |
|
Also a candidate of 6.0 backport. The first few versions of .NET 6 were shipped with dotnet/sdk#14093 but it didn't had non-portable fix. Then in 6.0.2xx we got the non-portable fix dotnet/sdk#22373 but it introduced regression for linux-musl-{non x64 arch} in portable builds which #14637 and this PR are trying to fix. |
Defaults the portable rid to match the rid. This works for portable builds. For non-portable builds, the portable rid can be controlled using PortableOSName. For source-build, we set PortableOSName based on the bootstrap SDK's portable rid.
tmds
force-pushed
the
bundled_portable_rid
branch
from
108511b to
faa8ecb
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
I've backported this fix to |
|
The SDK side of the fix is backported to |
| @@ -10,6 +10,10 @@ | |||
| <OverrideTargetRid Condition="'$(TargetOS)' == 'OSX'">osx-x64</OverrideTargetRid> | |||
| <OSNameOverride>$(OverrideTargetRid.Substring(0, $(OverrideTargetRid.IndexOf("-"))))</OSNameOverride> | |||
|
|
|||
| <!-- Determine target portable rid based on bootstrap SDK's portable rid --> | |||
| <_platformIndex>$(NETCoreSdkPortableRuntimeIdentifier.LastIndexOf('-'))</_platformIndex> | |||
| <PortableOS>$(NETCoreSdkPortableRuntimeIdentifier.Substring(0, $(_platformIndex)))</PortableOS> | |||
There was a problem hiding this comment.
It'd be nice if <PortableOS> could be set only if it is empty. Thus:
<PortableOS Condition="'$(PortableOS)' == ''">
This ensures that we can override the PortableOS if, for example, it was previously the wrong one.
There was a problem hiding this comment.
This ensures that we can override the PortableOS if, for example, it was previously the wrong one.
Do you know can override it if by setting /p:PortableOS, for example in SourceBuild.props, even when the Condition is not present?
There was a problem hiding this comment.
In depends where that override flows down. I suspect that in SourceBuild.props, any non-conditionnal setting of PortableOS would override what's in installer.proj. For Alpine, our version 6.0.110 of the SDK regressed in that --use-current-runtime came out as alpine-3.17-x64. Thus, we had to override whatever would come out of $(NETCoreSdkPortableRuntimeIdentifier), as it wouldn't be linux-musl-x64. For building 6.0.111, we set /p:PortableOS=linux-musl as an argument to build.sh, including the conditional setting of PortableOS.
There was a problem hiding this comment.
@ayakael is it good for you as is, or do you still want me to add the Condition?
There was a problem hiding this comment.
It'd be more prudent, but it's fine as-is. Once the new Portable rid flows down the condition isn't necessary anyways.
|
Thank you for making this fix; I think it's quite important and FYI that there are only a few days left to get this in for the 7.0.100 servicing December patch. cc @marcpopMSFT. I think we should mark this as servicing-consider if this is ready @tmds ? |
|
My understanding is this and the related PRs were done for source build so I usually leave it to the source build team to review and bring up to tactics. @MichaelSimons? |
|
@crummel - Can you prepare for and present this issue to tactics? TIA |
crummel
added
Servicing-consider
Servicing-approved
and removed
Servicing-consider
labels
Defaults the portable rid to match the rid.
This works for portable builds.
For non-portable builds, the portable rid can be
controlled using PortableOSName.
For source-build, we set PortableOSName based
on the bootstrap SDK's portable rid.