Improved Windows Install (PATH, ATOM_HOME, InstallLocation)

[confused-Techie]

Alright, so while this PR initially aimed to solve one thing, I've ended up taking the time to make many of the improvements I've wanted to see on Windows for a while now.

What this PR Does

During Installation

Now during installation, we use a custom NSIS script that will set the InstallLocation into the registry, which allows our GitHub Desktop Integration working.

Within Settings

Now within settings on the familiar "System" page, underneath the options:

• Register as file handler
• Show in file context menus
• Show in folder context menus

We will now show: "Add Pulsar to PATH"

Technically, there are two versions of this button that will appear. Since Pulsar can be installed two ways on windows, either under the User account or Machine account. So when this page loads Pulsar will quickly check if it believes this was a user installation and if so show "Add Pulsar to PATH (User)" but if it doesn't think it's a user install, Pulsar will instead show "Add Pulsar to PATH (Machine)" which the user can only click if Pulsar is open as an administrator, which is required to be able to make those changes.

The additional functionality to handle this has been placed within ./main-process/win-shell.js along with the rest of the handling for this page. Creating a new class PathOption that assists in setting and unsetting these values on the PATH.

Since of course there's issues with setting the PATH directly within the registry, this function will now callout to the included PowerShell script on Windows that can modify the PATH safely.

This now means that GitHub Desktop should work for all users, and within Windows there's a very easy way for users to add or remove Pulsar, PPM, and the ATOM_HOME value from their PATH.

---

One note all of this has made me think about, is in #274 there are concerns about some registry edits not being removed on an uninstall. Do we possibly want to build in some functionality to do a total cleanup of Pulsar on uninstall? This would mean removing Pulsar, PPM, ATOM_HOME from the Path, (and user environment variables) as well as removing Pulsar from the file/folder context menu, and as a file handler.

This is possible, would just need to be explicit if this is something we wanted to pursue.

The last note I'd like to make here, is since we can't access the machine variables within the registry, unless Pulsar is launched as Administrator on Windows, we can't set any of these variables as the machine user. We could add a second checkbox to accomplish this, that informs Pulsar must be relaunched as admin, or otherwise I don't think it's the biggest issue if we can't.

---

EDIT: Relates to #565

[confused-Techie]

FYI, this PR has been updated with new big changes, including simplifying our post install steps, and including the Windows PATH additions to the "System" menu within Pulsar's settings

[Daeraxa]

So the main thing I would say here is that (if I understand this correctly) we have two different methods of doing the same thing (well actually 3) between the different OSs.

Linux currently has nothing but macOS already has the `Install Shell Commands" command and menu item:

pulsar/menus/darwin.cson

Line 21 in 1b5c02e

{ label: 'Install Shell Commands', command: 'window:install-shell-commands' }

pulsar/src/register-default-commands.js

Lines 231 to 234 in d0d5398

	if (process.platform === 'darwin') {
	commandRegistry.add('atom-workspace', 'window:install-shell-commands', (function() {
	return commandInstaller.installShellCommandsInteractively();
	}), false);

pulsar/src/command-installer.js

Lines 21 to 45 in d0d5398

	installShellCommandsInteractively() {
	const showErrorDialog = error => {
	this.applicationDelegate.confirm(
	{
	message: 'Failed to install shell commands',
	detail: error.message
	},
	() => {}
	);
	};
	this.installAtomCommand(true, (error, atomCommandName) => {
	if (error) return showErrorDialog(error);
	this.installApmCommand(true, (error, apmCommandName) => {
	if (error) return showErrorDialog(error);
	this.applicationDelegate.confirm(
	{
	message: 'Commands installed.',
	detail: `The shell commands \`${atomCommandName}\` and \`${apmCommandName}\` are installed.`
	},
	() => {}
	);
	});
	});
	}

[Spiker985]

