elm / compiler

Compiler for Elm, a functional language for reliable webapps.
https://elm-lang.org/
BSD 3-Clause "New" or "Revised" License
7.48k stars 659 forks source link

Expand npm installer to include ARM64 Linux #2234

Open cllns opened 2 years ago

cllns commented 2 years ago

A quickfix to #2232 (see comments there), though it may be re-written better or removed entirely (the only main architecture/OS combo this seems to leave out now is ARM Windows).

Or maybe we want to un-do https://github.com/elm/compiler/commit/759bf04a298a357018d13c85cbb315549893c26d in its entirety?

(Note that this lets Apple Silicon users with Elm in docker containers use the npm installer)

I can't find any documentation about how to run the installers, so I can't test this. But v0.19.1-4 works and v0.19.1-5 does not.

github-actions[bot] commented 2 years ago

Thanks for suggesting these code changes. To set expectations:

Finally, please be patient with the core team. They are trying their best with limited resources.

sporto commented 2 years ago

Hi @evancz, could we please merge this fix? This is becoming a real issue as more developers are moving to Macs M1 and using Docker.

avh4 commented 2 years ago

How is a linux-arm64 docker container able to run linux-x64 binaries? Is that somehow using Rosetta inside the docker container? Does it only work on MacOS? Does your docker image have qemu-user-binfmt configured, or can all linux-arm64 docker images somehow run linux-x64 binaries (including on a linux-arm64 host)?

If this working relies on Rosetta and/or qemu-user-binfmt, then I think this is probably incorrect to merge since it will get and try to run binaries for the wrong architecture on machines that don't have emulation services configured.

Also for the record, imo we ought to make official linux-arm64 binaries and then update the npm installer to use those when appropriate.

sporto commented 2 years ago

@avh4 We tried downloading the binary for linux manually from releases and run it inside Docker on a M1. It works, but it is very slow. Unsure what is going on, but yes, doesn't seem like the right approach.

lydell commented 2 years ago

@sporto Which Docker image do you use? I don’t have an M1, but some colleagues do, and as far as I remember running the Linux x64 binary in node:16-bullseye failed for them. If you have time, creating a minimal Dockerfile that demonstrates successfully running Elm on an M1 would be great!

(Btw, elm-tooling currently does not have any arch checks, so you should be able to use it to install the (only) Linux binary in Docker.)

sporto commented 2 years ago

@lydell We used node:lts-buster. One of my collegues tried this as I don't have an M1 myself.

The dockerfile is something like

FROM node:lts-buster@sha256:a6c217d7c8f001dc6fc081d55c2dd7fb3fefe871d5aa7be9c0c16bd62bea8e0c

RUN apt-get update

RUN curl -L -o elm.gz https://github.com/elm/compiler/releases/download/0.19.1/binary-for-linux-64-bit.gz \
    && gunzip elm.gz \
    && chmod +x elm \
    && mv elm /usr/local/bin/
alshdavid commented 1 year ago

Perhaps adding a check to see if an elm binary is available before deciding to download would be better. That way people with Rosetta2 and Box64 can manually install the amd64 binary knowing its limitations while the project maintainers can add darwin-arm64 and linux-arm64 binaries later

alshdavid commented 1 year ago

Hey @evancz, any chance you can chime in here as the change in to add verification in the postinstall breaks the npm install on my project when compiled in Docker on MacOS / arm Linux.

I'd raise a PR with the changes myself, but from the GitHub pulse it seems this project is no longer maintained?

nicklayb commented 3 months ago

@evancz Any way we could get this merged? Of course we can download the binary manually but I came to a point where that doesn't work either. I'm using create-elm-app and has elm as a npm dependency, so it tries to download the binary anyway

lydell commented 3 months ago

@nicklayb This PR has merge conflicts, because the code looks nothing like this anymore since https://github.com/elm/compiler/pull/2287. So it won’t be merged (as-is).

Also, this PR just points to the regular x86 binary for Linux ARM, which is not going to work (in most cases). People with real Linux ARM computers need a Linux ARM binary (as far as I understood from people testing my PR), and people with macOS ARM running in Docker also need a Linux ARM binary. The @lydell/elm package exists for this reason. You can force npm to use my package instead, something like this:

{
  "overrides": {
    "elm": "npm:@lydell/elm@0.19.1-14"
  },
  "dependencies": {
    "create-elm-app": "^5.22.0"
  }
}

(Btw, create-elm-app isn’t maintained, so you might want to use some other tool like Vite, Parcel 2, Elm Land or elm-watch.)