Closed confused-Techie closed 7 months ago
DeeDeeG
commented
7 months ago 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
commented
7 months ago Well, I do get an error / stack trace:
installType is not defined
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?
confused-Techie
commented
7 months ago 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
commented
7 months ago 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?
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
commented
7 months ago 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
commented
7 months ago @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.
confused-Techie
commented
7 months ago 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
commented
7 months ago 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
commented
7 months ago 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
commented
7 months ago 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!
As we have seen, there have been a few reported issues of dealing with the
MachinePATH in Pulsar. These issues stemming from: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