Look up the target OS for crossgen2 using the full target RID #45566
Conversation
…nstead of an OS-only portion of the RID Our source-build partners only add the full OS-Arch nonportable RID to the RID graph, so the RID graph lookup would fail on an OS-only RID lookup. Also, since the linux-musl RIDs import the linux RID, we can simplify the lookup to not look up linux-musl specifically.
| var runtimeGraph = new RuntimeGraphCache(this).GetRuntimeGraph(RuntimeGraphPath); | ||
| string portablePlatform = NuGetUtils.GetBestMatchingRid( | ||
| runtimeGraph, | ||
| _targetPlatform, | ||
| new[] { "linux", "linux-musl", "osx", "win", "freebsd", "illumos" }, |
There was a problem hiding this comment.
A bit ootl here, is Linux-Musl now bundled together with Linux? Or why are we removing it here?
There was a problem hiding this comment.
The linux-musl RID has always had the linux RID as a parent. It's not 100% correct, but we can't really change it at this point due to back-compat.
There was a problem hiding this comment.
Basically, we could have done this all along (just use linux for both linux and linux-musl here), we just didn't.
| var runtimeGraph = new RuntimeGraphCache(this).GetRuntimeGraph(RuntimeGraphPath); | ||
| string portablePlatform = NuGetUtils.GetBestMatchingRid( | ||
| runtimeGraph, | ||
| _targetPlatform, | ||
| new[] { "linux", "linux-musl", "osx", "win", "freebsd", "illumos" }, | ||
| _targetRuntimeIdentifier, |
There was a problem hiding this comment.
// Use the full target rid instead of just the target OS as the runtime graph
// may only have the full target rid and not an OS-only rid for non-portable target rids
// added by our source-build partners.
@jkoritzinsky @ViktorHofer The change does not match the comment. We were not passing the target OS before, we were passing the target rid with the architecture removed. So this could be fedora.41 for example on a source-built SDK.
There was a problem hiding this comment.
The point of the change is to not pass an OS-only RID as our tooling to build a source-built SDK doesn't provide the OS-only RID (fedora.41 for example), only the OS+arch rid (fedora.41-x64).
I felt that the comment described this portion of the change well. If you think there's a better wording, we can change it.
There was a problem hiding this comment.
The point of the change is to not pass an OS-only RID as our tooling to build a source-built SDK doesn't provide the OS-only RID (fedora.41 for example), only the OS+arch rid (fedora.41-x64).
Ah, that makes clear why the change is needed to make the lookup work.
Target OS made me think of https://github.com/dotnet/arcade/blob/main/Documentation/UnifiedBuild/Unified-Build-Controls.md#output-controls.
| out _); | ||
|
|
||
| // For source-build, allow the bootstrap SDK rid to be unknown to the runtime repo graph. |
There was a problem hiding this comment.
This code was added because we ran into situations during the runtime repo build where the runtime graph used in the lookup above did not know the non-portable RID yet.
For the final produced non-portable SDK, the lookup above will works (because the runtime graph that ships with the SDK includes the non-portable rid).
There was a problem hiding this comment.
As long as we flow the RID graph within the runtime repo (which I think we do but I'm not 100% positive), we don't need this any more. We have changes in ASP.NET Core to flow the RID graph and this block isn't needed when we flow it.
There was a problem hiding this comment.
@jkoritzinsky whether this can be left out depends on the ordering of things in the runtime repo build. If it is still needed, we can always add it back.
There was a problem hiding this comment.
We also have custom versions of the tasks in the runtime repo, so we can use those in those scenarios if necessary.
Lookup the target OS for crossgen2 invocation based on the full RID instead of an OS-only portion of the RID. In source-build scenarios, we will likely only have a full
os-archRID in the RID graph. If we use only theosRID, we won't find a matching OS RID in the graph for the target OS. If we use the fullos-archRID, we'll be able to find our target in the graph.This work allows us to remove the
portablePlatform == null && _hostRuntimeIdentifier == _targetRuntimeIdentifierspecial case, as we'll now find the portable platform based on_targetRuntimeIdentifier.Also remove the special checks for
linux-muslas thelinux-muslRID imports thelinuxRID, so we can just look forlinuxand get the right behavior.