From 4d6498337d1a33261896609b00f76421598eae7e Mon Sep 17 00:00:00 2001 From: Eric Erhardt Date: Mon, 24 Jul 2023 11:22:21 -0500 Subject: [PATCH 1/3] Enable building with a dotnet not on PATH In #68918 we removed inspecting DOTNET_HOST_PATH environment variable. This broke the scenario where a specific dotnet "hive" was installed to a location not on the PATH. When the .NET SDK commands invoke a sub-process (for example MSBuild), it sets the DOTNET_HOST_PATH environment variable to tell the sub-process "this is where the dotnet.exe that invoked this command is located". See https://github.com/dotnet/roslyn/pull/21237 and https://github.com/dotnet/cli/pull/7311 for more info. This change reverts the behavior back to respect DOTNET_HOST_PATH, and if it isn't set it will just use "dotnet" and let the OS take care of finding the executable on the PATH. Fix #69150 --- src/Compilers/Shared/RuntimeHostInfo.cs | 37 ++++--------------------- 1 file changed, 6 insertions(+), 31 deletions(-) diff --git a/src/Compilers/Shared/RuntimeHostInfo.cs b/src/Compilers/Shared/RuntimeHostInfo.cs index 70be40e5f43df..31589a5375cd2 100644 --- a/src/Compilers/Shared/RuntimeHostInfo.cs +++ b/src/Compilers/Shared/RuntimeHostInfo.cs @@ -49,40 +49,15 @@ internal static (string processFilePath, string commandLineArguments, string too internal static bool IsCoreClrRuntime => true; + private const string DotNetHostPathEnvironmentName = "DOTNET_HOST_PATH"; + /// - /// Get the path to the dotnet executable. In the case the host did not provide this information + /// Get the path to the dotnet executable. In the case the .NET SDK did not provide this information /// in the environment this will return simply "dotnet". /// - /// - /// See the following issue for rationale why only %PATH% is considered - /// https://github.com/dotnet/runtime/issues/88754 - /// - internal static string GetDotNetPathOrDefault() - { - var (fileName, sep) = PlatformInformation.IsWindows - ? ("dotnet.exe", ';') - : ("dotnet", ':'); - - var path = Environment.GetEnvironmentVariable("PATH") ?? ""; - foreach (var item in path.Split(sep, StringSplitOptions.RemoveEmptyEntries)) - { - try - { - var filePath = Path.Combine(item, fileName); - if (File.Exists(filePath)) - { - return filePath; - } - } - catch - { - // If we can't read a directory for any reason just skip it - } - } - - return fileName; - } - + internal static string GetDotNetPathOrDefault() => + Environment.GetEnvironmentVariable(DotNetHostPathEnvironmentName) ?? + (PlatformInformation.IsWindows ? "dotnet.exe" : "dotnet"); #else internal static bool IsCoreClrRuntime => false; From 214cb7bc99c91da5a7db51ad0a3addd546aa81fd Mon Sep 17 00:00:00 2001 From: Eric Erhardt Date: Mon, 24 Jul 2023 14:30:19 -0500 Subject: [PATCH 2/3] Fix tests to workaround MSBuild searching the PATH itself. --- .../TestUtilities/TaskTestUtil.cs | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/src/Compilers/Core/MSBuildTaskTests/TestUtilities/TaskTestUtil.cs b/src/Compilers/Core/MSBuildTaskTests/TestUtilities/TaskTestUtil.cs index 80521b800d154..e804b44859081 100644 --- a/src/Compilers/Core/MSBuildTaskTests/TestUtilities/TaskTestUtil.cs +++ b/src/Compilers/Core/MSBuildTaskTests/TestUtilities/TaskTestUtil.cs @@ -4,6 +4,7 @@ using System; using System.Collections.Generic; +using System.IO; using System.Linq; using System.Text; using System.Threading.Tasks; @@ -40,7 +41,23 @@ public static void AssertCommandLine( var message = engine.BuildMessages.OfType().Single(); var commandLine = message.CommandLine.Replace(" ", " ").Trim(); - Assert.Equal($@"{RuntimeHostInfo.GetDotNetPathOrDefault()} exec ""{task.PathToManagedTool}"" {line}", commandLine); + + var dotnetPath = RuntimeHostInfo.GetDotNetPathOrDefault(); + var expectedCommandLine = $@"{dotnetPath} exec ""{task.PathToManagedTool}"" {line}"; + + bool isOnlyFileName = Path.GetFileName(dotnetPath).Length == dotnetPath.Length; + if (isOnlyFileName) + { + // When ToolTask.GenerateFullPathToTool() returns only a file name (not a path to a file), MSBuild's ToolTask + // will search the %PATH% (see https://github.com/dotnet/msbuild/blob/5410bf323451e04e99e79bcffd158e6d8d378149/src/Utilities/ToolTask.cs#L494-L513) + // and log the full path to the exe. In this case, only assert that the commandLine ends with the expected + // command line, and ignore the full path at the beginning. + Assert.EndsWith(expectedCommandLine, commandLine); + } + else + { + Assert.Equal(expectedCommandLine, commandLine); + } compilerTask.NoConfig = true; Assert.Equal("/noconfig", compilerTask.GenerateToolArguments()); From f3929185958b3f14e52bc87462b3a6e937266acd Mon Sep 17 00:00:00 2001 From: Eric Erhardt Date: Mon, 24 Jul 2023 15:10:44 -0500 Subject: [PATCH 3/3] Respond to PR feedback. Make the change smaller until @jaredpar gets back. Only make the minimal change required, which is to check DOTNET_HOST_PATH. --- .../TestUtilities/TaskTestUtil.cs | 19 +--------- src/Compilers/Shared/RuntimeHostInfo.cs | 37 +++++++++++++++++-- 2 files changed, 34 insertions(+), 22 deletions(-) diff --git a/src/Compilers/Core/MSBuildTaskTests/TestUtilities/TaskTestUtil.cs b/src/Compilers/Core/MSBuildTaskTests/TestUtilities/TaskTestUtil.cs index e804b44859081..80521b800d154 100644 --- a/src/Compilers/Core/MSBuildTaskTests/TestUtilities/TaskTestUtil.cs +++ b/src/Compilers/Core/MSBuildTaskTests/TestUtilities/TaskTestUtil.cs @@ -4,7 +4,6 @@ using System; using System.Collections.Generic; -using System.IO; using System.Linq; using System.Text; using System.Threading.Tasks; @@ -41,23 +40,7 @@ public static void AssertCommandLine( var message = engine.BuildMessages.OfType().Single(); var commandLine = message.CommandLine.Replace(" ", " ").Trim(); - - var dotnetPath = RuntimeHostInfo.GetDotNetPathOrDefault(); - var expectedCommandLine = $@"{dotnetPath} exec ""{task.PathToManagedTool}"" {line}"; - - bool isOnlyFileName = Path.GetFileName(dotnetPath).Length == dotnetPath.Length; - if (isOnlyFileName) - { - // When ToolTask.GenerateFullPathToTool() returns only a file name (not a path to a file), MSBuild's ToolTask - // will search the %PATH% (see https://github.com/dotnet/msbuild/blob/5410bf323451e04e99e79bcffd158e6d8d378149/src/Utilities/ToolTask.cs#L494-L513) - // and log the full path to the exe. In this case, only assert that the commandLine ends with the expected - // command line, and ignore the full path at the beginning. - Assert.EndsWith(expectedCommandLine, commandLine); - } - else - { - Assert.Equal(expectedCommandLine, commandLine); - } + Assert.Equal($@"{RuntimeHostInfo.GetDotNetPathOrDefault()} exec ""{task.PathToManagedTool}"" {line}", commandLine); compilerTask.NoConfig = true; Assert.Equal("/noconfig", compilerTask.GenerateToolArguments()); diff --git a/src/Compilers/Shared/RuntimeHostInfo.cs b/src/Compilers/Shared/RuntimeHostInfo.cs index 31589a5375cd2..0f08c684f7ef9 100644 --- a/src/Compilers/Shared/RuntimeHostInfo.cs +++ b/src/Compilers/Shared/RuntimeHostInfo.cs @@ -53,11 +53,40 @@ internal static (string processFilePath, string commandLineArguments, string too /// /// Get the path to the dotnet executable. In the case the .NET SDK did not provide this information - /// in the environment this will return simply "dotnet". + /// in the environment this tries to find "dotnet" on the PATH. In the case it is not found, + /// this will return simply "dotnet". /// - internal static string GetDotNetPathOrDefault() => - Environment.GetEnvironmentVariable(DotNetHostPathEnvironmentName) ?? - (PlatformInformation.IsWindows ? "dotnet.exe" : "dotnet"); + internal static string GetDotNetPathOrDefault() + { + if (Environment.GetEnvironmentVariable(DotNetHostPathEnvironmentName) is string pathToDotNet) + { + return pathToDotNet; + } + + var (fileName, sep) = PlatformInformation.IsWindows + ? ("dotnet.exe", ';') + : ("dotnet", ':'); + + var path = Environment.GetEnvironmentVariable("PATH") ?? ""; + foreach (var item in path.Split(sep, StringSplitOptions.RemoveEmptyEntries)) + { + try + { + var filePath = Path.Combine(item, fileName); + if (File.Exists(filePath)) + { + return filePath; + } + } + catch + { + // If we can't read a directory for any reason just skip it + } + } + + return fileName; + } + #else internal static bool IsCoreClrRuntime => false;