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

Experiment: Fix building of AppImage version #1069

Conversation

savetheclocktower
Copy link
Contributor

@savetheclocktower savetheclocktower commented Jul 28, 2024

My various comments in #1053 explain why this PR exists. Excerpts:

I strongly suspect the AppImage distribution of Pulsar, when invoked from a terminal, will invoke the main executable. This is like if you tried to do (e.g.) Pulsar.exe --version on Windows; it'll mostly work, but we want that stuff to go through the script first so that we can support things like --wait better.

[...]

[I should investigate] how to build the AppImage version so that executing it from the terminal ends up executing the inner pulsar.sh file instead of the main executable. This is feasible, according to the docs, but electron-builder might not make it easy. Once that's done, pulsar.sh should be able to infer its location (and the locations of pulsar and ppm) based on its working directory, but I'm sure there will be wrinkles.

The AppImage docs have this section about extracting an AppImage, and it's just what we need. It would allow us to edit the AppRun.sh script and then use appimagetool to turn the extracted AppImage back into an AppImage.

The AppImage spec envisions that you might want to set an arbitrary script as the entry point for an AppImage. It's called AppRun; you can make it a shell script, an executable, or even a symlink. Since electron-builder generates AppImages for us largely automatically, it must know how to make an AppRun script.

It took me quite a while just to be able to find where this is happening: in a dependency of electron-builder’s called app-builder. This script is where the magic appears to happen; it does a number of things, but in its function it isn't too different from what pulsar.sh already does.

The best solution would be to have a way to customize this script, or even swap in a new one entirely, but that's not an official feature. It's been requested in several issues that I've seen, but never with any sort of official response.

Still, I'm pretty sure we could do it somehow as part of the build process. It'd probably involve something ugly like extracting the AppImage, editing a file, then compressing it again… but I doubt we're the first people who've needed to do that.

The status quo of AppImage is presumably one in which invoking Pulsar.AppImage from the terminal is mostly but not exactly the same as invoking pulsar when it's symlinked to pulsar.sh. But the documentation page on installing terminal commands gets so much simpler if those two things are made identical; it means we can just tell people to alias pulsar to Pulsar.AppImage and ppm to Pulsar.AppImage -p.

Then, the breakthrough:

OK, I got an AppImage version of Pulsar to run pulsar.sh instead of the pulsar executable. These were the steps I used:

  • Downloaded the latest regular release in AppImage format to Linux-Pulsar-1.119.0.AppImage

  • Made it executable with chmod +x Linux-Pulsar-1.119.0.AppImage

  • Extracted it by running Linux-Pulsar-1.119.0.AppImage --appimage-extract

  • The AppImage extracted itself to a folder called squashfs-root; I renamed this to Pulsar.AppDir

  • cd Pulsar.AppDir && nano AppRun

  • The AppRun script is basically this one; there was a line that said

    BIN="$APPDIR/pulsar"
  • I edited the line to read

    BIN="$APPDIR/resources/pulsar.sh"
  • I then opened resources/pulsar.sh and changed it to be identical to what’s in Experiment: Redirect -p/--package to ppm via pulsar.sh… #1066

  • To repackage the directory, I downloaded appimagetool from here

  • I was then able to do ARCH=x86_x64 appimagetool-x86_64.AppImage Pulsar.AppRun (it wanted an ARCH flag for some reason), which eventually produced a Pulsar-x86_64.AppImage file.

And I was done. All the following worked as I expected them to:

  • ./Pulsar-x86_64.AppImage --version (showed the Pulsar version(s))

  • ./Pulsar-x86_64.AppImage --package --version (showed the PPM version(s))

  • ./Pulsar-x86_64.AppImage --wait foo.txt (waited as I edited foo.txt, then exited once I saved and closed foo.txt)

  • ./Pulsar-x86_64.AppImage --package install inline-autocomplete-textmate (installed a package successfully)

  • ./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)

I’m writing this down just so I remember what to do later. #1066 is a prerequisite, but then once it’s landed, I’ll make another PR that adds a couple steps to the build job for Linux. A self-contained version of appimagetool can apparently always be downloaded from GitHub, as illustrated by this heading in its README. So it’s just a matter of getting the right one for a given architecture, noting the original filename, extracting the AppImage, making a one-line change, then rebuilding the AppImage at the original filename.

It’d be great if electron-builder allowed us to do this, but it seems not to. I haven’t ruled out other approaches yet, but this is a low-risk strategy we can use to fix AppImage builds without affecting other Linux builds.

Testing

If you're on an x86 architecture, you should be able to run a lightweight Linux VM (I used LXLE originally) to verify that you can download the AppImage product from this PR’s CI run. You should be able to follow the steps detailed above; in general it should work identically to invoking pulsar from the command-line as is possible in other installation types.

@savetheclocktower
Copy link
Contributor Author

After a lot of floating trial balloons over the GitHub Actions wall, I've got it working! Next step is to clean it up.

@savetheclocktower
Copy link
Contributor Author

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.

@savetheclocktower savetheclocktower force-pushed the fix-linux-appimage-launching branch from 26186b5 to 0b44f36 Compare October 10, 2024 23:07
@savetheclocktower
Copy link
Contributor Author

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.

@savetheclocktower savetheclocktower linked an issue Oct 10, 2024 that may be closed by this pull request
5 tasks
@savetheclocktower savetheclocktower marked this pull request as ready for review October 10, 2024 23:22
@DeeDeeG
Copy link
Member

DeeDeeG commented Oct 16, 2024

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 👍
  • ./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

@DeeDeeG
Copy link
Member

DeeDeeG commented Oct 16, 2024

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!

@savetheclocktower
Copy link
Contributor Author

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!

@savetheclocktower
Copy link
Contributor Author

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

Copy link
Member

@DeeDeeG DeeDeeG left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hard to pass up the chance to review a user-facing improvement as fundamental as making CLI args work as expected.

(I hear users love user-facing enhancements!)

Big approve 👍

@DeeDeeG
Copy link
Member

DeeDeeG commented Oct 16, 2024

@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
Copy link
Contributor Author

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

@savetheclocktower savetheclocktower merged commit 23fba9d into pulsar-edit:master Oct 17, 2024
103 checks passed
@savetheclocktower savetheclocktower deleted the fix-linux-appimage-launching branch October 17, 2024 03:48
@DeeDeeG
Copy link
Member

DeeDeeG commented Oct 17, 2024

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

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.

pulsar -p behaves differently from ppm
2 participants