Respond to `dnup` feedback from prototype code review #51446

nagilson · 2025-10-24T20:59:49Z

This PR will remediate the issues discussed in #50988 to fix the prototype dnup code. It's not a complete review of all of that code, however. I have not reviewed the portion of the CLI and PATH manipulation, though that's not directly part of the library, so I think that will come later.

Splits up `ReleaseManifest` into 3 pieces: - A. Downloading of .NET Archives - B. Accessing the release manifest & release manifest data structures - C. Parsing a 'channel' into a ReleaseVersion This is a better separation of responsibility and makes each class a far more reasonable size.

What we were doing was setting the AppContext data such that the HOSTFXR_PATH pointed to the installed root location. We don't need to do that, the hostfxr interop(ped) API already points to the dotnet root location within which we want to resolve the hostfxr.

verification should happen in the downloader which will probably be deferred to the release deployment library

…ototype-code-rv

dsplaisted

Good improvements.

Main issue is that calls into hostfxr no longer work from a NativeAOT app.

dsplaisted · 2025-11-06T00:19:38Z

src/Installer/Microsoft.Dotnet.Installation/Internal/ChannelVersionResolver.cs

+
+    private IEnumerable<Product> GetProductsInMajorOrMajorMinor(IEnumerable<Product> index, int major, int? minor = null)
+    {
+        var validProducts = index.Where(p => p.ProductVersion.StartsWith(minor is not null ? $"{major}.{minor}" : $"{major}."));


Can we do this by checking a version object instead of checking if the string starts with something? I think we'll need to parse the version first but that would seem better to me.

dsplaisted · 2025-11-06T00:20:28Z

src/Installer/Microsoft.Dotnet.Installation/Internal/ChannelVersionResolver.cs

+    /// <param name="isLts">True for LTS (Long-Term Support), false for STS (Standard-Term Support)</param>
+    /// <param name="mode">InstallComponent.SDK or InstallComponent.Runtime</param>


Looks like these doc comments are out of date.

dsplaisted · 2025-11-06T00:27:19Z

src/Installer/Microsoft.Dotnet.Installation/Internal/DotnetArchiveDownloader.cs

+/// <summary>
+/// Handles downloading and parsing .NET release manifests to find the correct installer/archive for a given installation.
+/// </summary>
+internal class DotnetArchiveDownloader(HttpClient httpClient) : IDisposable


Do we need the constructor that specifies an HttpClient? It doesn't look like we use that outside of the class, and there's no way to specify both an httpClient and a ReleaseManifest.

we will when we want the library vs dnup to use a different custom agent header - or enable customization of timeout(s) for library callers

src/Installer/Microsoft.Dotnet.Installation/Internal/DotnetArchiveDownloader.cs

dsplaisted · 2025-11-06T01:05:21Z

src/Installer/Microsoft.Dotnet.Installation/Internal/DotnetArchiveDownloader.cs

+                Directory.CreateDirectory(Path.GetDirectoryName(destinationPath)!);
+
+                // Try to get content length for progress reporting
+                long? totalBytes = await GetContentLengthAsync(downloadUrl);


I think this adds an extra HTTP request. Is there a reason to do this separately rather than get the value from the header of the request which actually downloads?

No, this is a great idea.

dsplaisted · 2025-11-06T01:53:54Z

test/dnup.Tests/DnupE2Etest.cs

+    var args = DnupTestUtilities.BuildArguments(channel, testEnv.InstallPath, testEnv.ManifestPath);
+    (int exitCode, string output) = DnupTestUtilities.RunDnupProcess(args, captureOutput: true, workingDirectory: testEnv.TempRoot);
+    exitCode.Should().Be(0, $"dnup exited with code {exitCode}. Output:\n{output}");


Nit: indentation seems wrong here.

dsplaisted · 2025-11-06T01:54:29Z

test/dnup.Tests/DnupE2Etest.cs

+    (int exitCode, string firstInstallOutput) = DnupTestUtilities.RunDnupProcess(args, captureOutput: true, workingDirectory: testEnv.TempRoot);
+    exitCode.Should().Be(0, $"First installation failed with exit code {exitCode}. Output:\n{firstInstallOutput}");


Nit: indentation

dsplaisted · 2025-11-06T01:54:46Z

test/dnup.Tests/DnupE2Etest.cs

+    (exitCode, string output) = DnupTestUtilities.RunDnupProcess(args, captureOutput: true, workingDirectory: testEnv.TempRoot);
+    exitCode.Should().Be(0, $"Second installation failed with exit code {exitCode}. Output:\n{output}");


Indentation.

Yeah, the test files here are just wrong but also, I was hoping to add the .editorconfig in this PR to enforce format, using statement, etc on save

dsplaisted · 2025-11-06T01:56:17Z

test/dnup.Tests/Utilities/DnupTestUtilities.cs

+    using var process = new Process();
+    string repoDotnet = Path.Combine(repoRoot, ".dotnet", DnupUtilities.GetDotnetExeName());
+    process.StartInfo.FileName = File.Exists(repoDotnet) ? repoDotnet : DnupUtilities.GetDotnetExeName();


Nit: indentation

this file seems to be an erroneous revert - in fact, most of the changes to the tests files are wrong.

dsplaisted · 2025-11-06T01:56:31Z

test/dnup.Tests/Utilities/DnupTestUtilities.cs

+        process.StartInfo.CreateNoWindow = true;
+        process.StartInfo.RedirectStandardOutput = captureOutput;
+        process.StartInfo.RedirectStandardError = captureOutput;
+    process.StartInfo.WorkingDirectory = workingDirectory ?? Environment.CurrentDirectory;


Nit: indentation.

nagilson · 2025-11-07T00:34:09Z

src/Installer/dnup/DotnetInstallManager.cs

        string programFilesX86 = Environment.GetFolderPath(Environment.SpecialFolder.ProgramFilesX86);
        bool isAdminInstall = installDir.StartsWith(Path.Combine(programFiles, "dotnet"), StringComparison.OrdinalIgnoreCase) ||
-                              installDir.StartsWith(Path.Combine(programFilesX86, "dotnet"), StringComparison.OrdinalIgnoreCase);
+                              installDir.StartsWith(Path.Combine(programFilesX86, "dotnet"), StringComparison.OrdinalIgnoreCase); // TODO: This should be improved to not be windows-specific


Suggested change

installDir.StartsWith(Path.Combine(programFilesX86, "dotnet"), StringComparison.OrdinalIgnoreCase); // TODO: This should be improved to not be windows-specific

installDir.StartsWith(Path.Combine(programFilesX86, "dotnet"), StringComparison.OrdinalIgnoreCase); // TODO: This should be improved to not be windows-specific https://github.com/dotnet/sdk/issues/51601

nagilson added 16 commits October 24, 2025 13:56

Respond to dnup feedback from prototype code review

bee1fdd

Fix build issue

67ab374

Fix bad paran

7ae5d2e

Skip extra hostfxr logic

6a1457c

What we were doing was setting the AppContext data such that the HOSTFXR_PATH pointed to the installed root location. We don't need to do that, the hostfxr interop(ped) API already points to the dotnet root location within which we want to resolve the hostfxr.

Use hostfxr API to get .NET SDKs that are available to that dotnet root

330aa7f

Add reference to native wrapper for hostfxr

189f4e6

Use the same release index so we don't download it twice

73b6023

Delete unneeded sln which is replaced by slnf

34489cc

Fix library call

3076c56

Remove extra function

79b81c2

verification should happen in the downloader which will probably be deferred to the release deployment library

Use same naming convention as other classes

b87c196

Simplify archive extraction, remark on bad hostfxr logic, fix test

c996289

remove unused function

5c5a00d

Implement wrapper around hostfxr calls

de66740

Merge branch 'dnup' into nagilson-prototype-code-rv

5b4acf3

nagilson changed the base branch from dnup to release/dnup October 28, 2025 22:19

nagilson added 4 commits October 28, 2025 15:20

Merge branch 'release/dnup' into nagilson-prototype-code-rv

1942ea5

Merge remote-tracking branch 'upstream/release/dnup' into nagilson-pr…

e748293

…ototype-code-rv

add methods from merge

90bfe48

Fix merge issues

30bfd67

build-analysis bot mentioned this pull request Nov 4, 2025

Template engine tests hitting known parallelism issue in GetVisualStudioInstances #44878

Open

dsplaisted approved these changes Nov 6, 2025

View reviewed changes

nagilson commented Nov 7, 2025

View reviewed changes

dsplaisted mentioned this pull request Nov 13, 2025

Apply dotnetup code review feedback #51711

Open

		/// <param name="isLts">True for LTS (Long-Term Support), false for STS (Standard-Term Support)</param>
		/// <param name="mode">InstallComponent.SDK or InstallComponent.Runtime</param>

		(int exitCode, string firstInstallOutput) = DnupTestUtilities.RunDnupProcess(args, captureOutput: true, workingDirectory: testEnv.TempRoot);
		exitCode.Should().Be(0, $"First installation failed with exit code {exitCode}. Output:\n{firstInstallOutput}");

		(exitCode, string output) = DnupTestUtilities.RunDnupProcess(args, captureOutput: true, workingDirectory: testEnv.TempRoot);
		exitCode.Should().Be(0, $"Second installation failed with exit code {exitCode}. Output:\n{output}");

	installDir.StartsWith(Path.Combine(programFilesX86, "dotnet"), StringComparison.OrdinalIgnoreCase); // TODO: This should be improved to not be windows-specific
	installDir.StartsWith(Path.Combine(programFilesX86, "dotnet"), StringComparison.OrdinalIgnoreCase); // TODO: This should be improved to not be windows-specific https://github.com/dotnet/sdk/issues/51601

Respond to dnup feedback from prototype code review #51446

Are you sure you want to change the base?

Respond to dnup feedback from prototype code review #51446

Uh oh!

Conversation

nagilson commented Oct 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dsplaisted left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nagilson Nov 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nagilson Nov 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Respond to `dnup` feedback from prototype code review #51446

Respond to `dnup` feedback from prototype code review #51446

nagilson commented Oct 24, 2025 •

edited

Loading

nagilson Nov 7, 2025 •

edited

Loading

nagilson Nov 7, 2025 •

edited

Loading