Consider PATH again when searching for dotnet host #80859
Conversation
|
@333fred @RikkiGibson @dotnet/roslyn-compiler for a second review, thanks. This fixes a regression. |
| internal static string? GetToolDotNetRoot() | ||
| { | ||
| if (GetDotNetHostPath() is { } dotNetHostPath) | ||
| var directoryName = Path.GetDirectoryName(GetDotNetPathOrDefault()); |
There was a problem hiding this comment.
It looks like before we were looking specifically for the directory containing a suitable .NET runtime installation?
But now we are searching the path, and the dotnet we find on the PATH might be linked into a location like /usr/bin/dotnet. Is it fine to just return /usr/bin from this call?
There was a problem hiding this comment.
But now we are searching the path, and the dotnet we find on the PATH might be linked into a location like
/usr/bin/dotnet. Is it fine to just return/usr/binfrom this call?
Good catch, that wouldn't work. I guess we can resolve symlinks first.
| { | ||
| if (!returnFinalTarget) throw new NotSupportedException(); | ||
|
|
||
| path = Path.GetFullPath(path); |
There was a problem hiding this comment.
Is there a need for this method to work with relative paths?
I guess it's possible for a path coming from the environment (PATH, DOTNET_ROOT etc) to be relative..though that seems really weird, and likely to result in unpredictable behavior. I wonder if that is a condition we should log/throw/complain about, if it ever happens.
There was a problem hiding this comment.
There is probably no need by the current code, but this polyfill could be used in the future by others so it seems it should behave just like the BCL's equivalent.
But it seems CreateFileW would handle relative paths anyway so the GetFullPath call seems unnecessary, I will remove it.
| internal static string GetDotNetPathOrDefault() | ||
| { | ||
| if (GetDotNetHostPath() is { } pathToDotNet) | ||
| if (Environment.GetEnvironmentVariable(DotNetHostPathEnvironmentName) is { Length: > 0 } pathToDotNet) |
There was a problem hiding this comment.
This PR effectively means we set DOTNET_ROOT for our apphost based on the following priority order:
%DOTNET_HOST_PATH%%DOTNET_EXPERIMENTAL_HOST_PATH%- First
dotneton%PATH% - YOLO
dotnet
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Yeah I think this feels fine. @dsplaisted any concerns for finding-private-SDK-from-environment-variables?
There was a problem hiding this comment.
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.
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.
There was a problem hiding this comment.
Sounds like Rainer thinks this is fine, I'm going to merge tomorrow unless there are other concerns
5 participants
Fixes dotnet/msbuild#12669. Before the change to use csc apphost, we were searching for
dotnet.exeeither fromDOTNET_HOST_PATHorPATH. With csc apphost (#80026) we regressed this by considering the dotnet host only fromDOTNET_HOST_PATH(and passing that asDOTNET_ROOTto the apphost) - but that variable is not passed in MSBuilds older than 18.x (where the previous non-apphost implementation would work fine since it would fallback to findingdotnet.exeinPATH).