-
-
Notifications
You must be signed in to change notification settings - Fork 140
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
Rework command parsing to support --package
better
#1054
base: master
Are you sure you want to change the base?
Rework command parsing to support --package
better
#1054
Conversation
I spent 45 minutes getting this working on Windows and it's driving me nuts. First: Now there's one more headache: character encoding. With this PR checked out and $env:ATOM_DEV_RESOURCE_PATH = "C:\Path\To\Pulsar" I can run The headaches only start when you realize that The one thing I've done that's actually solved this is something that I think is just a bit too clever: alter So here are some options, in no particular order:
Other suggestions are welcome. |
I did some experiments that clarified my thinking here. My initial theory was that both I tested this with a small Node project with various scripts that log the text
I could not get the encoding bug to manifest in any of these scenarios. Running out of ideas, I started calling That said, I'm failing in my vague attempts to determine why this is true. If there were some explicit trick that Node were using, it would likely be visible in the Node source code, but I'm not seeing it. Maybe we'll get lucky and find out that this has been fixed in a future version of Electron, but for now I can think of only two ways to work around the problem:
Option 2 is easy enough (if unsatisfying), but I think we can manage option 1. It's true (as I said above) that there's no easy way to trap a Windows shell's equivalent of SIGINT. If But there are theoretical ways around this, depending on how ridiculous you're willing to get! I'll try two of the approaches described in this Stack Overflow answer and let you know how it goes. |
OK, it's even weirder. Let me start over and state a hypothesis:
Let's take the character Why three? Because the UTF-8 code point for Again, all this is exactly what you'd expect in this situation. Node's behavior is the unusual part! Either it's doing behind-the-scenes conversion of characters or it's telling the terminal to use UTF-8 (but only for the duration of a given command). When we run I had thought that our last-ditch option here would be to make sure we were emitting only characters that were mentioned in this guide, but this guide isn't what we need, because it assumes that encoding conversions are being done correctly. The box-drawing symbols we're using for Now I think our last-ditch option is to set an environment variable that indicates a Windows user's code page. If it's not 65001, An aside on the batch file exception thing: as expected, it was more complicated than it seemed. The best I could do was to copy this approach, but the annoying "Terminate batch job? (Y/N)" prompt was my undoing. This approach means that the prompt is shown twice in a row, and the cleanup code only happens if you answer "Y" to the first prompt and "N" to the second. |
Anyway, a couple more thoughts:
|
This is a concept for how we'd rewrite argument parsing to make
--package
work better and fix the issues detailed in #1053.I haven't tried any of this on Windows yet — nor have I written new specs or tried to run existing specs — but this passes some sanity tests on my macOS machine:
--version
is handled properly;pulsar --version
does what it always did, whereaspulsar --package --version
properly forwards toppm --version
.ppm
is moved tostart.js
so that it can usespawn
instead ofspawnSync
. This might be enough on its own to fix our issues with{ stdio: 'inherit' }
on Windows; but if it isn't, then it at least gives us more flexibility with using{ stdio: 'pipe' }
to produce the same effect.If I take this out of draft, it means that I've confirmed that these techniques fix our Windows issues, ensured existing specs pass, and written new specs for
--package
behavior.