-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Fix muxer version handling to compare file versions instead of SDK versions #51971
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: release/dnup
Are you sure you want to change the base?
Conversation
…rsions Co-authored-by: dsplaisted <145043+dsplaisted@users.noreply.github.com>
Co-authored-by: dsplaisted <145043+dsplaisted@users.noreply.github.com>
| /// <summary> | ||
| /// Gets the file version of a muxer executable. | ||
| /// </summary> | ||
| internal static Version? GetMuxerFileVersion(string muxerPath) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JeremyKuhne Copilot wrote this. Does the logic make sense for getting a file version to find out which version is newer? What's the difference between FileVersionInfo.FileVersion and the FileMajorPart etc. properties?
dsplaisted
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove the tests you added in MuxerVersionHandlingTests.cs. I don't think unit-level tests of this are going to be very valuable.
Instead, add two tests to LibraryTests that follow the pattern of those tests. One test should install .NET SDK 9.0 and then 10.0, and verify that the file version of the muxer has been updated. The other test should install 10.0 and then 9.0 and verify that the muxer version isn't updated.
- Remove MuxerVersionHandlingTests.cs as requested - Add two integration tests to LibraryTests: - MuxerIsUpdated_WhenInstallingNewerSdk: verifies muxer is upgraded when installing SDK 10 after SDK 9 - MuxerIsNotDowngraded_WhenInstallingOlderSdk: verifies muxer is not downgraded when installing SDK 9 after SDK 10 - Tests skip on non-Windows platforms since FileVersionInfo doesn't work with ELF binaries Co-authored-by: dsplaisted <145043+dsplaisted@users.noreply.github.com>
Done. Removed MuxerVersionHandlingTests.cs and added two integration tests to LibraryTests:
Both tests skip on non-Windows since FileVersionInfo doesn't extract version info from ELF binaries. Commit: 665d370 |
dsplaisted
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This logic also needs to work on Linux and Mac, where the file version info is not available.
On those platforms, assume that the version of the muxer corresponds to the version of the runtime included in the archive or the latest runtime installed in the dotnet install root folder. The latest runtime version will be the highest version folder under shared\Microsoft.NETCore.App in the install root.
When this is done, update the tests you added to run on all platforms.
| /// </summary> | ||
| private void HandleMuxerFromZip(ZipArchiveEntry entry, string muxerTargetPath) | ||
| { | ||
| var tempMuxerPath = Path.Combine(Directory.CreateTempSubdirectory().FullName, entry.Name); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like we should move the temp directory creation and extraction to happen if and only if there already exists a muxer, so we avoid this unnecessary work to compare muxer versions.
| /// <summary> | ||
| /// Handles the muxer from a tar entry, comparing file versions to determine if update is needed. | ||
| /// </summary> | ||
| private void HandleMuxerFromTar(TarEntry entry, string muxerTargetPath) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a generic Entry class that can be used for TarEntry and ZipEntry? I don't see why this logic needs to be different based on the archive type.
- Update GetMuxerFileVersion to fallback to runtime version detection on non-Windows platforms - Add GetRuntimeVersionFromArchive to extract runtime version from tar/zip archives - Add GetLatestRuntimeVersionFromInstallRoot to get highest runtime version from shared/Microsoft.NETCore.App - Update ShouldUpdateMuxer to accept archive path and install root for runtime version fallback - Pass archive and install root context through MuxerHandlingConfig - Remove Windows-only test restrictions - tests now run on all platforms Co-authored-by: dsplaisted <145043+dsplaisted@users.noreply.github.com>
Done. The muxer version handling now works on all platforms: On Windows: Uses On Linux/Mac: Falls back to runtime version detection when FileVersionInfo returns null:
Tests updated to run on all platforms without skipping. Commit: 0c5e201 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like we should move the temp directory creation and extraction to happen if and only if there already exists a muxer, so we avoid this unnecessary work to compare muxer versions.
Is there a generic Entry class that can be used for TarEntry and ZipEntry? I don't see why this logic needs to be different based on the archive type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This pull request fixes a critical bug in muxer version handling where SDK versions were incorrectly compared to determine muxer updates. The muxer version is tied to runtime version, not SDK version, so the fix replaces SDK version comparison with actual file version comparison using PE file metadata on Windows and runtime version detection as a fallback on Linux/Mac.
Key Changes
- Replaced flawed SDK comparison logic with file version comparison using
FileVersionInfo.GetVersionInfoon Windows and runtime version detection from archives/install root on Linux/Mac - Added robust version detection methods that scan
shared/Microsoft.NETCore.App/{version}directories in archives and install roots to determine runtime versions - Added comprehensive integration tests to verify muxer is upgraded when installing newer SDKs and not downgraded when installing older SDKs
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
src/Installer/Microsoft.Dotnet.Installation/Internal/DotnetArchiveExtractor.cs |
Core fix: removed ConfigureMuxerHandling method and added ShouldUpdateMuxer, GetMuxerFileVersion, and runtime version detection methods; updated tar/zip extraction to use version comparison |
test/dotnetup.Tests/LibraryTests.cs |
Added two integration tests to verify muxer upgrade/no-downgrade behavior when installing SDKs in different orders |
test/dotnetup.Tests/Utilities/UpdateChannelExtensions.cs |
Minor: alphabetized using statements |
test/dotnetup.Tests/DnupE2Etest.cs |
Minor: alphabetized using statements and formatting adjustments |
Comments suppressed due to low confidence (1)
test/dotnetup.Tests/LibraryTests.cs:10
- Missing required using directives for test framework. Add
using Xunit;andusing FluentAssertions;to support the[Fact]attributes and.Should()assertion methods used in the test methods below.
using System;
using System.Collections.Generic;
using System.IO;
using System.Text;
using Microsoft.Dotnet.Installation;
using Microsoft.Dotnet.Installation.Internal;
using Microsoft.DotNet.Tools.Dotnetup.Tests.Utilities;
| catch | ||
| { |
Copilot
AI
Dec 2, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Silent exception swallowing: The catch block silently suppresses all exceptions without any logging or diagnostic information. Consider adding logging or at least a comment explaining why exceptions are being suppressed. This can make debugging issues with version detection difficult in production.
| catch | |
| { | |
| catch (Exception ex) | |
| { | |
| // Swallowing exceptions here because failure to detect a runtime version is not always exceptional, | |
| // but log the exception for diagnostic purposes. | |
| Debug.WriteLine($"Failed to get latest runtime version from install root '{installRoot}': {ex}"); |
| private static string DecompressTarGzToTemp(string gzPath) | ||
| { | ||
| string tempPath = Path.Combine(Path.GetTempPath(), $"dotnet-{Guid.NewGuid()}.tar"); | ||
| using FileStream originalFileStream = File.OpenRead(gzPath); | ||
| using FileStream decompressedFileStream = File.Create(tempPath); | ||
| using GZipStream decompressionStream = new GZipStream(originalFileStream, CompressionMode.Decompress); | ||
| decompressionStream.CopyTo(decompressedFileStream); | ||
| return tempPath; |
Copilot
AI
Dec 2, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Code duplication: DecompressTarGzToTemp duplicates the decompression logic from DecompressTarGzIfNeeded. Consider extracting the core decompression logic into a shared private method to improve maintainability and reduce duplication.
| private static string DecompressTarGzToTemp(string gzPath) | |
| { | |
| string tempPath = Path.Combine(Path.GetTempPath(), $"dotnet-{Guid.NewGuid()}.tar"); | |
| using FileStream originalFileStream = File.OpenRead(gzPath); | |
| using FileStream decompressedFileStream = File.Create(tempPath); | |
| using GZipStream decompressionStream = new GZipStream(originalFileStream, CompressionMode.Decompress); | |
| decompressionStream.CopyTo(decompressedFileStream); | |
| return tempPath; | |
| /// <summary> | |
| /// Decompresses a .tar.gz file to a temporary location. | |
| /// </summary> | |
| private static string DecompressTarGzToTemp(string gzPath) | |
| { | |
| string tempPath = Path.Combine(Path.GetTempPath(), $"dotnet-{Guid.NewGuid()}.tar"); | |
| DecompressTarGz(gzPath, tempPath); | |
| return tempPath; | |
| } | |
| /// <summary> | |
| /// Decompresses a .tar.gz file to the specified destination path. | |
| /// </summary> | |
| private static void DecompressTarGz(string sourcePath, string destinationPath) | |
| { | |
| using FileStream originalFileStream = File.OpenRead(sourcePath); | |
| using FileStream decompressedFileStream = File.Create(destinationPath); | |
| using GZipStream decompressionStream = new GZipStream(originalFileStream, CompressionMode.Decompress); | |
| decompressionStream.CopyTo(decompressedFileStream); |
| if (versionInfo.FileMajorPart > 0 || versionInfo.FileMinorPart > 0) | ||
| { | ||
| return new Version( | ||
| versionInfo.FileMajorPart, | ||
| versionInfo.FileMinorPart, | ||
| versionInfo.FileBuildPart, | ||
| versionInfo.FilePrivatePart); | ||
| } |
Copilot
AI
Dec 2, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potentially incorrect fallback condition: The check versionInfo.FileMajorPart > 0 || versionInfo.FileMinorPart > 0 will skip valid version 0.0.x.y. Consider changing the condition to check if any part is non-zero, or if the FileVersion property exists but failed to parse, to handle edge cases more robustly. For example: if (versionInfo.FileMajorPart != 0 || versionInfo.FileMinorPart != 0 || versionInfo.FileBuildPart != 0 || versionInfo.FilePrivatePart != 0)
| } | ||
| catch | ||
| { | ||
| // If we can't read the archive, return null |
Copilot
AI
Dec 2, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Silent exception swallowing: The catch block silently suppresses all exceptions without any logging or diagnostic information. Consider adding logging or at least a comment explaining why exceptions are being suppressed. This can make debugging issues with version detection difficult in production.
| // If we can't read the archive, return null | |
| // Intentionally suppressing all exceptions here because failure to read the archive | |
| // should result in a null version (no version found). If debugging issues with version | |
| // detection, consider adding logging here to capture exception details. | |
| // Example: Console.WriteLine($"Failed to read archive '{archivePath}': {ex}"); | |
| // (Logging is omitted here to avoid introducing dependencies.) |
| catch | ||
| { | ||
| // On error, try fallback for non-Windows | ||
| if (!RuntimeInformation.IsOSPlatform(OSPlatform.Windows) && contextPath != null) | ||
| { | ||
| return GetRuntimeVersionFromContext(contextPath); | ||
| } | ||
| return null; | ||
| } |
Copilot
AI
Dec 2, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Silent exception swallowing: The catch block silently suppresses all exceptions without any logging or diagnostic information. Consider adding logging or at least a comment explaining why exceptions are being suppressed. This can make debugging issues with version detection difficult in production.
DotnetArchiveExtractor.ConfigureMuxerHandlingwas incorrectly comparing SDK versions to determine whether to update the muxer (dotnet.exe). The muxer version is tied to runtime version, not SDK version, so this comparison was fundamentally broken.Changes
ConfigureMuxerHandling- deleted the flawed SDK version comparison logicShouldUpdateMuxer(newMuxerPath, existingMuxerPath, archivePath, installRoot)- compares actual file versions usingFileVersionInfo.GetVersionInfoon Windows, with fallback to runtime version detection on Linux/MacGetMuxerFileVersion(muxerPath, contextPath)- extracts version from PE file metadata on Windows, falls back to runtime version on Linux/MacGetRuntimeVersionFromArchive- extracts runtime version from tar/zip archives by scanningshared/Microsoft.NETCore.App/{version}directoriesGetLatestRuntimeVersionFromInstallRoot- gets the highest runtime version from the install root'sshared/Microsoft.NETCore.AppdirectoryThis ensures installing .NET 10 then .NET 9 won't downgrade the muxer, while installing .NET 9 then .NET 10 will correctly upgrade it.
Platform Support
Windows: Uses
FileVersionInfo.GetVersionInfoto read the PE file version from the muxer executable.Linux/Mac: When FileVersionInfo returns null (ELF binaries), falls back to runtime version detection:
Tests
Added two integration tests to
LibraryTests.cs:MuxerIsUpdated_WhenInstallingNewerSdk: Installs SDK 9.0 then 10.0, verifies muxer version increasesMuxerIsNotDowngraded_WhenInstallingOlderSdk: Installs SDK 10.0 then 9.0, verifies muxer version stays the sameTests run on all platforms (Windows, Linux, Mac).
Fixes #51691
Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.