Skip to content

tools: run clang-format/clang-tidy in a separate bazel output_base #30213

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.

Sign up for GitHub

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
travisdowns merged 1 commit into from
Apr 20, 2026
Merged

tools: run clang-format/clang-tidy in a separate bazel output_base #30213

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions tools/bazel-tools/.gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
.cache/
54 changes: 53 additions & 1 deletion tools/bazel-tools/support/bazel-tool-trampoline.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,9 +52,61 @@ debug "running $BAZEL_TOOL_TRAMPOLINE_TARGET"
debug "bazel invoke cwd: $(pwd)"
debug "target cwd : $BAZEL_TOOL_TRAMPOLINE_CWD"

# If BAZEL_TRAMPOLINE_PARALLEL=1 use a separate output_base so this tool can run
# in parallel with the main bazel server without contending for the same lock.
# Best for tools that need to run frequently and don't need a ton of files in
# their output base. disk_cache will in general obviate the need to actually
# rebuild anything.
#
# Determining the default output_base is tricky: `bazel info output_base` itself
Copy link
Copy Markdown
Member

@StephanDollberg StephanDollberg Apr 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need the default output_base at all? Isn't that equivalent to running with no args?

Copy link
Copy Markdown
Member Author

@travisdowns travisdowns Apr 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We build the 2nd base output based on the first one, by appending _rptool, so it goes in the same directory, gets wiped when you wipe that, on the same mount, etc.

Instead of just dumping it somehwere in /tmp (which maybe also isn't a terrible idea, but it will accumulate).

Copy link
Copy Markdown
Member

@StephanDollberg StephanDollberg Apr 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right I see, guess if it works then we might as well just go with it now.

# takes the main server's lock, so if the main server is busy the query would
# block (defeating the point). Strategy (keyed on cache existence):
# - No cache yet: no choice but to block — call `bazel info output_base` and
# seed the cache with the result.
# - Cache exists: try `bazel --noblock_for_lock info output_base` to
# opportunistically refresh the cache; on lock-held (exit 9) just use the
# cached value.
if [[ ${BAZEL_TRAMPOLINE_PARALLEL:-0} -gt 0 ]]; then
cache_dir="$support_script_dir/../.cache"
cache_file="$cache_dir/output_base"
mkdir -p "$cache_dir"

if [[ ! -f $cache_file ]]; then
debug "output_base from: blocking bazel query (no cache yet)"
default_output_base="$(bazel info output_base)"
echo "$default_output_base" >"$cache_file"
else
# Bazel exits 9 (LOCK_HELD_NOBLOCK_FOR_LOCK) when --noblock_for_lock
# cannot acquire the server lock. Any other non-zero exit is a real
# error and should surface rather than silently falling back to cache.
rc=0
fresh=$(bazel --noblock_for_lock info output_base 2>/dev/null) || rc=$?
if ((rc == 0)); then
debug "output_base from: live bazel query (cache refreshed)"
default_output_base=$fresh
echo "$default_output_base" >"$cache_file"
elif ((rc == 9)); then
default_output_base="$(cat "$cache_file")"
debug "output_base from: cache ($cache_file, lock held)"
Comment on lines +74 to +90
Copy link

Copilot AI Apr 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cache updates write directly to output_base (echo ... >"$cache_file"). If the process is interrupted mid-write, the cache can become truncated/empty and later produce an invalid --output_base like "_rptool". Consider writing to a temp file and atomically mv-ing into place, and/or validating the cached value is non-empty before using it.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Member Author

@travisdowns travisdowns Apr 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you have to be pretty unlucky to get a truncated write of a single line :)

else
echo "ERROR: bazel-tool-trampoline.sh: bazel info output_base failed (exit $rc)" >&2
exit "$rc"
Comment on lines +83 to +93
Copy link

Copilot AI Apr 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The non-blocking bazel ... info output_base call discards stderr (2>/dev/null). For failures other than lock-held (rc=9), this makes the surfaced error much harder to diagnose because only a generic message is printed. Consider capturing stderr and printing it when rc != 0 && rc != 9 (while still keeping the lock-held path quiet).

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Member Author

@travisdowns travisdowns Apr 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, if it becomes a problem we can fix it

fi
fi

tool_output_base="${default_output_base}_rptool"
output_base_flag=(--output_base="$tool_output_base")

debug "tool output_base : $tool_output_base"
Comment on lines +97 to +100
Copy link

Copilot AI Apr 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When BAZEL_TRAMPOLINE_PARALLEL is enabled, bazel run executes against a different --output_base, but the clang-tidy plugin path (LOADS) is still derived from bazel info bazel-bin using the default output base (earlier in this script). That can cause clang-tidy to fail to find plugins.so (or load a plugin built in a different output base) on a clean checkout or when the main output base hasn't built it yet. Consider computing bazel-bin using the same --output_base as the tool invocation (or restructuring so output_base_flag is decided before resolving BAZEL_BIN/LOADS).

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Member Author

@travisdowns travisdowns Apr 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch — this was the reason I originally left the whole thing opt-in. Rather than try to compute plugin paths against the _rptool output_base (which would force clang-tidy to rebuild them), I split the shim: clang-format now goes through a new tools/clang-tool-parallel.sh that sets BAZEL_TRAMPOLINE_PARALLEL=1, while clang-tidy still points at clang-tool.sh and therefore keeps using the default output_base where plugins.so already lives.

else
output_base_flag=()
debug "tool output_base : (default)"
fi

# These flags are to hide a bunch of Bazel's built-in output.

exec bazel run "$BAZEL_TOOL_TRAMPOLINE_TARGET" \
exec bazel "${output_base_flag[@]}" \
run "$BAZEL_TOOL_TRAMPOLINE_TARGET" \
--run_under=//tools/bazel-tools/support:run \
--noshow_progress \
--ui_event_filters=,+error,+fail \
Expand Down
2 changes: 1 addition & 1 deletion tools/clang-format
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
clang-tool.sh
clang-tool-parallel.sh
23 changes: 23 additions & 0 deletions tools/clang-tool-parallel.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
#!/usr/bin/env bash
# Copyright 2026 Redpanda Data, Inc.
#
# Use of this software is governed by the Business Source License
# included in the file licenses/BSL.md
#
# As of the Change Date specified in that file, in accordance with
# the Business Source License, use of this software will be governed
# by the Apache License, Version 2.0

# Variant of clang-tool.sh that runs the tool against a separate bazel
# output_base so it can execute in parallel with the main bazel server.
# Only suitable for tools that don't depend on bazel-built artifacts
# (e.g. clang-tidy plugins) living in the default output_base.

export BAZEL_TRAMPOLINE_PARALLEL="${BAZEL_TRAMPOLINE_PARALLEL:-1}"

tools_dir="$(cd -- "$(dirname -- "$(realpath "${BASH_SOURCE[0]}")")" &>/dev/null && pwd)"

# `exec -a` doesn't propagate through a shebang into bash's $0, so forward
# the invocation name via an env var that clang-tool.sh consults.
export CLANG_TOOL_NAME="$(basename "$0")"
exec "$tools_dir/clang-tool.sh" "$@"
3 changes: 2 additions & 1 deletion tools/clang-tool.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,8 @@

tools_dir="$(cd -- "$(dirname -- "$(realpath "${BASH_SOURCE[0]}")")" &>/dev/null && pwd)"

export BAZEL_TOOL_TRAMPOLINE_TARGET="@current_llvm_toolchain_llvm//:bin/$(basename "$0")"
tool_name="${CLANG_TOOL_NAME:-$(basename "$0")}"
export BAZEL_TOOL_TRAMPOLINE_TARGET="@current_llvm_toolchain_llvm//:bin/$tool_name"

if [[ ${BAZEL_TRAMPOLINE_DEBUG:-0} -gt 0 ]]; then
echo "DEBUG: clang-tool.sh: CWD : $(pwd)" >&2
Expand Down
Loading