tests: check: use find_program for iwyu_tool#500
Draft
jordalgo wants to merge 1 commit intofacebook:mainfrom
Draft
tests: check: use find_program for iwyu_tool#500jordalgo wants to merge 1 commit intofacebook:mainfrom
jordalgo wants to merge 1 commit intofacebook:mainfrom
Conversation
Claude review of PR #500 (f6efb07)Clean, minimal PR that correctly replaces a hardcoded Suggestions
Nits
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
jordalgo
changed the title
qdeslandes
requested changes
Apr 14, 2026
Contributor
qdeslandes
left a comment
qdeslandes
left a comment
There was a problem hiding this comment.
Quick nit, otherwise LGTM!
Comment on lines
+35
to
+49
| if(IWYU_TOOL_BIN) | ||
| add_test(NAME "check.iwyu" | ||
| COMMAND | ||
| ${IWYU_TOOL_BIN} | ||
| -p ${CMAKE_BINARY_DIR} | ||
| ${CMAKE_SOURCE_DIR}/src/libbpfilter/cgen/program.c | ||
| ) | ||
|
|
||
| set_tests_properties("check.iwyu" PROPERTIES | ||
| LABELS "check" | ||
| ) | ||
| else() | ||
| message(WARNING "iwyu_tool not found, check.iwyu test will be skipped") | ||
| endif() | ||
|
|
Contributor
There was a problem hiding this comment.
I would remove the if(): if iwyu is not found, it should error out. Otherwise, we could miss the warning and proceed without it.
Author
There was a problem hiding this comment.
I had it without the if in the first iteration but I think it was causing a build failure in one or two of the CI jobs
jordalgo
force-pushed
the
fix_iwyu
branch
2 times, most recently
from
a5083a9 to
098817d
Compare
jordalgo
force-pushed
the
fix_iwyu
branch
2 times, most recently
from
ef86c98 to
991d799
Compare
This fixes a few issues: - instead of running iwyu on one file, run it on the whole build if the WITH_IWYU flag is added - use Cmake's native iwyu instead of relying on the path
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Use
find_programinstead of the absolute path for iwyu. This fixes an issue where this binary can't be found in a nix shell.Tested locally and now the iwyu tests pass and the binary can be located.