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.

Sign up for GitHub

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

Look up the target OS for crossgen2 using the full target RID #45566

Merged
merged 1 commit into from
Dec 30, 2024
Merged

Look up the target OS for crossgen2 using the full target RID #45566

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 5 additions & 28 deletions src/Tasks/Microsoft.NET.Build.Tasks/ResolveReadyToRunCompilers.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,42 +181,19 @@ private bool GetCrossgen2TargetOS(out string targetOS)

// Determine targetOS based on target rid.
// Use the runtime graph to support non-portable target rids.
// 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.
var runtimeGraph = new RuntimeGraphCache(this).GetRuntimeGraph(RuntimeGraphPath);
string portablePlatform = NuGetUtils.GetBestMatchingRid(
runtimeGraph,
_targetPlatform,
new[] { "linux", "linux-musl", "osx", "win", "freebsd", "illumos" },
Copy link
Member

Choose a reason for hiding this comment

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

A bit ootl here, is Linux-Musl now bundled together with Linux? Or why are we removing it here?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Basically, we could have done this all along (just use linux for both linux and linux-musl here), we just didn't.

_targetRuntimeIdentifier,
Copy link
Member

Choose a reason for hiding this comment

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

        // 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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

new[] { "linux", "osx", "win", "freebsd", "illumos" },
out _);

// For source-build, allow the bootstrap SDK rid to be unknown to the runtime repo graph.
Copy link
Member

Choose a reason for hiding this comment

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

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).

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

@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.

Copy link
Member Author

Choose a reason for hiding this comment

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

We also have custom versions of the tasks in the runtime repo, so we can use those in those scenarios if necessary.

if (portablePlatform == null && _targetRuntimeIdentifier == _hostRuntimeIdentifier)
{
if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows))
{
portablePlatform = "win";
}
else if (RuntimeInformation.IsOSPlatform(OSPlatform.Linux))
{
portablePlatform = "linux";
}
else if (RuntimeInformation.IsOSPlatform(OSPlatform.Create("FREEBSD")))
{
portablePlatform = "freebsd";
}
else if (RuntimeInformation.IsOSPlatform(OSPlatform.Create("ILLUMOS")))
{
portablePlatform = "illumos";
}
else if (RuntimeInformation.IsOSPlatform(OSPlatform.OSX))
{
portablePlatform = "osx";
}
}

targetOS = portablePlatform switch
{
"linux" => "linux",
"linux-musl" => "linux",
"osx" => "osx",
"win" => "windows",
"freebsd" => "freebsd",
Expand Down
Loading