pulsar-edit / pulsar

A Community-led Hyper-Hackable Text Editor
https://pulsar-edit.dev
Other
3.21k stars 133 forks source link

`pulsar -p` behaves differently from `ppm` #1053

Open savetheclocktower opened 2 months ago

savetheclocktower commented 2 months ago

Thanks in advance for your bug report!

What happened?

I tend not to use pulsar -p much because of muscle memory. But while investigating and documenting Windows build toolchain stuff for Pulsar, I realized that pulsar -p doesn't behave like ppm in a couple of crucial ways:

pulsar -p --version

Running pulsar -p --version should produce the same output as ppm --version — but instead it produces the same output as pulsar --version. This appears to happen on all platforms.

Windows issues

This might even warrant a separate issue, but I ran into several problems that seem to be Windows-specific:


Just as a sanity check, I verified that pulsar -p works as I'd expect on macOS, except for the --version bug described above.

Pulsar version

1.118

Which OS does this happen on?

🪟 Windows

OS details

Windows 10 (with cmd.exe and Windows Terminal)

Which CPU architecture are you running this on?

x86_64/AMD64

What steps are needed to reproduce this?

  1. Add pulsar to your path, either by the option in Settings or manually.
  2. Run pulsar -p X, where X is any valid command.
  3. Observe that nothing happens.

Additional Information:

No response

savetheclocktower commented 1 month ago

After some investigation, here's what I think:

It might be worth trying to clean up this code path a bit. The --package/-p feature happens entirely inside of the parseCommandLine function; if we moved it to start.js, we'd have a bit more control here. I think one proper fix would be to change from spawnSync to spawn, which is hard to do while this code lives in its current location.

savetheclocktower commented 1 month ago

An update:

So I'll probably end up working on two other tasks:

savetheclocktower commented 1 month ago

I'm digging into the AppImage stuff.

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.

savetheclocktower commented 1 month ago

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.

savetheclocktower commented 1 month ago

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

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

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.