-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Fix muxer version handling to use runtime 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
Changes from 1 commit
452ee7b
32e7fc2
7b7cd50
665d370
0c5e201
f16e380
39a5412
c8647dc
3a81ab6
5d66e54
1c467b5
a5b5556
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
- Remove try-catch blocks from muxer rename/restore operations (let exceptions escape) - Remove try-catch from GetLatestRuntimeVersionFromInstallRoot - Fix muxer version comparison: use GetRuntimeVersionFromArchive instead of GetLatestRuntimeVersionFromInstallRoot after extraction - Add back GetRuntimeVersionFromArchive and helper methods to read runtime version from tar/zip archives - Update tests to check actual muxer file (hash and size) instead of runtime versions - Add GetFileHash helper method in tests Co-authored-by: dsplaisted <[email protected]>
- Loading branch information
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -90,18 +90,11 @@ private void ExtractArchiveDirectlyToTarget(string archivePath, string targetDir | |||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Step 2: If there is an existing muxer, rename it to .tmp | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (hadExistingMuxer) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| try | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (File.Exists(muxerTempPath)) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (File.Exists(muxerTempPath)) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| File.Delete(muxerTempPath); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| File.Move(muxerTargetPath, muxerTempPath); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| catch | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // If we can't rename, just continue - extraction will overwrite | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| File.Delete(muxerTempPath); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| File.Move(muxerTargetPath, muxerTempPath); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Step 3: Extract the archive (all files directly since muxer has been renamed) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -117,37 +110,24 @@ private void ExtractArchiveDirectlyToTarget(string archivePath, string targetDir | |||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Step 4: If there was a previous muxer, compare versions and restore if needed | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (hadExistingMuxer && File.Exists(muxerTempPath)) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Version? newMuxerVersion = GetLatestRuntimeVersionFromInstallRoot(targetDir); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Get the runtime version from the archive we just extracted | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Version? newMuxerVersion = GetRuntimeVersionFromArchive(archivePath); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // If the new version is less than the old version, restore the old muxer | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (newMuxerVersion != null && existingMuxerVersion != null && newMuxerVersion < existingMuxerVersion) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| try | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (File.Exists(muxerTargetPath)) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| File.Delete(muxerTargetPath); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| File.Move(muxerTempPath, muxerTargetPath); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| catch | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (File.Exists(muxerTargetPath)) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // If we can't restore, the new muxer will remain | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| File.Delete(muxerTargetPath); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| File.Move(muxerTempPath, muxerTargetPath); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| else | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // New version is >= old version, or we couldn't determine versions - keep new muxer | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| try | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (File.Exists(muxerTempPath)) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| File.Delete(muxerTempPath); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| catch | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (File.Exists(muxerTempPath)) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Ignore cleanup errors | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| File.Delete(muxerTempPath); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -158,33 +138,144 @@ private void ExtractArchiveDirectlyToTarget(string archivePath, string targetDir | |||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// </summary> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| private static Version? GetLatestRuntimeVersionFromInstallRoot(string installRoot) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| try | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| var runtimePath = Path.Combine(installRoot, "shared", "Microsoft.NETCore.App"); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @copilot What happens if the aspnetcore runtime is newer than the regular runtime that a user had installed here? I don't think we can say for certain, but calling the dotnet host apis directly may be better here as opposed to enumerating over the directories. We are also trusting the directory name here, which in the event the user renames the folder, we will have incorrect information.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You're right that directory enumeration has limitations. The current approach:
However, this is the approach requested by @dsplaisted in the earlier discussion. The muxer version is specifically tied to the Calling dotnet host APIs would be more robust, but would require additional dependencies and complexity. If you'd like to pursue that approach, please coordinate with @dsplaisted on the preferred solution since this was the agreed-upon design. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (!Directory.Exists(runtimePath)) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| var runtimePath = Path.Combine(installRoot, "shared", "Microsoft.NETCore.App"); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (!Directory.Exists(runtimePath)) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return null; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Version? highestVersion = null; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| foreach (var dir in Directory.GetDirectories(runtimePath)) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| var versionString = Path.GetFileName(dir); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (Version.TryParse(versionString, out Version? version)) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (highestVersion == null || version > highestVersion) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| highestVersion = version; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return highestVersion; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// <summary> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// Gets the runtime version from an archive by examining the runtime directories. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// </summary> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| private static Version? GetRuntimeVersionFromArchive(string archivePath) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (archivePath.EndsWith(".zip", StringComparison.OrdinalIgnoreCase)) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| using var zip = ZipFile.OpenRead(archivePath); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return GetRuntimeVersionFromZipEntries(zip.Entries); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| else if (archivePath.EndsWith(".tar.gz", StringComparison.OrdinalIgnoreCase) || archivePath.EndsWith(".tar", StringComparison.OrdinalIgnoreCase)) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| string tarPath = archivePath; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| bool needsCleanup = false; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (archivePath.EndsWith(".gz", StringComparison.OrdinalIgnoreCase)) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return null; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| tarPath = DecompressTarGzToTemp(archivePath); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| needsCleanup = true; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Version? highestVersion = null; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| foreach (var dir in Directory.GetDirectories(runtimePath)) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| try | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| var versionString = Path.GetFileName(dir); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (Version.TryParse(versionString, out Version? version)) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| using var tarStream = File.OpenRead(tarPath); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| using var tarReader = new TarReader(tarStream); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return GetRuntimeVersionFromTarEntries(tarReader); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| finally | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (needsCleanup && File.Exists(tarPath)) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (highestVersion == null || version > highestVersion) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| File.Delete(tarPath); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return null; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// <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"); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 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; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 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); |
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.
@copilot What if we don't have permission to delete that file? We should also try to follow a 'RAII' pattern which deletes/renames the file back when we return out of this code block, because right now if an exception is thrown or the program is aborted during the time hereafter, we may not properly rename the file back or clean it up.
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.
Added exception handling for permission errors when deleting the .tmp file (catches UnauthorizedAccessException and IOException). Also wrapped the extraction and version comparison in a try-catch block that follows RAII pattern - if an exception occurs, it restores the original muxer from .tmp before re-throwing. If we can't delete the stale .tmp file, the code gracefully degrades to normal extraction without version comparison.
Commit: 1c467b5