fix: include clang-tidy in siso clang toolchain subtree - #910
claude[bot] wants to merge 1 commit into
Conversation
siso uploads the third_party/llvm-build/Release+Asserts:headers filegroup as a precomputed subtree and skips any other input under that directory. The electron/clang-tidy rule lists third_party/llvm-build/Release+Asserts/bin/clang-tidy as an explicit input, which only reached the RBE workers because Chromium's glob included bin/clang-*. https://crrev.com/c/8471431 narrowed that glob to bin/clang-cl, bin/clang-cl.exe and bin/clang-[0-9]*, so clang-tidy is now dropped and the remote tidy steps fail with exit 127 ("clang-tidy: No such file or directory"). Add bin/clang-tidy back to the filegroup's includes. This is a no-op on older Chromium, where bin/clang-* already matches it. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01V38FjDEdZgMvFpf5Fmp65m
https://crrev.com/c/8471431 narrowed the third_party/llvm-build/Release+Asserts:headers filegroup glob from bin/clang-* to bin/clang-cl, bin/clang-cl.exe and bin/clang-[0-9]*. siso uploads that filegroup as a precomputed subtree and drops other inputs under it, so bin/clang-tidy no longer reached the RBE workers and the clang-tidy steps failed with exit 127. build-tools now adds bin/clang-tidy back to that filegroup: electron/build-tools#910 Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01V38FjDEdZgMvFpf5Fmp65m
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because tools/main.star falls under the repo's infra CODEOWNERS (@ electron/wg-infra @ ckerr) and changes the siso build-graph toolchain configuration, a human look would still be worthwhile.
What was reviewed: the new llvm_headers block in init() that appends bin/clang-tidy to the third_party/llvm-build/Release+Asserts:headers filegroup's includes; confirmed it copies the filegroup dict before mutating rather than mutating the shared mod.filegroups entry in place; confirmed the if llvm_headers in filegroups guard makes the change a no-op if the filegroup is absent (e.g. on Chromium versions before the upstream glob narrowing).
Extended reasoning...
The change is a small, self-contained Starlark edit to tools/main.star that appends "bin/clang-tidy" to an existing filegroup's includes list, guarded by an existence check and using a copied dict to avoid mutating the shared original, with no auth/crypto/data-exposure surface. It matches the PR description (working around Chromium's crrev.com/c/8471431 glob narrowing) and no bugs were found by the hunt. Because the file is under the repo's blanket infra CODEOWNERS (wg-infra/ckerr) and affects the remote build toolchain's input set, a human with infra context should still confirm it before merge; a previously investigated concern (future Chromium DEPS rolls reshaping this filegroup and crashing init()) was ruled out as an acceptable narrow risk rather than a blocking bug.
Requested by Charles Kerr · Slack thread
siso uploads the
third_party/llvm-build/Release+Asserts:headersfilegroup as a precomputed subtree and skips other inputs under that directory, so the explicitbin/clang-tidyinput of theelectron/clang-tidyrule only reached RBE workers because Chromium's glob includedbin/clang-*. https://crrev.com/c/8471431 narrowed that glob tobin/clang-cl,bin/clang-cl.exeandbin/clang-[0-9]*, so the remote tidy steps now fail with exit 127 (clang-tidy: No such file or directory). This addsbin/clang-tidyback to that filegroup's includes (a no-op on older Chromium wherebin/clang-*already matches it), which unbreaks the clang-tidy jobs on electron/electron#54590. No.exevariant is needed: Windows hosts run the Linuxclang-tidystaged underRelease+Asserts_linux, which isn't in this filegroup.🤖 Generated with Claude Code
https://claude.ai/code/session_01V38FjDEdZgMvFpf5Fmp65m
Generated by Claude Code