Add Hostfxr Discovery Logic in place of Bad PATH from VS Code #2048
Conversation
This is so we can add hostfxr logic to find the path in the registry and dedupe logic and isolate the components.
nagilson
changed the title
nagilson
changed the title
|
As suggested in our early conversations, I think this direction is the correct one. We record global installation in a stable way and rely on it for critical scenarios. We rejected using the path to find the dotnet hive for the app host because it is sloppy and not really what the path is for, imo. That design approach can safely be used to solve this "lack of a reliable path" problem. Nice work. |
There was a problem hiding this comment.
Changes look good to me. Thanks for the detailed PR description. The only thought I had was about the design, which doesn't need to be changed for this PR. But my idea was that instead of just having a utility-style DotnetPathFinder, you'd instead make platform-specific implementation classes that know how to resolve the path for those platforms. Basically, turning the imperative os.platform() === 'win32' logic into a WindowsPathResolver or some such, and make ones for the other OS's. I believe this design is used for LinuxVersionResolver and thought it might fit for PATH too.
| { | ||
| let sdks: string[] = []; | ||
|
|
||
|
|
There was a problem hiding this comment.
These nits are good to have, thanks for taking a look :)
I'm going to follow up with them in an issue here: #2082 -- the pipeline appears to be broken and will be a bit of a blocker to deal with that in this PR now, so I've isolated these fixes.
| let installRecordKeysOfXBit = ''; | ||
| if(registryLookup.status === '0') | ||
| { | ||
|
|
| { | ||
| // /reg:32 is added because all keys on 64 bit machines are all put into the WOW node. They won't be on the WOW node on a 32 bit machine. |
There was a problem hiding this comment.
nit: This section has extra spaces in the indentation.
| { | ||
| if(registryQueryResult === '') | ||
| { | ||
| return []; |
Thank you for taking the time to look at this pretty nuanced issue, and in a timely manner. I like the idea of having this be isolated on a per OS basis. A lot of the logic is somewhat shared so it might be possible to do this with an abstract class so some methods are shared. I will add this idea in a new issue too as a future refactoring option. |
What This Does 🤘
This improves the detection of .NET installs to rely on the installation records containing paths to the dotnet host instead of just the PATH as a backup option for locating dotnet.
What I will describe below is documented here:
See dotnet/runtime#109974
See https://github.com/dotnet/designs/blob/main/accepted/2021/install-location-per-architecture.md
Thank you to @elinor-fung for help understanding these docs.
We've had confirmation from internal folks who were blocked that this fixed the issues for them and I did lots of manual testing.
Windows 🪟
For Windows, the registry includes records written by various installers. Anyone who makes an installer for .NET is supposed to follow the logic which is documented above.
Within
HKEY_LOCAL_MACHINE\\SOFTWARE\\dotnet\\Setup\\InstalledVersions\\${architecture}\\:The
sharedhostregistry key stores the path of dotnet that gets put on the PATH. As we want to mimic the terminal behavior as much as possible, we first check this location. For non native installs: sharedhost nodes may exist, but there will be no path key, so we can't use it in this case.The
InstallLocationkey stores other paths to the host that might not be on the PATH, such as an x64 host on an arm64 machine. The InstallLocation is always under the 32 bit reg node for our purposes, and the sharedhost is always on the native node. You can't install arm64 SDK or Runtime on x64, but you can do vice versa and they follow the same methodology and they follow the same rules.Unix 🐧
For Linux and Mac, installers are supposed to write to
/etc/dotnet/install_location_${requestedArchitecture}a file which contains only the path. I confirmed various mac installers do that. The ubuntu installer via feeds does this at least in--classicmode. The snap installer does as well but does so incorrectly, as it writes /usr/lib/dotnet/ to it, which is not actually where dotnet gets installed via snap. We might wanna alert them to that. Our existing code seems to do a good job with snap, vscode has not given us incorrect information here to my knowledge, so I am not too concerned.Prior to .NET 6, installers wrote to
/etc/dotnet/install_location. We use this as a backup and so does .NET 6+.Thank you to @richlander for coming up with this idea with me.
Context ❓
For years now, the
C#extension,UnityExtension,MAUI, etc, and now,C# DevKit, have been plagued by issues trying to find the .NET Installation on the PATH environment variable.Our team did a thorough analysis as to the methods in which .NET is installed across many operating systems and architectures, how to check the PATH, spawning a node.js process with different shells, resolving
realpaths,symlinks, and polymorphic executables (snap) etc., and released an API for PATH lookup.However, we still are facing issues with the PATH. Upon final conclusion, it seems no matter what we call, the PATH information we get as an extension calling into node's
child_processlibrary, orprocess.env, or any other method we could think of, the environment information is not complete. There is also thevscode.environmentVariableCollection, which we do leverage, but my understanding is that this is only for editing the terminal / getting workspace preferences. Using the process explorer, it seems that there are some subprocesses in VS Code that have access to more environment variables than others (presumably electron main vs subprocesses), but I believe the whatever runs the extension host is not one of them.I asked the VS Code team if we all been taking the incorrect approach here to try
process.env,vscode.environmentVariableCollection,child_process, etc. They confirmed our approach is correct.When does this cause problems? 🔍
Here is one such example [BUG] Cannot find .NET SDK installation from PATH environment (macOS) · Issue #1471 · microsoft/vscode-dotnettools (github.com).
The most recent example is from an internal member. There are like 20 to 60+ more. I'm not going to link them all. Many of those are because the PATH API has not been adopted in general, not just a problem with the PATH API, because it is not being called by either extension in RTM prod yet.
In a terminal with bin/bash, his PATH looks like:
When we call from our extension, using node:
child_process.spawnSync('env', [], { env: process.env, shell: '/bin/bash')Note that the environment pretends that it doesn't have dotnet or anaconda installed, amongst others. There is no alternative correct source of truth to get the actual env, from within vscode. This statement is not coming from just me, it is also coming from the VS Code team.
How VS Code Gets Env 💥 🐛
In VS Code, there is a file called shellEnv.ts. I discused this file with them as well. After further discussion with the VS Code team, the VS Code bootstrapping code changes whether vscode is launched in dock mode or from the terminal. In the dock, this
shellEnv.tslogic kicks in to try to correct and find the environment. It appears there is a problem with that code's ability to determine the environment. It is challenging to get a repro environment, and their logs are not sufficient to diagnose the problem without building from source on a customer's machine, which I am not going to pester someone to do.I may drive an effort for a fix in the vscode team, but this approach seemed like the fastest way to get to a resolution.
Interested Individuals 📲
cc @arunchndr @dibarbet @adrianvmsft @lifenglu @webreidi @baronfel
This is not a blocker to the PATH API but a further improvement for when VS Code gives incorrect information. No action is required from those who are adapting the API.