[core] (Windows) Remove all Machine PATH handling, add safety mechanisms

[confused-Techie]

As we have seen, there have been a few reported issues of dealing with the Machine PATH in Pulsar. These issues stemming from:

• Unclear instructions, as Pulsar must be opened as Admin for this to work at all
• Issues of the checkbox disappearing and reappearing
• Issues setting the PATH in some instances.

For all of the above reasons, it seems like a fair trade-off to stop attempting to manage the Machine PATH at all, and instead opt to only ever modify the User PATH. This does mean that Pulsar installed on a shared machine will require each user to enable Pulsar on their user profiles PATH, but if anything that almost makes complete sense to me, rather than Pulsar trying to ensure it's on everyone's PATH.

As such, removing this handling allowed much simply logic both in terms of displaying these UI elements, and the logic for controlling how the PowerShell script is started.

Even further it allows the PowerShell script to be much simpler, and faster, by removing any self-elevation logic, and pseudo progress bars.

Beyond that, I've ensured to add additional safety features here, such as much more clearly ensuring to not modify the PATH if we have already done it, saving a copy of the users path prior to any modification, and overall studying how other older programs handle this logic (namely Chocolatey).

From here, the only real downside that I can find within this code is that we don't handle auto expanding variables within the user's path. Which is (as far as I can tell) an uncommon use-case, and something we can't easily get around, since getting around this requires much more complex calls, and a more direct manipulation of the registry, which then brings in other issues we have to worry about like character limits, etc. So this seems like a fine trade off.

---

Additional testing should be required prior to merging, specifically I'd be most curious about behavior on uncommon installation locations, and various machine installs, which over the next couple of days I'll do my best to get around to.

---

Fixes #816

[DeeDeeG]

Sounds good from the PR description. I suppose I should volunteer to test this one myself some time, but will be busy with Regular release today I think. But if I haven't come back to test this and no-one else has, feel free to ping and request it!

[DeeDeeG]

Rubber-stamp approve in case I don't get time to test this properly.

EDIT: I am going to attempt to test this right now, though, will see how far I get with it. Reporting back soon if all goes well.

[DeeDeeG]

Well, I do get an error / stack trace:

installType is not defined

Full stack trace (click to expand):

installType is not defined
Hide Stack Trace
ReferenceError: installType is not defined
    at new PathOption (C:\Users\user\Downloads\Pulsar-1.114.2024031807-win\resources\app.asar\src\main-process\win-shell.js:81:24)
    at Object.<anonymous> (C:\Users\user\Downloads\Pulsar-1.114.2024031807-win\resources\app.asar\src\main-process\win-shell.js:228:20)
    at Object.<anonymous> (C:\Users\user\Downloads\Pulsar-1.114.2024031807-win\resources\app.asar\src\main-process\win-shell.js:230:3)
    at Module._compile (C:\Users\user\Downloads\Pulsar-1.114.2024031807-win\resources\app.asar\src\native-compile-cache.js:120:30)
    at Object.value [as .js] (C:\Users\user\AppData\Local\Programs\Pulsar\resources\app.asar\src\compile-cache.js:252:23)
    at Module.load (internal/modules/cjs/loader.js:935:32)
    at Module._load (internal/modules/cjs/loader.js:776:14)
    at Function.f._load (electron/js2c/asar_bundle.js:5:12913)
    at Function.o._load (electron/js2c/renderer_init.js:33:379)
    at Module.require (internal/modules/cjs/loader.js:959:19)
    at require (C:\Users\user\Downloads\Pulsar-1.114.2024031807-win\resources\app.asar\src\native-compile-cache.js:67:27)
    at Object.get [as WinShell] (C:\Users\user\AppData\Local\Programs\Pulsar\resources\app.asar\exports\atom.js:32:14)
    at Object.activate (C:\Users\user\AppData\Local\Programs\Pulsar\resources\app.asar\node_modules\settings-view\lib\main.js:55:56)
    at Package.activateNow (C:\Users\user\AppData\Local\Programs\Pulsar\resources\app.asar\src\package.js:242:27)
    at C:\Users\user\AppData\Local\Programs\Pulsar\resources\app.asar\src\package.js:211:27
    at Package.measure (C:\Users\user\AppData\Local\Programs\Pulsar\resources\app.asar\src\package.js:82:19)
    at C:\Users\user\AppData\Local\Programs\Pulsar\resources\app.asar\src\package.js:205:14
    at new Promise (<anonymous>)
    at Package.activate (C:\Users\user\AppData\Local\Programs\Pulsar\resources\app.asar\src\package.js:203:32)
    at PackageManager.activatePackage (C:\Users\user\AppData\Local\Programs\Pulsar\resources\app.asar\src\package-manager.js:780:36)
    at C:\Users\user\AppData\Local\Programs\Pulsar\resources\app.asar\src\package-manager.js:755:30
    at Config.transactAsync (C:\Users\user\Downloads\Pulsar-1.114.2024031807-win\resources\app.asar\src\config.js:925:22)
    at PackageManager.activatePackages (C:\Users\user\AppData\Local\Programs\Pulsar\resources\app.asar\src\package-manager.js:753:17)
    at PackageManager.activate (C:\Users\user\AppData\Local\Programs\Pulsar\resources\app.asar\src\package-manager.js:730:44)
    at C:\Users\user\AppData\Local\Programs\Pulsar\resources\app.asar\src\atom-environment.js:1044:21
    at async Promise.all (index 0)
    at AtomEnvironment.startEditorWindow (C:\Users\user\AppData\Local\Programs\Pulsar\resources\app.asar\src\atom-environment.js:1072:20)
