Move all yaml related code into test#750
Conversation
WalkthroughThe changes in this pull request focus on enhancing the CI/CD workflow for a C++ project by modifying 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 (
|
Signed-off-by: Alan Jowett <alan.jowett@microsoft.com>
ea0187a to
2b9fa51
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range comments (1)
.github/workflows/build.yml (1)
Workflow Not Triggering Expected Ubuntu Builds
It appears that the "build_ubuntu (tests)" and "build_ubuntu (library)" jobs are not being triggered as expected. Please review the workflow configuration to ensure that these targets are correctly set to run on pull requests.
- Verify the workflow triggers to confirm they include the necessary pull request events.
- Check for any conditional statements or syntax errors that might prevent the "tests" and "library" targets from executing.
🔗 Analysis chain
Line range hint
1-101: Consider similar optimizations for Windows build and verify changesThe changes made to the Ubuntu build process are excellent. To further improve the workflow, consider applying similar optimizations to the Windows build process. This could involve separating the installation of test dependencies for Windows as well, if applicable.
Additionally, it's important to verify that these changes don't have any unintended consequences on the overall build and test process.
To ensure the changes work as intended and don't affect other parts of the workflow, please run the following verification:
This script will help verify that the workflow is valid and that the test dependencies are installed only when necessary.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the impact of the changes on the workflow # Test 1: Check if the workflow file is valid gh workflow view "CPP CI" --yaml # Test 2: Check if the test dependencies are correctly installed only for the 'tests' target gh run list --workflow="CPP CI" --limit 10 | grep -q "build_ubuntu (tests" if [ $? -eq 0 ]; then echo "Recent 'tests' build found. Checking build logs..." run_id=$(gh run list --workflow="CPP CI" --limit 10 | grep "build_ubuntu (tests" | awk '{print $7}' | head -n 1) gh run view $run_id --log | grep -q "Install test dependencies" if [ $? -eq 0 ]; then echo "Test dependencies installation step found in 'tests' build." else echo "Warning: Test dependencies installation step not found in 'tests' build." fi else echo "No recent 'tests' build found. Unable to verify." fi # Test 3: Check if the library build excludes test dependencies gh run list --workflow="CPP CI" --limit 10 | grep -q "build_ubuntu (library" if [ $? -eq 0 ]; then echo "Recent 'library' build found. Checking build logs..." run_id=$(gh run list --workflow="CPP CI" --limit 10 | grep "build_ubuntu (library" | awk '{print $7}' | head -n 1) gh run view $run_id --log | grep -q "Install test dependencies" if [ $? -eq 0 ]; then echo "Warning: Test dependencies installation step found in 'library' build." else echo "Library build correctly excludes test dependencies installation." fi else echo "No recent 'library' build found. Unable to verify." fiLength of output: 3318
Script:
#!/bin/bash # Description: List all runs of the "CPP CI" workflow to find any builds beyond the last 10 gh run list --workflow="CPP CI" --limit 100Length of output: 13867
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (2)
- .github/workflows/build.yml (1 hunks)
- CMakeLists.txt (3 hunks)
🧰 Additional context used
🔇 Additional comments (5)
.github/workflows/build.yml (1)
32-37: Excellent separation of test dependencies!The changes made to separate the installation of general dependencies (lines 32-33) from test-specific dependencies (lines 34-37) are a great improvement. This separation, along with the conditional execution for test dependencies, enhances the clarity and efficiency of the workflow. It ensures that test-related packages are only installed when necessary, which is particularly beneficial for builds that don't require testing.
This modification aligns well with the PR objectives and represents a best practice in CI/CD workflow design.
CMakeLists.txt (4)
35-37: LGTM: Conditional inclusion of yaml-cppThis change correctly wraps the yaml-cpp package inclusion within the VERIFIER_ENABLE_TESTS condition. It aligns with the PR objective of improving dependency management by ensuring yaml-cpp is only included when tests are enabled.
Line range hint
131-135: LGTM: Conditional compilation of test executables and their configurationsThe changes to conditionally compile test executables (check, tests, run_yaml, conformance_check) and set their properties and link libraries only when VERIFIER_ENABLE_TESTS is enabled are well-implemented. This aligns perfectly with the PR objective of improving the modularity of the build process and separating test-specific configurations.
Also applies to: 141-155, 185-204
Line range hint
174-175: LGTM: Conditional inclusion of bpf_conformanceThe conditional inclusion of the bpf_conformance subdirectory when VERIFIER_ENABLE_TESTS is enabled is correct. This change ensures that the bpf_conformance library is only built when necessary for tests, which aligns with the PR's goal of optimizing the build configuration.
Line range hint
1-204: Overall assessment: Well-implemented changes with a minor issueThe changes in this CMakeLists.txt file successfully implement the PR objectives by conditionally including test-related configurations based on the VERIFIER_ENABLE_TESTS option. This improves the modularity and efficiency of the build process. The implementation is consistent throughout the file, covering package finding, external project configuration, executable compilation, and subdirectory inclusion.
There's only one minor issue to address:
- The duplication of "src/test/ebpf_yaml.hpp" in the ALL_TEST glob (lines 104-105).
Once this small correction is made, the changes will be fully aligned with the PR goals and ready for merging.
Signed-off-by: Alan Jowett <alan.jowett@microsoft.com>
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (1)
- CMakeLists.txt (4 hunks)
🧰 Additional context used
🔇 Additional comments (5)
CMakeLists.txt (5)
35-37: LGTM: Conditional yaml-cpp package inclusionThe conditional inclusion of yaml-cpp package aligns well with the PR objective. This change ensures that yaml-cpp is only required when tests are enabled, which is a good practice for managing dependencies.
111-111: LGTM: Conditional inclusion of yaml-related source fileThe conditional inclusion of "src/test/ebpf_yaml.cpp" in LIB_SRC when tests are enabled is a good practice. It aligns with the PR objective of moving yaml-related code into test and ensures that test-specific code is only included when necessary.
139-139: LGTM: Adding test-specific include directoryThe addition of the test-specific include directory for the run_yaml target is appropriate. It ensures that the target can access the necessary test-specific headers while maintaining proper encapsulation of test code.
Line range hint
1-224: LGTM: Improved overall structure and consistencyThe changes made throughout the CMakeLists.txt file demonstrate a consistent use of the VERIFIER_ENABLE_TESTS condition. This approach effectively separates test-specific configurations and aligns well with the PR objective of moving yaml-related code into test. The overall structure of the file has improved in terms of modularity and clarity, making it easier to maintain and understand the build configuration.
Line range hint
1-224: Summary: Successful refactoring of yaml-related codeThis PR successfully achieves its objective of moving yaml-related code into test-specific sections of the CMakeLists.txt file. The changes are consistent, well-structured, and improve the overall modularity of the build configuration. The use of the VERIFIER_ENABLE_TESTS condition throughout the file effectively separates test-specific configurations.
A minor suggestion was made regarding the YAML_CPP_LIBRARIES variable to improve robustness across different build configurations. Overall, these changes enhance the maintainability and clarity of the build system.
|
Thanks @elazarg for merging this. |
This pull request includes changes to streamline the installation of dependencies and conditionally include test dependencies. Additionally, it modifies the
CMakeLists.txtto conditionally find theyaml-cpppackage based on whether tests are enabled.Dependency Management:
.github/workflows/build.yml: Split the installation of test dependencies (libyaml-cpp-devandvalgrind) into a separate step that only runs if the target is 'tests'.CMake Configuration:
CMakeLists.txt: Added conditional statements to find theyaml-cpppackage only ifVERIFIER_ENABLE_TESTSis enabled. [1] [2] [3]Summary by CodeRabbit
New Features
VERIFIER_ENABLE_TESTSoption.Bug Fixes