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

[release/6.0] Pass TargetRid and SourceBuildNonPortable to the native scripts (backport of #74504) #77508

Merged

Conversation

ayakael
Copy link
Contributor

@ayakael ayakael commented Oct 26, 2022

Backport of #74504

Per @tmds:

Fixes https://github.com/dotnet/runtime/issues/74577.

Customer impact

Before this patch, additional RIDs would not properly flow down to runtime as __DistroRid would ignore previously set variables. This was problematic for source-build application, as every update we would have to update the RID graph, and porting to new distros would require modifying init-distro-rid.sh to output the correct RID.

Testing

Risk

Low, as it is already in main, and only activates when these options are defined.

@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Oct 26, 2022
@teo-tsirpanis teo-tsirpanis added this to the 6.0.x milestone Oct 26, 2022
@ghost
Copy link

ghost commented Oct 27, 2022

Tagging subscribers to this area: @hoyosjs
See info in area-owners.md if you want to be subscribed.

Issue Details

Backport of #74504

Per @tmds:

Fixes https://github.com/dotnet/runtime/issues/74577.
Author: ayakael
Assignees: -
Labels:

area-Infrastructure-coreclr, community-contribution

Milestone: 6.0.x

@ayakael ayakael force-pushed the backport/pr-74504-to-release/6.0 branch from a84dad2 to 2efe605 Compare November 2, 2022 15:43
@carlossanlop
Copy link
Member

@jkoritzinsky @MichaelSimons I see you approved the main PR. When/if this is ready, can you please add the servicing-consider label and send an email to Tactics requesting merge approval?

@carlossanlop
Copy link
Member

Actually, this is an infra change only. We just need a code review sign off from one or either of you, @jkoritzinsky @MichaelSimons. @ViktorHofer too, if possible.

@ViktorHofer
Copy link
Member

@tmds, do you remember if there's also a change required in Arcade for these set of changes?

@ViktorHofer
Copy link
Member

Something must be missing here:

 The "GenerateRuntimeGraph" task failed unexpectedly.
  /__w/1/s/artifacts/source-build/self/src/src/libraries/Microsoft.NETCore.Platforms/src/Microsoft.NETCore.Platforms.csproj(55,5): error MSB4018: System.InvalidOperationException: AdditionalRuntimeIdentifier banana.24-x64 was specified, which could not be found in any existing RuntimeGroup, and no parent was specified.

@ayakael
Copy link
Contributor Author

ayakael commented Nov 3, 2022

Something must be missing here:

 The "GenerateRuntimeGraph" task failed unexpectedly.
  /__w/1/s/artifacts/source-build/self/src/src/libraries/Microsoft.NETCore.Platforms/src/Microsoft.NETCore.Platforms.csproj(55,5): error MSB4018: System.InvalidOperationException: AdditionalRuntimeIdentifier banana.24-x64 was specified, which could not be found in any existing RuntimeGroup, and no parent was specified.

There's a patch somewhere that should be adding the AdditionalRuntimeIdentifier to runtime graph. Maybe it hasn't been backported yet? I think it was an installer level change

edit right, I suspect it's this #77510. Now that it's merged, I've rebased.

@ayakael
Copy link
Contributor Author

ayakael commented Nov 3, 2022

Something must be missing here:

 The "GenerateRuntimeGraph" task failed unexpectedly.
  /__w/1/s/artifacts/source-build/self/src/src/libraries/Microsoft.NETCore.Platforms/src/Microsoft.NETCore.Platforms.csproj(55,5): error MSB4018: System.InvalidOperationException: AdditionalRuntimeIdentifier banana.24-x64 was specified, which could not be found in any existing RuntimeGroup, and no parent was specified.

There's a patch somewhere that should be adding the AdditionalRuntimeIdentifier to runtime graph. Maybe it hasn't been backported yet? I think it was an installer level change

edit right, I suspect it's this #77510. Now that it's merged, I've rebased.

The rebase didn't work, there's some code missing somewhere for RIDs in AdditionalRuntimeIdentifier. I think it was implemented in source-build somewhere, so it might be an installer backport that needs doing. @tmds does this ring any bells?

@tmds
Copy link
Member

tmds commented Nov 4, 2022

This line is missing:

<InnerBuildArgs>$(InnerBuildArgs) /p:AdditionalRuntimeIdentifierParent=$(BaseOS)</InnerBuildArgs>

@ayakael ayakael force-pushed the backport/pr-74504-to-release/6.0 branch from bef1255 to 454cb63 Compare November 4, 2022 20:26
@ayakael
Copy link
Contributor Author

ayakael commented Nov 5, 2022

@ViktorHofer With above fix, CI is green.

@ViktorHofer
Copy link
Member

@carlossanlop can this be merged into release/6.0 or is the branch closed?

@carlossanlop
Copy link
Member

@carlossanlop can this be merged into release/6.0 or is the branch closed?

We're on time.

Infra change only (tell-mode). Signed off by area owner. No OOB package authoring changes needed. CI green. Ready to merge. :shipit:

@carlossanlop carlossanlop merged commit 552fcf0 into dotnet:release/6.0 Nov 7, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Dec 8, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Infrastructure-coreclr community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants