Experiment: Redirect -p/--package to ppm via pulsar.sh…

[savetheclocktower]

This one is in draft mode until I can get some Linux testers.

Identify the Bug

There are a handful of issues with pulsar.sh on macOS and Linux; they’re largely covered in #1053:

• There’s no reason that -p/--package need to get all the way to Pulsar before they’re handled. We should redirect them to ppm in pulsar.sh.
• Error messages generated by pulsar go to stdout instead of stderr.
• On Linux, pulsar.sh hard-codes an assumed install location — presumably the place it resides when installed via rpm or deb. If a user has downloaded the tar.gz version, pulsar.sh will fail to find their installation automatically, even though pulsar.sh lives within that directory.

Description of the Change

• All error messages now go to stderr.
• When -p or --package is present among the arguments, pulsar.sh will now handle them exactly how Pulsar itself does: by ignoring all arguments before -p/--package, then collecting all arguments after -p/--package and calling ppm instead of Pulsar.
• Linux now has similar detection logic to that of macOS; if pulsar.sh is invoked from a symlink, it should now be able to determine its real location, then traverse upward to find a binary called pulsar.
• The user should be able to supersede all this location logic (on either platform) by specifying the PULSAR_PATH environment variable manually.
• Like Rework command parsing to support --package better #1054, this will also fix the fact that pulsar -p --version prints the version information for Pulsar instead of ppm.

Alternate Designs

We already do the main alternative to handling --package in pulsar.sh — and we should keep it as a fallback.

Possible Drawbacks

Bash scripts are a bit harder to write and don’t have a great testing situation, so I’d love to get field reports on how well this works on Linux. I was able to test the macOS parts on my local machine, but on Linux I’m mainly flying blind.

Verification Process

• Verify all these commands produce the same output:
  • pulsar --package --version
  • pulsar -p -v
  • ppm --version
  • pulsar --wait --package --version (testing that any arguments before --package are ignored
• Verify these commands produce the same output
  • pulsar --package list
  • pulsar -p list
  • ppm list
  • pulsar -v --package list
• Verify that commands which don't use --package/-p behave like they always did:
  • pulsar .
  • pulsar --dev .
  • pulsar --wait foo.js
  • pulsar --test spec/*-spec.js

Extra testing for Linux:

• Download the tar.gz distribution of Pulsar, extract it somewhere, then symlink its ./resources/pulsar.sh to (e.g.) pulsar-test someplace on your path, like /usr/local/bin. Verify that pulsar-test passes all the manual tests described above.

Release Notes

• Improved the command-line pulsar script’s ability to find the user’s Pulsar installation location on Linux.
• On macOS and Linux, pulsar -p now invokes ppm without having to launch Pulsar itself.

[savetheclocktower]

OK, I grabbed a lightweight Ubuntu-flavored distro and did some testing on VirtualBox. The script needed some tweaks because of typos and my Bash naïvete, but now it works solidly on tar.gz distributions of Pulsar for Linux.

The same logic that works properly for Pulsar at an arbitrary location on disk will also work for Pulsar installed at the standard /opt/Pulsar path of the RPM and DEB packages — but if, somehow, it doesn't, then we'll still check a hard-coded /opt/Pulsar path to be safe.

I think the code path for CI is silly, but I want to understand it better before I take it out.

AppImage is another ball of wax and will have to be tackled in a separate PR.

[savetheclocktower]

Failing editor test is the one that's known to be flaky. Not going to bother with re-running it for now.

[DeeDeeG]

Re-running CI so I have some binaries to test. (Old ones are expired and deleted by GitHub Actions due to the passage of time.)

CI is passing as of before my doing so, for the record.

[DeeDeeG]

I'm not sure if pulsar.sh was ever expected to work for .tar.gz bundles, as it's just as easy to call the pulsar binary directly by absolute (full) path as the launcher, and it basically works fine.

(Come to think of it: Why do we really need the .sh launcher??? I have always found directly launching the actual Electron binary just as satisfactory to my needs as the launcher. I don't have a lot of love for mysterious "don't think about it too hard" kinds of complexity...)

So, if that works, I consider it above and beyond.

[DeeDeeG]

Sorry for the delay in review.

Your work on testing this has been thorough enough I am just about temped to round it up to being all the review this PR needs. Will try to make time to properly review it to whatever extent it still needs it beyond what you've already done to establish its bona fides.

[savetheclocktower]

I'm not sure if pulsar.sh was ever expected to work for .tar.gz bundles, as it's just as easy to call the pulsar binary directly by absolute (full) path as the launcher, and it basically works fine.

(Come to think of it: Why do we really need the .sh launcher??? I have always found directly launching the actual Electron binary just as satisfactory to my needs as the launcher. I don't have a lot of love for mysterious "don't think about it too hard" kinds of complexity...)

So, if that works, I consider it above and beyond.

I wouldn't quite say it works fine. It logs some main process output to the console. Some features don't work, like --wait. (Those last two, taken together, make it impossible to use Pulsar as a Git editor for commit messages, interactive rebases, etc.) Having the wrapper allowed us to fix a major encoding headache on Windows and it allows us to redirect --package to PPM on all platforms without making people wait for Electron to start up.

The goal is to be able to give people commands to run (in documentation, in Discord, etc.) and know they'll basically work the same everywhere, instead of “how did you install this? because if it was X, you'll run this, but if it's Y, you'll run this…”

This is also a prerequisite for AppImage users being able to invoke Pulsar properly (#1069). Right now it doesn't work at all because it tries to look for an executable in a fixed location on disk. Scratch that; right now the AppImage runs its own script that invokes the executable directly and behaves exactly like invoking the executable.

[DeeDeeG]

Interim review (leaving this here as I may have time to test on Linux, but thoughts for now based on the diff and macOS testing):

I can confirm the PR works as intended for me on macOS, as judged by the testing steps shared in the Verification Process section of the PR body text.

I can report the following findings about the state of things before this PR (Pulsar Regular release 1.120.0) (see expandable section):

Detailed testing results for Pulsar 1.120.0 Regular (without this PR's fixes) (click to expand):

• ppm --version is the only one of the original test steps that prints the ppm versions. Other three test commands report the Pulsar versions (pulsar --package --version, pulsar -p -v, and pulsar --wait --package --version).
• pulsar -v --package list prints the Pulsar versions instead of listing packages. Other test commands already worked as intended as of this PR's interpretation of how they should behave (pulsar --package list, pulsar -p list, and ppm list all worked as intended already).
• The "control" commands (i.e. "these should already be working and continue to work after the PR") all worked as I think they should, both on 1.120.0 Regular, and with this PR's binary:
  • pulsar . opens the current working dir of the terminal as a project in a new Pulsar window.
  • pulsar --dev . Opens the current dir as a new project in a new Pulsar window, in Dev mode if no other Pulsar windows are open in non-dev default mode yet (assuming env var ATOM_DEV_RESOURCE_PATH is set properly to a path where there is a bootstrapped copy of the pulsar repo on disk).
  • pulsar --wait foo.js opens a file foo.js for editing, which will be saved to the current working dir of the terminal if saved, waiting/blocking the terminal until the file (or was it the window the file was opened in?) is closed before unblocking/returning control to the terminal.
  • pulsar --test spec/*-spec.js runs any tests at [cwd]/spec/*-spec.js -- I tested this in the pulsar repo's root dir, as I know it has spec files in the proper directory structure to work with this test command -- Tests passed, by the way.

By contrast, with this PR's binary: all of the first set of test commands list the ppm versions; and All of the second set of test commands list packages. Which is as intended. ✅

Consistent with before this PR, the third set of "control" commands ("commands that should still work after this PR") still do work with this PR's binary, exactly the same way as reported above in the expandable details section.

So, yes, everything seems to be working as it should as of this PR. For me on macOS.

Might get time to test on Linux. Might not. If not after about the time it would have taken to do such testing if I could do it right away, I should probably just circle back and approve this.

Code looked reasonableish as much as I can say from looking at it more or less at a glance. Nothing alarming, comments are updated or added, which is always nice.

(Note: It did not take me all this time without breaks to test -- rather, I tested earlier and had another obligation I needed to attend to before writing up my findings, which took me away from this for over an hour. It didn't take me terribly long to test this actually, once I got my system set up for it, got the binaries, made sure my local Pulsar repo was properly bootstrapped, etc.)

[DeeDeeG]

Managed to test on Linux. With that, I can review-approve this, primarily on the basis of manual testing, and the code not having anything wild-looking that I could notice after a quick read.

Testing details:

I downloaded the .deb and the .tar.gz assets for this PR. Copied them to a live USB which had been set up of Ubuntu 22.04 "Jammy" (with the free space usable for storage, made by Rufus on Windows).

Booted up the Ubuntu live session, installed the .deb from this PR. Test commands worked as expected. (Except was not able to test pulsar --dev or pulsar --test since I didn't have a bootstrapped Pulsar repo ready to go in the live session!

Also tested the .tar.gz by extracting it and symlinking the extracted pulsar.sh to /usr/bin/pulsar-test, as described in the Verification Process section of the PR body above. Again, test commands worked as expected, while again not being able to test pulsar --dev or pulsar --test within limited time constraints on my end.

As a limitation of this testing, I did not verify that Pulsar commands were problematic on Linux before/without this PR. i.e I did not test Pulsar 1.120.0 Regular on Linux with respect to this PR in particular. So I can't necessarily confirm first-hand which changes were needed for Linux from this PR, but I can confirm the tests from Verification Process are working with this PR, and beyond that I trust any testing that may have been done thus far in the thread by the PR author (I don't recall off the top of my head right now and it's late where I am and I need to go to sleep soon!)

tl;dr: LGTM on Linux.