Add warning about version/distro-specific RID assets in .NET 8+ #32970
Conversation
ghost
added
src/Tests/Microsoft.NET.Build.Tests/GivenThatWeWantToBuildANetCoreApp.cs
Outdated
Show resolved
Hide resolved
| <_ValidRuntimeIdentifierPlatformsForAssets Include="any;freebsd;illumos;linux;linux-bionic;linux-musl;osx;solaris;unix;win" /> | ||
| <_ValidRuntimeIdentifierPlatformsForAssets Include="android;aot;browser;ios;iossimulator;maccatalyst;tvos;tvossimulator" /> |
There was a problem hiding this comment.
How will this work in source build for some custom distro. In my mental model if I create a amazetux distro and setup source build for it, the SDK should allow for a package to have amazetux specific assets for it, right?
Would it make sense to simplify this to basically say that any dot separated identifier is considered non-portable? So ubuntu would still work, but ubuntu.22 would not work. Hmm I guess that's not correct either.
I think we should add the source built native RID into this list then.
There was a problem hiding this comment.
I don't think we should hard-code these RuntimeIdentifiers here. They should probably come from or be generated in the Microsoft.NETCoreSdk.BundledVersions.props file which is created in the installer repo.
There was a problem hiding this comment.
Would putting the list in the (generation for) Microsoft.NETCoreSdk.BundledVersions.props also let source-builds append to it? Maybe conditionally adding $(OSName) or $(ProductMonikerRid)? I have not dealt much with how all the redist/generation comes together.
There was a problem hiding this comment.
Put up dotnet/installer#16578 for adding _KnownRuntimeIdentiferPlatforms items that this will then use.
There was a problem hiding this comment.
Based on discussion with @dsplaisted, switched to making ProcessFrameworkReferences do the processing of known runtime packs and set an output. That should also cover the source-build RID, since it should be part of the runtime pack available RIDs.
src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.targets
Outdated
Show resolved
Hide resolved
| <_ValidRuntimeIdentifierPlatformsForAssets Include="any;freebsd;illumos;linux;linux-bionic;linux-musl;osx;solaris;unix;win" /> | ||
| <_ValidRuntimeIdentifierPlatformsForAssets Include="android;aot;browser;ios;iossimulator;maccatalyst;tvos;tvossimulator" /> |
There was a problem hiding this comment.
I don't think we should hard-code these RuntimeIdentifiers here. They should probably come from or be generated in the Microsoft.NETCoreSdk.BundledVersions.props file which is created in the installer repo.
src/Tests/Microsoft.NET.Build.Tests/GivenThatWeWantToBuildANetCoreApp.cs
Outdated
Show resolved
Hide resolved
src/Tasks/Microsoft.NET.Build.Tasks/ProcessFrameworkReferences.cs
Outdated
Show resolved
Hide resolved
| <comment>{StrBegin="NETSDK1204: "}</comment> | ||
| </data> | ||
| <data name="NonPortableRuntimeIdentifierDetected" xml:space="preserve"> | ||
| <value>NETSDK1205: Found version-specific or distribution-specific runtime identifier(s): {0}. Affected libraries: {1}. In .NET 8.0 and higher, assets for version-specific and distribution-specific runtime identifiers will not be found by default. See https://aka.ms/dotnet/rid-usage for details.</value> |
There was a problem hiding this comment.
This aka.ms link should be created before merging this PR. It can point to this PR or the design doc or something similar for now.
There was a problem hiding this comment.
Done. Currently points to the breaking change notice.
src/Tests/Microsoft.NET.Build.Tests/GivenThatWeWantToBuildANetCoreApp.cs
Outdated
Show resolved
Hide resolved
| // The actual list comes from BundledVersions.props. For testing, we conditionally add a | ||
| // subset of the list if it isn't already defined (so running on an older version) | ||
| testProject.AddItem("_KnownRuntimeIdentiferPlatforms", | ||
| new Dictionary<string, string>() | ||
| { | ||
| { "Include", "unix" }, | ||
| { "Condition", "'@(_KnownRuntimeIdentiferPlatforms)'==''" } | ||
| }); |
There was a problem hiding this comment.
It's probably a good idea to go back and delete this once the installer changes have been merged and flowed into stage 0.
There was a problem hiding this comment.
Created #33152 so I don't forget this.
Co-authored-by: Daniel Plaisted <[email protected]>
src/Tasks/Microsoft.NET.Build.Tasks/ProcessFrameworkReferences.cs
Outdated
Show resolved
Hide resolved
4 participants
To help with the .NET 8 breaking change around no longer looking at version/distro-specific RID assets, we want to add a warning on build when we detect the dependencies that have non-portable-RID-specific assets.
This adds a warning when:
System.Runtime.Loader.UseRidGraph) is not set or not set totrueThe link in the message (https://aka.ms/dotnet/rid-usage) currently just points to the breaking change - we will have more doc in the future).
cc @agocke @baronfel @richlander @vitek-karas