Skip to content

cmake: optional dependency search paths #3

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
pknowles merged 1 commit into from
Feb 7, 2025
Merged

cmake: optional dependency search paths #3

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 10 additions & 7 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,18 +6,23 @@ project(decodeless_header)

set(CMAKE_CXX_STANDARD 20)

option(DECODELESS_SEARCH_DEPENDENCIES "Enable searching for dependencies in adjacent directories" ON)
option(DECODELESS_FETCH_DEPENDENCIES "Enable fetching dependencies with cmake FetchContent" OFF)
option(DECODELESS_SEARCH_DEPENDENCIES
"Enable searching for dependencies in adjacent directories" ON)
option(DECODELESS_FETCH_DEPENDENCIES
"Enable fetching dependencies with cmake FetchContent" OFF)

if(NOT TARGET decodeless::offset_ptr)
if(DECODELESS_SEARCH_DEPENDENCIES)
find_package(decodeless_offset_ptr REQUIRED CONFIG PATHS "${CMAKE_CURRENT_LIST_DIR}/../offset_ptr")
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}")
Comment on lines +16 to +19
Copy link

@coderabbitai coderabbitai bot Feb 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

Suggested change
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}")

elseif(DECODELESS_FETCH_DEPENDENCIES)
include(FetchContent)
FetchContent_Declare(
decodeless_offset_ptr
GIT_REPOSITORY https://github.com/decodeless/offset_ptr.git
GIT_TAG 1f87a9d7a8be23b90c06124014f286df91562006)
GIT_TAG 38ceefc6ce63fb4667cd207424b1277c3eed5f8d)
FetchContent_MakeAvailable(decodeless_offset_ptr)
else()
message(
Expand All @@ -29,9 +34,7 @@ endif()

add_library(decodeless_header INTERFACE)
target_include_directories(decodeless_header INTERFACE include)
target_link_libraries(
decodeless_header
INTERFACE decodeless::offset_ptr)
target_link_libraries(decodeless_header INTERFACE decodeless::offset_ptr)

add_library(decodeless::header ALIAS decodeless_header)

Expand Down
4 changes: 3 additions & 1 deletion test/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,10 @@ endif()

if(NOT TARGET decodeless::allocator)
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}")
Comment on lines 17 to +21
Copy link

@coderabbitai coderabbitai bot Feb 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 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.

Suggested change
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}")

elseif(DECODELESS_FETCH_DEPENDENCIES)
include(FetchContent)
FetchContent_Declare(
Expand Down