Handle prerelease dotnet runtimes correctly #1785
Conversation
CharliePoole
force-pushed
the
issue-1779a
branch
from
118ceb9 to
b7a793f
Compare
|
This PR also modifies the logic for finding the "best runtime" out of a list of runtimes. This is the part for which I specifically need some review, so I'll explain it here... Based on my observation the output of Based on this order, we use the best (i.e. highest) version with the same major component. Pre-releases are only used for the highest major value found, in my tests for WindowsDesktop version 10.0.0, for example. The tests are based on a modified version @JasonBock 's provided example of --list-runtimes output and are probably the easiest way to see what is being done. Note that any attempt to find runtimes to load version 11.0 runtimes will fail in this approach. This seems like the best thing we can do to ensure that the user has installed all the runtimes they want to use. @OsirisTerje I requested your review in additon to @manfred-brands. Feel free to limit yourself to reviewing how the selection of "best" runtime works so we are sure it's what you need for the adapter. |
manfred-brands
left a comment
manfred-brands
left a comment
There was a problem hiding this comment.
The code changes are fine.
I found an article about runtime selection in .net
Indirectly it says:
The .NET Runtime supports loading libraries that target older .NET Runtime versions. An app that's upgraded to a newer major .NET Runtime version can reference libraries and NuGet packages that target older .NET Runtime versions. It's unnecessary to simultaneously upgrade the target runtime version of all libraries and NuGet packages referenced by the app.
Which means that finding runtime based upon referenced assembly versions might not be correct, you also don't want to switch runtimes for each assembly you are trying to load. Currently the DotNetHelper.FindBestRuntime is stateless meaning it determines the runtime directory for every assembly passes to AssemblyLoadContext.Resolving over and over again. As an application can only use 1 runtime, either the first one is THE one, or you have to re-evaluate all previously loaded assemblies from another runtime.
Maybe you might have to allow specifying a major runtime.
That would allow users to test an suite compiled for say .net8.0 using the .net10 runtime or see if there are incompatibilities between 9.0.4 and 9.0.11 runtimes.
| Version = new Version(PackageVersion.Substring(0, dash)); | ||
| PreReleaseSuffix = PackageVersion.Substring(dash + 1); | ||
| } | ||
| else | ||
| Version = new Version(packageVersion); |
There was a problem hiding this comment.
Inconsistent. Uses PackageVersion property in if and packageVersion parameter in else.
More important PreReleaseSuffix will be null in the else case.
There was a problem hiding this comment.
The assumption here is that a caller should first check IsPreRelease before accessing the suffix. I'll add some XML comments to the public properties and methods before I merge. +1 for the inconsistency.
|
|
||
| public RuntimeInfo(string name, string version, string path) | ||
| : this(name, new Version(version), path) { } | ||
| public bool IsPreRelease { get; } |
There was a problem hiding this comment.
I don't see any external usages of the new properties.
D you have plans to allow pre-release using a runtime flag?
There was a problem hiding this comment.
Since this is a general utility method, I thought these were reasonable items to provide publicly for future use. We could add a feature to allow pre-release in the future. Right now it's only used in the limited case where you only have a pre-release of a specific major version. If we wanted, we could simply ignore pre-releases unless allowing them was specified. Let's discuss this for V4.
| if (bestRuntime is null || bestRuntime.Version > candidate.Version) | ||
| bestRuntime = candidate; | ||
| } | ||
| if (!DotNet.FindBestRuntime(assemblyName.Version, _runtimeName, _x86, out DotNet.RuntimeInfo runtime)) |
There was a problem hiding this comment.
As previously said this still assumes a link between assembly version and runtime.
I seem to have 3 9.0.x runtimes installed. All 3 have the same AssemblyVersion of 9.0.0.0 but different AssemblyFileVersions.
I have add 9.0.4 to your SimulatedListRuntimesOutput it still correctly selects the latest 9.0.11.
|
@manfred-brands For V4 I have some ideas about getting rid of this entirely, made possible by the fact that we run everything in a separate process under an agent, at least for the standard (non-netcore) runner. The console runner does accept a Thanks for the review. @OsirisTerje do you have any comments? |
CharliePoole
force-pushed
the
issue-1779a
branch
from
b7a793f to
e5329d9
Compare
|
@OsirisTerje If you have any comments post-merge, I'll deal with them in the release process. You can test using 3.21.0-alpha.4 |
Corrects the fix to already closed issue #1779, which causes an error when a prerelease version is encountered in the output of
dotnet --list-runtimes. The error no longer occurs.