fix(cmake): Correct linking of dependencies for interface (header-only) libraries. #45
Conversation
WalkthroughThis PR refines the linking logic within the Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant CMakeFunction as cpp_library()
Caller->>CMakeFunction: Invoke cpp_library()
alt Header-only library?
CMakeFunction->>CMakeFunction: Check if arg_cpp_lib_PRIVATE_LINK_LIBRARIES is defined
alt Private libraries defined?
CMakeFunction->>Caller: Emit fatal error
else
CMakeFunction->>Caller: Link public libraries as INTERFACE
end
else Non-header-only library
CMakeFunction->>Caller: Execute linking in else branch
end
Possibly related PRs
Suggested reviewers
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 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: 0
🧹 Nitpick comments (1)
CMake/ystdlib-cpp-helpers.cmake (1)
68-98: Consider adding explanatory comments for future maintainersWhile the code changes are technically correct, it might be helpful to add comments explaining why header-only libraries can only use INTERFACE linking. This would benefit developers who aren't familiar with CMake's library types.
if(_IS_INTERFACE_LIB) + # Header-only libraries must use INTERFACE linkage as they don't have a compiled component if(arg_cpp_lib_PRIVATE_LINK_LIBRARIES) message( FATAL_ERROR "`PRIVATE_LINK_LIBRARIES` disabled for header-only library ${_ALIAS_TARGET_NAME}." ) endif() add_library(${arg_cpp_lib_NAME} INTERFACE) target_link_libraries(${arg_cpp_lib_NAME} INTERFACE ${arg_cpp_lib_PUBLIC_LINK_LIBRARIES})
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
CMake/ystdlib-cpp-helpers.cmake(2 hunks)
🔇 Additional comments (3)
CMake/ystdlib-cpp-helpers.cmake (3)
68-73: Strong error handling for header-only librariesThis is a good addition that prevents misuse of the
PRIVATE_LINK_LIBRARIESparameter with header-only libraries. Since header-only libraries are implemented as CMake INTERFACE libraries, they can't have private linkage (as there's no compiled artifact). The error message clearly explains the issue to users.
75-75: Appropriate linking of public dependencies for header-only librariesThis change correctly uses the INTERFACE keyword for linking public dependencies to header-only libraries, which ensures that consumers of the library will inherit these dependencies.
92-98: Proper organization of linking commands for non-header-only librariesMoving the linking commands for both public and private libraries into the non-header-only branch is appropriate. This ensures the correct linking visibility is used based on library type.
Bill-hbrhbr
changed the title
davidlion
changed the title
2 participants
Description
For header-only libraries, we add the target using
add_library(${arg_cpp_lib_NAME} INTERFACE)call, making it an interface target. For interface targets, doing the following will failas interface targets can only have
INTERFACEproperties.Instead of adding a new arg called
INTERFACE_LINK_LIBRARIEStocpp_library(), we keep usingPUBLIC_LINK_LIBRARIES:and forbids users from specifying
PRIVATE_LINK_LIBRARIESas interface dependencies will get propagated.Checklist
breaking change.
Validation performed