pulsar-edit / pulsar

A Community-led Hyper-Hackable Text Editor
https://pulsar-edit.dev
Other
3.24k stars 137 forks source link

[windows] Add PATH manipulation to Pulsar installer #1071

Closed confused-Techie closed 2 weeks ago

confused-Techie commented 2 months ago

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

savetheclocktower commented 2 months ago

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

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

confused-Techie commented 1 month ago

@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 commented 1 month ago

@savetheclocktower How does this look?

image

This also includes having the boxes checked by default

savetheclocktower commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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

savetheclocktower commented 1 month ago

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:

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 commented 1 month ago

@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 commented 1 month ago

@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.

savetheclocktower commented 1 month ago

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 commented 1 month ago

@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 commented 2 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

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!