search

purescript / npm-installer

ISC License
10 stars 13 forks source link

Support macOS and Linux ARM64 binaries #44

Closed f-f closed 1 year ago

f-f commented 1 year ago

Fix #43.

This codebase is extremely frustrating. There are endless layers of indirection and it's impossible to get anything done in a reasonable amount of time.

There are over 2000 lines of JS in here, and I feel like we ought to get the job done in 100-200. I sort of want to rewrite the whole thing, but in this PR I limited myself to rewriting the module that downloads from a release to also look for ARM releases.

I ditched the dependency on the arch package in the process because it is a pile of code that implements the wrong thing, and we can use the builtin process.arch instead.

I have tested locally that this works, by running the instructions for "local development" and then running npx install-purescript --purs-ver=0.15.9-5.

JordanMartinez commented 1 year ago

Honestly, I'd be down for a rewrite...

rhendric commented 1 year ago

the arch package [...] is a pile of code that implements the wrong thing, and we can use the builtin process.arch instead.

Is this true? I thought the arch package was necessary to keep, for example, a 32-bit Node.js running on 64-bit Windows from reporting that the native architecture is 32-bit. I know we aren't publishing 32-bit binaries for any platform but wouldn't switching to process.arch mean that some users might get an unsupported platform error instead of the 64-bit binary that they could actually run?

Honestly, I'd be down for a rewrite...

Big same. Last time I poked around in here I remember briefly exploring doing it in PureScript and I don't remember what stopped me (probably the finitude of time).

f-f commented 1 year ago

I know we aren't publishing 32-bit binaries for any platform but wouldn't switching to process.arch mean that some users might get an unsupported platform error instead of the 64-bit binary that they could actually run?

I don't think we should really support the use case of "running a 32-bit Node on a 64-bit" machine?

In any case, the arch package does not do what we need here because it just cares about 32 vs 64 bits - comparison on my Mac M1:

> process.arch
'arm64'
> require('arch')()
'x64'
f-f commented 1 year ago

Last time I poked around in here I remember briefly exploring doing it in PureScript and I don't remember what stopped me (probably the finitude of time).

Indeed. After spending a couple days on this I don't really feel looking at it anymore for a while 😄

rhendric commented 1 year ago

In any case, the arch package does not do what we need here because it just cares about 32 vs 64 bits - comparison on my Mac M1:

Hm, I see. arch has an open PR for this but it appears to be abandoned.

How about this: if os.machine() is available, use that; otherwise, fall back to process.arch; and if a user both is running a version of Node too old to have os.machine and is in WoW64 or Rosetta, they fall through the cracks.

f-f commented 1 year ago

How about this: if os.machine is available, use that; otherwise, fall back to process.arch

Sure, I just pushed a commit that tries to use os.machine if it's available