cmake: optional dependency search paths #3
Conversation
|
Warning Rate limit exceeded@pknowles has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 15 minutes and 28 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughThe changes update dependency management in two CMake configuration files. New options have been introduced to specify custom search paths for dependencies: one for the decodeless_offset_ptr in the main CMakeLists.txt and another for the decodeless_allocator in the test/CMakeLists.txt. The find_package commands now reference these options instead of hardcoded paths, with minor reformatting and a slight reorganization of the conditional logic. Error handling and linking behavior remain unchanged. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant C as CMake
participant F as find_package
participant D as decodeless_offset_ptr
U->>C: Run configuration with DECODELESS_OFFSET_PTR_SEARCH_PATH option
C->>F: Execute find_package using provided search path
F->>D: Search for decodeless_offset_ptr package
alt Package Found
D-->>F: Return package info
F-->>C: Supply dependency details
else Package Not Found
F-->>C: Return error details
C->>U: Display fatal error message
end
sequenceDiagram
participant U as User
participant C as CMake_test
participant F as find_package
participant D as decodeless_allocator
U->>C: Run test configuration with DECODELESS_ALLOCATOR_SEARCH_PATH option
C->>F: Execute find_package using provided search path
F->>D: Search for decodeless_allocator package
alt Package Found
D-->>F: Return package info
F-->>C: Supply dependency details
else Package Not Found
F-->>C: Return error details
C->>U: Display fatal error message
end
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 (
|
![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: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
CMakeLists.txt(2 hunks)test/CMakeLists.txt(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build (windows-latest, Release, cl)
- GitHub Check: build (windows-latest, Debug, cl)
🔇 Additional comments (2)
CMakeLists.txt (2)
9-12: Clarify Boolean Options for Dependency ControlThe boolean options DECODELESS_SEARCH_DEPENDENCIES and DECODELESS_FETCH_DEPENDENCIES are clearly defined with descriptive help texts. This makes it easy for users to override dependency management behavior.
37-37: Linking Update ConsistencyThe update to the target_link_libraries() invocation on line 37 now cleanly links the decodeless_header interface library to decodeless::offset_ptr. This reformatting is consistent and improves readability.
| option(DECODELESS_OFFSET_PTR_SEARCH_PATH | ||
| "${CMAKE_CURRENT_LIST_DIR}/../offset_ptr") | ||
| find_package(decodeless_offset_ptr REQUIRED CONFIG PATHS | ||
| "${DECODELESS_OFFSET_PTR_SEARCH_PATH}") |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reconsider Using option() for a Path Variable
A new option DECODELESS_OFFSET_PTR_SEARCH_PATH has been introduced and used to pass a custom search path to find_package. However, the option() command is meant for boolean values. For a path string, it is best practice to use set() with a CACHE entry of type PATH to ensure proper type handling and caching behavior.
Consider replacing the declaration with the following diff:
- option(DECODELESS_OFFSET_PTR_SEARCH_PATH
- "${CMAKE_CURRENT_LIST_DIR}/../offset_ptr")
+ set(DECODELESS_OFFSET_PTR_SEARCH_PATH "${CMAKE_CURRENT_LIST_DIR}/../offset_ptr" CACHE PATH "Path to decodeless_offset_ptr")📝 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.
| option(DECODELESS_OFFSET_PTR_SEARCH_PATH | |
| "${CMAKE_CURRENT_LIST_DIR}/../offset_ptr") | |
| find_package(decodeless_offset_ptr REQUIRED CONFIG PATHS | |
| "${DECODELESS_OFFSET_PTR_SEARCH_PATH}") | |
| set(DECODELESS_OFFSET_PTR_SEARCH_PATH "${CMAKE_CURRENT_LIST_DIR}/../offset_ptr" CACHE PATH "Path to decodeless_offset_ptr") | |
| find_package(decodeless_offset_ptr REQUIRED CONFIG PATHS | |
| "${DECODELESS_OFFSET_PTR_SEARCH_PATH}") |
| if(DECODELESS_SEARCH_DEPENDENCIES) | ||
| option(DECODELESS_ALLOCATOR_SEARCH_PATH | ||
| "${CMAKE_CURRENT_LIST_DIR}/../allocator") | ||
| find_package(decodeless_allocator REQUIRED CONFIG PATHS | ||
| "${CMAKE_CURRENT_LIST_DIR}/../../allocator") | ||
| "${DECODELESS_ALLOCATOR_SEARCH_PATH}") |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
🛠️ Refactor suggestion
Refactor String-Based Dependency Path Option
A new option DECODELESS_ALLOCATOR_SEARCH_PATH is introduced to customize the search path for decodeless_allocator. Similar to the main CMakeLists.txt, using option() here for a path is not ideal because option() is reserved for booleans. Switching to set() with a CACHE type PATH will ensure the variable is treated as a string instead of a boolean value.
Apply this change with the following diff:
- option(DECODELESS_ALLOCATOR_SEARCH_PATH
- "${CMAKE_CURRENT_LIST_DIR}/../allocator")
+ set(DECODELESS_ALLOCATOR_SEARCH_PATH "${CMAKE_CURRENT_LIST_DIR}/../allocator" CACHE PATH "Path to decodeless_allocator")This change will make the configuration more robust when users provide custom paths.
📝 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.
| if(DECODELESS_SEARCH_DEPENDENCIES) | |
| option(DECODELESS_ALLOCATOR_SEARCH_PATH | |
| "${CMAKE_CURRENT_LIST_DIR}/../allocator") | |
| find_package(decodeless_allocator REQUIRED CONFIG PATHS | |
| "${CMAKE_CURRENT_LIST_DIR}/../../allocator") | |
| "${DECODELESS_ALLOCATOR_SEARCH_PATH}") | |
| if(DECODELESS_SEARCH_DEPENDENCIES) | |
| - option(DECODELESS_ALLOCATOR_SEARCH_PATH | |
| - "${CMAKE_CURRENT_LIST_DIR}/../allocator") | |
| + set(DECODELESS_ALLOCATOR_SEARCH_PATH "${CMAKE_CURRENT_LIST_DIR}/../allocator" CACHE PATH "Path to decodeless_allocator") | |
| find_package(decodeless_allocator REQUIRED CONFIG PATHS | |
| "${DECODELESS_ALLOCATOR_SEARCH_PATH}") |
Summary by CodeRabbit