Conversation
|
I read the CONTRIBUTING.md but it was not entirely clear to me whether you wanted to discuss this bug in a discussion first. I took a chance, hope this was OK 🤞 |
|
Submitted an issue also: #4996 |
devlead
left a comment
There was a problem hiding this comment.
Thanks for the PR and for filing #4996 — the Ubuntu / Microsoft.Data.SqlClient RID issue is real, and loading a distro-patched SDK graph is a reasonable way to teach NuGet about RIDs like ubuntu.26.04-x64.
A few things we’d like to see before this can go in:
Cake IO abstractions / testability
NuGetContentResolver already has IFileSystem, ICakeEnvironment, and IGlobber. The new lookup uses System.IO (File.OpenRead, Directory.Exists, Directory.GetDirectories, Path.Combine) on a static Lazy<RuntimeGraph>, so it cannot be faked.
That means:
dotnet testwill load whatever SDK is on the machine, not the embeddedruntime.json.FakeFileSystem/FakeEnvironmentcannot set up a patched graph or prove the Ubuntu case.Lazy<T>caches exceptions, so a bad SDK folder can break addin resolution for the whole process.
Please move graph loading to instance state (the resolver is already a singleton) and discover/read the file through Cake types:
ICakeEnvironment.GetEnvironmentVariable("DOTNET_ROOT")(and arch-specificDOTNET_ROOT_*) first, then the runtime-directory heuristic.DirectoryPath/FilePath+IFileSystem(Exist,GetDirectories,OpenRead).IGlobberis optional here; a well-known filename does not need it.
Existing tests should keep using the embedded graph unless the fixture explicitly plants an SDK layout.
Hardening
- Don’t
new Version(dir.Name)— preview folders like10.0.100-preview.6.*throw.NuGetVersion.TryParseis safer. - Don’t return a path unless the JSON file exists; if the highest SDK dir is incomplete, try the next, then fall back to the embedded graph. Wrap IO in a fallback so we never fail addin load because of graph discovery.
- Cake currently only targets net8/9/10, and will soon only target net10/11 as net8 and net9 go out of support, so the
< 8/RuntimeIdentifierGraph.jsonbranch is unused. string?will warn CS8632 (nullable is not enabled); Release treats warnings as errors.
Tests
Please keep the tests hermetic and cover the actual failure mode:
- Default fixture → embedded graph (no real SDK).
- Fake
DOTNET_ROOT+sdk/<version>/PortableRuntimeIdentifierGraph.json→ SDK graph is used. - Package layout like SqlClient:
lib/net9.0(throws on Unix) vsruntimes/unix/runtimes/win, with a distro RID that is only in the planted graph.
Adding [Fact] to Should_Return_Runtimes_Assemblies_If_CoreCLR is good (those tests were not running). Changing the TFM to net10.0 looks like it may be compensating for the host SDK graph; with a fake FS that should not be necessary.
Scope suggestion
SDK-graph loading only helps when an SDK is installed next to the runtime. Self-contained / runtime-only Cake would still miss distro RIDs. A complementary approach: if RuntimeInformation.RuntimeIdentifier is not in the graph, fall back to a portable RID (linux-x64, win-x64, …). That would fix SqlClient (runtimes/unix) without disk access. If we keep SDK-graph loading, it should be a best-effort overlay on top of that.
Also: please reference GH-4996 in the PR/commit, and complete the CLA when you can.
Happy to help iterate on the shape if useful.
|
@microsoft-github-policy-service agree company="Frende" |
|
It took some time to get the proper approval. |
devlead
left a comment
There was a problem hiding this comment.
Thanks for the CLA and for picking this back up.
#5073 is now on develop (PR, #5072). It dropped Microsoft.NETCore.Platforms / embedded runtime.json and loads Cake.NuGet.PortableRuntimeIdentifierGraph.json from instance state via IFileSystem + ICakeConfiguration. This branch conflicts with that (mergeable: CONFLICTING) and would not compile as-is: the fallback resource name Cake.NuGet.runtime.json is gone.
This PR is still complementary to #4996. The Microsoft portable graph we now embed does not list distro RIDs such as ubuntu.26.04-x64. Loading a distro-patched SDK graph at runtime is still a reasonable way to teach NuGet about those RIDs. Reloading the stock PortableRuntimeIdentifierGraph.json from a vanilla SDK is not — that is the same file we already vendor.
Please rebase onto current develop and reshape as a best-effort overlay on NuGetContentResolver.LoadRuntimeGraph (do not add a second static Lazy or System.IO):
- Keep the embed as the default (self-contained / no SDK / fixture default).
NuGet_UseLegacyRidGraphstays a separate hatch (sidecarruntime.jsonnext toCake.NuGet.dll). Do not reuse it for the SDK path.- Discover the SDK with Cake types only:
ICakeEnvironment.GetEnvironmentVariable("DOTNET_ROOT")and arch-specificDOTNET_ROOT_*first, then the runtime-directory heuristic.DirectoryPath/FilePath+IFileSystem(Exist,GetDirectories,OpenRead). - Parse folder names with
NuGetVersion.TryParse(preview folders must not throw). Same major as the host, newest first; skip a folder unlessPortableRuntimeIdentifierGraph.jsonexists; wrap IO so discovery never fails addin load. - Drop the
< 8/RuntimeIdentifierGraph.jsonbranch (Cake is net10/net11). Avoidstring?(CS8632, warnings-as-errors). - Hermetic tests: default fixture → embed; planted
DOTNET_ROOT+ patched JSON containingubuntu.26.04-x64→ SDK graph; SqlClient-style layout (lib/net*throws on Unix vsruntimes/unix/runtimes/win) with a distro RID that exists only in the planted graph. The[Fact]s on the runtimes tests are already ondevelop; do not switch TFM to net10.0 to compensate for a real host SDK.
Also keep the portable-RID fallback from the earlier review: if RuntimeInformation.RuntimeIdentifier is not in the graph, map it to a portable RID (ubuntu.*-x64 → linux-x64). That fixes SqlClient runtimes/unix with no SDK on disk. The SDK overlay is then only needed for packages that ship only runtimes/ubuntu.26.04-x64/.
Bug
While loading addins with platform-specific DLLs the NuGetContentResolver resolves the correct runtime through an embedded runtime graph.
Some distributions (like ubuntu) use the older RID types like
ubuntu.26.04-x64. This is not supported by the microsoft-provided runtime graph for newer distros since .net 8: https://learn.microsoft.com/en-us/dotnet/core/compatibility/sdk/8.0/rid-graph. The distribution patches the RID graph with support for the RID, but Cake uses the microsoft-supplied graph.Change
This change will load the sdk-provided RID graph if available.