-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Enable "portable" RID graph when targeting .NET 8 and higher #34279
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me! Just a few little comments 🙂
|
||
public override bool Execute() | ||
{ | ||
JToken json; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we have a "fast path" here for if AdditionalRuntimeIdentifiers is null? Then it should just be a copy, right?
foreach (var rid in AdditionalRuntimeIdentifiers) | ||
{ | ||
var importedRids = rid.GetMetadata("Imports").Split(';'); | ||
runtimes.Add(rid.ItemSpec, new JObject(new JProperty("#import", new JArray(importedRids)))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is correct and updates json["runtimes"] rather than making a copy...but can we have a regression test for that? Or perhaps just add json["runtimes"] = runtimes at the end?
@@ -48,6 +48,19 @@ Copyright (c) .NET Foundation. All rights reserved. | |||
<_GenerateSingleFileBundlePropertyInputsCache>$([MSBuild]::NormalizePath($(MSBuildProjectDirectory), $(_GenerateSingleFileBundlePropertyInputsCache)))</_GenerateSingleFileBundlePropertyInputsCache> | |||
</PropertyGroup> | |||
|
|||
<!-- For .NET 8 and higher, we will by default use a simplified "portable" RID graph --> | |||
<PropertyGroup Condition="'$(UseRidGraph)' == ''"> | |||
<UseRidGraph Condition="'$(TargetFrameworkIdentifier)' == '.NETCoreApp' AND $([MSBuild]::VersionGreaterThanOrEquals($(TargetFrameworkVersion), '8.0'))">false</UseRidGraph> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was tripped up on this once and almost said this is incorrect when it's correct. UseRidGraph to me sounds like it means use the new thing, especially since the new thing is the "portable" rid graph...can we add portable to this name and flip its true/falseness? Or add full and not flip it?
Is something needed (in addition to this PR) to use this during source-build? |
Yes, there will need to be logic during source-build to add an |
Will someone look into this? Should we create an issue to track it? |
I assume this will make RC1. Yes? |
Not necessarily, it somewhat depends on dotnet/runtime#89598 |
We should really push to make RC1. This change is pretty significant so it gets more concerning after that. Thoughts @elinor-fung? |
I assume NuGet will use the minified graph as well for .NET 8 targeting projects with this change? How will it be able to find RID specific assets, i.e.
|
It can still find portable RIDs like linux, linux-musl, osx, and win. It won't be able to find alpine or debian. |
With #34349, these test failures may now be fixed. /azp run |
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
83efef8
to
1df8f4f
Compare
I've merged this, so it should make it into RC1. We have the following breaking change issue: dotnet/docs#36466 I think we should also have breaking change notices for the following:
Do we have breaking change issues filed for these? Maybe we should have one issue that describes all of these related changes, and if not they should at least be linked somehow. |
@dsplaisted is there an open issue for updating the graph to include the non-portable rid during source-build? |
Fixes #90000 Follow-up on dotnet/sdk#34279
If there isn't one, can you create one @tmds ? |
dotnet/source-build#3578 is also related to these overall changes, so we could use that to track adding RIDs to the graph during source-build if we want. |
This is the breaking change notice for the runtime behaviour change for looking up RID-specific assets: https://learn.microsoft.com/dotnet/core/compatibility/deployment/8.0/rid-asset-list I opened dotnet/docs#36527 for this SDK change. @dsplaisted could you fill it in with more details? |
Added When you commit this breaking change:
Tagging @dotnet/compat for awareness of the breaking change. |
I've updated the breaking change issue with the details on how to revert to the old build behavior. |
@dsplaisted - When you refer to redist.csproj as the location for where these |
@mthalman It actually needs to go in the project in the SDK repo: https://github.com/dotnet/sdk/blob/main/src/Layout/redist/redist.csproj I would put it in or near the |
Resolves #34033
Projects targeting .NET 8 and higher will by default use the smaller, "portable" RuntimeIdentifier graph. They can opt in to the old, full RID graph by setting
UseRidGraph
to true. Likewise, projects targeting .NET 7 and earlier can use the trimmed-down RID graph by settingUseRidGraph
to false (there may not be any scenario where this is needed though).Additional runtime identifiers can be added to the portable RuntimeIdentifier graph when building the .NET SDK by defining
AdditionalRuntimeIdentifier
items in theredist.csproj
project. This is to support source-built versions of the .NET SDK to add their own Runtime Identifiers. Here is an example of the format for these items: