Repository navigation
Conversation
UebelAndre
left a comment
UebelAndre
left a comment
There was a problem hiding this comment.
In general I think this is a great idea! We should probably add the following to the .bazelrc files around the repo to ensure we are regression sufficient.
# https://github.com/bazelbuild/bazel/issues/19981
common --incompatible_auto_exec_groups
| progress_message = "Running Cargo build script {}".format(pkg_name), | ||
| env = env, | ||
| toolchain = None, | ||
| toolchain = rust_common.toolchain_type, |
There was a problem hiding this comment.
Should this be the Cargo toolchain?
| toolchain = rust_common.toolchain_type, | |
| toolchain = rust_common.cargo_toolchain_type, |
There was a problem hiding this comment.
I don't think so, actually.
Arguably, a better answer is a named exec group for the build-script action that declares the Rust, Cargo and cc toolchains together. Bazel then picks a machine that has all three and they can't disagree. This requires a larger change, to find_toolchain, find_cargo and find_cc_toolchain, maybe more. The rust toolchain works without that.
There was a problem hiding this comment.
I should put "better" in quotes because it's more semantic but in practice will never make a difference for Cargo. For C/C++ it's plausible it could make a difference in weird edge cases, e.g. Mac host with C++ toolchain registered, Linux RBE workers with no C/C++ toolchain for some reason, and a crate that compiles C code from build.rs.
There was a problem hiding this comment.
Oh I didn't answer what's wrong with using cargo toolchain. It's optional to specify one and is therefore often a fallback value, rust_toolchain.cargo. And build scripts still run with RUSTC which comes from the rust toolchain.
If misunderstanding, let me know - will of course just switch it to cargo toolchain and re-test, or create the first-class exec group for build scripts.
…er and proc-macros for the Rust toolchain's group
…with their tools built for it
This would require a bunch more cleanup fixes - want that here, separate PR(s), defer? |
Fixes #4360
speculative fixes for AEG