Skip to content
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

ci: add Linux aarch64 build #15

Closed
wants to merge 42 commits into from

Conversation

asabil
Copy link
Contributor

@asabil asabil commented Jan 31, 2024

No description provided.

@facebook-github-bot
Copy link
Contributor

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
Copy link
Contributor Author

asabil commented Jan 31, 2024

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

@facebook-github-bot
Copy link
Contributor

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

@asabil asabil marked this pull request as draft February 1, 2024 20:56
@asabil asabil force-pushed the feature/linux-aarch64-build branch from c877002 to 3c5e78c Compare February 1, 2024 21:03
@asabil asabil force-pushed the feature/linux-aarch64-build branch from bc70980 to 11d61f1 Compare February 3, 2024 22:21
@robertoaloi
Copy link
Contributor

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
Copy link
Contributor Author

asabil commented Feb 5, 2024

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
Copy link
Contributor

@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
Copy link
Contributor

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
Copy link
Contributor Author

asabil commented Feb 15, 2024

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.

@asabil asabil closed this Feb 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants