Respond to dnup feedback from prototype code review

nagilson · dsplaisted · commit e83630d9110e · 2025-11-10T16:09:23.000-05:00
diff --git a/src/Installer/Microsoft.Dotnet.Installation/Internal/ArchiveDotnetExtractor.cs b/src/Installer/Microsoft.Dotnet.Installation/Internal/ArchiveDotnetExtractor.cs
@@ -62,30 +62,6 @@ private void VerifyArchive(string archivePath)
         }
     }
 
-
-
-    internal static string ConstructArchiveName(string? versionString, string rid, string suffix)
-    {
-        // If version is not specified, use a generic name
-        if (string.IsNullOrEmpty(versionString))
-        {
-            return $"dotnet-sdk-{rid}{suffix}";
-        }
-
-        // Make sure the version string doesn't have any build hash or prerelease identifiers
-        // This ensures compatibility with the official download URLs
-        string cleanVersion = versionString;
-        int dashIndex = versionString.IndexOf('-');
-        if (dashIndex >= 0)
-        {
-            cleanVersion = versionString.Substring(0, dashIndex);
-        }
-
-        return $"dotnet-sdk-{cleanVersion}-{rid}{suffix}";
-    }
-
-
-
     public void Commit()
     {
         Commit(GetExistingSdkVersions(_request.InstallRoot));
@@ -205,9 +181,7 @@ private string DecompressTarGzIfNeeded(string archivePath, out bool needsDecompr
             return archivePath;
         }
 
-        string decompressedPath = Path.Combine(
-            Path.GetDirectoryName(archivePath) ?? Directory.CreateTempSubdirectory().FullName,
-            "decompressed.tar");
+        string decompressedPath = archivePath.Replace(".gz", "");
 
         using FileStream originalFileStream = File.OpenRead(archivePath);
         using FileStream decompressedFileStream = File.Create(decompressedPath);
@@ -293,7 +267,7 @@ private void ExtractTarFileEntry(TarEntry entry, string targetDir, MuxerHandling
     private void HandleMuxerUpdateFromTar(TarEntry entry, string muxerTargetPath)
     {
         // Create a temporary file for the muxer first to avoid locking issues
-        var tempMuxerPath = Path.Combine(Path.GetTempPath(), Path.GetRandomFileName());
+        var tempMuxerPath = Directory.CreateTempSubdirectory().FullName;
         using (var outStream = File.Create(tempMuxerPath))
         {
             entry.DataStream?.CopyTo(outStream);
@@ -378,7 +352,7 @@ private void ExtractZipEntry(ZipArchiveEntry entry, string targetDir, MuxerHandl
      */
     private void HandleMuxerUpdateFromZip(ZipArchiveEntry entry, string muxerTargetPath)
     {
-        var tempMuxerPath = Path.Combine(Path.GetTempPath(), Path.GetRandomFileName());
+        var tempMuxerPath = Directory.CreateTempSubdirectory().FullName;
         entry.ExtractToFile(tempMuxerPath, overwrite: true);
 
         try
diff --git a/src/Installer/dnup/DnupSharedManifest.cs b/src/Installer/dnup/DnupSharedManifest.cs
@@ -49,7 +49,7 @@ private string GetManifestPath()
 
         // Default location
         return Path.Combine(
-            Environment.GetFolderPath(Environment.SpecialFolder.UserProfile),
+            RuntimeInformation.IsOSPlatform(OSPlatform.Windows) ? Environment.GetFolderPath(Environment.SpecialFolder.LocalApplicationData) : Environment.GetFolderPath(Environment.SpecialFolder.UserProfile),
             "dnup",
             "dnup_manifest.json");
     }
diff --git a/test/dnup.Tests/DnupE2Etest.cs b/test/dnup.Tests/DnupE2Etest.cs
@@ -8,11 +8,10 @@
 using System.Threading.Tasks;
 using FluentAssertions;
 using Microsoft.Deployment.DotNet.Releases;
-using Microsoft.Dotnet.Installation;
 using Microsoft.DotNet.Tools.Bootstrapper;
 using Microsoft.DotNet.Tools.Dnup.Tests.Utilities;
+using Microsoft.Dotnet.Installation;
 using Xunit;
-using Microsoft.Dotnet.Installation.Internal;
 
 namespace Microsoft.DotNet.Tools.Dnup.Tests;
 
@@ -50,7 +49,7 @@ public void Test(string channel)
         var updateChannel = new UpdateChannel(channel);
         var expectedVersion = new ManifestChannelVersionResolver().Resolve(
             new DotnetInstallRequest(
-                new DotnetInstallRoot(testEnv.InstallPath, InstallerUtilities.GetDefaultInstallArchitecture()),
+                new DotnetInstallRoot(testEnv.InstallPath, DnupUtilities.GetDefaultInstallArchitecture()),
                 updateChannel,
                 InstallComponent.SDK,
                 new InstallRequestOptions()));
@@ -60,17 +59,15 @@ public void Test(string channel)
         Console.WriteLine($"Channel '{channel}' resolved to version: {expectedVersion}");
 
         // Execute the command with explicit manifest path as a separate process
-        var args = DnupTestUtilities.BuildArguments(channel, testEnv.InstallPath, testEnv.ManifestPath);
-
-        DnupProcessResult result = DnupTestUtilities.RunDnupProcess(args, captureOutput: true, workingDirectory: testEnv.TempRoot);
-        result.ExitCode.Should().Be(0,
-            $"dnup exited with code {result.ExitCode}. Output:\n{DnupTestUtilities.FormatOutputForAssertions(result)}");
+    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}");
 
         Directory.Exists(testEnv.InstallPath).Should().BeTrue();
         Directory.Exists(Path.GetDirectoryName(testEnv.ManifestPath)).Should().BeTrue();
 
         // Verify the installation was properly recorded in the manifest
-        List<DotnetInstall> installs = [];
+        List<DotnetInstall> installs = new();
         using (var finalizeLock = new ScopedMutex(Constants.MutexNames.ModifyInstallationStates))
         {
             var manifest = new DnupSharedManifest(testEnv.ManifestPath);
@@ -83,10 +80,7 @@ public void Test(string channel)
         matchingInstalls.Should().ContainSingle();
 
         var install = matchingInstalls[0];
-        install.Component.Should().Be(InstallComponent.SDK);
-
-        DnupTestUtilities.ValidateInstall(install).Should().BeTrue(
-            $"ArchiveInstallationValidator failed for installed version {install.Version} at {testEnv.InstallPath}");
+        install.Component.Should().Be(Microsoft.Dotnet.Installation.InstallComponent.SDK);
 
         // Verify the installed version matches what the resolver predicted
         if (!updateChannel.IsFullySpecifiedVersion())
@@ -126,36 +120,31 @@ public void TestReusesExistingInstall()
 
         // Execute dnup to install the SDK the first time with explicit manifest path as a separate process
         Console.WriteLine($"First installation of {channel}");
-        DnupProcessResult firstInstall = DnupTestUtilities.RunDnupProcess(args, captureOutput: true, workingDirectory: testEnv.TempRoot);
-        firstInstall.ExitCode.Should().Be(0,
-            $"First installation failed with exit code {firstInstall.ExitCode}. Output:\n{DnupTestUtilities.FormatOutputForAssertions(firstInstall)}");
+    (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}");
 
-        List<DotnetInstall> firstDnupInstalls = [];
+        List<DotnetInstall> firstDnupInstalls = new();
         // Verify the installation was successful
         using (var finalizeLock = new ScopedMutex(Constants.MutexNames.ModifyInstallationStates))
         {
             var manifest = new DnupSharedManifest(testEnv.ManifestPath);
             firstDnupInstalls = manifest.GetInstalledVersions().ToList();
         }
 
-        var firstInstallRecord = firstDnupInstalls.Single(i => DnupUtilities.PathsEqual(i.InstallRoot.Path, testEnv.InstallPath));
-        DnupTestUtilities.ValidateInstall(firstInstallRecord).Should().BeTrue(
-            $"ArchiveInstallationValidator failed for initial install of {channel} at {testEnv.InstallPath}");
+        firstDnupInstalls.Where(i => DnupUtilities.PathsEqual(i.InstallRoot.Path, testEnv.InstallPath)).Should().ContainSingle();
 
         // Now install the same SDK again and capture the console output
         Console.WriteLine($"Installing .NET SDK {channel} again (should be skipped)");
-        DnupProcessResult secondInstall = DnupTestUtilities.RunDnupProcess(args, captureOutput: true, workingDirectory: testEnv.TempRoot);
-        secondInstall.ExitCode.Should().Be(0,
-            $"Second installation failed with exit code {secondInstall.ExitCode}. Output:\n{DnupTestUtilities.FormatOutputForAssertions(secondInstall)}");
+    (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}");
 
-        DnupTestUtilities.AssertOutput(secondInstall, output =>
-        {
-            output.Should().Contain("is already installed, skipping installation",
-                "dnup should detect that the SDK is already installed and skip the installation");
+        // Verify the output contains a message indicating the SDK is already installed
+        output.Should().Contain("is already installed, skipping installation",
+            "dnup should detect that the SDK is already installed and skip the installation");
 
-            output.Should().NotContain("Downloading .NET SDK",
-                "dnup should not attempt to download the SDK again");
-        });
+        // The output should not contain download progress
+        output.Should().NotContain("Downloading .NET SDK",
+            "dnup should not attempt to download the SDK again");
 
         List<DotnetInstall> matchingInstalls = new();
         // Verify the installation record in the manifest hasn't changed
@@ -170,8 +159,6 @@ public void TestReusesExistingInstall()
         matchingInstalls.Should().ContainSingle();
         // And it should be for the specified version
         matchingInstalls[0].Version.ToString().Should().Be(channel);
-        DnupTestUtilities.ValidateInstall(matchingInstalls[0]).Should().BeTrue(
-            $"ArchiveInstallationValidator failed after reinstall attempt for {channel} at {testEnv.InstallPath}");
     }
 }
 
@@ -181,82 +168,32 @@ public void TestReusesExistingInstall()
 [Collection("DnupConcurrencyCollection")]
 public class ConcurrentInstallationTests
 {
-    public static IEnumerable<object[]> ConcurrentInstallChannels => new List<object[]>
+    [Fact]
+    public async Task ConcurrentInstallsSerializeViaGlobalMutex()
     {
-        new object[] { "9.0.103", "9.0.103", false },
-        new object[] { "9.0.103", "preview", true }
-    };
+        const string channel = "9.0.103";
 
-    [Theory]
-    [MemberData(nameof(ConcurrentInstallChannels))]
-    public async Task ConcurrentInstallsSerializeViaGlobalMutex(string firstChannel, string secondChannel, bool expectDistinct)
-    {
         using var testEnv = DnupTestUtilities.CreateTestEnvironment();
+        var args1 = DnupTestUtilities.BuildArguments(channel, testEnv.InstallPath, testEnv.ManifestPath);
+        var args2 = DnupTestUtilities.BuildArguments(channel, testEnv.InstallPath, testEnv.ManifestPath);
 
-        var resolver = new ManifestChannelVersionResolver();
-        ReleaseVersion? firstResolved = resolver.Resolve(
-            new DotnetInstallRequest(
-                new DotnetInstallRoot(testEnv.InstallPath, InstallerUtilities.GetDefaultInstallArchitecture()),
-                new UpdateChannel(firstChannel),
-                InstallComponent.SDK,
-                new InstallRequestOptions()));
-
-        ReleaseVersion? secondResolved = resolver.Resolve(
-            new DotnetInstallRequest(
-                new DotnetInstallRoot(testEnv.InstallPath, InstallerUtilities.GetDefaultInstallArchitecture()),
-                new UpdateChannel(secondChannel),
-                InstallComponent.SDK,
-                new InstallRequestOptions()));
-
-        firstResolved.Should().NotBeNull($"Channel {firstChannel} should resolve to a version");
-        secondResolved.Should().NotBeNull($"Channel {secondChannel} should resolve to a version");
-
-        if (expectDistinct && string.Equals(firstResolved!.ToString(), secondResolved!.ToString(), StringComparison.OrdinalIgnoreCase))
-        {
-            Console.WriteLine($"Skipping concurrent distinct-version scenario because both channels resolved to {firstResolved}");
-            return;
-        }
-        var args1 = DnupTestUtilities.BuildArguments(firstChannel, testEnv.InstallPath, testEnv.ManifestPath);
-        var args2 = DnupTestUtilities.BuildArguments(secondChannel, testEnv.InstallPath, testEnv.ManifestPath);
-
-        var installTask1 = Task.Run(() => DnupTestUtilities.RunDnupProcess(args1, captureOutput: true, workingDirectory: testEnv.TempRoot));
-        var installTask2 = Task.Run(() => DnupTestUtilities.RunDnupProcess(args2, captureOutput: true, workingDirectory: testEnv.TempRoot));
-
-        DnupProcessResult[] results = await Task.WhenAll(installTask1, installTask2);
+    var installTask1 = Task.Run(() => DnupTestUtilities.RunDnupProcess(args1, captureOutput: true, workingDirectory: testEnv.TempRoot));
+    var installTask2 = Task.Run(() => DnupTestUtilities.RunDnupProcess(args2, captureOutput: true, workingDirectory: testEnv.TempRoot));
 
-        results[0].ExitCode.Should().Be(0,
-            $"First concurrent install failed with exit code {results[0].ExitCode}. Output:\n{DnupTestUtilities.FormatOutputForAssertions(results[0])}");
-        results[1].ExitCode.Should().Be(0,
-            $"Second concurrent install failed with exit code {results[1].ExitCode}. Output:\n{DnupTestUtilities.FormatOutputForAssertions(results[1])}");
+        var results = await Task.WhenAll(installTask1, installTask2);
 
-        var installs = new List<DotnetInstall>();
+        results[0].exitCode.Should().Be(0,
+            $"First concurrent install failed with exit code {results[0].exitCode}. Output:\n{results[0].output}");
+        results[1].exitCode.Should().Be(0,
+            $"Second concurrent install failed with exit code {results[1].exitCode}. Output:\n{results[1].output}");
 
-        using (var finalizeLock = new ScopedMutex(Constants.MutexNames.ModifyInstallationStates))
+        string manifestMutexName = DnupTestUtilities.GetManifestMutexName(testEnv.ManifestPath);
+        using (var finalizeLock = new ScopedMutex(manifestMutexName))
         {
             var manifest = new DnupSharedManifest(testEnv.ManifestPath);
-            installs = manifest.GetInstalledVersions()
-                .Where(i => DnupUtilities.PathsEqual(i.InstallRoot.Path, testEnv.InstallPath))
-                .ToList();
-        }
-
-        int expectedInstallCount = string.Equals(firstResolved!.ToString(), secondResolved!.ToString(), StringComparison.OrdinalIgnoreCase) ? 1 : 2;
-        installs.Should().HaveCount(expectedInstallCount);
-
-        var expectedVersions = new HashSet<string>(StringComparer.OrdinalIgnoreCase)
-        {
-            firstResolved.ToString()!,
-            secondResolved!.ToString()!
-        };
-
-        foreach (var install in installs)
-        {
-            install.Component.Should().Be(InstallComponent.SDK);
-            expectedVersions.Should().Contain(install.Version.ToString());
-            DnupTestUtilities.ValidateInstall(install).Should().BeTrue(
-                $"ArchiveInstallationValidator failed for concurrent install {install.Version} at {testEnv.InstallPath}");
+            var installs = manifest.GetInstalledVersions().Where(i => DnupUtilities.PathsEqual(i.InstallRoot.Path, testEnv.InstallPath)).ToList();
+            installs.Should().ContainSingle();
+            installs[0].Version.ToString().Should().Be(channel);
         }
-
-        var actualVersions = installs.Select(i => i.Version.ToString()).ToHashSet(StringComparer.OrdinalIgnoreCase);
-        actualVersions.Should().BeEquivalentTo(expectedVersions);
     }
 }
diff --git a/test/dnup.Tests/Utilities/DnupTestUtilities.cs b/test/dnup.Tests/Utilities/DnupTestUtilities.cs

Original file line number	Diff line number	Diff line change
`@@ -49,7 +49,7 @@ private string GetManifestPath()`
`49`	`49`
`50`	`50`	`// Default location`
`51`	`51`	`return Path.Combine(`
`52`		`- Environment.GetFolderPath(Environment.SpecialFolder.UserProfile),`
	`52`	`+ RuntimeInformation.IsOSPlatform(OSPlatform.Windows) ? Environment.GetFolderPath(Environment.SpecialFolder.LocalApplicationData) : Environment.GetFolderPath(Environment.SpecialFolder.UserProfile),`
`53`	`53`	`"dnup",`
`54`	`54`	`"dnup_manifest.json");`
`55`	`55`	`}`