script: Clean up pulsar and ppm on uninstall
#297
Conversation
For Linux .rpm and .deb targets only.
There was a problem hiding this comment.
The code looks solid, but I'm not in a position to quite test. But seeing @mauricioszabo's approval is encouraging, so we should be good to go
Use `test -L` rather than `test -f` for `post-install.sh`. (Note: `[ ]` is an alternate invocation of the `test` shell builtin.) `[ -f some_path ]`, for a symlink "dereferences" the symlink, meaning it checks if its target (or the ultimate target, in the case of a chain of symlinks) exists and is a real file. By coincidence, as this runs after we extract/install ppm from the deb or rpm, the symlink target exists and is a real file. However, if we changed the install path for our package manager, the target might not exist, and this step might fail. `[ -L some_path ]` instead checks for if the path exists and is a symlink. This is more correct -- it's more directly what we're trying to check. So we will use `-L` rather than `-f`.
|
Update: I tested it, and this works. 👍 Something related I noticed: I was using Still on the same theme of cleaning up correctly, sort of? So I added it to this same PR. Would appreciate another review, but it's such a tiny change (it's a one character diff!), and I tested it pretty thoroughly on my machine, so I may merge if no-one gets around to reviewing. |
There was a problem hiding this comment.
My approval still applys that the code looks solid, and if you were able to test and ensure this works that's awesome.
Would you like me to post the binary this creates on Discord so we can try to have others tests? Otherwise if you feel your system is a good example for what's expected here lets get it merged
|
Just had a try on MX Linux and yup, |
|
@confused-Techie yeah, I'm confident Was away from the project for the weekend, thanks for the approves @mauricioszabo and @confused-Techie, thanks for testing and confirming it works @Daeraxa. Merging this now!! |
Spiker985
deleted the
Linux-clean-up-pulsar-and-ppm-bins-on-uninstall
branch
For Linux .rpm and .deb targets only.
Requirements for Contributing a Bug Fix (from template, click to expand):
Identify the Bug
Follow-up to:
Description of the Change
When uninstalling the .
deband.rpmpackages on Linux, clean up thepulsar(pulsar.sh) andppm(symlink to/opt/Pulsar/resources/app/ppm/bin/apm) stuff from/usr/bin.(Note: These are only automatically created for the
.deband.rpmLinux packages. So there is nothing to clean up on other OSes or targets, other than the tangentially related but off-topic for this PR macOS bins users have to manually request installation of.)Relevant docs: https://www.electron.build/configuration/linux#linuxtargetspecificoptions-apk-freebsd-pacman-p5p-and-rpm-options
Alternate Designs
Not cleaning up is bad, so I didn't consider just not cleaning up. However:
I do want to move these from
/usr/bin(distro packages install here and there are obscure conflicts with appmbin from a Perl package manager package, and with a binpulsarfrom an MRI science package calledodin) to/usr/local/bin, which is coincidentally where Atom installed itsatomandapmbins to, presumably for the same reason.(See: https://pkgs.org/search/?q=%2Fusr%2Fbin%2Fpulsar and https://pkgs.org/search/?q=%2Fusr%2Fbin%2Fppm)
BUT: IMO we need to have this cleanup code out in the wild for a while to clean up all the old bins people have installed, before making the move to
/usr/local/bin.Possible Drawbacks
I would say there is a risk of deleting the Perl package manager
ppmor the MRI sciencepulsarbin from Odin... but hey, we have already been clobbering these with our own bins! So... It's already too late for that! Oops. No drawbacks that I can think of that would be new as of this PR.So yeah, let's move these to
/usr/local/bininstead of/usr/bin, after about a month or so, say?Verification Process
TODO: Grab one of the built packages from Cirrus and test this out. I'll be testing the .deb package, personally.
Release Notes
Clean up
pulsarandppmbins/scripts/symlinks upon uninstalling the.rpmor.debpackages.