Migrate to Bzlmod #626
Migrate to Bzlmod #626
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Wow. I did not expect this one so quickly. |
There was a problem hiding this comment.
+@MarcusSorealheis +@adam-singer +@allada
CC @blakehatch @kubevalet it could be interesting to see how the toolchains in .bazelrc are set up. Essentially, the build uses whatever it finds on your system locally (no configuration, default behavior) but if you pass --config=linux_zig or --config=lre it switches over to a specific toolchain. Ultimately a goal is to have all our supported platforms mapped out with fully hermetic and remote-exec compatible toolchains and then to enable BAZEL_DO_NOT_DETECT_CPP_TOOLCHAIN=1 globally in .bazelrc.
Reviewable status: 0 of 3 LGTMs obtained (waiting on @adam-singer, @allada, and @MarcusSorealheis)
There was a problem hiding this comment.
Reviewed 41 of 43 files at r1, all commit messages.
Reviewable status: 0 of 3 LGTMs obtained (waiting on @allada and @MarcusSorealheis)
local-remote-execution/rbe-configs-gen.nix line 4 at r1 (raw file):
pkgs.buildGoModule rec { pname = "bazel-toolchains"; version = "5.1.3-rc1";
do we want to depend on an rc here or is this auto generated code we might not control?
tools/rules_rust_deduplicate_sysroot.diff line 12 at r1 (raw file):
+ if not toolchain._incompatible_no_rustc_sysroot_env: + env["SYSROOT"] = toolchain.sysroot + env["DYLD_LIBRARY_PATH"] = toolchain.sysroot + "/lib"
Hard to tell is this env always set or just set on mac? I think its only a mac respected env variable. Did these rules patches come from the upstream PR thats not merged into rules_rust?
There was a problem hiding this comment.
What is the quick tl'dr for someone to test this on mac, would bazel test //... be a good smoke screen when pulling down this patch?
Reviewable status: 0 of 3 LGTMs obtained (waiting on @allada and @MarcusSorealheis)
There was a problem hiding this comment.
Reviewable status: 0 of 3 LGTMs obtained (waiting on @adam-singer, @allada, and @MarcusSorealheis)
.github/workflows/native-bazel.yaml line 59 at r1 (raw file):
What is the quick tl'dr for someone to test this on mac, would
bazel test //...be a good smoke screen when pulling down this patch?
Yes. This is the CI workflow that tests it. Note that this only works using a native toolchain but not in a nix shell (that would be tested in the nix-bazel workflow instead).
local-remote-execution/rbe-configs-gen.nix line 4 at r1 (raw file):
Previously, adam-singer (Adam Singer) wrote…
do we want to depend on an
rchere or is this auto generated code we might not control?
This version includes changes to the java toolchains with Bazel 7. Between 5.1.2 and this ´rc´are just six commits bazelbuild/bazel-toolchains@v5.1.2...v5.1.3-rc1
There was a problem hiding this comment.
@aaronmondal worked for me out of the box!
Reviewed 2 of 43 files at r1.
Reviewable status: 1 of 3 LGTMs obtained (waiting on @allada and @MarcusSorealheis)
aaronmondal
force-pushed
the
migrate-to-bzlmod
branch
from
8bc74b1
to
bc2bcf4
Compare
There was a problem hiding this comment.
Reviewable status: 1 of 3 LGTMs obtained, and pending CI: Analyze (javascript-typescript), Analyze (python), Bazel Dev / ubuntu-22.04, Cargo Dev / macos-13, Cargo Dev / ubuntu-22.04, Local / ubuntu-22.04, Remote / large-ubuntu-22.04, asan / ubuntu-22.04, docker-compose-compiles-nativelink (20.04), docker-compose-compiles-nativelink (22.04), integration-tests (20.04), integration-tests (22.04), macos-13, pre-commit-checks, publish-image, ubuntu-20.04, ubuntu-20.04 / stable, ubuntu-22.04, ubuntu-22.04 / stable, windows-2022 / stable (waiting on @allada and @MarcusSorealheis)
tools/rules_rust_deduplicate_sysroot.diff line 12 at r1 (raw file):
Previously, adam-singer (Adam Singer) wrote…
Hard to tell is this env always set or just set on mac? I think its only a mac respected env variable. Did these rules patches come from the upstream PR thats not merged into rules_rust?
This was bazelbuild/rules_rust#2422 with your added change for DYLD_LIBRARY_PATH. This actually doesn't seem to have any effect, so I changed it back to the original and added the sources of the patches to the MODULE.bazel.
There was a problem hiding this comment.
Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: 1 of 3 LGTMs obtained, and pending CI: asan / ubuntu-22.04 (waiting on @allada and @MarcusSorealheis)
There was a problem hiding this comment.
Reviewable status: 1 of 3 LGTMs obtained, and pending CI: asan / ubuntu-22.04 (waiting on @allada and @MarcusSorealheis)
There was a problem hiding this comment.
Reviewable status: 1 of 3 LGTMs obtained, and pending CI: asan / ubuntu-22.04 (waiting on @allada and @MarcusSorealheis)
.github/workflows/native-bazel.yaml line 59 at r1 (raw file):
Previously, aaronmondal (Aaron Siddhartha Mondal) wrote…
What is the quick tl'dr for someone to test this on mac, would
bazel test //...be a good smoke screen when pulling down this patch?Yes. This is the CI workflow that tests it. Note that this only works using a native toolchain but not in a nix shell (that would be tested in the nix-bazel workflow instead).
sgtm, I'd say one impediment that gets to me is not being able to run the rust formatter or get the clippy rules checking, which I assume doesn't require nix? Getting past that for now is a huge win
There was a problem hiding this comment.
Reviewed 37 of 43 files at r1, 1 of 2 files at r2.
Reviewable status: 1 of 3 LGTMs obtained, and pending CI: asan / ubuntu-22.04 (waiting on @MarcusSorealheis)
-- commits line 20 at r2:
Hmmm, should we push on rust_rules people to fix this and consider it blocking?
I'm not sure we should force people to run more complex bazel commands to get going.
.github/workflows/main.yml line 53 at r2 (raw file):
docker run --rm --net=host -w /root/nativelink -v $PWD:/root/nativelink trace_machina/nativelink:builder sh -c ' \ bazel clean && \ bazel test --config=linux_zig //... \
nit: We should have at least one workflow not use zig.
local-remote-execution/MODULE.bazel line 12 at r2 (raw file):
bazel_dep(name = "platforms", version = "0.0.8") bazel_dep(name = "rules_cc", version = "0.0.9") bazel_dep(name = "rules_java", version = "7.3.2")
nit: Why rules_java?
local-remote-execution/MODULE.bazel line 14 at r2 (raw file):
bazel_dep(name = "rules_java", version = "7.3.2") # register_execution_platforms("//generated/config:platform")
nit: Should be removed?
tools/rules_rust_bindgen_linkopts.diff line 1 at r2 (raw file):
diff --git a/bindgen/private/bindgen.bzl b/bindgen/private/bindgen.bzl
Did we upstream this? We should reference the upstream PR if we did, and if not we should upstream it.
tools/rules_rust_deduplicate_sysroot.diff line 1 at r2 (raw file):
diff --git a/rust/private/rustc.bzl b/rust/private/rustc.bzl
ditto.
aaronmondal
force-pushed
the
migrate-to-bzlmod
branch
from
bc2bcf4
to
200eda0
Compare
There was a problem hiding this comment.
Reviewable status: 1 of 3 LGTMs obtained, and pending CI: Analyze (javascript-typescript), Bazel Dev / ubuntu-22.04, Vercel, asan / ubuntu-22.04, docker-compose-compiles-nativelink (20.04), pre-commit-checks, publish-image, ubuntu-20.04 / stable (waiting on @allada and @MarcusSorealheis)
Previously, allada (Nathan (Blaise) Bruer) wrote…
Hmmm, should we push on
rust_rulespeople to fix this and consider it blocking?I'm not sure we should force people to run more complex bazel commands to get going.
This actually simplifies the setup as the only command required for building is bazel build. The lockfile is autogenerated and the new approach obsoletes the CARGO_BAZEL_REPIN=1 commands for dependency updates.
I think rules_rust is aware of this, but it's apparently a nontrivial thing to fix. Once they do fix it we can add the lockfile back, but it'll be an invisible change for users.
local-remote-execution/MODULE.bazel line 12 at r2 (raw file):
Previously, allada (Nathan (Blaise) Bruer) wrote…
nit: Why
rules_java?
This is similar to rules_cc. The java rules were originally built into Bazel, but are now superceded by rules_java. It's often not visible because most projects use the legacy builtin rules.
tools/rules_rust_bindgen_linkopts.diff line 1 at r2 (raw file):
Previously, allada (Nathan (Blaise) Bruer) wrote…
Did we upstream this? We should reference the upstream PR if we did, and if not we should upstream it.
This is from bazelbuild/rules_rust#2422
It's documented in the git_override for rules_rust in the MODULE.bazel file.
tools/rules_rust_deduplicate_sysroot.diff line 1 at r2 (raw file):
Previously, allada (Nathan (Blaise) Bruer) wrote…
ditto.
This is from bazelbuild/rules_rust#2428
It's documented in the git_override for rules_rust in the MODULE.bazel file.
aaronmondal
force-pushed
the
migrate-to-bzlmod
branch
from
200eda0
to
eb42a99
Compare
aaronmondal
force-pushed
the
migrate-to-bzlmod
branch
from
eb42a99
to
b645cd0
Compare
There was a problem hiding this comment.
Reviewed 2 of 2 files at r3, all commit messages.
Reviewable status: 2 of 3 LGTMs obtained (waiting on @MarcusSorealheis)
.github/workflows/native-bazel.yaml line 75 at r3 (raw file):
fail-fast: false matrix: os: [ubuntu-20.04, ubuntu-22.04]
nit: Lets add 18.04.
cc: @chrisstaite-menlo , you should be happy about this, as it'll make 18.04 officially supported 😄
local-remote-execution/MODULE.bazel line 12 at r2 (raw file):
Previously, aaronmondal (Aaron Siddhartha Mondal) wrote…
This is similar to
rules_cc. The java rules were originally built into Bazel, but are now superceded byrules_java. It's often not visible because most projects use the legacy builtin rules.
Lets add a comment for that.
aaronmondal
force-pushed
the
migrate-to-bzlmod
branch
from
b645cd0
to
bdaca3b
Compare
There was a problem hiding this comment.
Reviewable status: 2 of 3 LGTMs obtained, and pending CI: Vercel, pre-commit-checks (waiting on @MarcusSorealheis)
.github/workflows/native-bazel.yaml line 75 at r3 (raw file):
Previously, allada (Nathan (Blaise) Bruer) wrote…
nit: Lets add 18.04.
cc: @chrisstaite-menlo , you should be happy about this, as it'll make 18.04 officially supported 😄
I tried to, but 18.04 isn't supported in GHA anymore. We need to set up self-hosted Ubuntu 18 runners if we want that.
The new builds bootstrap the crate_universe binary which increases compatibility with various platforms. New supported builds: - Native Bazel on MacOS - Nix-wrapped Cargo fon MacOS - Native Bazel builds with zig-cc on Ubuntu 20.04 and 22.04 In addition, add a new `--config=linux_zig` which allows building nativelink with a more hermetic (but not perfectly hermetic) toolchain on systems that can't use the LRE toolchains. The zig-cc toolchain runs a lot slower than the LRE toolchains but is more similar to the LRE toolchain and at least somewhat reduces variation between compilers. This commit is a slight regression in terms of hermeticity and reproducibility. The bzlmod migration moved the Cargo.Bazel.lock file to an autogenerated MODULE.bazel.lock, but we have to gitignore that file for now since rules_rust currently doesn't create the same lockfiles on Linux and MacOS.
There was a problem hiding this comment.
Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: 2 of 3 LGTMs obtained, and pending CI: pre-commit-checks (waiting on @MarcusSorealheis)
-- commits line 2 at r4:
nit: This is not Breaking from what I can tell.
aaronmondal
force-pushed
the
migrate-to-bzlmod
branch
from
bdaca3b
to
bf72c99
Compare
There was a problem hiding this comment.
Reviewable status: 2 of 2 LGTMs obtained
Previously, allada (Nathan (Blaise) Bruer) wrote…
nit: This is not
Breakingfrom what I can tell.
True. I'll remove the [Breaking] when squash merging.
There was a problem hiding this comment.
Dismissed @adam-singer from a discussion.
Reviewable status:complete! 2 of 2 LGTMs obtained
The new builds bootstrap the crate_universe binary which increases compatibility with various platforms.
New supported builds:
In addition, a new
--config=linux_zigwas added which allows building nativelink with a more hermetic (but not perfectly hermetic) toolchain on systems that can't use the LRE toolchains. The zig-cc toolchain runs a lot slower than the LRE toolchains but is more similar to the LRE toolchain and at least somewhat reduces variation between compilers.This commit is somewhat of a regression in terms of hermeticity and reproducibility. The bzlmod migration moved the Cargo.Bazel.lock file to an autogenerated MODULE.bazel.lock, but we have to gitignore that file for now since rules_rust currently doesn't create the same lockfiles on Linux and MacOS.
This change is