The error was thrown from the settings-view package.

This is referring to C:\Users\user\AppData\Local\Programs\Pulsar\[... files here ...], which is empty on my machine.

So, maybe the error happens when there is no per-user install, only a system-wide (C:\Program Files\) install?

Error message encountered in testing

[confused-Techie]

Alright, for anyone that's willing to give this a review, especially @DeeDeeG since you've already taken a look, I've gone ahead and fixed the issues present, and tested this more thoroughly.

The issue you first encountered was me simply forgetting to remove a reference to a now non-existent variable. Otherwise I've also opted to log the users current PATH only when logging the path, meanwhile previously we were appending to the file.
Lastly seems some changes were needed to ensure the script could run, and I also added a reboot warning to the setting.

The reason I added the reboot, and a warning to anyone who tests, when you add Pulsar to the PATH in this script, it adds it to the Windows Registry, which doesn't immediately populate the current shell value, meaning that right away you can't use pulsar on the CLI until you close and reopen the shell session.

But this also means that Pulsar is unable to remove it until it's shell session has been restarted, which it seems that NodeJS must cache this info somewhere, because I had to fully restart explorer.exe to get NodeJS to refresh it's PATH variable, which at that point could the value then actually be removed via Pulsar.

All of this will be taken care of though if users have restarted between turning this setting on and turning it off, which should be rather common I'd think.

[DeeDeeG]

I might be barking up the wrong tree with this suggestion, but would it be possible to use something like setx to modify the PATH, rather than editing the registry?

• https://serverfault.com/questions/33681/how-can-i-modify-a-user-s-path-environment-variable-without-logging-out
• https://ss64.com/nt/setx.html

And to detect if the PATH string we want to add is already present, maybe scan the current PATH string for it, rather than consulting the registry?

🤔

[DeeDeeG]

Thanks for addressing this, I was going to report that this wasn't working for me in testing, but I felt bad just reporting "it didn't work for me" and another stack trace, so I was looking into it further.

I got a bit stumped about the right way forward though, something about the quoting vs the `` template string stuff but wasn't sure how much you'd already tried and ruled out. Heh.

Hopefully I'll be able to test this latest revision soon and report back.

[DeeDeeG]

Also, in some quick testing, the checkbox didn't seem to want to do anything if I had a system-wide install that I wanted on my per-user PATH.

It strikes me as reasonable to install once system-wide, and want that copy added to my PATH some way or other. I hope that's doable, I might have misunderstood since I haven't dived deep into the code for this yet.

[confused-Techie]

@DeeDeeG Alright, that's my bad, seems I didn't test Machine handling as thoroughly as I thought.

Things should now be functional. Seems there was issues because the installation location of Pulsar was being detected via it's registry entry, which on this rewrite only ever checks the current user's registry, which of course has no entry for Pulsar for a machine install, so Pulsar wasn't able to find the installation location on machine installs to provide when setting it's value in the PATH.

---

But to clarify on expected behavior.
Essentially now, no matter what type of installation Pulsar is, when a user clicks to add it to their PATH, it's only ever added to the Current User's PATH. The biggest change here besides any safety additions, or having the process log much much more, is that we no longer attempt in any way to modify the machine PATH.

[DeeDeeG]

Okay, I finally managed to test all configurations.

• System-wide and per-user installs, add to path from per-user install: Works. ✅ Launches the per-user install.
• System-wide and per-user installs, add to path from system-wide install: Works. ✅ Launches the system-wide install.
• System-wide install only: ⚠️ Error message in console? (Something about permission denied to access a .txt file living under C:\Program Files\Pulsar\ . . ., see full error message below...) But still works. ✅
• Per-user install only: Works. ✅

Note: All tested scenarios did appear to require a restart for the cmd.exe to load the new PATH entries and have pulsar --version and where pulsar work, which is just as the message says below the checkbox in the Pulsar settings-view UI for this preference.

The error message in console when adding the system-wide install to PATH was this:

Add Pulsar to PATH:
C:\Users\user\Downloads\Pulsar-1.115.2024032609-win\resources\app.asar\src\main-process\win-shell.js:132 stdout: 
C:\Users\user\Downloads\Pulsar-1.115.2024032609-win\resources\app.asar\src\main-process\win-shell.js:133 stderr: out-file : Access to the path 'C:\Program Files\Pulsar\prior2addition.txt' is denied.
At C:\Users\user\AppData\Local\Programs\Pulsar\resources\modifyWindowsPath.ps1:28 char:3
+   $env:Path > prior2addition.txt;
+   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    + CategoryInfo          : OpenError: (:) [Out-File], UnauthorizedAccessException
    + FullyQualifiedErrorId : FileOpenFailure,Microsoft.PowerShell.Commands.OutFileCommand

(From C:\Users\user\Downloads\Pulsar-1.115.2024032609-win\resources\app.asar\src\main-process\win-shell.js:133)

(Tangent: I don't know why DevTools thinks the JS file being run was loaded from my Downloads folder, I launched portable Pulsar from that path once and have since moved it to a different location several reboots ago, DevTools are just confused. The JS file doesn't even exist at that path anymore... What the heck DevTools?)

Side note about a potential "weird user experience" edge case: I also got an error when trying to re-add it after manually removing the PATH entries in Windows' env var editing UI without rebooting first. It said "Pulsar is already on the PATH" in the dev tools. Which was probably true if consulting the registry, but not true if checking the active PATH entries one could see in cmd.exe. So, users may find his unintuitive, but the solution is to reboot and then check if it's actually in the PATH, if not then re-add and it should work. And implicit in this solution is: Don't manually mess with the PATH entries if you don't have to, as an end-user of Pulsar trying to use the "Add to PATH" UI checkbox in Pulsar's settings-view.

---

I found the code somewhat intimidating to look through and review, so I'm not planning on doing that. Approving on the basis of it's tested and working. 👍

However, some time in the future, if it's possible to avoid messing with the registry, rebooting, etc. etc. etc., and drastically simplify the code, perhaps by calling setx instead, I would highly recommend a simplified approach for reliability, readability, and potentially better user experience, assuming the implementation as I'm envisioning it is in fact possible, which is not a given.

---

In any case, thanks for all the work on improving this!

[confused-Techie]

Thanks a ton for the review!

With that I'll probably want to merge this, but will address some of the points you brought up and leave it open for a tad longer in case there's anything you'd want to respond to with that, but otherwise no need to.

But as for the error message you see about access denied, that part is expected, I did see it in testing and should've called it out. But when we now mess with the PATH we try and save a copy of the users path to Pulsar's resources folder, but for machine installs, since Pulsar and the PowerShell script isn't launched as admin, we can't do that. And from there I couldn't think of any other good place to save this file. In all likelihood we will never need it, it essentially exists just in the off chance we somehow corrupt someone's existing PATH, they have the ability to manually reenter themselves. The only possibly situation where I could see this happening is if someone were to be so close to PATH limits that the automatic variable expansion that happens when reading these values via PowerShell were to cause them to go over the limit, but in all reality that would likely just mean that the Pulsar values themselves are lost, or it was a dangerous situation to be in anyway. And from what I can tell very very few programs actually use variables here, so I'm not too worried about it. Just thought it'd be a nice piece of mind safety thing. So I don't see this as a loss on not being possible on machine installs.

As for reboots. This one may be possible to fix in the future. Essentially when a shell in Windows is started, it "caches" the machine and user path values (along with other stuff) and uses that when running. This means that the values in the registry could change, but that shell session wouldn't be aware of it.
Now, when we modify the registry we could in fact manually overwrite that shells PATH value with the machine and user PATH values from the registry, which would give the sense of pulsar being available on the registry right away (this is what tools like Chocolatey do during installation) the thing is, I wasn't able to determine if that would work.
I was unable to determine if the "cached" PATH value is specific to each shell session, or shared across all shell sessions. Since we could do this reload, but it may only effect the child process within Pulsar itself, not the user's actual shell. Without being able to find good information there, I've opted to leave that alone for now, and ask that the user reboot. Which isn't the most uncommon thing, and in practice they should just have to restart their shell session.

Finally your comment on setx, while we could use it, I'll detail why I haven't. Honestly partially is much of the complexity will still exist, since mostly a lot of what we are doing is validating the existing PATH and validating how we add it, such as having a trailing ; and such. We would still have to do all of that with setx. The other more prominent issue is setx can only work with values up to 1024 characters long, which while yes it's longer than any single Windows path can be (256 for reference) it's still much shorter than the default Windows PATH environment variable limit of 2048 characters, and much shorter than the theoretical maximum of the PATH at 32,760 characters (but we could never hit that limit in practice, or especially have any of it be very useful after 2048 characters).
So basically, I didn't use setx because it'd be nearly just as complicated, and has the big potential to lose values from a user's PATH, if they have a lot of stuff in there. (For example when originally testing methodologies here, I did in fact use setx and ruined the PATH on my machine, since I was over that limit) Then considering I was much more familiar with PowerShell I thought it seemed a fine way to go.

[DeeDeeG]

Okay, sounds well thought out. I especially didn't realize setx limits to so few characters.

(There are profound pros and cons to Windows not breaking a lot of its low-level stuff from decades ago... I guess one of them is CLI tools literally from another decade or several ago having weird limits that aren't that relevant to modern hardware.)

[ . . . ] And from there I couldn't think of any other good place to save this file. [ . . . ]

It could be put somewhere in the .pulsar folder, I guess? (I don't know if you like that suggestion and whether you'd want to do that in this PR or a follow-up one, but I don't mean to encourage endless review cycles on this one, so doing it as a follow-up, if it sounds like a good idea in the first place, would be a-ok in my book. Leaving that all to your preference/judgment.)

[DeeDeeG]

Also: It was possible to add multiple copies of the Pulsar paths to the PATH. I didn't reverse-engineer the exact steps to reproduce, though. It might be checking the box multiple times in a row before ever re-booting?

EDIT: I don't think this is a huge deal, since "searching the same paths for bins multiple times" wouldn't really have any adverse effects that I'm aware of. It's simply unexpected and adds a slight amount of clutter and extra length to the PATH.

[DeeDeeG]

If there are any improvements possible, I am quite sure we can do them as follow-ups. So, no reason not to merge. Thank you!