Skip to content

Commit

Permalink
bazel: Better remote caching (#29367)
Browse files Browse the repository at this point in the history
~~This PR improves our remote caching by changing the way we use
[`--workspace_status_command`](https://bazel.build/reference/command-line-reference#flag--workspace_status_command)
which enables us to fetch 100% of actions from the remote cache when
applicable.~~

~~Previously I noticed CI jobs would always appear to rebuild the leafs
of our repo (e.g. `environmentd` and `clusterd`), which unfortunately
take the longest because of linking. This was because our workspace
status command required generating some Rust code which was breaking our
remote caching. Now with this change I've observed the remote caching
working exactly as a local cache, e.g. a change in the `sql` crate only
requires re-building `environmentd` whereas it used to also cause a
recompile of `clusterd` and some other crates.~~

Edit: Unfortunately there is no free lunch and my testing was flawed.
The original approach from this PR of tweaking the way we use
`workspace_status_command` did not work, and we were actually just
providing a constant value for the git hash instead of the updated
value. After investigating further it turns out that Bazel treats local
cache artifacts differently than remote cache artifacts when it comes to
the workspace status and specificallly the `volatile-status.txt` file we
rely on here, see bazelbuild/bazel#10075.

The only way to prevent `build_info!()` from causing unnecessary
rebuilds _and_ update the git hash for anything that does get rebuilt,
is to side channel the necessary information like we're doing here. The
approach is we write the current git hash to a known location, in this
case `/tmp/mz_git_hash.txt` and then read it via Rust. `bin/bazel` has
been updated to write this file, and I'll make the same change to
`mzbuild`. When running with `--config=release-stamp` we fallback to the
original behavior with `workspace_status_command` and formally "stamp"
the builds which causes everything to get rebuilt.

This is definitely a hack but from what I can tell there is no other way
to get the same behavior. An alternative is we always just return a
constant git hash when not running with `--config=release-stamped`, not
sure how important stamping development builds is.

### Motivation

Progress towards
https://github.com/MaterializeInc/materialize/issues/26796

* Improve remote caching speed

### Checklist

- [x] This PR has adequate test coverage / QA involvement has been duly
considered. ([trigger-ci for additional test/nightly
runs](https://trigger-ci.dev.materialize.com/))
- [x] This PR has an associated up-to-date [design
doc](https://github.com/MaterializeInc/materialize/blob/main/doc/developer/design/README.md),
is a design doc
([template](https://github.com/MaterializeInc/materialize/blob/main/doc/developer/design/00000000_template.md)),
or is sufficiently small to not require a design.
  <!-- Reference the design in the description. -->
- [x] If this PR evolves [an existing `$T ⇔ Proto$T`
mapping](https://github.com/MaterializeInc/materialize/blob/main/doc/developer/command-and-response-binary-encoding.md)
(possibly in a backwards-incompatible way), then it is tagged with a
`T-proto` label.
- [x] If this PR will require changes to cloud orchestration or tests,
there is a companion cloud PR to account for those changes that is
tagged with the release-blocker label
([example](MaterializeInc/cloud#5021)).
<!-- Ask in #team-cloud on Slack if you need help preparing the cloud
PR. -->
- [x] If this PR includes major [user-facing behavior
changes](https://github.com/MaterializeInc/materialize/blob/main/doc/developer/guide-changes.md#what-changes-require-a-release-note),
I have pinged the relevant PM to schedule a changelog post.
ParkMyCar authored Sep 5, 2024

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
1 parent 593c889 commit d5f7831
Showing 9 changed files with 86 additions and 33 deletions.
2 changes: 1 addition & 1 deletion .bazelrc
Original file line number Diff line number Diff line change
@@ -28,7 +28,7 @@ build --copt=-Wno-error=deprecated-declarations
# Required to stamp our development builds with the current git hash.
#
# This script gets run before every build, see the script for more info.
build --workspace_status_command "python3 misc/bazel/build-info/workspace_status.py"
build:release-stamp --stamp --workspace_status_command "python3 misc/bazel/build-info/workspace_status.py"

# Output all test output by default, this makes it most like cargo.
#
2 changes: 1 addition & 1 deletion Cargo.toml
Original file line number Diff line number Diff line change
@@ -333,7 +333,7 @@ store = { path = "misc/cargo-vet" }
# DO NOT EDIT. Automatically generated by bin/gen-lints.
[workspace.lints.rust]
unknown_lints = "allow"
unexpected_cfgs = { level = "warn", check-cfg = ['cfg(bazel, coverage, nightly_doc_features, release, tokio_unstable)'] }
unexpected_cfgs = { level = "warn", check-cfg = ['cfg(bazel, stamped, coverage, nightly_doc_features, release, tokio_unstable)'] }

[workspace.lints.rustdoc]
unportable_markdown = "allow"
5 changes: 5 additions & 0 deletions misc/bazel/build-info/BUILD.bazel
Original file line number Diff line number Diff line change
@@ -38,3 +38,8 @@ gen_build_info(
rust_file = "build_info.rs",
visibility = ["//visibility:public"],
)

config_setting(
name = "stamped",
values = {"stamp": "true"},
)
35 changes: 35 additions & 0 deletions misc/python/materialize/cli/bazel.py
Original file line number Diff line number Diff line change
@@ -17,6 +17,10 @@
from materialize import MZ_ROOT, ui
from materialize.bazel.utils import output_paths as bazel_output_paths

# Path where we put the current revision of the repo that we can side channel
# into Bazel.
MZ_GIT_HASH_FILE = "/tmp/mz_git_hash.txt"


def main() -> int:
parser = argparse.ArgumentParser(
@@ -27,6 +31,9 @@ def main() -> int:

(args, sub_args) = parser.parse_known_args()

# Always update our side-channel git hash incase some command needs it.
write_git_hash()

if args.action == "gen":
gen_cmd(sub_args)
elif args.action == "fmt":
@@ -149,5 +156,33 @@ def output_path(target) -> pathlib.Path:
return pathlib.Path(path)


def write_git_hash():
"""
Temporary file where we write the current git hash, so we can side channel
it into Bazel.
For production releases we stamp builds with the `workspace_status_command`
but this workflow is not friendly to remote caching. Specifically, the
"volatile status" of a workspace is not supposed to cause builds to get
invalidated, and it doesn't when the result is cached locally, but it does
when it's cached remotely.
See: <https://bazel.build/docs/user-manual#workspace-status>
<https://github.com/bazelbuild/bazel/issues/10075>
"""

repo = MZ_ROOT / ".git"
cmd_args = ["git", f"--git-dir={repo}", "rev-parse", "HEAD"]
result = subprocess.run(
cmd_args, text=True, stdout=subprocess.PIPE, stderr=subprocess.DEVNULL
)

if result.returncode == 0:
with open(MZ_GIT_HASH_FILE, "w") as f:
f.write(result.stdout.strip())
else:
ui.warn(f"Failed to get current revision of {MZ_ROOT}, falling back to all 0s")


if __name__ == "__main__":
main()
2 changes: 1 addition & 1 deletion misc/python/materialize/cli/gen-lints.py
Original file line number Diff line number Diff line change
@@ -178,7 +178,7 @@

EXCLUDE_CRATES = ["workspace-hack"]

CHECK_CFGS = "bazel, coverage, nightly_doc_features, release, tokio_unstable"
CHECK_CFGS = "bazel, stamped, coverage, nightly_doc_features, release, tokio_unstable"


def main() -> None:
2 changes: 1 addition & 1 deletion misc/wasm/Cargo.toml
Original file line number Diff line number Diff line change
@@ -16,7 +16,7 @@ store = { path = "../cargo-vet" }
# DO NOT EDIT. Automatically generated by bin/gen-lints.
[workspace.lints.rust]
unknown_lints = "allow"
unexpected_cfgs = { level = "warn", check-cfg = ['cfg(bazel, coverage, nightly_doc_features, release, tokio_unstable)'] }
unexpected_cfgs = { level = "warn", check-cfg = ['cfg(bazel, stamped, coverage, nightly_doc_features, release, tokio_unstable)'] }

[workspace.lints.rustdoc]
unportable_markdown = "allow"
20 changes: 16 additions & 4 deletions src/build-info/BUILD.bazel
Original file line number Diff line number Diff line change
@@ -22,15 +22,24 @@ rust_library(
normal = True,
proc_macro = True,
),
compile_data = ["@//misc/bazel/build-info:gen_build_info"],
compile_data = select({
"@//misc/bazel/build-info:stamped": ["@//misc/bazel/build-info:gen_build_info"],
"//conditions:default": [],
}),
crate_features = [
"default",
"semver",
],
data = [],
proc_macro_deps = [] + all_crate_deps(proc_macro = True),
rustc_env = {"BAZEL_GEN_BUILD_INFO": "$(execpath @//misc/bazel/build-info:gen_build_info)"},
rustc_flags = ["--cfg=bazel"],
rustc_env = select({
"@//misc/bazel/build-info:stamped": {"BAZEL_GEN_BUILD_INFO": "$(execpath @//misc/bazel/build-info:gen_build_info)"},
"//conditions:default": {},
}),
rustc_flags = ["--cfg=bazel"] + select({
"@//misc/bazel/build-info:stamped": ["--cfg=stamped"],
"//conditions:default": [],
}),
version = "0.0.0",
deps = [":mz_build_info_build_script"] + all_crate_deps(normal = True),
)
@@ -57,7 +66,10 @@ rust_test(
proc_macro_dev = True,
),
rustc_env = {},
rustc_flags = ["--cfg=bazel"],
rustc_flags = ["--cfg=bazel"] + select({
"@//misc/bazel/build-info:stamped": ["--cfg=stamped"],
"//conditions:default": [],
}),
version = "0.0.0",
deps = [] + all_crate_deps(
normal = True,
18 changes: 4 additions & 14 deletions src/build-info/Cargo.toml
Original file line number Diff line number Diff line change
@@ -20,17 +20,7 @@ default = ["semver", "workspace-hack"]
[package.metadata.cargo-udeps.ignore]
normal = ["workspace-hack"]

[package.metadata.cargo-gazelle.lib]
compile_data = ["@//misc/bazel/build-info:gen_build_info"]
rustc_flags = ["--cfg=bazel"]

# Skip generating doc tests because there isn't a way to set the rustc flags
# used for the test, so we can't set the `--cfg=bazel` flag.
[package.metadata.cargo-gazelle.test.doc]
skip = true

[package.metadata.cargo-gazelle.lib.rustc_env]
BAZEL_GEN_BUILD_INFO = "$(execpath @//misc/bazel/build-info:gen_build_info)"

[package.metadata.cargo-gazelle.test.lib]
rustc_flags = ["--cfg=bazel"]
# This crate requires some pretty specific configuration, so we manually
# maintain the BUILD file instead of having cargo-gazelle generate it for us.
[package.metadata.cargo-gazelle]
skip_generating = true
33 changes: 22 additions & 11 deletions src/build-info/src/lib.rs
Original file line number Diff line number Diff line change
@@ -90,14 +90,34 @@ macro_rules! build_info {
};
}

#[cfg(bazel)]
#[cfg(all(bazel, stamped))]
#[macro_export]
macro_rules! __git_sha_internal {
() => {
$crate::private::bazel_variables::GIT_COMMIT_HASH
};
}

// To improve the effectiveness of remote caching we only stamp "release" builds
// and otherwise side-channel git status through a known file.
//
// See: <https://github.com/bazelbuild/bazel/issues/10075>
#[cfg(all(bazel, not(stamped)))]
#[macro_export]
macro_rules! __git_sha_internal {
() => {
$crate::private::run_command_str!(
"sh",
"-c",
r#"if [ -f /tmp/mz_git_hash.txt ]; then
cat /tmp/mz_git_hash.txt
else
echo "0000000000000000000000000000000000000000"
fi"#
)
};
}

#[cfg(not(bazel))]
#[macro_export]
macro_rules! __git_sha_internal {
@@ -125,15 +145,6 @@ macro_rules! __git_sha_internal {
}
}

#[cfg(bazel)]
#[macro_export]
macro_rules! __build_time_internal {
() => {
$crate::private::bazel_variables::MZ_BUILD_TIME
};
}

#[cfg(not(bazel))]
#[macro_export]
macro_rules! __build_time_internal {
() => {
@@ -148,7 +159,7 @@ pub mod private {
// Bazel has a "workspace status" feature that allows us to collect info from
// the workspace (e.g. git hash) at build time. These values get incleded via
// a generated file.
#[cfg(bazel)]
#[cfg(all(bazel, stamped))]
#[allow(unused)]
pub mod bazel_variables {
include!(std::env!("BAZEL_GEN_BUILD_INFO"));

0 comments on commit d5f7831

Please sign in to comment.