Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions documentation/general/dotnetup/designs/dotnetup-env.md
Original file line number Diff line number Diff line change
Expand Up @@ -154,9 +154,11 @@ DotnetAccessMode.Shell = old ShellProfile // shell profile file onl
DotnetAccessMode.Everywhere = old FullPathReplacement // env-var PATH + shell profile
```

The JSON serialization writes the new lowercase names (`none` / `shell` / `everywhere`).
On read, the converter also accepts the legacy spellings (`full`, `fullpathreplacement`,
etc.) so configs written by earlier internal builds keep their chosen mode.
The JSON serialization writes the new lowercase names (`none` / `shell` / `everywhere`)
via the built-in string-enum converter (camel-case, integer values rejected). Legacy
spellings from earlier internal builds (`full` / `fullpathreplacement`, and the old
`pathPreference` property) are **not** accepted: an unrecognized value is treated as a
corrupt config, which re-defaults on the next write rather than silently mapping to a mode.

## `dotnetup` on PATH: an orthogonal setting

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ protected override void ExecuteCore()
private string ResolveDotnetPath()
{
var configuredRoot = _dotnetEnvironment.GetCurrentPathConfiguration();
if (configuredRoot is not null && configuredRoot.InstallType == InstallType.User)
if (configuredRoot is { IsDotnetupHive: true })
Comment thread
dsplaisted marked this conversation as resolved.
{
return configuredRoot.Path;
}
Expand Down
5 changes: 3 additions & 2 deletions src/Installer/dotnetup.Library/Commands/Init/InitWorkflows.cs
Original file line number Diff line number Diff line change
Expand Up @@ -446,9 +446,10 @@ internal static List<string> FormatMigrationDisplayItems(List<MigrationWorkflow.
return [];
}

// Find the system install path for display purposes
// Find the system install path for display purposes. Whether the dotnet winning on PATH is
// 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)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

? currentInstall.Path
: DotnetEnvironmentManager.GetSystemDotnetPaths().FirstOrDefault() ?? "the system .NET location";

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -338,7 +338,7 @@ private void RecordInstallTelemetry(
_command.SetCommandTag(TelemetryTagNames.InstallRequestedVersion, VersionSanitizer.Sanitize(requestedVersionOrChannel));
_command.SetCommandTag(TelemetryTagNames.InstallPathExplicit, (explicitInstallPath is not null).ToString());
_command.SetCommandTag(TelemetryTagNames.InstallHasGlobalJson, (globalJson?.GlobalJsonPath is not null).ToString());
_command.SetCommandTag(TelemetryTagNames.InstallExistingInstallType, currentInstallRoot?.InstallType.ToString() ?? "none");
_command.SetCommandTag(TelemetryTagNames.InstallExistingInstallType, currentInstallRoot is null ? "none" : InstallPathClassifier.ClassifyInstallPath(currentInstallRoot.Path));
_command.SetCommandTag(TelemetryTagNames.InstallPathType, InstallPathClassifier.ClassifyInstallPath(pathResolution.ResolvedInstallPath, pathResolution.PathSource));
_command.SetCommandTag(TelemetryTagNames.InstallPathSource, pathResolution.PathSource.ToString().ToLowerInvariant());
_command.SetCommandTag(TelemetryTagNames.InstallResolvedVersion, resolved.ResolvedVersion.ToString());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -155,7 +155,9 @@ private static void CheckAndReportStillPresent(

/// <summary>
/// Resolves the install path for uninstall using the same logic as the install command:
/// only use the configured path if it's a user install, otherwise fall back to default.
/// only use the configured path if it is a dotnetup-managed hive, otherwise fall back to
/// default. This prevents uninstall from targeting a dotnet that dotnetup does not own (e.g.
/// a system install or a hand-extracted dotnet that happens to win on PATH).
/// </summary>
internal static string ResolveInstallPath(string? explicitInstallPath, IDotnetEnvironmentManager dotnetEnvironment)
{
Expand All @@ -165,7 +167,7 @@ internal static string ResolveInstallPath(string? explicitInstallPath, IDotnetEn
}

var configuredInstall = dotnetEnvironment.GetCurrentPathConfiguration();
if (configuredInstall is { InstallType: InstallType.User })
if (configuredInstall is { IsDotnetupHive: true })
{
return configuredInstall.Path;
}
Expand Down
14 changes: 9 additions & 5 deletions src/Installer/dotnetup.Library/DotnetEnvironmentManager.cs
Original file line number Diff line number Diff line change
Expand Up @@ -43,11 +43,15 @@ public DotnetEnvironmentManager()
ResolveCurrentInstallRootPath(foundDotnet),
InstallerUtilities.GetDefaultInstallArchitecture());

// 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);
return new(currentInstallRoot, isAdminInstall ? InstallType.System : InstallType.User);
// Report whether the resolved dotnet is a dotnetup-managed hive — an install dotnetup owns
// and may run or uninstall from — rather than classifying it as "system vs user". A dotnet
// that merely lives in a user-writable location (e.g. a hand-extracted C:\dotnet on PATH) is
// NOT dotnetup's and must not be treated as such. Today the only supported hive is the
// default install path; configurable hives are tracked in
// https://github.com/dotnet/sdk/issues/55346, at which point this check would also consult
// the persisted root.
bool isDotnetupHive = DotnetupUtilities.PathsEqual(currentInstallRoot.Path, GetDefaultDotnetInstallPath());
Comment thread
dsplaisted marked this conversation as resolved.
Outdated
return new(currentInstallRoot, isDotnetupHive);
}

public string GetDefaultDotnetInstallPath()
Expand Down
7 changes: 6 additions & 1 deletion src/Installer/dotnetup.Library/IDotnetEnvironmentManager.cs
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,11 @@ internal interface IDotnetEnvironmentManager
{
string GetDefaultDotnetInstallPath();

/// <summary>
/// Resolves the <c>dotnet</c> that currently wins on <c>PATH</c> and reports whether it is a
/// dotnetup-managed hive (i.e. an install dotnetup owns and may run or uninstall from).
/// Returns <c>null</c> when no <c>dotnet</c> is found on <c>PATH</c>.
/// </summary>
DotnetInstallRootConfiguration? GetCurrentPathConfiguration();

string? GetLatestInstalledSystemVersion();
Expand Down Expand Up @@ -75,7 +80,7 @@ public string? SdkPath

public record DotnetInstallRootConfiguration(
DotnetInstallRoot InstallRoot,
InstallType InstallType)
bool IsDotnetupHive)
Comment thread
dsplaisted marked this conversation as resolved.
{
public string Path => InstallRoot.Path;
}
2 changes: 1 addition & 1 deletion src/Installer/dotnetup.Library/Strings.resx
Original file line number Diff line number Diff line change
Expand Up @@ -283,7 +283,7 @@ Or open a new terminal.</value>
<value>Open a new terminal for the change to take effect.</value>
</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>

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

</data>
<data name="EnvShellRequiredForProfile" xml:space="preserve">
<value>Could not detect the current shell, which is required to update the dotnetup profile entry. Re-run with --shell &lt;{0}&gt; to specify it explicitly.</value>
Expand Down
4 changes: 2 additions & 2 deletions src/Installer/dotnetup.Library/xlf/Strings.cs.xlf

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions src/Installer/dotnetup.Library/xlf/Strings.de.xlf

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions src/Installer/dotnetup.Library/xlf/Strings.es.xlf

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions src/Installer/dotnetup.Library/xlf/Strings.fr.xlf

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions src/Installer/dotnetup.Library/xlf/Strings.it.xlf

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions src/Installer/dotnetup.Library/xlf/Strings.ja.xlf

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions src/Installer/dotnetup.Library/xlf/Strings.ko.xlf

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions src/Installer/dotnetup.Library/xlf/Strings.pl.xlf

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions src/Installer/dotnetup.Library/xlf/Strings.pt-BR.xlf

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions src/Installer/dotnetup.Library/xlf/Strings.ru.xlf

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions src/Installer/dotnetup.Library/xlf/Strings.tr.xlf

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions src/Installer/dotnetup.Library/xlf/Strings.zh-Hans.xlf

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions src/Installer/dotnetup.Library/xlf/Strings.zh-Hant.xlf

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

15 changes: 8 additions & 7 deletions test/dotnetup.Tests/DotnetCommandTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -150,9 +150,9 @@ public void DotnetCommand_WhenDotnetFound_RunsProcess()
}

[TestMethod]
public void DotnetCommand_UsesConfiguredInstallType_WhenUserInstall()
public void DotnetCommand_UsesConfiguredPath_WhenDotnetupHive()
{
// Arrange - two paths: a configured user install path and a default path
// Arrange - two paths: a configured dotnetup hive path and a default path
var userDir = Directory.CreateTempSubdirectory("dotnetup-user-test");
var defaultDir = Directory.CreateTempSubdirectory("dotnetup-default-test");
try
Expand All @@ -164,13 +164,13 @@ public void DotnetCommand_UsesConfiguredInstallType_WhenUserInstall()
defaultInstallPath: defaultDir.FullName,
configuredRoot: new DotnetInstallRootConfiguration(
new DotnetInstallRoot(userDir.FullName, InstallArchitecture.x64),
InstallType.User));
IsDotnetupHive: true));

var parseResult = Parser.Parse(["dotnet", "--version"]);
var command = new TestableDotnetCommand(parseResult, mock);
var exitCode = command.Execute();

// Should succeed because it used the configured user path, not the default
// Should succeed because it used the configured hive path, not the default
exitCode.Should().Be(0);
}
finally
Expand Down Expand Up @@ -206,9 +206,10 @@ public void DotnetCommand_FallsBackToDefault_WhenNoConfiguredInstall()
}

[TestMethod]
public void DotnetCommand_FallsBackToDefault_WhenAdminInstall()
public void DotnetCommand_FallsBackToDefault_WhenNotDotnetupHive()
{
// Arrange - configured install is Admin, not User; should fall back to default
// Arrange - configured install is not a dotnetup hive (e.g. an admin or loose install);
// should fall back to default
var adminDir = Directory.CreateTempSubdirectory("dotnetup-admin-test");
var defaultDir = Directory.CreateTempSubdirectory("dotnetup-default-test");
try
Expand All @@ -220,7 +221,7 @@ public void DotnetCommand_FallsBackToDefault_WhenAdminInstall()
defaultInstallPath: defaultDir.FullName,
configuredRoot: new DotnetInstallRootConfiguration(
new DotnetInstallRoot(adminDir.FullName, InstallArchitecture.x64),
InstallType.System));
IsDotnetupHive: false));

var parseResult = Parser.Parse(["dotnet", "--version"]);
var command = new TestableDotnetCommand(parseResult, mock);
Expand Down
Loading
Loading