WhatsApp / erlang-language-platform

Erlang Language Platform. LSP server and CLI.
https://whatsapp.github.io/erlang-language-platform/
Apache License 2.0
209 stars 16 forks source link

ci: add Linux aarch64 build #15

Closed asabil closed 4 months ago

facebook-github-bot commented 5 months ago

Hi @asabil!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks!

asabil commented 5 months ago

Looks like elp is properly built for aarch64, however, that doesn't seem to be the case for eqwalizer

facebook-github-bot commented 5 months ago

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!

robertoaloi commented 5 months ago

Hi @asabil Thanks for working on this. Maybe it would be a good idea to split the refactoring into smaller PRs, to avoid a single PR that modifies the whole CI pipeline and to reduce/speed up the review process, making it more incremental.

asabil commented 5 months ago

Yes of course, what I have here is more of a draft. I initially opened this PR thinking I was mostly done, until I realized that elp depended on eqwalizer which is built in Scala and makes use of GraalVM to produce a native binary.

Life would have been easier if GraalVM's native-image supported cross-architecture cross compilation, as it turns out, it doesn't.

Before I head into thinking about how to split this into multiple PRs, there are few points that I would very much appreciate feedback/opinion on the following:

1. Pipeline architecture

The pipeline architecture in this PR is much more involved, which makes it more flexible, but also more complex: The build makes use separate workflows and jobs, sharing artifacts between the jobs using actions/upload-artifact and actions/download-artifacts.

This inevitably slows down the build in exchange for flexibility (with some minor effort we should also be able to support windows).

2. eqwalizer on macOS/arm64 is actually built for macOS/x86_64

This is not new to this PR, but was also an issue in the previous PR. The binary works thanks to Rosetta. This can only be resolved by using a macos building running arm64.

3. eqwalizer build for linux/arm64 takes 1.5h on the standard GitHub runner

This is due using qemu for emulation. Again, this could be sped up by using a native Linux/arm64 runner.

4. Change in naming convention

I chose to using standardized triplets and arch names such as x86_64-unknown-linux-gnu and aarch64-apple-darwin so as to be able to support other linux builds if needed/wanted, such as musl libc based ones.

Thank you.

robertoaloi commented 5 months ago

@asabil

1. Pipeline architecture

The CI pipeline originally started as multiple workflows, but that led to various discrepancies so unifying was also a way to avoid code duplication and keep everything consistent. The easier the better. I like the idea of splitting jobs such as publish-extension.

2. eqwalizer on macOS/arm64 is actually built for macOS/x86_64

3. eqwalizer build for linux/arm64 takes 1.5h on the standard GitHub runner

Correct. This is because (as far as I know) GitHub public runners do not include a arm variant. As long as the eqwalizer and elp are separate, I wonder if instead we should skip building eqwalizer itself and use the artifacts from there, instead.

4. Change in naming convention

That should already be the case when it comes to targets. See for example:

platform-arch: ubuntu-20.04-x64
platform: ubuntu-20.04
os: linux
target: x86_64-unknown-linux-gnu
vscode-target: linux-x64

I like the existing kind of structure since it allows us to provide "aliases" for the target which we can use in different contexts (e.g. when you publish to the marketplace you need to map the standard target to something like linux-x64. This mechanism allows us to avoid things like:

          case "${{ inputs.target }}" in
            x86_64-unknown-linux-gnu) vscode_target=linux-x64 ;;
            aarch64-unknown-linux-gnu) vscode_target=linux-arm64 ;;
            x86_64-apple-darwin) vscode_target=darwin-x64 ;;
            aarch64-apple-darwin) vscode_target=darwin-arm64 ;;
            *)
              echo "Unsupported target platform" >&2
              exit 1
          esac

Which can distract from what the action is trying to do and are more error prone (in my view).

robertoaloi commented 5 months ago

As an additional consideration, it is now be possible to run ELP without eqwalizer (see #14), so for Linux Arm we could simply publish a version of ELP which does not include eqwalizer and report the issue in whatsapp/eqwalizer. You can still build eqwalizer from source (yes, it's inconvenient).

asabil commented 4 months ago

Thank you for the feedback. It has been a bit of a busy week for me, but I will try to pick this up again soon and open a new set of PRs.