Dotnetup: add env command to update PathPreference mode#54545
Conversation
ba382b0 to
76aceb7
Compare
…for how to activate them
There was a problem hiding this comment.
Pull request overview
This PR reworks dotnetup’s environment configuration surface by replacing the legacy defaultinstall command family with a new env command family (env set/show/script/clear). It also refactors the underlying model to treat “dotnet access mode” and “dotnetup-on-PATH” as two orthogonal settings, adds drift/activation analysis, and updates tests and localized strings accordingly.
Changes:
- Introduces
dotnetup envcommands (set/show/script/clear) and keeps a hiddenprint-env-scriptalias for compatibility. - Replaces the old
PathPreferenceconfig withDotnetAccessMode+DotnetupOnPath, adds compatibility shims for legacy config shapes, and adds drift/activation logic. - Updates shell/profile wiring and adds/updates unit tests for the new behavior.
Reviewed changes
Copilot reviewed 65 out of 65 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| test/dotnetup.Tests/TestShellProvider.cs | Adds a reusable test shell provider for profile-entry tests. |
| test/dotnetup.Tests/ShellProfileManagerTests.cs | Updates profile-entry expectations to use env script and new flags. |
| test/dotnetup.Tests/ParserTests.cs | Updates parsing tests for the new env command surface (and legacy alias). |
| test/dotnetup.Tests/MockDotnetInstallManager.cs | Extends test double to track new env-application calls/flags. |
| test/dotnetup.Tests/InstallWorkflowTests.cs | Updates onboarding/config detection to the new config model. |
| test/dotnetup.Tests/InitWorkflowTests.cs | Updates init workflow tests to use DotnetAccessMode. |
| test/dotnetup.Tests/FakeEnvironmentStateInspector.cs | Adds a test double for observed environment state. |
| test/dotnetup.Tests/EnvShellProviderTests.cs | Updates provider tests to env script/new activation behavior. |
| test/dotnetup.Tests/EnvSettingsApplierTests.cs | Adds tests for applying/removing composed env settings. |
| test/dotnetup.Tests/EnvScriptSelectionResolverTests.cs | Adds tests for env script selection-flag resolution. |
| test/dotnetup.Tests/EnvDriftAnalyzerTests.cs | Adds tests for configured-vs-observed drift reporting. |
| test/dotnetup.Tests/EnvCommandTests.cs | Adds behavioral tests for env set/clear/show. |
| test/dotnetup.Tests/EnvActivationStatusTests.cs | Adds tests for “is it active in this terminal?” evaluation. |
| test/dotnetup.Tests/DotnetupConfigTests.cs | Updates config tests for new schema + legacy read-compat. |
| test/dotnetup.Tests/DefaultInstallCommandTests.cs | Removes tests for the deleted defaultinstall command. |
| src/Installer/dotnetup.Library/xlf/Strings.zh-Hant.xlf | Updates localization entries for renamed mode strings. |
| src/Installer/dotnetup.Library/xlf/Strings.zh-Hans.xlf | Updates localization entries for renamed mode strings. |
| src/Installer/dotnetup.Library/xlf/Strings.tr.xlf | Updates localization entries for renamed mode strings. |
| src/Installer/dotnetup.Library/xlf/Strings.ru.xlf | Updates localization entries for renamed mode strings. |
| src/Installer/dotnetup.Library/xlf/Strings.pt-BR.xlf | Updates localization entries for renamed mode strings. |
| src/Installer/dotnetup.Library/xlf/Strings.pl.xlf | Updates localization entries for renamed mode strings. |
| src/Installer/dotnetup.Library/xlf/Strings.ko.xlf | Updates localization entries for renamed mode strings. |
| src/Installer/dotnetup.Library/xlf/Strings.ja.xlf | Updates localization entries for renamed mode strings. |
| src/Installer/dotnetup.Library/xlf/Strings.it.xlf | Updates localization entries for renamed mode strings. |
| src/Installer/dotnetup.Library/xlf/Strings.fr.xlf | Updates localization entries for renamed mode strings. |
| src/Installer/dotnetup.Library/xlf/Strings.es.xlf | Updates localization entries for renamed mode strings. |
| src/Installer/dotnetup.Library/xlf/Strings.de.xlf | Updates localization entries for renamed mode strings. |
| src/Installer/dotnetup.Library/xlf/Strings.cs.xlf | Updates localization entries for renamed mode strings. |
| src/Installer/dotnetup.Library/Strings.resx | Renames mode-related resources to match the new model. |
| src/Installer/dotnetup.Library/Shell/ZshEnvShellProvider.cs | Updates profile/activation generation signatures/behavior. |
| src/Installer/dotnetup.Library/Shell/ShellProviderHelpers.cs | Updates helpers for new flag model and activation commands. |
| src/Installer/dotnetup.Library/Shell/ShellProfileManager.cs | Adds “find profiles with managed block” helper + new entry flags. |
| src/Installer/dotnetup.Library/Shell/PowerShellEnvShellProvider.cs | Updates profile/activation generation signatures/behavior. |
| src/Installer/dotnetup.Library/Shell/IEnvShellProvider.cs | Updates provider API to reflect new selection-flag model. |
| src/Installer/dotnetup.Library/Shell/BashEnvShellProvider.cs | Updates profile/activation generation signatures/behavior. |
| src/Installer/dotnetup.Library/Parser.cs | Wires up new env commands and keeps hidden legacy alias. |
| src/Installer/dotnetup.Library/ObservedEnvironmentState.cs | Adds a model for observed “real machine” env wiring state. |
| src/Installer/dotnetup.Library/IEnvironmentStateInspector.cs | Adds abstraction for inspecting live environment state. |
| src/Installer/dotnetup.Library/IDotnetEnvironmentManager.cs | Extends environment manager API for composed settings. |
| src/Installer/dotnetup.Library/EnvTerminalState.cs | Adds model for terminal activation/removal needs. |
| src/Installer/dotnetup.Library/EnvSettingsApplier.cs | Adds composed apply/remove logic driven by observed state. |
| src/Installer/dotnetup.Library/EnvironmentStateInspector.cs | Implements live environment inspection into observed state. |
| src/Installer/dotnetup.Library/EnvActivationStatus.cs | Adds logic to evaluate “active in current process PATH”. |
| src/Installer/dotnetup.Library/EnvActivationCommandBuilder.cs | Builds paste-able activation commands based on shell/config. |
| src/Installer/dotnetup.Library/DotnetupConfig.cs | Replaces PathPreference with DotnetAccessMode + compatibility shims. |
| src/Installer/dotnetup.Library/DotnetEnvironmentManager.cs | Updates profile/env-var wiring and adds dotnetup user-PATH support. |
| src/Installer/dotnetup.Library/DotnetAccessModeJsonConverter.cs | Adds custom serialization/legacy read compat for access mode. |
| src/Installer/dotnetup.Library/Commands/Shared/InstallWorkflow.cs | Updates onboarding gating to the new config field. |
| src/Installer/dotnetup.Library/Commands/PrintEnvScript/PrintEnvScriptCommandParser.cs | Removes the old print-env-script parser (now under env script). |
| src/Installer/dotnetup.Library/Commands/Init/InitWorkflows.cs | Updates init flow prompts/config/apply to the new env model. |
| src/Installer/dotnetup.Library/Commands/Env/EnvShowCommandParser.cs | Adds parser for env show. |
| src/Installer/dotnetup.Library/Commands/Env/EnvShowCommand.cs | Adds env show implementation (config display + drift/activation). |
| src/Installer/dotnetup.Library/Commands/Env/EnvSettingsWriter.cs | Shared apply/persist path for env set + env clear. |
| src/Installer/dotnetup.Library/Commands/Env/EnvSetCommandParser.cs | Adds parser for env set (mode + dotnetup-on-path). |
| src/Installer/dotnetup.Library/Commands/Env/EnvSetCommand.cs | Adds env set implementation (re-sync semantics). |
| src/Installer/dotnetup.Library/Commands/Env/EnvScriptSelectionResolver.cs | Adds pure selection resolution for env script. |
| src/Installer/dotnetup.Library/Commands/Env/EnvScriptCommandParser.cs | Adds parser for env script (shell/install-path/selection flags). |
| src/Installer/dotnetup.Library/Commands/Env/EnvScriptCommand.cs | Updates script command to use config-driven selection by default. |
| src/Installer/dotnetup.Library/Commands/Env/EnvDriftAnalyzer.cs | Adds pure drift analyzer for configured vs observed state. |
| src/Installer/dotnetup.Library/Commands/Env/EnvCommandParser.cs | Adds the env command root and subcommands. |
| src/Installer/dotnetup.Library/Commands/Env/EnvClearCommandParser.cs | Adds parser for env clear. |
| src/Installer/dotnetup.Library/Commands/Env/EnvClearCommand.cs | Adds env clear implementation. |
| src/Installer/dotnetup.Library/Commands/DefaultInstall/DefaultInstallCommandParser.cs | Removes the old defaultinstall parser. |
| src/Installer/dotnetup.Library/Commands/DefaultInstall/DefaultInstallCommand.cs | Removes the old defaultinstall command implementation. |
nagilson
left a comment
There was a problem hiding this comment.
A quick pass through today, but I'll have more feedback soon. Let me know if you'd prefer to wait for a full review pass for me to submit any feedbacl.
…tup-path-mode # Conflicts: # src/Installer/Microsoft.Dotnet.Installation/DotnetInstallException.cs # src/Installer/dotnetup.Library/Commands/Init/InitWorkflows.cs # src/Installer/dotnetup.Library/Strings.resx # src/Installer/dotnetup.Library/xlf/Strings.cs.xlf # src/Installer/dotnetup.Library/xlf/Strings.de.xlf # src/Installer/dotnetup.Library/xlf/Strings.es.xlf # src/Installer/dotnetup.Library/xlf/Strings.fr.xlf # src/Installer/dotnetup.Library/xlf/Strings.it.xlf # src/Installer/dotnetup.Library/xlf/Strings.ja.xlf # src/Installer/dotnetup.Library/xlf/Strings.ko.xlf # src/Installer/dotnetup.Library/xlf/Strings.pl.xlf # src/Installer/dotnetup.Library/xlf/Strings.pt-BR.xlf # src/Installer/dotnetup.Library/xlf/Strings.ru.xlf # src/Installer/dotnetup.Library/xlf/Strings.tr.xlf # src/Installer/dotnetup.Library/xlf/Strings.zh-Hans.xlf # src/Installer/dotnetup.Library/xlf/Strings.zh-Hant.xlf # test/dotnetup.Tests/DefaultInstallCommandTests.cs # test/dotnetup.Tests/DotnetupConfigTests.cs # test/dotnetup.Tests/EnvShellProviderTests.cs # test/dotnetup.Tests/InitWorkflowTests.cs # test/dotnetup.Tests/ParserTests.cs # test/dotnetup.Tests/ShellProfileManagerTests.cs
nagilson
left a comment
There was a problem hiding this comment.
Thanks for applying those changes; still looking at the PR but wanted to send what I have for now.
| } | ||
| } | ||
|
|
||
| if (OperatingSystem.IsWindows()) |
There was a problem hiding this comment.
I worry that this adds another maintenance point about only supporting 'full' path on windows - which I think will change shortly since we need to support mac gui apps and linux gui apps not launched from the shell. I would suggest changing this pattern.
There was a problem hiding this comment.
This code could be made more future-proof, but I'm pretty sure it works as is and changing it would add some risk of breaking it. Plus, I don't know what "everywhere" mode on non-Windows will look like, and we might still end up with OS-specific code in the end. So I'd prefer to leave it as is for now.
There was a problem hiding this comment.
Totally understandable, let's keep this as is.
There was a problem hiding this comment.
Thanks for your patience in review. I've completed a full pass of everything now.
The feedback is varied; many nitpicks but a few actual concerns of potential bugs I found. Let me know if there's anything I can clarify better. I tried to evaluate the feature e2e which also meant looking at the install root manager & env provider even though they aren't changed as much in this PR but happy to move any of that to an issue if you think that's best.
I'm excited to get this feature out the door as I think it will provide a lot of value! Looking forward to your replies.
Was looking at registry for Windows regardless of whether the current PATH pointed to the system install, leading to inconsistencies
nagilson
left a comment
There was a problem hiding this comment.
Thank you for diligently responding, then waiting for my final review! ![]()
I have two more potential bug/behavioral concerns that I think are worth calling out but they can likely be addressed in another PR.
| // Classify the resolved dotnet by location. InstallPathClassifier.IsAdminInstallPath is the | ||
| // canonical "is this a system/admin-managed install?" check (Program Files on Windows; the | ||
| // standard /usr and /opt locations on Unix) used across dotnetup, so both platforms share it. | ||
| bool isAdminInstall = InstallPathClassifier.IsAdminInstallPath(currentInstallRoot.Path); |
There was a problem hiding this comment.
I think we'll probably want to split the original method here up into two separate methods - or, at least 2 separate concepts.
GetPathWinningDotnet VS IsAdminInstallPath is not the same.
The problem that got fixed before was that the shell made dotnetup's dotnet win on the PATH so it would be labeled as System.
But now I think we have the opposite potential problem. If the admin path / reg key value is set to something outside of the expected program files location, then it would be classified as a user install path. I believe this makes dotnetup dotnet use the system PATH and also makes uninstall workflow try to uninstall a system PATH.
There was a problem hiding this comment.
I didn't want to reset the approval so I created a follow-up PR to address this: #55349
|
|
||
| var userChanges = installRootManager.GetUserInstallRootChanges(); | ||
| bool succeeded = InstallRootManager.ApplyUserInstallRoot( | ||
| InstallRootManager.ApplyUserInstallRoot( |
There was a problem hiding this comment.
GetUserInstallRootChanges does not take in the dotnetRoot parameter, which I believe means that everywhere mode will not work and will ignore a custom root.
There was a problem hiding this comment.
This led me to a broader design issue around how we handle the install path. I've filed #55346 for how I think it should work.
Replace
dotnetup defaultinstall <user|system>with a noun-verbenvcommand familybacked by a renamed three-value
PathPreferenceenum:env setis idempotent — running it again with the same mode is a no-op (and is alsohow you re-sync after something else has clobbered your PATH; see the key scenario
below).
Modes:
none— Don't modify your environment; usedotnetup dotnetto invoke.shell— Wire dotnetup into your shell's profile file (only shells that loadprofiles see the user dotnet).
everywhere— Wire dotnetup into your shell profile and your user-levelPATH/DOTNET_ROOT so cmd.exe, IDEs, and shortcuts also see it.
You can also configure whether dotnetup itself should be added to the path with
--dotnetup-on-path trueor--dotnetup-on-path falseas parameters toenv set.Some sample outputs: