dotnetup: classify PATH dotnet by managed hive; fix env config doc by dsplaisted · Pull Request #55349 · dotnet/sdk

dsplaisted · 2026-07-17T20:09:55Z

Follow-up to #54545 addressing review feedback (especially #54545 (comment)). Also includes improvement to some wording I noticed when testing.

Copilot description:

Changes

1. Classify the PATH dotnet by "is it a dotnetup-managed hive?" (not system vs user)

GetCurrentPathConfiguration previously returned an InstallType (User/System) derived from IsAdminInstallPath. But the consumers actually need to know whether the dotnet winning on PATH is a hive dotnetup owns — something it may run or uninstall from. Under the old axis, a dotnet that merely lives in a user-writable location (e.g. a hand-extracted C:\dotnet on PATH) was labeled User, so dotnetup dotnet and dotnetup uninstall (without --install-path) would run or target an install dotnetup does not own.

DotnetInstallRootConfiguration.InstallType → bool IsDotnetupHive.
IsDotnetupHive is true only when the resolved root is dotnetup's managed hive (today, the default install path; configurable hives tracked in dotnetup: Support custom dotnet roots #55346).
Consumers realigned: DotnetCommand / UninstallWorkflow act on the configured path only when it's the hive; InitWorkflows uses InstallPathClassifier.IsAdminInstallPath directly where it genuinely needs "is this a system install?"; install telemetry records the existing install's location bucket via ClassifyInstallPath.
Tests updated to the hive semantics, plus a new test locking in that an unmanaged user install falls back to the default hive.

Behavior change: dotnetup dotnet / uninstall no longer act on a non-managed dotnet that happens to win on PATH; they fall back to the managed hive.

2. Doc fix: `DotnetAccessMode` legacy spellings are not accepted

The dotnetup env design doc claimed the JSON converter accepts legacy spellings (full / fullpathreplacement). It does not — DotnetAccessModeJsonConverter is a plain camel-case string-enum converter (integer values rejected), and unrecognized values are treated as a corrupt config that re-defaults on next write. Corrected the doc to match the code and the rest of the doc.

Testing

Built dotnetup.Tests (Release); the affected UninstallWorkflowTests and DotnetCommandTests pass (23/23).

Specify whether the active path is the dotnetup managed hive rather than whether it is a system-installed path

azure-pipelines · 2026-07-17T20:10:11Z

Azure Pipelines: Successfully started running 2 pipeline(s). 1 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run.

Copilot

Pull request overview

This PR refines dotnetup’s notion of the dotnet that wins on PATH to distinguish dotnetup-managed hives from arbitrary user-writable installs, preventing dotnetup dotnet/uninstall from acting on installs it does not own. It also aligns documentation with actual DotnetAccessMode JSON parsing behavior and updates a user-facing env message (with localization updates).

Changes:

Replaces InstallType (User/System) classification from GetCurrentPathConfiguration with IsDotnetupHive and realigns DotnetCommand, UninstallWorkflow, and InitWorkflows accordingly.
Updates telemetry for “existing install type” to use InstallPathClassifier.ClassifyInstallPath buckets.
Updates tests and documentation; adjusts one env-related user-facing string and propagates it to XLFs.

Reviewed changes

Copilot reviewed 23 out of 23 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
test/dotnetup.Tests/UninstallWorkflowTests.cs	Updates uninstall path-resolution tests to reflect “dotnetup hive vs not” semantics and adds a regression test for unmanaged user installs.
test/dotnetup.Tests/DotnetCommandTests.cs	Updates `dotnetup dotnet` tests to gate use of the PATH-resolved root on `IsDotnetupHive`.
src/Installer/dotnetup.Library/xlf/Strings.zh-Hant.xlf	Updates localized resource unit for the changed env message.
src/Installer/dotnetup.Library/xlf/Strings.zh-Hans.xlf	Updates localized resource unit for the changed env message.
src/Installer/dotnetup.Library/xlf/Strings.tr.xlf	Updates localized resource unit for the changed env message.
src/Installer/dotnetup.Library/xlf/Strings.ru.xlf	Updates localized resource unit for the changed env message.
src/Installer/dotnetup.Library/xlf/Strings.pt-BR.xlf	Updates localized resource unit for the changed env message.
src/Installer/dotnetup.Library/xlf/Strings.pl.xlf	Updates localized resource unit for the changed env message.
src/Installer/dotnetup.Library/xlf/Strings.ko.xlf	Updates localized resource unit for the changed env message.
src/Installer/dotnetup.Library/xlf/Strings.ja.xlf	Updates localized resource unit for the changed env message.
src/Installer/dotnetup.Library/xlf/Strings.it.xlf	Updates localized resource unit for the changed env message.
src/Installer/dotnetup.Library/xlf/Strings.fr.xlf	Updates localized resource unit for the changed env message.
src/Installer/dotnetup.Library/xlf/Strings.es.xlf	Updates localized resource unit for the changed env message.
src/Installer/dotnetup.Library/xlf/Strings.de.xlf	Updates localized resource unit for the changed env message.
src/Installer/dotnetup.Library/xlf/Strings.cs.xlf	Updates localized resource unit for the changed env message.
src/Installer/dotnetup.Library/Strings.resx	Updates the source string shown after env changes so it’s clearer that restarts are required for other terminals/apps.
src/Installer/dotnetup.Library/IDotnetEnvironmentManager.cs	Updates the contract to return `DotnetInstallRootConfiguration` with `IsDotnetupHive` and adds clarifying XML docs.
src/Installer/dotnetup.Library/DotnetEnvironmentManager.cs	Implements hive classification by comparing the PATH-resolved root to the default dotnetup install path.
src/Installer/dotnetup.Library/Commands/Shared/UninstallWorkflow.cs	Ensures uninstall only targets the PATH-resolved root when it’s a dotnetup-managed hive.
src/Installer/dotnetup.Library/Commands/Shared/InstallWorkflow.cs	Records “existing install type” telemetry using path classification buckets rather than `InstallType`.
src/Installer/dotnetup.Library/Commands/Init/InitWorkflows.cs	Restores explicit “is system/admin install” checks where that’s the real requirement (migration display).
src/Installer/dotnetup.Library/Commands/Dotnet/DotnetCommand.cs	Ensures `dotnetup dotnet` only uses the PATH-resolved root when it’s the managed hive.
documentation/general/dotnetup/designs/dotnetup-env.md	Corrects the design doc to match actual JSON converter behavior (legacy spellings are not accepted).

nagilson · 2026-07-21T18:32:06Z

+        // a dotnetup hive is irrelevant here; we want its location only when it is a system install.
        var currentInstall = dotnetEnvironment.GetCurrentPathConfiguration();
-        string systemPath = currentInstall?.InstallType == InstallType.System
+        string systemPath = currentInstall is not null && InstallPathClassifier.IsAdminInstallPath(currentInstall.Path)


I think this should be using a method to check GetPathWinningDotnet rather than IsAdminInstallPath since we'd want to convert from whatever toolset was winning in the past but I acknowledge this might be worth doing in a separate issue.

After the init, you can use --migrate-from-system to go back and do the migration. I think it's simpler to keep parity with that. If we wanted to support more broader migration then we'd want to think about how it would work as a follow-up command too. I think it's not a mainline scenario so probably not worth worrying about, but feel free to file a follow-up issue if you prefer.

nagilson · 2026-07-21T18:36:28Z

        {
+            // Allow override for testing — lets tests point the managed hive at a temp directory
+            // without touching the real user profile.
+            var overridePath = Environment.GetEnvironmentVariable("DOTNET_TESTHOOK_DEFAULT_DOTNET_PATH");


Rather than make DOTNET_TESTHOOK_DEFAULT_DOTNET_PATH why don't we make this an explicit setting here such as DOTNETUP_HOME?

I think what we'd want for the mainline user scenario is to set this via the configuration, not via an environment variable. That's covered by #55346.

nagilson · 2026-07-21T18:38:00Z

  </data>
  <data name="EnvOpenNewTerminalForOtherSurfaces" xml:space="preserve">
-    <value>Open a new terminal for the change to take effect in other terminals and applications.</value>
+    <value>Other open terminals and applications will pick up this change after they're restarted.</value>


I think it's better to start with an action verb. Why was this changed? I'm on board with trying to make it clear this applies to apps & terminals earlier in the message.

Open a new terminal for the change to take effect in other terminals and applications.

This seems incorrect to me. To me it's telling you that opening a new terminal will cause existing terminals and applications to get the update.

Great point, I hadn't considered that. I lean slightly towards starting with a verb but good to move forward with this!

nagilson · 2026-07-21T18:47:04Z

+        // a dotnetup hive is irrelevant here; we want its location only when it is a system install.
        var currentInstall = dotnetEnvironment.GetCurrentPathConfiguration();
-        string systemPath = currentInstall?.InstallType == InstallType.System
+        string systemPath = currentInstall is not null && InstallPathClassifier.IsAdminInstallPath(currentInstall.Path)


nit: It'd be nice to have a regression test - e.g. mock the registry reading behavior to point to a non expected system install besides dotnetup and see if it uses the correct dotnet executable or not (one could be an exe that fails, the other returns 0)

I'm not following. This code just controls what is printed when its asking whether you want to migrate existing installs. Can you clarify?

I meant for this comment to be in the dotnetup dotnet section, so this was focused on adding a test for the executable selected there. While this would be nice to have, I think we should move forward on this fix.

OK, in that case I think we probably have the coverage you were looking for (copilot thought we did at least).

nagilson

Thanks for taking my feedback into account here, I appreciate the follow up as well from the prior PR!

nagilson · 2026-07-22T20:29:35Z

  </data>
  <data name="EnvOpenNewTerminalForOtherSurfaces" xml:space="preserve">
-    <value>Open a new terminal for the change to take effect in other terminals and applications.</value>
+    <value>Other open terminals and applications will pick up this change after they're restarted.</value>


Great point, I hadn't considered that. I lean slightly towards starting with a verb but good to move forward with this!

nagilson · 2026-07-22T20:34:03Z

+        // a dotnetup hive is irrelevant here; we want its location only when it is a system install.
        var currentInstall = dotnetEnvironment.GetCurrentPathConfiguration();
-        string systemPath = currentInstall?.InstallType == InstallType.System
+        string systemPath = currentInstall is not null && InstallPathClassifier.IsAdminInstallPath(currentInstall.Path)


nagilson · 2026-07-22T20:35:03Z

+        // a dotnetup hive is irrelevant here; we want its location only when it is a system install.
        var currentInstall = dotnetEnvironment.GetCurrentPathConfiguration();
-        string systemPath = currentInstall?.InstallType == InstallType.System
+        string systemPath = currentInstall is not null && InstallPathClassifier.IsAdminInstallPath(currentInstall.Path)


I meant for this comment to be in the dotnetup dotnet section, so this was focused on adding a test for the executable selected there. While this would be nice to have, I think we should move forward on this fix.

dsplaisted added 3 commits July 17, 2026 15:58

Change return of GetCurrentPathConfiguration

0fbc96c

Specify whether the active path is the dotnetup managed hive rather than whether it is a system-installed path

Fix outdated doc

83503e1

Improve EnvOpenNewTerminalForOtherSurfaces text

3134f0e

Copilot AI review requested due to automatic review settings July 17, 2026 20:09

Copilot started reviewing on behalf of dsplaisted July 17, 2026 20:10 View session

dsplaisted mentioned this pull request Jul 17, 2026

Dotnetup: add env command to update PathPreference mode #54545

Merged

Copilot AI reviewed Jul 17, 2026

View reviewed changes

Comment thread src/Installer/dotnetup.Library/Commands/Dotnet/DotnetCommand.cs

nagilson mentioned this pull request Jul 17, 2026

dotnetup system vs admin install detection may incorrectly classify install roots #55350

Open

Fix test and style warning

4d8f61a

dsplaisted mentioned this pull request Jul 21, 2026

dotnetup: Improve "everywhere" access mode #55360

Merged

nagilson reviewed Jul 21, 2026

View reviewed changes

Apply code review feedback

d4dad88

nagilson approved these changes Jul 22, 2026

View reviewed changes

dsplaisted merged commit 1be552c into dotnet:release/dnup Jul 22, 2026
28 checks passed

Uh oh!

Conversation

dsplaisted commented Jul 17, 2026

Changes

1. Classify the PATH dotnet by "is it a dotnetup-managed hive?" (not system vs user)

2. Doc fix: DotnetAccessMode legacy spellings are not accepted

Testing

Uh oh!

azure-pipelines Bot commented Jul 17, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nagilson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

2. Doc fix: `DotnetAccessMode` legacy spellings are not accepted