Fix: strip target triples from replayed compile commands - #1652
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe PR updates compile database normalization for target-triple-prefixed compilers and GCC-only flags. It also updates CPU runner provisioning and documentation to use clang-tidy 18 with builtin headers. ChangesClang-tidy CPU alignment
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/workflows/ci-self-cpu.yml (1)
191-191: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDo not assume that the version appears on output line 2.
Line 191 assumes a fixed
clang-tidy --versionlayout. If the installed package uses another layout, the log can omit the selected version or show unrelated metadata. Clang-tidy documents--versionfor version display, but the workflow should not depend on a fixed line number. (clang.llvm.org)Print the complete output, or parse and validate the expected major version.
Proposed fix
- PATH="$RUNNER_TEMP/bin:$PATH" clang-tidy --version | sed -n 2p + PATH="$RUNNER_TEMP/bin:$PATH" command -v clang-tidy + PATH="$RUNNER_TEMP/bin:$PATH" clang-tidy --version🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci-self-cpu.yml at line 191, Update the clang-tidy version logging command in the workflow so it does not pipe output through sed -n 2p or depend on a fixed output line; print the complete output from clang-tidy --version, while preserving the PATH override that selects the installed executable.Source: MCP tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/lint/clang_tidy.py`:
- Around line 238-245: Update _parse_compile_database to validate every entry is
an object containing a valid "file" field before returning the entries, raising
ValueError for malformed entries. Keep _rewrite_entry and the later
entry["file"] access unchanged so _load_compile_database can handle invalid
databases through its existing recovery or skip behavior.
---
Nitpick comments:
In @.github/workflows/ci-self-cpu.yml:
- Line 191: Update the clang-tidy version logging command in the workflow so it
does not pipe output through sed -n 2p or depend on a fixed output line; print
the complete output from clang-tidy --version, while preserving the PATH
override that selects the installed executable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 211b281c-5724-4f19-b178-e50a7e4922e7
📒 Files selected for processing (3)
.github/workflows/ci-self-cpu.ymldocs/ci.mdtests/lint/clang_tidy.py
|
/run-cpu |
|
✅ /run-cpu lane passed — https://github.com/hw-native-sys/simpler/actions/runs/30799621412
|
c18abc0 to
f6f8b58
Compare
A compiler whose program name carries a target-triple prefix — conda and crosstool GCC ship `<triple>-g++` with `g++` as a symlink to it, and CMake records the resolved name — makes clang-tidy adopt that triple as its target. It then matches no installed GCC, so it receives no C++ standard library include dirs and fails on the first `#include <cstdint>`. Sema drops the statements that fail to build, which additionally reports constructors with a real body as modernize-use-equals-default: one toolchain mismatch surfaces as lint errors that name real files and real checks. Every target in a sim compile database is host-compiled, so the prefix names nothing clang-tidy needs. Drop it while rewriting each command, next to the GCC-only flags already stripped there, and write the database back whenever a rewrite changed it rather than only on a flag match. A database whose entries are not objects naming a file is now rejected at parse time, so it takes the existing reconfigure-or-skip recovery instead of raising out of the rewrite. The self-hosted cpu lane is where this fires: its agents resolve `g++` to a conda GCC 15 prefix, and its clang-tidy is the distro LLVM 12, which unlike 18 does not fall back to a compatible triple. Verified on an agent against the changed files of the PR that first exercised the hook there — LLVM 12 fails before the rewrite and passes after it; LLVM 18 passes either way. The lane's pre-commit job now also prefers a `clang-tidy-18` when an agent provides one, so it lints with the major ci.yml uses.
|
@coderabbitai Addressed the review-body nitpick as well:
The inline finding on |
|
Tip For best results, initiate chat on the files or code changes.
If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! You are interacting with an AI system. |
1 participant
Summary
tests/lint/clang_tidy.pyreplays each changed file's compile command throughclang-tidy. clang-tidy does not run the compiler — it drives its own clang
frontend, and clang's driver reads the program name in argv[0] as a target
triple when that name carries one (
ToolChain::getTargetAndModeFromProgramName,the mechanism behind
aarch64-linux-gnu-gcc).conda-forge and crosstool GCC install
<triple>-g++and makeg++a symlink toit; CMake records the resolved name. So a database entry reads
.../bin/aarch64-conda-linux-gnu-g++ …, clang-tidy takesaarch64-conda-linux-gnuas its target, matches no installed GCC (the box has/usr/lib/gcc/aarch64-linux-gnu/10.3.1— different vendor segment), and is leftwith no C++ standard library include dirs at all. Every
#include <cerrno>/<cstdint>fails, and because Sema drops statements that fail to build, aconstructor with a real body is additionally reported as
modernize-use-equals-default. One toolchain mismatch, presented as lint errorsnaming real files and real checks.
Every target in a sim compile database is host-compiled, so the triple prefix
names nothing clang-tidy needs. This drops it while rewriting each command —
next to the GCC-only flags already stripped there — and writes the database back
whenever a rewrite changed something, rather than only when a
-fno-gnu-uniquewas present. Directory and all other arguments are untouched.
ci-self-cpu.yml'spre-commitjob additionally prefers aclang-tidy-18whenan agent provides one, so the emergency lane lints with the same clang-tidy
major as
ci.yml(ubuntu ships 18; HCE 2.0 packages only LLVM 12).Where it fires
The self-hosted
cpulane. Its agents resolveg++to a conda GCC 15 prefix,and their
clang-tidyis the distro LLVM 12 — which, unlike 18, does not fallback to a compatible triple. It stayed latent because every earlier lane run
reported
clang-tidy … (no files to check) Skipped; #1587 was the firstC++-touching PR through the lane, and its lane
pre-commitfailed on thosephantom diagnostics while the GitHub-hosted
pre-commitfor the same commitpassed.
Testing
Run on a
cpuagent, against the changed-file set of #1587, restoring thetriple-prefixed database before each case:
'cstdint' file not found+ phantommodernize-use-equals-defaultruff check/ruff format/pyrightclean on the hook(
/usr/bin/g++,g++-15,/opt/llvm/bin/clang++)