Notify processes to refresh their environment variables

[DavisVaughan]

Closes #1614

This currently only handles the powershell installer, not msi. If someone wants to add and test msi support that would be great! Also, can someone please take over updating snapshot tests for me? I tried but got weird diffs like this:

And a few other failures I couldn't figure out.

---

The opening post here #1614 (comment) discusses the problem in detail.

I opted to bring in the (well known?) implementation from the powershell gallery
https://www.powershellgallery.com/packages/PSCI/1.0.4/Content/core%5Cutils%5CUpdate-EnvironmentVariables.ps1

I tested this on a Windows machine and this does indeed seem to work for me! Here's a video of me showing that

• previously I did not have .local/ in the registry
• then I ran the patched installer
• then it was in the registry (but still the cli tool air was not accessible yet)
• then I restarted Powershell and air was available

https://vimeo.com/1040789276?share=copy#t=0

---

@fdcastel noted that it did not work for him. I believe I can explain that. Likely what happened was:

• He ran the old installer once. It updated the registry, but did not tell processes to update their environments
• He patched the old installer with the code in this PR, but did not restart his machine
• He ran the patched installer. It detected that the registry had been updated already, and did not update it again (thus, not pushing an environment refresh notification).
• Because no refresh notification was pushed, it looks like nothing happened

Running Get-Item -Path "HKCU:\Environment" would likely show an updated Path for him, even though ruff won't be accessible at the command line until after a system restart.

[duckinator]

Hi! Thank you very much for taking this on!

I can take over updating the snapshot tests. 🙂

(EDIT: Thank you Misty for taking over!)

[mistydemeo]

I tried but got weird diffs like this:

Ah, I bet that comes down to the cargo insta version. 1.41.0 added support for a second snapshot format so it added the snapshot_kind: key; if you're using an older version, it'll remove those keys when updating snapshots. That's not a big deal though, for now insta will transparently read snapshots whether it's got them or not.

[mistydemeo]

I've pushed an update with snapshots and a couple minor tweaks:

• Silencing a couple PSScriptAnalyzer errors that aren't relevant to us, and
• Linking to the source and version number of the function.

[DavisVaughan]

@fdcastel makes a good point in #1614 (comment) that I do think should be considered before merging

I thought for sure SetEnvironmentVariable() was only for the current process, but it does seem like it can modify the user registry with something like this:

Environment.SetEnvironmentVariable(userEnvVar, user, EnvironmentVariableTarget.User);

If target is EnvironmentVariableTarget.User, the environment variable is stored in the HKEY_CURRENT_USER\Environment key of the local computer's registry. It is also copied to instances of File Explorer that are running as the current user. The environment variable is then inherited by any new processes that the user launches from File Explorer.

If target is User or Machine, other applications are notified of the set operation by a Windows WM_SETTINGCHANGE message.

The main thing that strikes me as odd is why this hasn't been recommended by any online resources that I found! It does seem "more official" than this approach though.

At the very bottom in "Applies to" it looks like it does support quite old .Net

https://learn.microsoft.com/en-us/dotnet/api/system.environment.setenvironmentvariable?view=net-9.0
https://learn.microsoft.com/en-us/dotnet/api/system.environmentvariabletarget?view=net-9.0
https://learn.microsoft.com/en-us/powershell/module/microsoft.powershell.core/about/about_environment_variables?view=powershell-7.4#set-environment-variables-with-setenvironmentvariable

[fdcastel]

@DavisVaughan, absolutely!

As a Windows user and programmer for, err... longer than I care to admit, I can vouch for this method being old. But it works😉.

That said, it is practically a newborn compared to those ancient Win32 messages -- which predates even the .NET framework itself! (being there, done that...) 😄

[DavisVaughan]

Whew, okay, holy cow. Windows powershell is SUCH a minefield with anything PATH related.

I have determined that there are a few issues, all related to the fact that for PATH in particular, it is not uncommon for users to use expandable variables in their PATH, like: PATH = \.local\bin;%EXPANDME%, where they also have another environment variable of EXPANDME = 'my\path'. The idea is that when you print PATH at Command Prompt or Powershell, those get "expanded" into the underlying value somewhat dynamically.

The issue with SetEnvironmentVariable()

SetEnvironmentVariable() cannot be used for PATH. It is a very useful tool, but not for PATH. In the Registry it forcibly changes PATH from a REG_EXPAND_SZ data type to a REG_SZ data type. This means that %EXPANDME% will be in the PATH but will no longer ever expand to the right value.

There is no workaround to SetEnvironmentVariable() for this. It will always set the data type to REG_SZ.

The current use of Get-ItemPropertyValue is already expanding

The way cargo-dist currently works is to "get" the current Path value, tweak as required, and then re "set" it.

The problem is that currently "get"ting the Path value using Get-ItemPropertyValue is expanding it!

cargo-dist/cargo-dist/templates/installer/installer.ps1.j2

Line 517 in 7872825

$OldPath = $Item | Get-ItemPropertyValue -Name $PropertyName

i.e. from the above example %EXPANDME% would be expanded into my\path as we retrieved it, then we'd hardcode it back to my\path as we "set" the Path, meaning if I update EXPANDME in the future then my Path won't respond to the change anymore. I believe it has already permanently expanded my %NVM_HOME% and %NVM_SYMLINK% variables 😬.

Here is a way to avoid expansion:

$Item = Get-Item -LiteralPath 'registry::HKEY_CURRENT_USER\Environment'
$Item.GetValue('Path', '', 'DoNotExpandEnvironmentNames')

The current use of New-ItemProperty is forcing a REG_SZ type

Right here: https://github.com/axodotdev/cargo-dist/blob/7872825c67f02f23d483432499fb394a9405c8de/cargo-dist/templates/installer/installer.ps1.j2#L539C13-L539C29

Note -PropertyType String. That causes the Path to get stored as REG_SZ, which is the same problem as SetEnvironmentVariable(). Luckily, if we use -PropertyType ExpandString then it will correctly get stored as REG_EXPAND_SZ.

Worth noting that rustup is careful to also use ExpandString when resetting the PATH right before sending out the broadcast message:

• https://github.com/rust-lang/rustup/blob/dc9d072c34eccbee2d9249daea177b9c4263c5da/src/cli/self_update/windows.rs#L483
• https://github.com/microsoft/windows-rs/blob/ad8d166e070484066ccccf9754af64f7415025d8/crates/libs/registry/src/key.rs#L113-L119

You can see this issue in this screen shot which was taken after using the current version of the installer. See how everything else is REG_EXPAND_SZ?

Is there no hope?

There is an excellent post all about this here:
https://stackoverflow.com/questions/69236623/adding-path-permanently-to-windows-using-powershell-doesnt-appear-to-work/69239861#69239861

It includes an Add-Path helper that solves all of these problems and more:

• It uses GetValue('Path', '', 'DoNotExpandEnvironmentNames') when retrieving the current Path
• It uses -Type ExpandString when creating/updating the Path
• It broadcasts WM_SETTINGCHANGE by utilizing [Environment]::SetEnvironmentVariable() to create and then immediately remove a dummy environment variable, which has the side effect of sending out WM_SETTINGCHANGE. Pretty clever I think. And uses the "officially exposed" API to broadcast that message.
• It even updates the current Path by modifying $env:Path before it exits, so you don't even technically have to restart your shell (we could remove this part if it makes us uncomfortable)