build: Use target_include_directories instead of FILE_SET - #48
Conversation
WalkthroughThis pull request introduces two primary modifications: an update to the GitHub Actions workflow for unit tests and changes to the CMake configuration in the Spider project. The workflow now supports testing across multiple Ubuntu versions (22.04 and 24.04) using a matrix strategy. In the CMake configuration, the header file inclusion method for Changes
Possibly related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms (3)
🔇 Additional comments (1)
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/unit-tests.yaml(1 hunks)src/spider/CMakeLists.txt(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: non-storage-unit-tests (ubuntu-24.04)
- GitHub Check: lint
- GitHub Check: non-storage-unit-tests (ubuntu-22.04)
🔇 Additional comments (2)
.github/workflows/unit-tests.yaml (1)
21-24: Consider Ubuntu Noble's pre-release statusThe matrix strategy is a good approach for testing across Ubuntu versions. However, Ubuntu 24.04 (Noble) is currently in development and not officially released. This might lead to potential instability in the CI pipeline.
Consider one of these approaches:
- Add a
continue-on-errorflag for Noble builds until its official release- Wait for Noble's official release before adding it to the matrix
src/spider/CMakeLists.txt (1)
35-35: Verify minimum CMake version requirementThe changes remove
FILE_SETfor better compatibility with CMake 3.22.1 (Ubuntu Jammy). Let's verify the minimum CMake version requirement in the repository.Also applies to: 153-154
✅ Verification successful
CMake version requirement verified
The changes align with the repository's minimum CMake version requirement of 3.22.1, as specified in both root and example CMakeLists.txt files.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Find and check minimum CMake version requirement find . -name CMakeLists.txt -exec grep -l "cmake_minimum_required" {} \; -exec grep "cmake_minimum_required" {} \;Length of output: 251
| BASE_DIRS ${CMAKE_CURRENT_SOURCE_DIR}/.. | ||
| FILES ${SPIDER_CORE_HEADERS} | ||
| ) | ||
| target_sources(spider_core PUBLIC ${SPIDER_CORE_HEADERS}) |
There was a problem hiding this comment.
Add missing include directories for spider_core
While removing FILE_SET improves CMake compatibility, the header visibility needs to be maintained. The spider_core library is missing the corresponding target_include_directories command that was implicitly provided by FILE_SET.
Add this after line 35:
target_sources(spider_core PUBLIC ${SPIDER_CORE_HEADERS})
+target_include_directories(spider_core PUBLIC ..)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| target_sources(spider_core PUBLIC ${SPIDER_CORE_HEADERS}) | |
| target_sources(spider_core PUBLIC ${SPIDER_CORE_HEADERS}) | |
| target_include_directories(spider_core PUBLIC ..) |
| target_sources(spider_client PUBLIC ${SPIDER_CLIENT_SHARED_HEADERS}) | ||
| target_include_directories(spider_client PUBLIC ..) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use absolute paths in target_include_directories
Using a relative path ("..") in target_include_directories might cause issues depending on where the CMake configuration is included from. It's better to use CMake variables to construct absolute paths.
Replace the relative path with:
-target_include_directories(spider_client PUBLIC ..)
+target_include_directories(spider_client PUBLIC ${CMAKE_CURRENT_SOURCE_DIR}/..)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| target_sources(spider_client PUBLIC ${SPIDER_CLIENT_SHARED_HEADERS}) | |
| target_include_directories(spider_client PUBLIC ..) | |
| target_sources(spider_client PUBLIC ${SPIDER_CLIENT_SHARED_HEADERS}) | |
| target_include_directories(spider_client PUBLIC ${CMAKE_CURRENT_SOURCE_DIR}/..) |
1 participant
Description
#46 uses
FILE_SETto specify the public headers. CMake adds support forFILE_SETintarget_sourcesin version 3.23. The default cmake on ubuntu jammy is still 3.22.1, so we need to replaceFILE_SETwith good oldtarget_include_directoriesfor backward compatibility.Change unit test workflow to run on both ubuntu jammy and noble.
Validation performed
Summary by CodeRabbit
Release Notes
Chores
CI/CD
These changes improve our testing infrastructure and build configuration, ensuring better compatibility and maintainability of the project.