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

[core] Windows: Only remove Pulsar/PPM from PATH during uninstall #1183

Merged
merged 2 commits into from
Jan 17, 2025

Conversation

confused-Techie
Copy link
Member

@confused-Techie confused-Techie commented Jan 5, 2025

Fixes #1157

When I originally wrote the section of our NSIS that removes Pulsar from the PATH during uninstallation, I was aware that this code would also be run during an upgrade, since the upgrade process uninstalls the old version then installs the new.
But I was under the impression that those steps were linear, running uninstallation steps then running installation steps.

But it seems that isn't the case. Meaning that even though the installation steps would add items to the PATH, our custom uninstallation steps were run afterwards, ultimately removing Pulsar/PPM from the PATH.

Luckily, electron-builder makes it easy to check if we are running an upgrade rather than a full uninstall. Meaning we can check how we are running, and only remove items from the PATH during a complete uninstall.

@savetheclocktower
Copy link
Contributor

I wish I could promise I'll be able to verify this before the 15th, but it's a bit iffy. I'll try to remember to do it tomorrow.

@savetheclocktower
Copy link
Contributor

So this might be an improvement over the status quo — I haven't tested the exact code path of this script yet — but I can say that it doesn't add pulsar and ppm to the PATH if they're not already there and Pulsar is already installed.

If it prevents them from being removed during an upgrade (I'll try that next ) then I'm happy to approve it, but let's do the other half of it in a separate PR for next month's release.

@savetheclocktower
Copy link
Contributor

OK, I can confirm that

  • This adds pulsar and ppm to your path if you check those options on a fresh install (when Pulsar is not currently installed at the specified location)
  • This does not remove pulsar and ppm from your path if you re-run setup and reinstall over an existing installation at the same location
  • This does remove pulsar and ppm from your path if you run the uninstaller via the Control Panel

Therefore: 👍

We can revisit the last remaining goal (adding to path during a reinstall if the options are selected, and removing them from the path on a reinstall when the options are unchecked) in a future PR.

@confused-Techie
Copy link
Member Author

@savetheclocktower Thanks a ton for the review! To my knowledge those last items you listed should be covered, but I haven't verified. So good idea we can double check those at a later time in another PR.

For now, lets get this one merged, thanks!

@confused-Techie confused-Techie merged commit ebed0ba into master Jan 17, 2025
103 checks passed
@confused-Techie confused-Techie deleted the fix-windows-path-addition branch January 17, 2025 04:50
@savetheclocktower
Copy link
Contributor

those last items you listed should be covered, but I haven't verified.

I tested them and they didn't work. :-)

@confused-Techie
Copy link
Member Author

@savetheclocktower I see. Well in that case, great point, I'll try to tackle them in a future PR. Thanks for the heads up about it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Windows Installer in update mode does not update path on Windows
2 participants