Remove hard-coded architecture on Mac #141
Conversation
|
Commenting for posterity, since I already went through the trouble of tracing back in time through the blame view fully (following your lead), and posted it in the Discord... copy-pasting here for the historical record.
|
Oh, I forgot to mention: this is because I use |
There was a problem hiding this comment.
tl;dr: FIX LOOKS GOOD TO ME. 👍
Long version:
Can confirm that, on my x64 (intel) MacBook, Node returns x64 when evaluating process.arch. Which matches what the hard-coded string would have been from the switch/case there was before this PR.
From that, and judging by the fact you're reporting this works for you on ARM (Apple Silicon), behavior as updated by this PR should absolutely be correct across x64 and ARM Macs alike now.
Given the history of un-hard-coding these values to solve issues over time, this fix falls along a long and steady line of the same fix being implemented individually for every major OS family. It only now comes back to macOS since they were the latest to diversify from one to multiple arches. (See my comment just above, as well as the note from your original PR post.)
So, the original hard-coding was not flexible enough, and thankfully we can do process.arch now. I have to guess/assume this wasn't available back in Node 0.11, and before Electron existed yet... So, maybe fudging the arch was important back when using Node in really non-standard ways??? Who knows?!
Also: By the way, this code is the only instance of ATOM_ARCH either in this repo or Pulsar core repo. So, I think it's just a convenient way to make ppm rebuild for a different arch. Might as well leave it, but I would be surprised if there are no real users of that env var, and the getElectronArch() function may be superfluous to simply using the process.arch method directly. But again, doesn't seem like a big deal to have this code path, might be nice to use in a pinch, and any users who did need it would (hypothetically???) probably sorely miss it. So, might as well keep it, I guess.
|
Yeah, I like the ability to specify |
|
Very good to have this fix, thank you for the persistence in tracking this down!! (Re-reading through #126 again, hindsight is 20/20, and this finding aligns with all of what was said there.) |
Fixes #126.
We've known for a while about an issue where
ppm rebuildfails to work properly on Apple Silicon. That command is run whenever a native module needs to be recompiled — either for a new processor architecture or for a new version of Electron — and there's even a GUI for it.This is what it might look like for an end user, as it did for me last week when I got a new M3 MacBook Pro:
package-rebuilding-failure.mov
The built-in
update-package-dependenciespackage is a frontend toppm rebuild. Notice how it doesn't say “failed to rebuild the package”; it thinks it's succeeded, and yet after a window reload it once again flags the native module as being for the wrong architecture.This was on my radar because some of
pulsar-ide-python’s users had complained about it.zadehwas used inatom-languageclient— I moved away from it in my@savetheclocktower/atom-languageclientfork in order to work around this issue — so it affected a number of packages. It was strange to learn, then, thatnpm rebuildseemed not to be susceptible to this issue. This reduced it to an exercise in finding out exactly what the discrepancy was betweenppm rebuildandnpm rebuild.For a few hours I was certain it was some sort of artifact of a too-old version of Node, NPM, or
node-gyp. I noticed a behavior difference between two of my local versions of Node —14.16.0did the wrong thing, whereas16.19.0did the right thing — and I generally wasted my own time until I looked closely enough at the logging and traced the issue back to the PPM source itself.PPM provides a bunch of build flags for NPM, processor architecture among them; on
darwinit's been hard-codingx64for years and years and years. I don't know why, but it was implemented in such a way that you couldn't even override it with an environment variable.Removing the hard-coded special case seems to fix it for me. We should ensure it works as expected on an Intel Mac, but this feels like the obvious obvious obvious fix. I'm glad that it's as simple as this, though somewhat annoyed that I didn't notice it earlier.
Since neither
ppm rebuildnornpm rebuildseems to need a subsequent pass throughelectron-rebuild, this should fix #126 altogether.(This might even make it so that (e.g.)(Yeah, seems to work.)ppm install autocomplete-pathsinstalls the package properly the first time around; I'll check that next.)I hope we can get this into the next release as a quality-of-life issue, but it can wait a month if people are nervous about it.