Use a different strategy for pulsar -p on Windows…
#1063
Conversation
…by handling the argument processing in the batch file.
|
It looks fine in theory, I'll have to see if I can get this onto my work laptop, otherwise I'll have to set up a QEMU instance in order to test I switched off my Arch Linux install, over to NixOS, and in the jump I didn't migrate my MacOS builds or Windows build. |
That's kind of you. I added you because I seemed to remember you had a Windows machine, but hopefully you won't have to go to such lengths and we'll be able to find a couple more Windows testers. |
|
I have tested this! Here are my notes, following from the "Testing" section of the PR body text above. All test commands were run in Rolling 1.119.2024071703 (control)(From the commit on Testing notes for Rolling (click to expand):
Perhaps as expected for current Rolling, these test commands did not work successfully: Furthermore, the command just above with the
Nope, as probably to be expected, these tests failed. Outputs were distinct and did not match. Setting aside much of the output being broken, as it's off-topic for this particular PR, I guess. See output below:
Results varied. Stuff mostly seems to work, even on Rolling, but with some quirks:
Binary from CI for this PR #1063 (seeing if the fix works)Testing notes for this PR (click to expand):
Yes, fix appears to have worked, in the sense that output is consistent (consistently broken, but that is beyond the scope of and therefore beside the point of the present PR, I think):
Yes, output is the same each time and doesn't seem messed up in any particular way:
Yeah, seems like these work the same as before. My exact notes from testing this, but any/all quirks seem the same as Rolling before/without this PR:
Summary of FindingsFix appears to work where it's intended to -- remaining quirks are presumably out of scope for the present PR, but may for all I know be addressed in #1054. In any case, I don't see any issues from this testing per se for the current PR. 👍 |
resources/win/pulsar.cmd
Outdated
| :trim_args_for_package_mode | ||
| if not "%1"=="--package" ( | ||
| if not "%1" == "-p" ( | ||
| REM Roughly the same as `ARGV.shift`. Everything except the initial argument | ||
| REM moves forward, and the second argument is removed from the list. | ||
| SHIFT /1 | ||
| goto :trim_args_for_package_mode | ||
| ) | ||
| ) | ||
| REM When we reach this point, `-p`/`--package` is the argument in the %1 | ||
| REM position. Now we'll call `shift` once more to remove it from the list. | ||
| SHIFT /1 |
There was a problem hiding this comment.
It occurs to me someone could ask folks to copy-paste some long pulsar command ending with -p install [some package] in order to obfuscate the true purpose of the command. And that's basically remote arbitrary code execution at that point, for example given the flexibility npm lifecycle scripts have, and given how arbitrarily capable packages can be in general.
This sort of short-circuiting is fine for --version, since hopefully "just doing a version print and then immediately exiting Pulsar" won't be chainable into an exploit. I think package installation is a little more sensitive, though.
Can we make -p / --package throw an error and print a usage hint if it's not the first argument after pulsar itself? I think that'd be safer. And I think not much would be lost, at least in terms of I don't think this puts much of a barrier up for things a user should reasonably expect to be able to do (or would ever need to do).
Thoughts on this?
P.S. if this is what we've been doing on Linux and macOS all this time, I'd consider it a to-do to align Linux and macOS with whatever we settle on.
There was a problem hiding this comment.
Can we make
-p/--packagethrow an error and print a usage hint if it's not the first argument afterpulsaritself? I think that'd be safer. And I think not much would be lost, at least in terms of I don't think this puts much of a barrier up for things a user should reasonably expect to be able to do (or would ever need to do).
We can, but I'd ask for that to be filed as a separate issue so that we can make it behave that way on all platforms simultaneously.
EDIT: To elaborate… the way this PR handles -p/--package is exactly how Pulsar itself handles them. That was intentional. There's a fair point to be made about that not making a ton of sense, but that is how it's currently handled regardless of your platform.
There was a problem hiding this comment.
I've gone ahead and done some manual testing with this.
I just placed the new contents of pulsar.cmd into my existing copy and ran through all of the commands you listed, using different variations of -p and --package.
Mostly everything seemed to execute exactly as it should, including running commands meant for just Pulsar.
But I did encounter an error running pulsar --package install atom-clock which returns:
Unrecognized command: installatom-clock
So seems maybe we are loosing spaces when passing the values to ppm?
Weird! I'll put in extra spaces just to be safe. |
There was a problem hiding this comment.
Fantastic work!
Testing now and everything seems to be happy as can be.
I'll be clear and say I haven't tested every single command, but from what I have things are looking good.
I'd also really like to point out this resolves the long-standing bug of windows not getting output from ppm.
Take a look at the output of the following commands:
Before this PR:
> pulsar --package install atom-clock
>
After this PR:
> pulsar --package install atom-clock
Installing atom-clock to C:\Users(user)\.pulsar\packages done
>
I'll also go ahead and make some confirmations:
ppm -vstill workspulsar -p -vreturns ppm infopulsar -vreturns pulsar infopulsar --package --versionreturns ppm infoppm --versionstill workspulsar --wait --package --versionreturns ppm info
So seems everything here works, and goes above and beyond to solve a longstanding and really annoying bug. Love to see it!
I'd say we are good to go, but if anyone else is testing then of course we want to allow ample time for different systems to ensure everything is expected
|
I'll give this one a week or so just in case anyone else wants to take a look at it. |
|
With two approvals, and a week has passed allowing other tests, I'd assume we are good to merge. Any loose ends on this one @savetheclocktower? |
…by handling the argument processing in the batch file.
This one is a bit ridiculous, but somehow less ridiculous than what I was doing in #1054.
We opt into a bunch of headaches by trying to use the Electron app itself to handle command-line output. Most of the features of
pulsarin the terminal are necessary evils, but for the reason discovered in #1054 (Electron can't do magical encoding conversion on its output the way Node can) we should try to minimize the stuff we askpulsarto write to STDOUT. (Windows especially, since that's the platform least likely to have terminal output rendered as UTF-8; but it's a theoretical concern on other platforms as well.)If we could intercept
-p/--packageat the script level, we could redirect those calls toppmbefore they hit Electron. Then we wouldn't be creating any new encoding issues for ourselves.Luckily, we already have some wrapper scripts meant to handle calls to
pulsar. On Windows, it'spulsar.cmd, a file written in ancient batch file syntax. Imagine all the things you'd want from a scripting language if you were inventing one in the year 2024; batch files seem like they were intentionally designed to have none of those features. But there are lots of masochists online! Stack Overflow and SS64.com were quite useful in figuring out how to do this.The idea is to behave identically in the batch file as we already do in
parse-command-line.jswhen we see a-por--packageflag:-p/--package, then-p/--package, thenppm.This is convoluted in batch files because they don't have loops or array methods, but it's possible nonetheless.
Testing
This is very important to test thoroughly on Windows, since we can't really write automated tests for this. Here's what I did, and it all seemed to work great:
pulsar --package --versionpulsar -p -vppm --versionpulsar --wait --package --version(testing that any arguments before--packageare ignoredpulsar --package listpulsar -p listppm listpulsar -v --package list--package/-pbehave like they always did:pulsar .pulsar --dev .pulsar --wait foo.jspulsar --test spec/*-spec.jsShould we do this on all platforms?
Yes! But Windows is the most salient because of the extant bugs with
pulsar -pon that platform.Right now,
pulsar --package liston my macOS machine starts the Pulsar icon bouncing in the dock for a brief second before we get output. That's because we're launching a GUI app just to launch a command-line app. It'll be faster if we cut out the middle man.Once we do this on all platforms, should we remove the equivalent functionality from Pulsar?
No. Because we do handle lots of command-line stuff via the main process, most of the command-line switches will still work if you invoke
Pulsar.exeor the actualpulsarElectron executable. And we should keep handling those for these reasons:PATHmanually and incorrectly assumed that they should add the folder that holdsPulsar.exe, rather than the one that holdspulsar.cmd.AppImagefile from the terminal is much like invoking the actualpulsarexecutable (instead ofpulsar.sh). (And as I understand it, the-pflag was basically invented for this use case!) If we can change the AppImage so that invoking it somehow hits the logic inpulsar.sh, then that'd be grand; failing that, handling-p/--packageredundantly in the main process is a fallback that helps AppImage users not to be second-class citizens.For the same reasons, this PR is a companion to #1054, not a replacement.