pulsar-edit
Experiment: Fix building of AppImage version
This is another draft based on my work in #1066 and tries to use CI to automate the process I described here of fixing the AppImage after it's generated. I'm not certain I'm doing the processor architecture stuff right, but we'll see what CI says.
After a lot of floating trial balloons over the GitHub Actions wall, I've got it working! Next step is to clean it up.
OK, I've cleaned up the script and explained it thoroughly so that people understand why it needs to exist.
At this point, I've proven that an AppImage artifact generated by the CI job on this PR branch produces an AppImage that invokes pulsar.sh instead of the pulsar.executable. It does the right things when I run (e.g.) ./Pulsar.AppImage --package list and ./Pulsar.AppImage --wait foo.txt.
I have not yet tried the ARM Linux job on Cirrus, but the step is present in .cirrus.yml.
I'll keep this in draft at least until #1066 is landed, but at least I can think about other things for a while.
I've squashed all the silly trial-and-error commits and rebased this atop latest master. Hopefully it'll pass CI!
Since I made this PR, I've switched from an Intel Mac to an M3 Mac, so this might be tricky to test. (Ideally, a Linux user would be able to review this; but we saw how well that worked with #1066.) If you're willing to review this, ping me on Discord and I'd be happy to guide you through it.
Looks good in testing!
Testing notes:
(In summary: This all lines up with what you saw, also tested the latest Rolling AppImage to see which things changed and which were the same. This PR's AppImage fared either better or the same as Rolling, depending on the individual flags/modes etc. under test.)
-
./Pulsar-x86_64.AppImage --version (showed the Pulsar version(s))
- Both Rolling and this PR's AppImages work to print the Pulsar version info 👍
-
./Pulsar-x86_64.AppImage --package --version (showed the PPM version(s))
- Only this PR's AppImage works to show ppm version (master/Rolling AppImage shows Pulsar version info instead of ppm version info)
-
./Pulsar-x86_64.AppImage --wait foo.txt (waited as I edited foo.txt, then exited once I saved and closed foo.txt)
- Waiting worked with this PR's AppImage, didn't work properly on master Rolling AppImage, so this is an improvement
-
./Pulsar-x86_64.AppImage --package install inline-autocomplete-textmate (installed a package successfully)
- Both worked just fine :thumbsup:
-
./Pulsar-x86_64.AppImage --package install --check (failed with a cryptic node-gyp error, but the same one I got when I ran this command against the original AppImage before I modified it)
- Cryptic node-gyp error indeed. Maybe something about insufficient permissions to make /tmp/[xyz] dirs. Not a regression though, just as you noted, happens on master branch's (Rolling) AppImage as well.
FWIW: Tested in a Fedora LiveUSB environment again. https://torrent.fedoraproject.org/torrents/Fedora-Workstation-Live-x86_64-40.torrent
This was the Rolling binary used: https://github.com/pulsar-edit/pulsar-rolling-releases/releases/download/v1.121.2024101617/Pulsar-1.121.2024101601.AppImage
This was this PR's binary used: https://github.com/pulsar-edit/pulsar/actions/runs/11283342186/artifacts/2043016356
I have been following this saga closely enough to be familiar with the steps required to make this happen.
So, the script looks reasonable to me.
Glad there are some comments for those not sure what's going on, but maybe a quick summary comment would mostly cover it? Something like "We need to repackage this to launch our custom launcher script, rather than the bare Electron binary directly, for complete functionality. No easy tooling to do this, so we unpack, modify, and repack." Almost feels a bit overwhelming with this degree of comments. But better than no context at all, that's for sure!
Glad there are some comments for those not sure what's going on, but maybe a quick summary comment would mostly cover it? Something like "We need to repackage this to launch our custom launcher script, rather than the bare Electron binary directly, for complete functionality. No easy tooling to do this, so we unpack, modify, and repack."
Added. Thanks for the review!
@DeeDeeG I should mention that I haven't checked any of this on arm64 — the step is in place in .cirrus.yml but hasn't been tested. Should I temporarily enable the build-arm64-binaries job on this PR just so I can get a binary to test?
@savetheclocktower I tried to spin up a Cirrus run of this, but had a 401 error downloading ripgrep for unknown reasons.
Wouldn't mind seeing a passing Cirrus run just to be safe. EDIT: But would be surprised if this fails, and I would have trouble actually testing an aarch64 AppImage, so... not something I feel I can do due dilligence as a reviewer beyond "Cirrus doesn't explode with this change."
Nice to have but not necessarily a blocker, I guess.
@savetheclocktower I tried to spin up a Cirrus run of this, but had a 401 error downloading ripgrep for unknown reasons.
Wouldn't mind seeing a passing Cirrus run just to be safe.
Fun. My only concern is whether the CI job needs extra dependencies, but I suppose we'll find out when the binary job runs. If the job succeeds, the AppImage should run just fine.
I'll be around in the next day or two and summonable in case we run into trouble.
Just want to mention on this Pull Request thread why we had to change aarch64 to arm_aarch64 in one part of the script... as as discussed on Discord...
It's due to this: https://github.com/AppImage/AppImageKit/blob/e8dadbb09fed3ae3c3d5a5a9ba2c47a072f71c40/src/appimagetool.c#L360-L393
Have to speak appimagetool's language.
Specifically this bit here:
https://github.com/AppImage/AppImageKit/blob/e8dadbb09fed3ae3c3d5a5a9ba2c47a072f71c40/src/appimagetool.c#L386-L387