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

First cut at multi-arch cross compiling toolchain #4945

Merged

Conversation

SuburbanDad
Copy link
Contributor

@SuburbanDad SuburbanDad commented Feb 26, 2020

currently supports arm64 and amd64 via docker cross compiler image

Resolves #2849

cross-compiling docker toolchain. Currently only arm64 and amd64 are working.

See tools/cross-toolchain/README.md


Description

Write why you are making the changes in this pull request

Write a summary of the changes you are making

Link anything that would be helpful or relevant to the reviewers

@SuburbanDad SuburbanDad changed the title WIP first cut at multi-arch cross compiling toolchain. curren… WIP first cut at multi-arch cross compiling toolchain Feb 26, 2020
@SuburbanDad SuburbanDad changed the title WIP first cut at multi-arch cross compiling toolchain First cut at multi-arch cross compiling toolchain Feb 26, 2020
Copy link
Member

@prestonvanloon prestonvanloon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you considered using bazel's remote execution protocol?

https://docs.bazel.build/versions/2.0.0/remote-execution-sandbox.html

WORKSPACE Outdated Show resolved Hide resolved
@SuburbanDad
Copy link
Contributor Author

SuburbanDad commented Feb 27, 2020

Have you considered using bazel's remote execution protocol?

https://docs.bazel.build/versions/2.0.0/remote-execution-sandbox.html

This looks like a great option, but it doesn't seem to work as described in the link. It isn't even trying to start a docker container for the execution environment. I think it is making some assumptions about the host environment being the same as the execution environment, since it just immediately fails to resolve a toolchain

I will keep at it, but I am not going to block on this.

edit: it looks like the docker remote execution dost not work when the host and execution environments are not the same (e.g. can't use remote exec from an OSX host to build with a linux exec): bazelbuild/bazel#5397

I can see that it would be useful if one had a linux x86_64 host/build environment though (like a ci/cd pipeline on a commodity vm), so I added configs for it. I don't have access to a linux x86_64 host to test it from though.

.bazelrc Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Mar 2, 2020

Codecov Report

Merging #4945 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #4945   +/-   ##
=======================================
  Coverage   41.79%   41.79%           
=======================================
  Files         226      226           
  Lines       17664    17664           
=======================================
  Hits         7382     7382           
  Misses       9009     9009           
  Partials     1273     1273

@garyschulte garyschulte force-pushed the clang-cross-compile branch from 6b05d04 to d2fcbbc Compare March 2, 2020 08:33
@garyschulte garyschulte force-pushed the clang-cross-compile branch from d2fcbbc to 0df6ca1 Compare March 2, 2020 08:35
@SuburbanDad
Copy link
Contributor Author

@prestonvanloon , after finding all the nooks and crannies necessary to get this to work with a static docker image, I think docker is the right way to go. I think trying to do something like this with grailbio's toolchain would be too difficult and fragile. Especially considering the variety of host platforms that would need to be supported.

However, all four targets are working along with docker remote execution (if on linux amd64 host). Please kick the tires and lmk

@prestonvanloon
Copy link
Member

Looking now!

@prestonvanloon
Copy link
Member

Overall this looks great.

The validator does not build for windows, but I think that is a dependency issue. I'll push an official Prysmatic Labs docker image. Then we can update the README and merge.

I agree with the trade offs you've made and I think this will work great in our CI pipeline. Thanks so much for doing this. It is a really valuable feature!

@CLAassistant
Copy link

CLA assistant check
All committers have signed the CLA.

terencechain
terencechain previously approved these changes Mar 5, 2020
Copy link
Member

@terencechain terencechain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Only comment is if there's already an exisiting issue for building with libkafka, feel free to add the issue # there

- Local runs require clang compiler and the appropriate cross compilers installed. These runs should only be considered for a power user or user with specific build requirements. See the Dockerfile setup scripts to understand what dependencies must be installed and where.
- Docker sandbox is *slow*. Like really slow! The purpose of the docker sandbox is to test RBE builds without deploying a full RBE system. Each build action is executed in its own container. Given the large number of small targets in this project, the overhead of creating docker containers makes this strategy the slowest of all, but requires zero additional setup.
- Remote Build Execution is by far the fastest, if you have a RBE backend available. This is another advanced use case which will require two config flags above as well as additional flags to specify the `--remote_executor`. Some of these flags are present in the project `.bazelrc` with example values, but commented out.
- Building with libkafka (`--define kafka_enabled=true`) is not supported with docker-sandbox or RBE at this time. Likely due to missing cmake dependencies in the builder image or lack of configuration via toolchains.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there's an existing issue number, feel free to add it here

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is not one yet. It's not a very widely used feature yet.

nisdas
nisdas previously approved these changes Mar 5, 2020
@prestonvanloon prestonvanloon dismissed stale reviews from nisdas and terencechain via be9b9e3 March 5, 2020 05:31
@prestonvanloon
Copy link
Member

Fixed linter by running buildifer on the generated files.

Copy link
Member

@prestonvanloon prestonvanloon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So happy to have this. ❤️

@prylabs-bulldozer prylabs-bulldozer bot merged commit e2a6f5a into prysmaticlabs:master Mar 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for cross compilation in Bazel
6 participants