Skip to content
Merged
Show file tree
Hide file tree
Changes from all 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 @@ -59,13 +59,13 @@ protected override void ExecuteCore()
}

/// <summary>
/// Resolves the dotnet installation path using the same logic as other dotnetup commands:
/// configured install type (user install) falls back to the default install path.
/// Resolves the dotnet installation path: when the PATH-resolved install is a
/// dotnetup-managed hive, use it; otherwise fall back to the default install path.
/// </summary>
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 @@ -10,9 +10,12 @@ namespace Microsoft.DotNet.Tools.Bootstrapper.Commands.Shared;
internal static class InstallPathClassifier
{
/// <summary>
/// Determines whether the given path is an admin/system-managed .NET install location.
/// These locations are managed by system package managers or OS installers and should not
/// be used by dotnetup for user-level installations.
/// Determines whether the given path is in a well-known system/admin .NET install location
/// (for example <c>Program Files\dotnet</c> on Windows). These locations are usually managed
/// by system package managers or OS installers and should not be used by dotnetup.
///
/// This is a location heuristic only, it doesn't verify that the install there is actually managed
/// by an OS installer or package manager.
/// </summary>
public static bool IsAdminInstallPath(string path)
{
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
22 changes: 16 additions & 6 deletions src/Installer/dotnetup.Library/DotnetEnvironmentManager.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
// The .NET Foundation licenses this file to you under the MIT license.

using Microsoft.Dotnet.Installation.Internal;
using Microsoft.DotNet.Tools.Bootstrapper.Commands.Shared;
using Microsoft.DotNet.Tools.Bootstrapper.Shell;
using Microsoft.DotNet.Tools.Bootstrapper.Telemetry;
using Spectre.Console;
Expand Down Expand Up @@ -43,11 +42,22 @@ 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.
//
// Resolve the default path's symlinks too so the comparison is symmetric: currentInstallRoot.Path
// is already realpath-resolved above, and the default path's base (LocalApplicationData /
// XDG_DATA_HOME, which is not required to be a real directory) may itself be a symlink. Without
// this, a symlinked data directory would make dotnetup fail to recognize its own hive.
string defaultInstallPath = GetDefaultDotnetInstallPath();
string resolvedDefaultInstallPath = ExecutablePathResolver.ResolveRealPath(defaultInstallPath) ?? defaultInstallPath;
bool isDotnetupHive = DotnetupUtilities.PathsEqual(currentInstallRoot.Path, resolvedDefaultInstallPath);
return new(currentInstallRoot, isDotnetupHive);
}

public string GetDefaultDotnetInstallPath()
Expand Down
9 changes: 9 additions & 0 deletions src/Installer/dotnetup.Library/DotnetupPaths.cs
Original file line number Diff line number Diff line change
Expand Up @@ -121,10 +121,19 @@ public static string ManifestPath
/// Gets the default dotnet install path managed by dotnetup.
/// This is the user-local dotnet root (e.g. %LOCALAPPDATA%\dotnet on Windows).
/// </summary>
/// <remarks>
/// Can be overridden via DOTNET_TESTHOOK_DEFAULT_DOTNET_PATH environment variable.
/// </remarks>
public static string DefaultDotnetInstallPath
{
get
{
var overridePath = Environment.GetEnvironmentVariable("DOTNET_TESTHOOK_DEFAULT_DOTNET_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.

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

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 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.

if (!string.IsNullOrEmpty(overridePath))
{
return overridePath;
}

var baseDir = GetBaseDirectory();
if (string.IsNullOrEmpty(baseDir))
{
Expand Down
23 changes: 19 additions & 4 deletions src/Installer/dotnetup.Library/ExecutablePathResolver.cs
Original file line number Diff line number Diff line change
Expand Up @@ -22,13 +22,28 @@ internal static class ExecutablePathResolver
/// </summary>
public static string? ResolveRealDirectory(string? executablePath)
{
if (string.IsNullOrEmpty(executablePath))
string? resolvedPath = ResolveRealPath(executablePath);
return resolvedPath is null ? null : Path.GetDirectoryName(resolvedPath);
}

/// <summary>
/// Returns <paramref name="path"/> itself with symlinks resolved (unlike
/// <see cref="ResolveRealDirectory"/>, which resolves the containing directory of an
/// executable). Use this to canonicalize a directory that may be reached through a symlinked
/// parent, such as a dotnet install root under a symlinked <c>LocalApplicationData</c> /
/// <c>XDG_DATA_HOME</c>. Returns <c>null</c> when the path is null or empty.
/// <see cref="FileInterop.ResolveRealPath"/> is a no-op on Windows, and on Unix returns
/// <c>null</c> for a path that does not yet exist; in both cases the normalized full path is
/// returned so callers still get a usable value.
/// </summary>
public static string? ResolveRealPath(string? path)
{
if (string.IsNullOrEmpty(path))
{
return null;
}

string fullPath = Path.GetFullPath(executablePath);
string resolvedPath = FileInterop.ResolveRealPath(fullPath) ?? fullPath;
return Path.GetDirectoryName(resolvedPath);
string fullPath = Path.GetFullPath(path);
return FileInterop.ResolveRealPath(fullPath) ?? fullPath;
}
}
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;
}
4 changes: 2 additions & 2 deletions src/Installer/dotnetup.Library/Strings.resx
Original file line number Diff line number Diff line change
Expand Up @@ -163,7 +163,7 @@
<value>Arguments to forward to dotnet.</value>
</data>
<data name="DotnetCommandDotnetNotFound" xml:space="preserve">
<value>Error: dotnet executable not found at '{0}'.</value>
<value>dotnet executable not found at '{0}'.</value>
</data>
<data name="DotnetCommandInstallFirst" xml:space="preserve">
<value>Run 'dotnetup install' to install a .NET SDK first.</value>
Expand Down 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
8 changes: 4 additions & 4 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.

8 changes: 4 additions & 4 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.

8 changes: 4 additions & 4 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.

8 changes: 4 additions & 4 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.

8 changes: 4 additions & 4 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.

8 changes: 4 additions & 4 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.

Loading
Loading