-
Notifications
You must be signed in to change notification settings - Fork 1
Enhance CMakeLists for vendored dependency handling #1
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.
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
Conversation
Added support for vendored dependency installation and export rules.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR enhances the CMakeLists.txt to better support liboai when used as a vendored dependency via add_subdirectory. The changes allow parent projects to provide dependencies as existing targets and control whether install/export rules are generated.
- Added conditional install/export rules that are disabled by default when liboai is not the top-level project
- Modified dependency detection to check for existing targets before calling find_package for both nlohmann_json and CURL
- Made install rules optional via the LIBOAI_INSTALL option
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| endif() | ||
| endif() | ||
|
|
||
| # Try to find CURL, but if not found, use the vendored target if it exists |
Copilot
AI
Jan 5, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment says "but if not found, use the vendored target if it exists" which is misleading. The code actually checks if the CURL::libcurl target exists first, and only calls find_package if it doesn't exist. Consider revising to: "Try to use CURL::libcurl target if it exists, otherwise try to find CURL package".
| # Try to find CURL, but if not found, use the vendored target if it exists | |
| # Try to use CURL::libcurl target if it exists, otherwise try to find CURL package |
| endif() | ||
| option(LIBOAI_INSTALL "Enable liboai install/export rules" ${_LIBOAI_DEFAULT_INSTALL}) | ||
|
|
||
| # Try to find nlohmann_json, but if not found, use a target if it exists |
Copilot
AI
Jan 5, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment says "but if not found, use a target if it exists" which is misleading. The code actually checks if nlohmann_json targets exist first, and only calls find_package if no target exists. Consider revising to: "Try to use nlohmann_json target if it exists, otherwise try to find nlohmann_json package".
| # Try to find nlohmann_json, but if not found, use a target if it exists | |
| # Try to use nlohmann_json target if it exists, otherwise try to find nlohmann_json package |
| else() | ||
| find_package(CURL QUIET) | ||
| if(NOT CURL_FOUND) | ||
| message(FATAL_ERROR "CURL not found and CURL::libcurl target does not exist") |
Copilot
AI
Jan 5, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For consistency with the nlohmann_json target detection (lines 23-27), consider also checking for a non-namespaced CURL target. Some vendored CURL builds may create a libcurl or curl target without the CURL:: namespace. This would make the dependency detection more robust.
| else() | |
| find_package(CURL QUIET) | |
| if(NOT CURL_FOUND) | |
| message(FATAL_ERROR "CURL not found and CURL::libcurl target does not exist") | |
| elseif(TARGET libcurl) | |
| set(CURL_FOUND TRUE) | |
| add_library(CURL::libcurl ALIAS libcurl) | |
| elseif(TARGET curl) | |
| set(CURL_FOUND TRUE) | |
| add_library(CURL::libcurl ALIAS curl) | |
| else() | |
| find_package(CURL QUIET) | |
| if(NOT CURL_FOUND) | |
| message(FATAL_ERROR "CURL not found and no suitable libcurl target exists") |
| # dependencies (eg. CURL::libcurl) are not part of liboai's export set. | ||
| # Only enable install/export when building liboai as the top-level project. | ||
| set(_LIBOAI_DEFAULT_INSTALL OFF) | ||
| if(PROJECT_IS_TOP_LEVEL) |
Copilot
AI
Jan 5, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The variable PROJECT_IS_TOP_LEVEL is only available in CMake 3.21 and later, but this file requires CMake 3.13 (line 1). This will cause a build failure on CMake versions between 3.13 and 3.20. Either update the minimum required version to 3.21, or add a check to define PROJECT_IS_TOP_LEVEL for older versions.
Added support for vendored dependency installation and export rules.