Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[windows] Add PATH manipulation to Pulsar installer #1071

Merged
merged 6 commits into from
Sep 13, 2024

Conversation

confused-Techie
Copy link
Member

@confused-Techie confused-Techie commented Jul 30, 2024

EDIT: WIP: Forgot to set this to draft, I still need to implement the removal of the in-pulsar method of PATH manipulation.


After #957 which was intended to be the end all be all of adding Pulsar to the PATH on Windows, it unfortunately had one big flaw. If the user has their ExecutionPolicy set to a higher security setting, the script would fail.

This is seen in #978.

In this PR, we remove adding Pulsar to the PATH from the editor entirely. Instead it is now an option to add Pulsar to the PATH during installation. Just like my last PR to address issues here, we only ever manipulate the User's PATH, but now it is an optional checkbox during installation of Pulsar.

image

Pros

This is now essentially bullet-proof. Being able to run on every single Windows system without issue.

We don't need any trickery to figure out where Pulsar is installed, since we know where it is during installation.

We are easily able to remove the values from the user's PATH during uninstall.

Cons

If the user decides later that they want Pulsar on the PATH, they must reinstall Pulsar.

Technically, the APIs available to modify the user's PATH are less safe, than what we have available via PowerShell. Which is why I originally wrote the script in PowerShell and worked to keep it there. Since within the NSIS docs the recommended way to do this is with EnvVarUpdate. But on that link you'll notice the big warning not to use this software anymore. The alternative EnVar_Plugin is recommended, but unfortunately I cannot find any way to include that into electron-builder. But luckily it seems the reason EnvVarUpdate is no longer recommended has actually been resolved. Because of this it seems this script is now safe to use, and as you can see, is what I am using for this implementation.


I plan to work over the next few days to get the "Change"/"Modify" options to appear in the Windows application menu, allowing users to easily and intuitively re-run the installer. So that they will have an easier method of adding Pulsar to the PATH after the fact. This way we can get rid of one of the few cons on this method.


Fixes #978

Copy link
Sponsor Contributor

@savetheclocktower savetheclocktower left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I might be able to test this tomorrow, but in the meantime I can give some feedback about phrasing and arrangement.

I'm amazed that you threw this together in an evening. I could stare at this farkakte scripting language for eleven years and not understand it any better.

resources/win/installer.nsh Outdated Show resolved Hide resolved
resources/win/installer.nsh Outdated Show resolved Hide resolved
resources/win/installer.nsh Outdated Show resolved Hide resolved
confused-Techie and others added 3 commits July 29, 2024 23:59
Co-authored-by: Andrew Dupont <github@andrewdupont.net>
Co-authored-by: Andrew Dupont <github@andrewdupont.net>
Co-authored-by: Andrew Dupont <github@andrewdupont.net>
@savetheclocktower
Copy link
Sponsor Contributor

savetheclocktower commented Jul 31, 2024

Field report: a bit rough, but it did what it set out to do:

  • Uninstalled all versions of Pulsar from my Windows machine and removed the existing entries from my PATH.

  • Downloaded the binary artifacts from the CI job and ran the setup EXE.

  • Selected a local installation.

  • Once I got to the new screen, I noticed that we need to make more room for my verbosity:

    windows-setup-step-3-cut-off

  • It would also be great if these options were checked by default.

  • I checked both options and clicked on Next. I waited for about 30 seconds while the setup wizard appeared to be nonresponsive; I was wondering if I should force-quit the process when, suddenly, it sprang back to life. I did have some of the windows still open from having removed the entries from my PATH environment variable, so I suppose it's possible that it was unable to modify the PATH until I confirmed my previous removal. But it's probably got nothing to do with that.

  • I then saw the standard progress screen. It got most of the way across the screen, then stopped here for about two minutes:

    windows-setup-step-4-interminable

    It was taking so long that I thought I should take that screenshot above — and grabbed it in the nick of time, since the progress bar started advancing again right afterward.

  • Finished setup and checked my PATH; both entries were there.

  • Running pulsar from the terminal worked great, as did some of the advanced-level usages like pulsar --wait.

