-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Consider PATH again when searching for dotnet host #80859
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,112 @@ | ||
| // Licensed to the .NET Foundation under one or more agreements. | ||
| // The .NET Foundation licenses this file to you under the MIT license. | ||
| // See the LICENSE file in the project root for more information. | ||
|
|
||
| using System; | ||
| using System.IO; | ||
| using System.Runtime.InteropServices; | ||
| using Microsoft.CodeAnalysis.Test.Utilities; | ||
| using Roslyn.Test.Utilities; | ||
| using Roslyn.Utilities; | ||
| using Xunit; | ||
| using Xunit.Abstractions; | ||
|
|
||
| namespace Microsoft.CodeAnalysis.BuildTasks.UnitTests; | ||
|
|
||
| public sealed class RuntimeHostInfoTests(ITestOutputHelper output) | ||
| { | ||
| private readonly ITestOutputHelper _output = output; | ||
|
|
||
| [Fact, WorkItem("https://github.com/dotnet/msbuild/issues/12669")] | ||
| public void DotNetInPath() | ||
| { | ||
| var previousPath = Environment.GetEnvironmentVariable("PATH"); | ||
| try | ||
| { | ||
| using var tempRoot = new TempRoot(); | ||
| var testDir = tempRoot.CreateDirectory(); | ||
| var globalDotNetDir = testDir.CreateDirectory("global-dotnet"); | ||
| var globalDotNetExe = globalDotNetDir.CreateFile($"dotnet{PlatformInformation.ExeExtension}"); | ||
| Environment.SetEnvironmentVariable("PATH", globalDotNetDir.Path); | ||
|
|
||
| Assert.Equal(globalDotNetDir.Path, RuntimeHostInfo.GetToolDotNetRoot(_output.WriteLine)); | ||
| } | ||
| finally | ||
| { | ||
| Environment.SetEnvironmentVariable("PATH", previousPath); | ||
| } | ||
| } | ||
|
|
||
| [Fact, WorkItem("https://github.com/dotnet/msbuild/issues/12669")] | ||
| public void DotNetInPath_None() | ||
| { | ||
| var previousPath = Environment.GetEnvironmentVariable("PATH"); | ||
| try | ||
| { | ||
| Environment.SetEnvironmentVariable("PATH", ""); | ||
|
|
||
| Assert.Null(RuntimeHostInfo.GetToolDotNetRoot(_output.WriteLine)); | ||
| } | ||
| finally | ||
| { | ||
| Environment.SetEnvironmentVariable("PATH", previousPath); | ||
| } | ||
| } | ||
|
|
||
| [Fact, WorkItem("https://github.com/dotnet/msbuild/issues/12669")] | ||
| public void DotNetInPath_Symlinked() | ||
| { | ||
| var previousPath = Environment.GetEnvironmentVariable("PATH"); | ||
| try | ||
| { | ||
| using var tempRoot = new TempRoot(); | ||
| var testDir = tempRoot.CreateDirectory(); | ||
| var globalDotNetDir = testDir.CreateDirectory("global-dotnet"); | ||
| var globalDotNetExe = globalDotNetDir.CreateFile($"dotnet{PlatformInformation.ExeExtension}"); | ||
| var binDir = testDir.CreateDirectory("bin"); | ||
| var symlinkPath = Path.Combine(binDir.Path, $"dotnet{PlatformInformation.ExeExtension}"); | ||
|
|
||
| // Create symlink from binDir to the actual dotnet executable | ||
| File.CreateSymbolicLink(path: symlinkPath, pathToTarget: globalDotNetExe.Path); | ||
|
|
||
| Environment.SetEnvironmentVariable("PATH", binDir.Path); | ||
|
|
||
| Assert.Equal(globalDotNetDir.Path, RuntimeHostInfo.GetToolDotNetRoot(_output.WriteLine)); | ||
| } | ||
| finally | ||
| { | ||
| Environment.SetEnvironmentVariable("PATH", previousPath); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| #if !NET | ||
| file static class NativeMethods | ||
RikkiGibson marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| { | ||
| extension(File) | ||
| { | ||
| /// <remarks> | ||
| /// Only used by tests currently (might need some hardening if this is to be used by production code). | ||
| /// </remarks> | ||
| public static void CreateSymbolicLink(string path, string pathToTarget) | ||
| { | ||
| bool ok = CreateSymbolicLink( | ||
| lpSymlinkFileName: path, | ||
| lpTargetFileName: pathToTarget, | ||
| dwFlags: SYMBOLIC_LINK_FLAG_ALLOW_UNPRIVILEGED_CREATE); | ||
| if (!ok) | ||
| { | ||
| throw new System.ComponentModel.Win32Exception(Marshal.GetLastWin32Error()); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| [DllImport("kernel32.dll", CharSet = CharSet.Unicode, SetLastError = true)] | ||
| private static extern bool CreateSymbolicLink( | ||
| string lpSymlinkFileName, | ||
| string lpTargetFileName, | ||
| uint dwFlags); | ||
|
|
||
| private const uint SYMBOLIC_LINK_FLAG_ALLOW_UNPRIVILEGED_CREATE = 0x2; | ||
| } | ||
| #endif | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,6 +6,7 @@ | |
|
|
||
| using System; | ||
| using System.IO; | ||
| using Microsoft.CodeAnalysis.CommandLine; | ||
| using Roslyn.Utilities; | ||
|
|
||
| namespace Microsoft.CodeAnalysis | ||
|
|
@@ -32,29 +33,32 @@ internal static class RuntimeHostInfo | |
| /// <summary> | ||
| /// The <c>DOTNET_ROOT</c> that should be used when launching executable tools. | ||
| /// </summary> | ||
| internal static string? GetToolDotNetRoot() | ||
| internal static string? GetToolDotNetRoot(Action<string, object[]>? logger) | ||
| { | ||
| if (GetDotNetHostPath() is { } dotNetHostPath) | ||
| var dotNetPath = GetDotNetPathOrDefault(); | ||
|
|
||
| // Resolve symlinks to dotnet | ||
| try | ||
| { | ||
| return Path.GetDirectoryName(dotNetHostPath); | ||
| var resolvedPath = File.ResolveLinkTarget(dotNetPath, returnFinalTarget: true); | ||
| if (resolvedPath != null) | ||
| { | ||
| dotNetPath = resolvedPath.FullName; | ||
| } | ||
| } | ||
|
|
||
| return null; | ||
| } | ||
|
|
||
| private static string? GetDotNetHostPath() | ||
| { | ||
| if (Environment.GetEnvironmentVariable(DotNetHostPathEnvironmentName) is { Length: > 0 } pathToDotNet) | ||
| catch (Exception ex) | ||
| { | ||
| return pathToDotNet; | ||
| logger?.Invoke("Failed to resolve symbolic link for dotnet path '{0}': {1}", [dotNetPath, ex.Message]); | ||
| return null; | ||
| } | ||
|
|
||
| if (Environment.GetEnvironmentVariable(DotNetExperimentalHostPathEnvironmentName) is { Length: > 0 } pathToDotNetExperimental) | ||
| var directoryName = Path.GetDirectoryName(dotNetPath); | ||
| if (string.IsNullOrEmpty(directoryName)) | ||
| { | ||
| return pathToDotNetExperimental; | ||
| return null; | ||
| } | ||
|
|
||
| return null; | ||
| return directoryName; | ||
| } | ||
|
|
||
| /// <summary> | ||
|
|
@@ -64,11 +68,16 @@ internal static class RuntimeHostInfo | |
| /// </summary> | ||
| internal static string GetDotNetPathOrDefault() | ||
| { | ||
| if (GetDotNetHostPath() is { } pathToDotNet) | ||
| if (Environment.GetEnvironmentVariable(DotNetHostPathEnvironmentName) is { Length: > 0 } pathToDotNet) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This PR effectively means we set
The first two are definitely standard practice for tools inside the SDK. I'm not sure about (3) though. Essentially what is the appropriate way to invoke a tool when the .NET 10 SDK is loaded into MSBuild 17.x? @baronfel, @rainersigwald?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. FWIW, it's equivalent to how we always found dotnet host for launching tools, this PR just fixes a recent regression where we stopped doing (3) when we introduced apphosts.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah I think this feels fine. @dsplaisted any concerns for finding-private-SDK-from-environment-variables?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Agree it's taking us back to the old state. At the same time there's been a lot of discussion on how should we be launching .NET based processes: both in the SDK and more generally. I'd rather us be part of the agreed on standard practice vs. deviating if possible.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sounds like Rainer thinks this is fine, I'm going to merge tomorrow unless there are other concerns |
||
| { | ||
| return pathToDotNet; | ||
| } | ||
|
|
||
| if (Environment.GetEnvironmentVariable(DotNetExperimentalHostPathEnvironmentName) is { Length: > 0 } pathToDotNetExperimental) | ||
| { | ||
| return pathToDotNetExperimental; | ||
| } | ||
|
|
||
| var (fileName, sep) = PlatformInformation.IsWindows | ||
| ? ("dotnet.exe", new char[] { ';' }) | ||
| : ("dotnet", new char[] { ':' }); | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.