Improve dotnetup Muxer Handling, In-Use Muxer + Perf
#52696
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
When moving the .NET SDK build to use `dotnetup`, I hit several issues. The main issue is that the dotnet muxer may be in use when dotnetup tries to replace it, which would cause a failure. I don't think we want to fail in this case and instead we want to emit a warning. Maybe there should be an option to fail instead, however. What I also realized is that the approach of renaming the muxer even if it doesn't need to be replaced would cause the warning message to appear even if we didn't need to replace the muxer. Then I realized, we can do something clever. The windowsdesktop runtime has no runtime so we can skip muxer logic there. The other runtimes versions are the deciding factor for the muxer version since the muxer doesn't have its own version, so we can skip the resolution to find the version there. For the SDK, what we can do to avoid an extra rename of the existing muxer if it doesn't need to be replaced, is to extract out the new muxer to a different name temporarily, and then look and see if the latest runtime in the dotnet install root target changed after that install. If so then the runtime that came with the SDK is newer so the new muxer should be used. This lets us gracefully handle when the muxer is in use as well as avoid any extra work we don't need to do.
nagilson
changed the base branch from
nagilson-dnup-install-runtimes-impl
to
release/dnup
…etup-in-use-muxer
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates dotnetup’s install flow and the underlying installation library to handle muxer (dotnet/dotnet.exe) replacement more safely and with less unnecessary work—especially when the existing muxer is in use—while adding a switch to optionally fail instead of warning.
Changes:
- Add
--require-muxer-updateand plumb it throughdotnetupworkflows into the installation library. - Refactor archive extraction to extract the muxer to a temp path first, then decide post-extraction whether to replace the installed muxer.
- Add targeted tests covering muxer replacement decisions and “muxer in use” behavior.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| test/dotnetup.Tests/MuxerHandlerTests.cs | New unit tests for muxer replacement + locked-muxer scenarios |
| src/Installer/dotnetup/CommonOptions.cs | Adds --require-muxer-update option |
| src/Installer/dotnetup/Commands/Shared/InstallWorkflow.cs | Threads RequireMuxerUpdate through workflow options |
| src/Installer/dotnetup/Commands/Shared/InstallExecutor.cs | Propagates RequireMuxerUpdate into install request options |
| src/Installer/dotnetup/Commands/Sdk/Install/SdkInstallCommandParser.cs | Exposes + registers the new option for SDK installs |
| src/Installer/dotnetup/Commands/Sdk/Install/SdkInstallCommand.cs | Passes RequireMuxerUpdate into the workflow |
| src/Installer/dotnetup/Commands/Runtime/Install/RuntimeInstallCommandParser.cs | Registers the new option for runtime installs |
| src/Installer/dotnetup/Commands/Runtime/Install/RuntimeInstallCommand.cs | Intended to pass RequireMuxerUpdate, but currently contains unresolved conflict |
| src/Installer/Microsoft.Dotnet.Installation/Microsoft.Dotnet.Installation.csproj | Adds Spectre.Console dependency to installation library |
| src/Installer/Microsoft.Dotnet.Installation/Internal/MuxerHandler.cs | New muxer handling logic (temp extract + conditional replace + in-use handling) |
| src/Installer/Microsoft.Dotnet.Installation/Internal/DotnetInstall.cs | Adds RequireMuxerUpdate to install request options |
| src/Installer/Microsoft.Dotnet.Installation/Internal/DotnetArchiveExtractor.cs | Integrates MuxerHandler into tar/zip extraction |
src/Installer/dotnetup/Commands/Runtime/Install/RuntimeInstallCommand.cs
Outdated
Show resolved
Hide resolved
src/Installer/Microsoft.Dotnet.Installation/Internal/MuxerHandler.cs
Outdated
Show resolved
Hide resolved
src/Installer/Microsoft.Dotnet.Installation/Internal/MuxerHandler.cs
Outdated
Show resolved
Hide resolved
src/Installer/Microsoft.Dotnet.Installation/Microsoft.Dotnet.Installation.csproj
Outdated
Show resolved
Hide resolved
could go in a separate PR and is somewhat opinionated - but I think the intent is clear to install to a specific path.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #51691
This also addresses the exception raised in this issue #51689
When moving the .NET SDK build to use
dotnetupin nagilson#8, I hit several issues. The main issue is that the dotnet muxer may be in use when dotnetup tries to replace it, which would cause a failure.I don't think we want to fail in this case and instead we want to emit a warning. I've added an option to fail instead, however.
What I also realized is that the approach of renaming the muxer even if it doesn't need to be replaced would cause the warning message to appear even if we didn't need to replace the muxer. Then I realized, we can do something clever. The windowsdesktop runtime has no runtime so we can skip muxer logic there. The other runtimes versions are the deciding factor for the muxer version since the muxer doesn't have its own version, so we can skip the resolution to find the version there.
For the SDK, what we can do to avoid an extra rename of the existing muxer if it doesn't need to be replaced, is to extract out the new muxer to a different name temporarily, and then look and see if the latest runtime in the dotnet install root target changed after that install. If so then the runtime that came with the SDK is newer so the new muxer should be used. This lets us gracefully handle when the muxer is in use as well as avoid any extra work we don't need to do.
#52696 this will fail quite often until this other PR is merged due to a dotnetup bug.