So it sounds like the to-do list is something like:

  • Fix the cut-off text on the new screen.
  • Figure out how to allow the setup wizard to be re-run via the Modify button in Add/Remove Software section (or Settings > Apps & features on Windows 10, or whatever they've renamed it to on Windows 11).
  • Remove the existing method of setting path via the Windows-specific pane in Settings.

@confused-Techie
Copy link
Member Author

@savetheclocktower Thanks for giving this a test!

I will say the timing issues you experienced are as far as I can tell identical to previous versions of Pulsar. Likely coming down at least partially to Windows not handling lots of little files well, and that being the entire contents of our asar and unpacked.asar files. So I'm hoping this isn't causing any negative effects there but will keep an eye out for it.

Appreciate the todo list, as that seems exactly right.

But I'm currently working on removing the previous PATH modification systems, and like we discussed on Discord the likely-hood of having the "Modify" button functional may be a bit further down the road. But I'll test out some new sizes and see if I can't get the new strings to show up properly.

@confused-Techie
Copy link
Member Author

@savetheclocktower How does this look?

image

This also includes having the boxes checked by default

This does add a notice about the change instead. Which should removed within a few versions.
@savetheclocktower
Copy link
Sponsor Contributor

It looks great.

I think it's still worth testing a bit more and maybe getting a third opinion from a Windows user. I'll try to remember to test it tomorrow as a machine install rather than a single-user install.

@confused-Techie
Copy link
Member Author

Great point, getting testing from a Windows user would be important.

I'll see what I can do about testing a few other platforms (especially making sure to test on Windows 11). Otherwise @Spiker985 if it's easy at all to test, it'd be appreciated

@savetheclocktower
Copy link
Sponsor Contributor

Still have to make time to test this again. I'll see if I can get to it tomorrow.

@savetheclocktower
Copy link
Sponsor Contributor

My second attempt at reviewing this (by downloading the binaries produced by CI) went just fine:

windows-setup-step-3

The only things that would possibly give me pause are the things I mentioned last time that are still present:

  • After choosing PATH options, the setup wizard went nonresponsive for at least 10 seconds
  • During the actual installation, the progress bar stops at about 2/3 along and stays there for an annoying amount of time

I don't remember what my installation experience was like before, but if these aren't regressions from what's typical for Windows, then I'm happy to live with them.

@confused-Techie
Copy link
Member Author

@savetheclocktower Thanks for another look at this PR.

But yeah from my experience these aren't regressions. At least the installation stopping halfway is something that's been pretty standard for quite some time, in my experience. As for the pause, I didn't see any difference on my installation. But I can look into if there's any way to add some additional logging to the installer, so we can attempt to determine if that pause is the result of this PR or not. So I'll get back to you with that

@confused-Techie
Copy link
Member Author

@savetheclocktower So I looked around, and it seems the only simple way to get some debug logging is not very simple). Essentially requires an entire custom NSIS binary.

Maybe an easier answer is running a recent installer and seeing if that also happens on your system. Otherwise I'd be happy to test the install on a few other systems and see if I notice any difference.

Copy link
Member

@DeeDeeG DeeDeeG left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lite-approve, sorry it took me so long to test this.

Now with testing this myself, I can see it "just works". Even adds Pulsar instantly to the PATH, just had to open a cmd.exe window and pulsar --version "just works". Brand new Windows install, so I know it wasn't a left over copy.

Whatever other technical details, I haven't looked super thoroughly through, and I do like to be more technical, but this is such an improvement to the user experience I am willing to go with it as-is.

I may spend some time later in the month looking into it closer, but did not want to block things for lack of a review+, IMO personally.

Thanks for doing this, and the patience for review!

P.S. EDIT to add: It didn't seem to freeze up after checking or unchecking the "Add to the User PATH" buttons. FWIW. Install speed seemed reasonable, as far as any install of an app with a lot of small files in it on Windows would be (expected to be slow, IMO, but still just a couple of minutes. SATA SSD for what it's worth.)

@savetheclocktower
Copy link
Sponsor Contributor

P.S. EDIT to add: It didn't seem to freeze up after checking or unchecking the "Add to the User PATH" buttons. FWIW. Install speed seemed reasonable, as far as any install of an app with a lot of small files in it on Windows would be (expected to be slow, IMO, but still just a couple of minutes. SATA SSD for what it's worth.)

Happy to learn that this might just be my own experience. (Also an SSD in my case, but it's a Windows 10 machine from 2016ish, so occasional quirks are hardly surprising to me.)

@confused-Techie
Copy link
Member Author

@DeeDeeG Thanks for giving this a review!
I know there was some other details you wanted to touch on, on the more technical side, so I'm more than happy to leave this open a little while longer prior to a release, in case you get the chance to dive into those. But if you don't very glad to see we can get this one merged before the next release

@DeeDeeG
Copy link
Member

DeeDeeG commented Sep 13, 2024

I'd like to merge this one and had in mind to do it early in the cycle to get one or two people testing it in Rolling, but I'm not sure how many people install Rolling on Windows, as opposed to just grabbing a .zip until next Regular??

I can see I'm unlikely to be able to dedicate time to learning .nsh file format, so yeah, more in-depth technical review from me is likely not forthcoming.

Considering the above, I would be more comfortable getting this into Regular than waiting and seeing this likely not get merged for quite a while otherwise.

I think this will be convenient enough that folks may not want to "go back" to alternatives. Could be wrong. Oh, well. But it's a nice improvement for the users, IMO.

@DeeDeeG
Copy link
Member

DeeDeeG commented Sep 13, 2024

One thing I'd intended to test was if multiple re-installs added to the PATH multiple times, but hadn't set aside time to test it specifically at this point. But I don't want to hold everything up. Maybe I can get to testing it today, otherwise I don't want to hold things up.

EDIT: Should be fine:

Duplicates [sic] entries [ . . . ] are removed from the contents of PATH before performing the requested operation. This step prevents the corruption of the variable's contents (e.g., when removing a target pathname that is a subset of another pathname).

Per: https://nsis.sourceforge.io/Path_Manipulation#Description

@confused-Techie
Copy link
Member Author

I appreciate the update @DeeDeeG, and in that case I'll go ahead and merge this one now, thanks again for the review and feedback!

@confused-Techie confused-Techie merged commit a1095a8 into master Sep 13, 2024
103 checks passed
@confused-Techie confused-Techie deleted the win-path-during-install branch September 13, 2024 20:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Add Pulsar to PATH
3 participants