[fx][bazel] Ensure proper Ninja build plan regeneration.
`fx bazel` first check whether the Ninja build plan needs
to be regenerated before rebuilding the workspace.
The regeneration itself was performed directly in update_workspace.py
by invoking Ninja, but didn't define environment variables in the
same way as `fx build`, resulting problems, such as broken RBE-enabled
builds when invoking `fx bazel` just after modifying a `BUILD.gn`
file.
This CL fixes the issue by doing the following:
- Moving the fast 'build plan freshness' check to its own
Python script, invoked from `fx bazel`.
- Ensure that `fx build` is called to regenerate the build plan
if needed, to ensure consistent environment variables in
all cases.
+ Ensure `fx-rbe-enabled` is not called twice by `fx build`
when `--fint-params` is not used (the function is already
called by `fx-run-ninja` defined lib `lib/vars.sh`.
Manual testing:
1) Ensure your have RBE enabled in your args.gn
2) `touch BUILD.gn`, to force a Ninja regen on the next invocation.
3) `fx bazel`, which will launch the regen operation before
invoking Ninja.
Before this CL, step 3) would print many Ninja errors such as:
```
../../build/rbe/reclient_cxx.sh: line 112: RBE_server_adress: unbound variable
```
This does not happen after the CL, because the environment variable
is correctly defined when invoking Ninja.
Bug: b/315393497
Change-Id: I584eb734c4fde4f92a97ece1924e8fb3dfafb39a
Reviewed-on: https://fuchsia-review.googlesource.com/c/fuchsia/+/957556
Commit-Queue: David Turner <digit@google.com>
Reviewed-by: Oliver Newman <olivernewman@google.com>
Reviewed-by: David Fang <fangism@google.com>
diff --git a/build/bazel/BUILD.gn b/build/bazel/BUILD.gn
index 37cb11e..40cf8d7 100644
--- a/build/bazel/BUILD.gn
+++ b/build/bazel/BUILD.gn
@@ -87,6 +87,9 @@
# The script reads this configuration file
"//build/bazel/config/main_workspace_top_dir",
+ # The script imports this python file
+ "//build/bazel/scripts/check_ninja_build_plan.py",
+
# The script reads RBE configs
rewrapper_config_file,
reproxy_config_file,
diff --git a/build/bazel/scripts/BUILD.gn b/build/bazel/scripts/BUILD.gn
index 442fc05..2cf2088 100644
--- a/build/bazel/scripts/BUILD.gn
+++ b/build/bazel/scripts/BUILD.gn
@@ -7,7 +7,10 @@
if (is_host) {
python_host_test("update_workspace_test") {
main_source = "update_workspace_test.py"
- sources = [ "update_workspace.py" ]
+ sources = [
+ "check_ninja_build_plan.py",
+ "update_workspace.py",
+ ]
libraries = [ "//third_party/parameterized" ]
}
}
diff --git a/build/bazel/scripts/check_ninja_build_plan.py b/build/bazel/scripts/check_ninja_build_plan.py
new file mode 100755
index 0000000..b379fd4
--- /dev/null
+++ b/build/bazel/scripts/check_ninja_build_plan.py
@@ -0,0 +1,76 @@
+#!/usr/bin/env python3
+# Copyright 2023 The Fuchsia Authors. All rights reserved.
+# Use of this source code is governed by a BSD-style license that can be
+# found in the LICENSE file.
+
+"""Check very quickly whether the Ninja build plan needs to be rebuilt."""
+
+import argparse
+import os
+import sys
+
+# NOTE: Do not use pathlib.Path here since benchmarking shows that this makes
+# this script _significantly_ slower (e.g. 42ms vs 16ms !) compared to os.path
+# operations.
+
+
+def ninja_plan_is_up_to_date(ninja_build_dir: str) -> bool:
+ """Return True if the Ninja build plan is up-to-date.
+
+ This expects the Ninja build plan to be generated by GN, which
+ produces a `build_ninja.d` file with the right set of dependencies.
+
+ Args:
+ ninja_build_dir: Ninja build output directory.
+
+ Returns:
+ True if the build plan is up-to-date, False if the next Ninja call
+ would invoke a regen step.
+ """
+ # This reads the build.ninja.d directly and tries to stat() all
+ # dependencies in it directly (around 7000+), which is much
+ # faster than Ninja trying to stat all build graph paths!
+ build_ninja_d = os.path.join(ninja_build_dir, "build.ninja.d")
+ if not os.path.exists(build_ninja_d):
+ return False
+
+ with open(build_ninja_d) as f:
+ build_ninja_deps = f.read().split(" ")
+
+ assert len(build_ninja_deps) > 1
+ # The first item is the top-level manifest file followed by a colon,
+ # e.g. 'build.ninja:'
+ ninja_stamp = os.path.join(ninja_build_dir, build_ninja_deps[0][:-1])
+ ninja_stamp_timestamp = os.stat(ninja_stamp).st_mtime
+
+ try:
+ for dep in build_ninja_deps[1:]:
+ dep_path = os.path.join(ninja_build_dir, dep)
+ dep_timestamp = os.stat(dep_path).st_mtime
+ if dep_timestamp > ninja_stamp_timestamp:
+ return False
+ except FileNotFoundError:
+ return False
+
+ return True
+
+
+def main():
+ parser = argparse.ArgumentParser(description=__doc__)
+ parser.add_argument("ninja_build_dir", help="Ninja build directory.")
+ parser.add_argument(
+ "--quiet", action="store_true", help="Do not print anything."
+ )
+ args = parser.parse_args()
+ if ninja_plan_is_up_to_date(args.ninja_build_dir):
+ if not args.quiet:
+ print("up-to-date!")
+ return 0
+
+ if not args.quiet:
+ print("regen required!")
+ return 1
+
+
+if __name__ == "__main__":
+ sys.exit(main())
diff --git a/build/bazel/scripts/update_workspace.py b/build/bazel/scripts/update_workspace.py
index 6f4c440..9ed474b 100755
--- a/build/bazel/scripts/update_workspace.py
+++ b/build/bazel/scripts/update_workspace.py
@@ -43,6 +43,8 @@
import sys
from typing import Sequence
+import check_ninja_build_plan
+
def get_host_platform() -> str:
"""Return host platform name, following Fuchsia conventions."""
@@ -624,21 +626,14 @@
)
)
- if maybe_regenerate_ninja(gn_output_dir, ninja_binary):
- log(
- "Re-generating Ninja build plan and incrementally rebuilding Bazel main workspace (to make sure all dependencies are up-to-date)!"
+ if not check_ninja_build_plan.ninja_plan_is_up_to_date(gn_output_dir):
+ print(
+ "Ninja build plan is not up-to-date, please run `fx build` before this script!",
+ file=sys.stderr,
)
- subprocess.run(
- [
- ninja_binary,
- "-C",
- gn_output_dir,
- "build.ninja",
- "bazel_workspace",
- ]
- )
- else:
- log2("Ninja build plan up to date.")
+ return 1
+
+ log2("Ninja build plan up to date.")
generated = GeneratedFiles()
diff --git a/tools/devshell/bazel b/tools/devshell/bazel
index c867379..bb80571 100755
--- a/tools/devshell/bazel
+++ b/tools/devshell/bazel
@@ -20,13 +20,17 @@
##
## All arguments are passed to the Bazel binary.
-source "$(cd "$(dirname "${BASH_SOURCE[0]}")" >/dev/null 2>&1 && pwd)"/lib/vars.sh || exit $?
+_script_dir="${BASH_SOURCE[0]%/*}";
+if [[ "${_script_dir}" == "${BASH_SOURCE[0]}" ]] then _script_dir="."; fi
+readonly _script_dir
+
+source "${_script_dir}/lib/vars.sh" || exit $?
fx-config-read
if [[ ! -d "${FUCHSIA_BUILD_DIR}" ]]; then
fx-error "No Fuchsia build directory, please run `fx set` or `fx gen`."
fi
-source "$(cd "$(dirname "${BASH_SOURCE[0]}")" >/dev/null 2>&1 && pwd)"/lib/bazel_utils.sh || exit $?
+source "${_script_dir}/lib/bazel_utils.sh" || exit $?
fx-bazel "$@"
diff --git a/tools/devshell/build b/tools/devshell/build
index 24a3061..5650b3d 100755
--- a/tools/devshell/build
+++ b/tools/devshell/build
@@ -153,15 +153,16 @@
# to kick off the build and wait for the failure.
goma-check
- local rbe_wrapper=()
- if fx-rbe-enabled
- then
- rbe_wrapper=("${RBE_WRAPPER[@]}")
- fi
-
if [ -n "${fint_params_path}" ]; then
readonly fint="${FX_CACHE_DIR}/fint"
"$FUCHSIA_DIR/tools/integration/bootstrap.sh" -o "$fint" || exit $?
+
+ local rbe_wrapper=()
+ if fx-rbe-enabled
+ then
+ rbe_wrapper=("${RBE_WRAPPER[@]}")
+ fi
+
# It's not ideal that we resort to constructing the textproto file as a
# string, but it's easier than writing a Go tool solely for the purpose of
# constructing a protobuf with a couple top-level string fields set.
diff --git a/tools/devshell/lib/bazel_utils.sh b/tools/devshell/lib/bazel_utils.sh
index 86f64f0..75713a4 100644
--- a/tools/devshell/lib/bazel_utils.sh
+++ b/tools/devshell/lib/bazel_utils.sh
@@ -11,7 +11,7 @@
# $1: Optional workspace name, defaults to "main".
#
local INPUT_FILE="${FUCHSIA_DIR}/build/bazel/config/${1:-main}_workspace_top_dir"
- local TOPDIR=$(cat "${INPUT_FILE}")
+ local TOPDIR=$(<"${INPUT_FILE}")
echo "${FUCHSIA_BUILD_DIR}/${TOPDIR}"
}
@@ -28,6 +28,15 @@
# Regenerate Bazel workspace and launcher script if needed.
# Note that this also regenerates the Ninja build plan if necessary.
fx-update-bazel-workspace () {
+ # First, refresh Ninja build plan if needed.
+ local check_script="${FUCHSIA_DIR}/build/bazel/scripts/check_ninja_build_plan.py"
+ if ! "${PREBUILT_PYTHON3}" -S "${check_script}" --quiet "${FUCHSIA_BUILD_DIR}"; then
+ # Calling fx build is needed to ensure RBE environment variables are
+ # properly set when generating the Ninja build plan. See b/315393497
+ echo "fx-bazel: Regenerating Ninja build plan to ensure all workspace dependencies are correct!"
+ fx-command-run build build.ninja
+ fi
+ # Second, refresh Bazel workspace files if needed.
"${FUCHSIA_DIR}"/build/bazel/scripts/update_workspace.py
}
