Skip to content

fix: include clang-tidy in siso clang toolchain subtree - #910

Open
claude[bot] wants to merge 1 commit into
mainfrom
claude/siso-clang-tidy-subtree
Open

claude[bot] wants to merge 1 commit into
mainfrom
claude/siso-clang-tidy-subtree

Conversation

@claude

@claude claude Bot commented Sep 30, 2026

Copy link
Copy Markdown
Contributor

Requested by Charles Kerr · Slack thread

siso uploads the third_party/llvm-build/Release+Asserts:headers filegroup as a precomputed subtree and skips other inputs under that directory, so the explicit bin/clang-tidy input of the electron/clang-tidy rule only reached 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 the remote tidy steps now fail with exit 127 (clang-tidy: No such file or directory). This adds bin/clang-tidy back to that filegroup's includes (a no-op on older Chromium where bin/clang-* already matches it), which unbreaks the clang-tidy jobs on electron/electron#54590. No .exe variant is needed: Windows hosts run the Linux clang-tidy staged under Release+Asserts_linux, which isn't in this filegroup.

🤖 Generated with Claude Code

https://claude.ai/code/session_01V38FjDEdZgMvFpf5Fmp65m


Generated by Claude Code

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
@claude
claude Bot requested review from a team and ckerr as code owners September 30, 2026 18:19
claude Bot pushed a commit to electron/electron that referenced this pull request Sep 30, 2026
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

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants