When including ebpf-verifier as a submodule, default to tests off#751
Conversation
Signed-off-by: Alan Jowett <alan.jowett@microsoft.com>
WalkthroughThe changes in this pull request modify the Changes
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range comments (2)
CMakeLists.txt (2)
Line range hint
95-96: LGTM: Consistent application of test enabling condition.The
VERIFIER_ENABLE_TESTScondition is correctly applied to all test-related build components, ensuring they are only built when tests are enabled. This is a thorough implementation of the PR objective.For consistency, consider wrapping the
add_subdirectory("external/bpf_conformance/src")call on line 181 in anif(VERIFIER_ENABLE_TESTS)block as well, if this directory is specifically for test-related code.Also applies to: 104-108, 150-153, 159-173, 180-181, 191-196
Test-Related Targets Not Conditioned on VERIFIER_ENABLE_TESTS
Several test-related targets and configurations are defined outside the
VERIFIER_ENABLE_TESTScondition. This may lead to tests being built even when the project is used as a submodule.
- Lines 104-109: Definition of test source files.
- Lines 129-132: Addition of test executables (
check,tests,conformance_check).- Lines 138-154: Setting target properties and dependencies for test executables.
- Lines 191-199: Configuration of compile options and link libraries for the
teststarget.Please ensure these sections are properly conditioned on
VERIFIER_ENABLE_TESTSto prevent unintended test builds when the project is integrated as a submodule.🔗 Analysis chain
Line range hint
1-196: Summary: PR objective successfully implemented.The changes effectively implement the goal of defaulting to tests off when the ebpf-verifier is used as a submodule, while maintaining the existing functionality when used as a standalone project. The implementation is consistent and well-structured.
To ensure completeness:
Please run the following command to check if there are any remaining references to test-related targets or variables that are not conditioned on
VERIFIER_ENABLE_TESTS:If this command returns any results, please review them to determine if they also need to be conditioned on
VERIFIER_ENABLE_TESTS.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for test-related keywords that are not within VERIFIER_ENABLE_TESTS blocks rg -n '(test|check|conformance)' CMakeLists.txt | rg -v 'VERIFIER_ENABLE_TESTS'Length of output: 1851
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (1)
- CMakeLists.txt (1 hunks)
🧰 Additional context used
🔇 Additional comments (2)
CMakeLists.txt (2)
8-15: LGTM: Conditional test enabling based on project structure.This change effectively implements the PR objective. It enables tests and installs Git hooks only when the
.gitdirectory is present (indicating it's not a submodule), and disables tests otherwise. This approach simplifies usage as a submodule by reducing dependencies.
17-17: Good addition: Informative build message.This message clearly communicates whether tests are being built, which is helpful for users and aligns well with the changes made to the test enabling logic.
When using ebpf-verifier as a submodule, default to not building the tests collateral. This makes using this as a submodule simpler as it reduced the list of dependencies that need to be pre-installed.
Summary by CodeRabbit
.gitdirectory.