The last note I'd like to make here, is since we can't access the machine variables within the registry, unless Pulsar is launched as Administrator on Windows, we can't set any of these variables as the machine user. We could add a second checkbox to accomplish this, that informs Pulsar must be relaunched as admin, or otherwise I don't think it's the biggest issue if we can't.

Something we can do as well, is make an option for Add to Path (Machine) which would attempt to self-elevate the PS script

If the elevation fails, we can have it return an error code specific to that, and then handle that with an Alert inside Pulsar. It can be the same script, with different calling parameters which calls the self-elevation function, before creating a new powershell instance with the script, but with the Runas verb instead

Something else that may need to occur, is the signing of the PowerShell script as most systems are set as Restricted or RemoteSigned ExecutionPolicy by default.

Edit: Should've read the files more closely, you already have it configured for explicit Bypass

[confused-Techie]

@Spiker985
Wait your last point here is totally valid. Yes if the PowerShell script is called via NSIS installer, then yes that is configured explicitly as 'Bypass', but in the current implementation we are actually calling the PowerShell script via ChildProcess within Pulsar, meaning that signing the script may still be required. Is this a process you are familiar with? Since we currently don't sign anything on Windows, so I'd hate to do that kind of signing here on the script, but obviously if it's necessary

[Spiker985]

Well, here's Microsoft's doc on the subject: https://learn.microsoft.com/en-us/powershell/module/microsoft.powershell.core/about/about_signing?view=powershell-7.3 (Note this is for 7.3 which is the new LTS, but any machine from Windows 8.1+ will have 5.1 and anything before that will have 5, 3, or 2)

But I personally haven't ever signed any scripts, unfortunately

[confused-Techie]

@Daeraxa You are totally correct. Realistically trying to sync up these abilities I think would be a really good idea.

I'd ideally see us have a "System" settings page that changes based on what OS you are on, where that contains either this implementation of Windows settings, or a new macOS page that shows a GUI to install shell commands, and who knows maybe the same thing (if at all needed) in Linux.

Then of course, we bring the windows handling to match macOS where it gets a Command Palette option, and maybe even menu.

Ideally so that all operating systems have their own set of "System Settings" whatever that is depends on the specific platform obviously. But then you'd be able to do any of those, from any platform in the same:

• Command Palette: Ideally with a similar name, or even a new section for "os" or something
• Menu: Ideally in the same location, or at least under the same header. So someone coming from one OS to another knows where to look
• System Settings Page: So that they can always be found here easily

Obviously all of that is a bit out of scope here, but just wanting to touch on what I feel like might be good steps for the next PR on this subject, so that we can try to get everything feeling for cohesive between each OS.

[Daeraxa]

Yeah I like this approach with a dedicated settings screen more than I do the menu item.

It would be good to have something for linux but I'm not certain what exactly because of all the different kinds of packages - for example this would be helpful for appimages.

Which actually brings me to another question - what happens if you run this from the .zip version of pulsar in windows?

[confused-Techie]

So this might not be as good of a solution as I had hoped.

@Spiker985 you're mention of needing to sign the PowerShell script sent me down a rabbit whole of research here. But you are 100% right, and it seems the signature needed for PowerShell Scripts is the exact same signature needed to sign the Windows binary. In other words we have all discussed this before, and the cost just does not meet the benefit. So I actually don't think this is possible to accomplish in it's current implementation.

That is either, I switch this to run a bat file instead, run the PowerShell script as a single line command, or see if there's some sort of JavaScript library we could try to us here. Or I could rewrite this to interact directly with the registry, which again could have some negative side effects if we go over, or a user is already over PATH length limitations (As in if we aren't safe about it editing values directly in the registry could corrupt the registry as a whole, which is why I wanted to use PowerShell for this purpose).

So there might be a bit of going back to the drawing board here.

---

