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

Add hermetic_cc_toolchain for a hermetic cc toolchain #12135

Merged
merged 62 commits into from
Jun 26, 2023

Conversation

prestonvanloon
Copy link
Member

@prestonvanloon prestonvanloon commented Mar 15, 2023

What type of PR is this?

Other

What does this PR do? Why is it needed?

This PR introduces a replacement C/C++ toolchain for Prysm. Previously, we were using a LLVM toolchain from github.com/grailbio/bazel-toolchain and only when the user requested --config=llvm.

This PR aims to solve two problems:

Building C++ Depends on Host Tooling -- When building anything that depends on C or C++ code in prysm, bazel will use the host machine gcc. This can cause undesired behavior where identical builds on different linux machines result in different outputs. Namely, if your host is using a recent version of gcc then non-backwards compatible glibc libraries will be requested. See #7750 for an example. This behavior is non-hermetic and violates the desired principal of reproducible builds.

llvm Config Doesn't Work on Many OSes -- The way that the existing cc toolchain works is that it downloads an appropriate version of llvm based on the host's OS. Some users have complained that the LLVM toolchain fails when using an operating system like Pop!OS or even Arch Linux. Again, this may be non-hermetic behavior as the host's OS should not influence how the CC toolchain compiles Prysm.

The key features/changes are:

  • Default to using this cc_toolchain instead of the host / gcc toolchain
  • Pin glibc to 2.31 to support ubuntu 20.04 explicitly. (EOL is April, 2025)
  • Improve cross compilation by having a cc_toolchain that "just works". Enables support for multi-arch compilation for docker: Multiarch docker containers #12428

Which issues(s) does this PR fix?

Supercedes #12100

Fixes #7750
Fixes #9515

Other notes for review

Testing plan

To test this PR, we want to see that the release assets will build and everything else should build, but some manual targets are expected to fail on some configurations. (Some things are linux only)

# Build release assets
bazel build --config=release //cmd/beacon-chain //cmd/validator //cmd/client-stats //cmd/prysmctl

# Everything should work
bazel build //...

# Optionally, build everything including manual tests
bazel build //... --build_manual_tests
  • Builds on linux amd64
  • Builds on linux arm64 (untested)
  • Builds on darwin amd64 (Mac OS without M1/M2 chip) ❌ Currently failing
  • Builds on darwin arm64 (Mac OS with M1/M2 chip) ❌ Currently failing

❌ Builds on windows amd64 (Not supported)

⚠️ For darwin builds, I have added a hack that will allow a local C++ toolchain to be used until we can figure out how to hermetically provide a sysroot. We know this is possible with the docker cross compile, but we have to add that upstream to the toolchain provider. Additionally, there is a known issue where zig can't compile cgo on darwin. For the moment, we'll defer adding hermetic builds on darwin until those items can be resolved.

Additionally, we want to see cross compile continues to work on from linux amd64

bazel build --config=release --config=linux_arm64_docker  //cmd/beacon-chain //cmd/validator //cmd/client-stats //cmd/prysmctl
bazel build --config=release --config=osx_amd64_docker  //cmd/beacon-chain //cmd/validator //cmd/client-stats //cmd/prysmctl
bazel build --config=release --config=osx_arm64_docker  //cmd/beacon-chain //cmd/validator //cmd/client-stats //cmd/prysmctl
bazel build --config=release --config=windows_amd64_docker  //cmd/beacon-chain //cmd/validator //cmd/client-stats //cmd/prysmctl
  • Cross compile still works on linux amd64

@prestonvanloon prestonvanloon marked this pull request as ready for review April 24, 2023 16:48
@prestonvanloon prestonvanloon requested a review from a team as a code owner April 24, 2023 16:49
@prestonvanloon prestonvanloon changed the title Add bazel-zig-cc for a hermetic cc toolchain Add hermetic_cc_toolchain for a hermetic cc toolchain May 2, 2023
.bazelrc Show resolved Hide resolved
build:llvm-asan --config=asan
build:llvm-asan --linkopt -fuse-ld=ld.lld

build:fuzz --@io_bazel_rules_go//go/config:tags=fuzz
Copy link
Contributor

Choose a reason for hiding this comment

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

just a note that we are removing fuzzing and the sanitize_address stuff is used for this.

build --incompatible_enable_cc_toolchain_resolution

# Add a no-op warning for users still using --config=llvm
build:llvm --unconditional_warning="llvm config is no longer used as clang is now the default compiler"
Copy link
Contributor

Choose a reason for hiding this comment

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

we don't need to mention zig here at all right? even though it's using clang underneath?

WORKSPACE Outdated
"@zig_sdk//toolchain:linux_arm64_gnu.2.31",
"@zig_sdk//toolchain:darwin_amd64",
"@zig_sdk//toolchain:darwin_arm64",
"@zig_sdk//toolchain:windows_amd64",
Copy link
Contributor

Choose a reason for hiding this comment

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

+1 to your suggestion in huddle to rename this to match the upstream repo.

Copy link
Member Author

Choose a reason for hiding this comment

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

@@ -0,0 +1,19 @@
# From Bazel's perspective, this is almost equivalent to always specifying
Copy link
Contributor

Choose a reason for hiding this comment

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

another thing to rename zig-cc.bazelrc

build --incompatible_enable_cc_toolchain_resolution

# Add a no-op warning for users still using --config=llvm
build:llvm --unconditional_warning="llvm config is no longer used as clang is now the default compiler"
Copy link
Contributor

Choose a reason for hiding this comment

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

will this force a build failure with llvm or just display a warning (that could get missed in the piles of compiler output)?

Copy link
Member Author

Choose a reason for hiding this comment

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

It is just a warning. Without some build:llvm, the build will fail entirely when using --config=llvm

@prestonvanloon
Copy link
Member Author

darwin arm64 currently failing with

external/go_sdk/pkg/tool/darwin_arm64/link: running external/zig_sdk/tools/aarch64-macos-none/c++ failed: exit status 1
warning(link): framework not found for '-framework CoreFoundation'
warning(link): Framework search paths:
error: FrameworkNotFound

Looks like ziglang/zig#10513

@prestonvanloon
Copy link
Member Author

Windows fails to build too. I think the bazel rules for protoc are not compatible... I'm not sure we ever supported building on windows.

@prestonvanloon
Copy link
Member Author

Another blocker for mac amd64 ziglang/zig#15438

Copy link
Contributor

@james-prysm james-prysm left a comment

Choose a reason for hiding this comment

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

was building fine locally so approving this

@prylabs-bulldozer prylabs-bulldozer bot merged commit a190440 into develop Jun 26, 2023
@prylabs-bulldozer prylabs-bulldozer bot deleted the bazel-zig-cc branch June 26, 2023 14:31
james-prysm pushed a commit that referenced this pull request Jun 27, 2023
* Add bazel-zig-cc for a hermetic cc toolchain

* gazelle

* Remove llvm

* remove wl

* Add new URLs for renamed repo

* gazelle

* Update to v2.0.0-rc1

* bump to rc2

* Some PR feedback

* use v2.0.0 from rc2

* Disable hermetic builds for mac and windows.

* bump bazel version, add darwin hack

* fix

* Add the no-op emtpy cc toolchain code

* typo and additional copy

* update protobuf and fix vaticle warning

* Revert "update protobuf and fix vaticle warning"

This reverts commit 7bb4b6b.

---------

Co-authored-by: prylabs-bulldozer[bot] <58059840+prylabs-bulldozer[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants