Add additional fallback probes #126
Conversation
| match exe with | ||
| | None -> failwith "No dotnet binary could be found via the DOTNET_HOST_PATH or DOTNET_ROOT environment variables, the PATH environment variable, or the default install locations" | ||
| | Some exe -> |
There was a problem hiding this comment.
here's the new change to fail fast and informatively
| let private tryFindFromEnvVar () = | ||
| potentialDotnetHostEnvVars | ||
| |> List.tryPick (fun (envVar, transformer) -> | ||
| match Environment.GetEnvironmentVariable envVar |> existingEnvVarValue with | ||
| | Some varValue -> Some(transformer varValue |> FileInfo) | ||
| | None -> None) | ||
|
|
||
| let private PATHSeparator = | ||
| if isUnix then | ||
| ':' | ||
| else | ||
| ';' | ||
|
|
||
| let private tryFindFromPATH () = | ||
| System | ||
| .Environment | ||
| .GetEnvironmentVariable("PATH") | ||
| .Split(PATHSeparator, StringSplitOptions.RemoveEmptyEntries ||| StringSplitOptions.TrimEntries) | ||
| |> Array.tryPick (fun d -> | ||
| let fi = Path.Combine(d, dotnetBinaryName) |> FileInfo | ||
|
|
||
| if fi.Exists then | ||
| Some fi | ||
| else | ||
| None) | ||
|
|
||
|
|
||
| let private tryFindFromDefaultDirs () = | ||
| let windowsPath = $"C:\\Program Files\\dotnet\\{dotnetBinaryName}" | ||
| let macosPath = $"/usr/local/share/dotnet/{dotnetBinaryName}" | ||
| let linuxPath = $"/usr/share/dotnet/{dotnetBinaryName}" | ||
|
|
||
| let tryFindFile p = | ||
| let f = FileInfo p | ||
|
|
||
| if f.Exists then | ||
| Some f | ||
| else | ||
| None | ||
|
|
||
| if isWindows then | ||
| tryFindFile windowsPath | ||
| else if isMac then | ||
| tryFindFile macosPath | ||
| else if isLinux then | ||
| tryFindFile linuxPath | ||
| else | ||
| None | ||
|
|
||
| /// <summary> | ||
| /// provides the path to the `dotnet` binary running this library, respecting various dotnet <see href="https://docs.microsoft.com/en-us/dotnet/core/tools/dotnet-environment-variables#dotnet_root-dotnet_rootx86%5D">environment variables</see> | ||
| /// provides the path to the `dotnet` binary running this library, respecting various dotnet <see href="https://docs.microsoft.com/en-us/dotnet/core/tools/dotnet-environment-variables#dotnet_root-dotnet_rootx86%5D">environment variables</see>. | ||
| /// Also probes the PATH and checks the default installation locations | ||
| /// </summary> | ||
| let dotnetRoot = | ||
| potentialDotnetHostEnvVars | ||
| |> List.tryPick | ||
| (fun (envVar, transformer) -> | ||
| match Environment.GetEnvironmentVariable envVar |> existingEnvVarValue with | ||
| | Some varValue -> Some(transformer varValue |> FileInfo) | ||
| | None -> None) | ||
| |> Option.defaultWith | ||
| (fun _ -> | ||
| System | ||
| .Diagnostics | ||
| .Process | ||
| .GetCurrentProcess() | ||
| .MainModule | ||
| .FileName | ||
| |> FileInfo) | ||
| lazy (tryFindFromEnvVar () |> Option.orElseWith tryFindFromPATH |> Option.orElseWith tryFindFromDefaultDirs) |
There was a problem hiding this comment.
here's the meat of the logic changes. we no longer allow the process's main module.filename, as that's really only realistic for the dotnet muxer application. instead, we try the env var route, the path route, and finally the default directory route.
|
Failing test on OSX - maybe we should increase sleep calls that we have around (https://github.com/ionide/proj-info/blob/main/test/Ionide.ProjInfo.Tests/Tests.fs#L921 etc)? From the error message, it seems like it just didn't manage to finish all parsings in time |
| @@ -167,7 +168,7 @@ module Init = | |||
|
|
|||
| match System.Environment.GetEnvironmentVariable "DOTNET_HOST_PATH" with | |||
| | null | |||
| | "" -> Environment.SetEnvironmentVariable("DOTNET_HOST_PATH", Paths.dotnetRoot.FullName) | |||
| | "" -> Environment.SetEnvironmentVariable("DOTNET_HOST_PATH", dotnetExe.FullName) | |||
There was a problem hiding this comment.
Hmmm, My problem in #123 was more that this environment variable was being set within the process to an incorrect value, and then never unset. This caused FSharp.Compiler.Service to do incorrect things.
So is the fix here that dotnetExe will now be a good value?
But even if it's a good value, I feel there should be an unset of this environment variable after sub-process launch, in the cases where it gets set. I had to add an explicit unset here for example: https://github.com/fsprojects/FSharp.Formatting/blob/main/src/fsdocs-tool/BuildCommand.fs#L851
There was a problem hiding this comment.
It turns out that we do need a value of DOTNET_HOST_PATH set, because the F# targets rely on it to set the fsc binary to invoke to fsc.dll instead of fsc.exe. This logic is here. Without this value set, our tests start failing left and right. I could probably try a version where we set these msbuild properties in this library explicitly, but I worry about the fragility of that, as these properties are subject to change at any time, while DOTNET_HOST_PATH is documented.
Another option would be for the init function to return an IDisposable that you could dispose to 'pop' the changes to the environment variables, but that's a bit of a foot-gun - if disposed then everything in this library becomes unusable until init is called again, so you'd want some sort of scoping on the client side.
This should fix #123 by
initcall and notifying the caller what they need to do in order to become compliant.I thought about making this return a result, but this is kinda a catastrophic failure mode - if we can't find the dotnet binary then nothing else works, either.