@Daeraxa As for what happens for the portable application that depends, I'm assuming the portable application makes no attempt to modify the registry? In which case will likely fail. Since to add Pulsar to the PATH obviously, it needs to know where Pulsar is located, which it does by explicitly checking the InstallLocation within the the registry entry for Pulsar. So if the portable version doesn't touch the registry at all, then (While there does seem to be some kinks I'm still working on on the returned errors from this) the user should get a message along the lines "Unable to find Pulsars PATH". Which tracks, it wouldn't make much sense (at least to me) to add a temporary path to the user's PATH, as if they move the zip folder, it'll break.

[Daeraxa]

I agree it makes no sense, I was thinking more about useful errors and making sure we cover the bases because you just know somebody is going to open an issue for it so for that kind of thing we would probably want to somehow detect it is the portable one and grey out the option or something.

[Spiker985]

That is either, I switch this to run a bat file instead, run the PowerShell script as a single line command, or see if there's some sort of JavaScript library we could try to us here. Or I could rewrite this to interact directly with the registry, which again could have some negative side effects if we go over, or a user is already over PATH length limitations (As in if we aren't safe about it editing values directly in the registry could corrupt the registry as a whole, which is why I wanted to use PowerShell for this purpose).

So technically, you can launch powershell with the -EncodedCommand argument, and provide a Base64 encoded string and it will execute it. This is ironically how a lot of threat actors utilize powershell.

So we can technically ship the script, get-content it, convert it to base64, and then execute it. Since it isn't reading from a file, and only uses memory it isn't considered a script - so the native LanguageMode doesn't enact. Double up with explicitly specifying ExecutionPolicy Bypass, and it should work fine - albeit, annoying.

Not a great solution, but it is a solution. (I have to think about this type of stuff all the time to work around work policies)

[savetheclocktower]

If the PowerShell script can be run more easily via an installation process than after the fact (which I think is my understanding from reading this thread?) then it makes perfect sense for there to be different processes for installing shell commands based on platform.

Ideally, this is how I imagine it working:

• On all platforms, if we can tell that shell commands are not installed — or if we can't tell one way or another — we should put an “Install Shell Commands…” menu item in the appropriate menu location for the given OS.
• Since Windows has an installer and other platforms don't, it'd also be natural to run the install-shell-commands script during the installation process if the user checks the box.

If Windows doesn't make it easy to run a PowerShell script programmatically unless it's signed, then the “Install Shell Commands…” option on Windows could fall back to a page that tells the user how to run the script manually. That'd still likely be easier than having them find the environment variables screen that dates back to WIndows 95 and trusting them not to make a typo when editing the PATH variable.

I know that code signing on Windows is a gigantic can of worms and it's probably going to be a pain in the ass, but I suspect we're not done with it yet. If I get time, I'll try to find an Electron project that's in a similar situation as ours — open source, no big bankroll behind it — and figure out what they've done.

[confused-Techie]

@Daeraxa You make a great point about returning the proper error messages so users can see what's going on. One issue with that, is technically the same error of "Unable to find Pulsar's Path" would be returned either if the installation is a Machine install, or it's a portable install. There's no easy way to differentiate. This does loop in slightly with the next part of my message. But I think really we could safely return the error as being one of those two, and we trust the user knows enough about their setup to determine which is the case here.

---

@Spiker985 I do love the suggestion to have the script attempt to self elevate, and we just include two options here, for User install, and machine install, so I think I'll go through with adding that.

Otherwise the idea to get the content of the script and run it that way is a fantastic one! So I'll look into that to see what we can do, thanks for the awesome suggestion!

---

@savetheclocktower So actually, I'm not sure if it's easier to do this during installation. I originally intended this to happen during installation, which again I wanted to do via a PowerShell script for the safety features I've mentioned, but after a full day of troubleshooting, and not getting anywhere far in running the PowerShell script, I decided to finally agree with the NSIS community recommendation against modifying the PATH during installation. And instead expose this option after install like we do for macOS. Although the idea of this whole thing being reduced to a snippet of text or even a link informing users on how to run the PowerShell script and set the install folder isn't a bad idea at all. So I may be inclined to agree with that idea tbh.

---

But I'll take another look at this and see what we can do. I'm seeing some issues where errors aren't being returned at all with the current implementation, and think it has something to do with later calls not waiting for the promise to return, so I'll see what we can do here. Thanks everyone for all the help

[confused-Techie]

Alright, so an update here.

I've gotten this working for both the User install, and Machine install. I'll of course do so more rigorous testing tomorrow.

But feel free to give this a spin, as there shouldn't be any negative effects.

We now have two new entries:

• "Add Pulsar to PATH"
• "Add Pulsar to PATH (Machine Install)"

Each option will only work, if selected for the proper install. As in the first button will only work if Pulsar is actually installed as the User, the Machine method will only work installed as Machine, and portable will never work.

This is included in the notes under the machine notice, but Pulsar MUST be opened as Administrator to check this box. The box is actually unclickable unless Pulsar is opened as Admin. Additionally, if Pulsar is opened as the Administrator, and the box is checked, the user will see a UAC prompt from PowerShell. Then for whatever reason this script takes some time to execute, while that happens a PowerShell box will actually appear and show a progress bar. I'm not sure why it takes so much time, but I thought this was a fine compromise.

For reference, the user PATH setting is nearly instant, and there will be no additional dialogs or windows while it happens.

I've also opted to emit a notification if this fails, since this settings page doesn't do so itself at all.

---

There are two concerns I have left here:

• If a user successfully sets the machine PATH, then opens Pulsar as a non-admin, the box will be unchecked as there is no way to find out if the setting is enabled. If opened as admin again it will become checked, but could be confusing. Either we say this is expected behavior, or we cache the status of this item, and only if we are not opened as admin we show the cached status instead. A bit more logic, but would love to hear other's thoughts.
• Secondly, I had a lot of difficulty dealing with spaces on the Machine install when launching from child_process. There's likely a magic combo of escapes that would work, so feel free to tell me if there is, but otherwise I've opted to manually replace the space. This solution works for the default machine install, but if a user installs elsewhere, it will fail. So might be worth looking into.

Otherwise, I'm pretty happy with this implementation. Like I said I'll test, but other than those last points this should be good to start reviewing the code content itself. But lastly, here's a preview of what the implementation looks like:

[savetheclocktower]

Despite the fact that the checkbox here is fundamentally different from what we do on macOS — a menu item that says “Install Shell Commands” — I’d reluctantly be OK with it. But I think we need to avoid having two different settings on this screen considering that only one of them will ever apply to a given installation.

Is there a technical reason that we can’t detect which sort of installation the user has, so that we can just present one checkbox to the user?

And in those cases where we can’t do this automatically, can we detect failure and show a notification to the user that will take them to a page that explains how to do the same steps manually?

[confused-Techie]

@savetheclocktower So essentially the reason just detecting it automatically isn't easy, and why I stayed away from doing so.

When the user has a "User" install, Pulsar can find out itself, that much is super simple. And in fact with these changes it does.

But if the user has a "Machine" install, Pulsar can't 'normally' find this out. Since essentially unless Pulsar is launched as an Administrator there's no way we can read the "Machine" environment variables.

So what that means if we wanted to detect this all automatically. When Pulsar is launched as a non-Admin user, all we can know is that Pulsar is not added to the Users PATH. This means one of the following:

• Pulsar is installed via "Machine"
• Pulsar has never been added to the PATH
• Pulsar is in portable mode

Since us not finding Pulsar available on the "User" PATH means one of these many things, it's hard to know what we would want to do. Really the only possible way I could think of doing this automatically. Would be something like this.

1. Pulsar doesn't see Pulsar on the User PATH
2. Pulsar reports that the user can click to enable this.
3. The user clicks to add Pulsar to the PATH
4. Which now as the script runs, Pulsar realizes it can't find it within the registry for the user install at all.
5. Now we have a choice. Currently we use winreg to find the install location of Pulsar on the computer, even for Machine installs, that's why Pulsar has to be opened as admin. But lets say we decide to rewrite the PowerShell script to read this value itself.
6. So now that Pulsar can't find an install under the "User" environment, it will launch the PowerShell script. But since the PowerShell script doesn't have "User" variables, then it will have to decide to self-elevate.
7. Now the User gets a UAC prompt, which can be alarming. Since there was no real warning.
8. If granted Admin permissions, now the script can look for the install within the "Machine" environment.
9. It still might not find it, maybe the user is using the portable install. At this point still nothing might not happen. But lets say it does find it
10. Finally after finding the machine install it can begin to modify the machine environment to add Pulsar to the PATH.
11. But here's the big kicker, like I mentioned earlier, lets say this does work, because we didn't first force Pulsar to be open as Admin, the user will be told it works, but the box won't be checked. Because Pulsar can't find out if it worked, from within Pulsar. So the user still might think it's broken. By at least forcing Pulsar to be launched as an admin, even if temporarily it can show that the command worked, even if confusing later on.

And if you look through my original PR, I didn't include the Machine option at all, figuring we could let docs help the user make it work. So I mean, if we don't want the two options I'd say lets remove the machine install, that one comes with all the technical issues, and makes things much more complex otherwise to figure out. I'd be happy just keeping the user install.

But sorry, went off there, I probably spent way to long yesterday trying to get the machine option working at all.

TLDR; there is a technical reason. Unless Pulsar is launched as an Admin, it can't know anything about a Machine install. Not even if it was able to set it up correctly. Additionally, when launched normally, the lack of a User install doesn't always mean it's a machine install.

---

One thing I want to mention here. A little while ago I had talked on Discord about somehow using release channels to specify the mode of installation, such as "Portable", "Setup", "Chocolatey" and so on. If we wanted to find the easiest solution for my PR, it'd probably be to pursue that idea further. If Pulsar itself had something to know what kind of release it was, we could check that. And it'd remove the guess work of trying to figure out if it's a machine, user, or portable version.

[savetheclocktower]

So I mean, if we don't want the two options I'd say lets remove the machine install, that one comes with all the technical issues, and makes things much more complex otherwise to figure out. I'd be happy just keeping the user install.

Would it be possible to do this?

1. The user visits this settings page and checks the “Add Pulsar to PATH” option.
2. We run a script that attempts to do this automatically — without trying to elevate permissions, and without caring about the difference between a user install and a machine install. If the script works, great.
3. If the script fails, we show a notification that says “Failed to add Pulsar to PATH automatically; please visit this page and follow the instructions to do so manually.”

If so, I’d vote for that as a compromise option.

[confused-Techie]

@savetheclocktower We absolutely could. And really, if we go this route, it we don't touch anything else and don't want elevating perms, really we can just undo my last two commits, and that's the existing reality. So very doable

[confused-Techie]

Alright, I've taken the advice here, and made hopefully one of the last changes.

Now instead of showing both options all the time, Pulsar will attempt to quickly guess at what type of install you have. What I mean by guess is that it should be right 100% of the time for the default, User install. But otherwise can't tell the difference between Dev Mode, Machine install, or Portable install.

If Pulsar believes this is a User install, the User install path checkbox will appear. Otherwise the machine install checkbox will appear. All the same from before applies to the machine install, such as needing to launch Pulsar as Admin to change it's setting. But otherwise here's what it now looks like:

Alright, but otherwise, I'm pretty happy with where this sits, and in testing both situations are working for me as expected, if anyone else would like to test it

[confused-Techie]

Over on Discord we have discussed the effects and changes of this PR to confirm it's good to go. I'll be merging it so it's merged in time for this regular release.