-
Notifications
You must be signed in to change notification settings - Fork 55
feat(bazel): enforce that BUILD files match their sources #491
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
Merged
Merged
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,14 @@ | ||
| load("@rules_go//go:def.bzl", "go_binary", "go_library") | ||
|
|
||
| go_library( | ||
| name = "admin-mock_lib", | ||
| srcs = ["main.go"], | ||
| importpath = "nvcf-cli/scripts/admin-mock", | ||
| visibility = ["//visibility:private"], | ||
| ) | ||
|
|
||
| go_binary( | ||
| name = "admin-mock", | ||
| embed = [":admin-mock_lib"], | ||
| visibility = ["//visibility:public"], | ||
| ) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,86 @@ | ||
| #!/usr/bin/env bash | ||
| # SPDX-FileCopyrightText: Copyright (c) NVIDIA CORPORATION & AFFILIATES. All rights reserved. | ||
| # SPDX-License-Identifier: Apache-2.0 | ||
| # | ||
| # Fail if Gazelle, run from the repository root, would change any BUILD file. | ||
| # | ||
| # Why this exists: nothing currently notices when a BUILD file falls behind its | ||
| # sources. nvsnap's checkpoint_v2.go and restore_v2.go landed 2026-07-14 while | ||
| # internal/agent/BUILD.bazel was last regenerated 2026-07-02, so the build was | ||
| # broken for twelve days and no job reported it. That was not unique: at the | ||
| # time this check was written, more than a dozen packages had a .go file newer | ||
| # than their BUILD.bazel. | ||
| # | ||
| # Root-run matters. Gazelle resolves dependencies against the module it runs | ||
| # in, so running it per-subtree produces different, and during consolidation | ||
| # increasingly wrong, results. The root run is the one whose output has to stay | ||
| # stable as services migrate into the root module. | ||
| # | ||
| # --mode=diff makes Gazelle report rather than rewrite, so CI never leaves a | ||
| # dirty tree and the failure output is the patch a developer needs to apply. | ||
| set -euo pipefail | ||
|
|
||
| root="$(cd "$(dirname "${BASH_SOURCE[0]}")/../.." && pwd)" | ||
| cd "${root}" | ||
|
|
||
| : "${BAZEL:=bazel}" | ||
|
|
||
| diff_out="$(mktemp)" | ||
| trap 'rm -f "${diff_out}"' EXIT | ||
|
|
||
| # Gazelle exits non-zero in diff mode when there is a diff, which is the signal | ||
| # we want rather than an error, so capture the status instead of letting set -e | ||
| # take it. | ||
| # Scope: Go packages only for now. Gazelle also wants to reorder attributes in | ||
| # the src/libraries/java BUILD files, which is cosmetic and would churn files | ||
| # the Java modules own for no functional gain. Those modules move into the root | ||
| # in Phase 6 of docs/dev/bazel-consolidation.md; widen this then, in the change | ||
| # that moves them, rather than reordering another team's files ahead of it. | ||
| GO_ROOTS=( | ||
| src/clis | ||
| src/libraries/go | ||
| src/compute-plane-services | ||
| src/control-plane-services | ||
| src/invocation-plane-services | ||
| ) | ||
|
|
||
| status=0 | ||
| "${BAZEL}" run //:gazelle -- --mode=diff "${GO_ROOTS[@]}" > "${diff_out}" 2>&1 || status=$? | ||
|
|
||
| # A real failure (missing target, fetch error, Starlark error) is not the same | ||
| # as "there is a diff". Distinguish them, otherwise a broken build reads as | ||
| # tidy BUILD files. | ||
| if grep -qE '^(ERROR|FATAL):' "${diff_out}"; then | ||
| echo "error: gazelle did not run to completion" >&2 | ||
| cat "${diff_out}" >&2 | ||
| exit 2 | ||
| fi | ||
|
|
||
| # Gazelle prints unified diffs for files it would change, and exits non-zero | ||
| # when it produces any. So a non-zero status with diff markers is the normal | ||
| # "out of date" signal, while a non-zero status without them is a failure that | ||
| # happened to print neither ERROR nor FATAL. Treating the latter as success | ||
| # would report tidy BUILD files for a run that never checked them, which is the | ||
| # exact false green this whole check exists to prevent. | ||
| if ! grep -qE '^(\+\+\+|---) ' "${diff_out}"; then | ||
| if [ "${status}" -ne 0 ]; then | ||
| echo "error: gazelle exited ${status} without producing a diff" >&2 | ||
| cat "${diff_out}" >&2 | ||
| exit 2 | ||
| fi | ||
| echo "gazelle: BUILD files are up to date" | ||
| exit 0 | ||
| fi | ||
|
|
||
| { | ||
| echo "error: BUILD files are out of date with their sources." | ||
| echo | ||
| echo "Regenerate from the repository root and commit the result:" | ||
| echo | ||
| echo " bazel run //:gazelle" | ||
| echo | ||
| echo "Gazelle would apply the following:" | ||
| echo | ||
| cat "${diff_out}" | ||
| } >&2 | ||
| exit 1 | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,135 @@ | ||
| #!/usr/bin/env bash | ||
| # SPDX-FileCopyrightText: Copyright (c) NVIDIA CORPORATION & AFFILIATES. All rights reserved. | ||
| # SPDX-License-Identifier: Apache-2.0 | ||
| # | ||
| # Behavioral test for tools/ci/check-gazelle. | ||
| # | ||
| # The check has to keep three outcomes apart: BUILD files are current, BUILD | ||
| # files are stale, and Gazelle did not run. Conflating the last two is the | ||
| # dangerous one, because a fetch or Starlark failure reported as "up to date" | ||
| # means CI claims BUILD files were verified when nothing verified them. | ||
| # | ||
| # BAZEL is stubbed so none of this touches a real Bazel or the network. | ||
| set -euo pipefail | ||
|
|
||
| script="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)/check-gazelle" | ||
| fail=0 | ||
|
|
||
| stub_dir="$(mktemp -d)" | ||
| trap 'rm -rf "${stub_dir}"' EXIT | ||
|
|
||
| # Build a fake bazel that prints a fixed payload and exits with a fixed status. | ||
| invocation_log="${stub_dir}/invocation" | ||
|
|
||
| make_bazel() { # make_bazel <exit-status> <stdout-payload> | ||
| local status="$1" payload="$2" path="${stub_dir}/bazel" | ||
| { | ||
| echo '#!/usr/bin/env bash' | ||
| # Record the working directory and full argv so the tests can assert | ||
| # how gazelle was invoked, not only how its output was classified. | ||
| printf 'printf "cwd=%%s\\n" "$PWD" > %s\n' "${invocation_log}" | ||
| printf 'printf "%%s\\n" "$@" >> %s\n' "${invocation_log}" | ||
| printf 'cat <<%s\n%s\n%s\n' "PAYLOAD_EOF" "${payload}" "PAYLOAD_EOF" | ||
| echo "exit ${status}" | ||
| } > "${path}" | ||
| chmod +x "${path}" | ||
| printf '%s' "${path}" | ||
|
balajinvda marked this conversation as resolved.
|
||
| } | ||
|
|
||
| run() { # run <exit-status> <payload> | ||
| local b; b="$(make_bazel "$1" "$2")" | ||
| set +e | ||
| out="$(BAZEL="${b}" bash "${script}" 2>&1)" | ||
| rc=$? | ||
| set -e | ||
| } | ||
|
|
||
| expect() { # expect <desc> <want-rc> [substring] | ||
| local desc="$1" want="$2" substr="${3:-}" | ||
| if [ "${rc}" != "${want}" ]; then | ||
| printf 'FAIL %s: want exit %s, got %s\n%s\n' "${desc}" "${want}" "${rc}" "${out}" | ||
| fail=1; return | ||
| fi | ||
| # -F so the needle is literal: "new_test.go" as a regex would also accept | ||
| # "newXtest.go". Herestring rather than a pipeline so grep exiting early | ||
| # cannot SIGPIPE the producer and trip pipefail on large output. | ||
| if [ -n "${substr}" ] && ! grep -Fq -- "${substr}" <<<"${out}"; then | ||
| printf 'FAIL %s: output missing %q\n%s\n' "${desc}" "${substr}" "${out}" | ||
| fail=1; return | ||
| fi | ||
|
balajinvda marked this conversation as resolved.
|
||
| printf 'ok %s\n' "${desc}" | ||
| } | ||
|
|
||
| # Clean: Gazelle succeeds and prints no diff. | ||
| run 0 'no changes' | ||
| expect "clean tree exits 0" 0 "up to date" | ||
|
|
||
| # Stale: Gazelle prints a unified diff. It exits non-zero when it does, which is | ||
| # the expected signal, not a failure. | ||
| run 1 '--- a/src/foo/BUILD.bazel | ||
| +++ b/src/foo/BUILD.bazel | ||
| @@ -1 +1,2 @@ | ||
| + srcs = ["new_test.go"],' | ||
| expect "diff exits 1" 1 "out of date" | ||
| expect " ... and shows the patch" 1 "new_test.go" | ||
|
|
||
| # A diff reported with a zero exit is still a diff. | ||
| run 0 '--- a/src/foo/BUILD.bazel | ||
| +++ b/src/foo/BUILD.bazel' | ||
| expect "diff with zero status still exits 1" 1 "out of date" | ||
|
|
||
| # Invocation failure: Gazelle could not run. This must never read as clean. | ||
| run 1 'ERROR: no such package @@gazelle//: fetch failed' | ||
| expect "ERROR output exits 2" 2 "did not run to completion" | ||
|
|
||
| run 1 'FATAL: Starlark evaluation failed' | ||
| expect "FATAL output exits 2" 2 "did not run to completion" | ||
|
|
||
| # The case the status check exists for: a non-zero exit that prints neither a | ||
| # recognised error prefix nor a diff. Without checking the status this fell | ||
| # through to the success path and reported tidy BUILD files. | ||
| run 34 'Server terminated abruptly' | ||
| expect "silent non-zero exit fails closed" 2 "without producing a diff" | ||
|
|
||
| # And its mirror: a zero exit with no diff is genuinely clean, not a failure. | ||
| run 0 '' | ||
| expect "empty output with zero status is clean" 0 "up to date" | ||
|
|
||
| # How gazelle is invoked is part of the contract. Without these, the suite | ||
| # still passes if check-gazelle drops --mode=diff, renames the target, runs | ||
| # from the wrong directory, or silently widens its scope. | ||
| run 0 'no changes' | ||
| inv="$(cat "${invocation_log}")" | ||
|
|
||
| expect_inv() { # expect_inv <desc> <needle> | ||
| if grep -Fq -- "$2" <<<"${inv}"; then | ||
| printf 'ok %s\n' "$1" | ||
| else | ||
| printf 'FAIL %s: invocation missing %q\n%s\n' "$1" "$2" "${inv}" | ||
| fail=1 | ||
| fi | ||
| } | ||
|
|
||
| expect_inv "runs the root gazelle target" "//:gazelle" | ||
| expect_inv "asks for a diff, does not rewrite" "--mode=diff" | ||
| expect_inv "runs from the repository root" "cwd=$(cd "$(dirname "${script}")/../.." && pwd)" | ||
| for root in src/clis src/libraries/go src/compute-plane-services \ | ||
| src/control-plane-services src/invocation-plane-services; do | ||
| expect_inv "scopes ${root}" "${root}" | ||
| done | ||
|
|
||
| # Scope is Go-only on purpose: Gazelle would reorder attributes in the Java | ||
| # BUILD files, which is cosmetic churn in files the Java modules own. Assert | ||
| # the exclusion so widening it is a deliberate edit, not an accident. | ||
| if grep -Fq -- "src/libraries/java" <<<"${inv}"; then | ||
| printf 'FAIL java is in scope; it is deliberately excluded\n%s\n' "${inv}" | ||
| fail=1 | ||
| else | ||
| printf 'ok leaves src/libraries/java out of scope\n' | ||
| fi | ||
|
|
||
| if [ "${fail}" -ne 0 ]; then | ||
| echo "check-gazelle: FAILED" >&2 | ||
| exit 1 | ||
| fi | ||
| echo "check-gazelle: all checks passed" | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.