build(cmake): Add support for building library targets that contain source files and linked libraries.#36
Conversation
WalkthroughThis pull request introduces a new helper function, Changes
Sequence Diagram(s)sequenceDiagram
participant C as cpp_library
participant H as check_if_header_only
participant L as Library Builder
participant U as Unit Test Linker
C->>H: Validate PUBLIC_HEADERS and PRIVATE_SOURCES
H-->>C: Return header-only status (or first invalid file)
alt All headers are valid
C->>L: Create interface or regular library based on header status
L-->>C: Library target created
C->>U: Link unit tests with the new alias target
else Invalid header detected
C-->>C: Trigger fatal error with invalid header detail
end
Possibly Related PRs
Suggested Reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms (2)
🔇 Additional comments (9)
🪧 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 (2)
CMake/ystdlib-cpp-helpers.cmake (2)
8-9: Consider expanding header file extension detectionThe current regex only handles .h, .hpp, and .inc extensions. You might want to consider other common C++ header extensions like .hxx, .h++, or .tpp for template implementations.
- if(${src_file} MATCHES ".*\\.(h|hpp|inc)") + if(${src_file} MATCHES ".*\\.(h|hpp|inc|hxx|h\\+\\+|tpp)")
85-85: Consider adding flexibility for dependency visibilityCurrently, all dependencies are linked with PUBLIC visibility. Consider allowing PRIVATE or INTERFACE linkage for dependencies that don't need to be exposed to consumers of the library.
- target_link_libraries(${arg_cpp_lib_NAME} PUBLIC ${arg_cpp_lib_DEPENDS}) + # Split dependencies into PUBLIC and PRIVATE if needed + target_link_libraries(${arg_cpp_lib_NAME} PUBLIC ${arg_cpp_lib_DEPENDS})In the future, you might want to expand this to allow specifying the visibility for each dependency:
# Example enhancement for future consideration function(cpp_library) # ... existing parameters ... set(multiValueArgs # ... existing multi-value args ... PUBLIC_DEPENDS PRIVATE_DEPENDS INTERFACE_DEPENDS ) # ... # Link with appropriate visibility if(arg_cpp_lib_PUBLIC_DEPENDS) target_link_libraries(${arg_cpp_lib_NAME} PUBLIC ${arg_cpp_lib_PUBLIC_DEPENDS}) endif() if(arg_cpp_lib_PRIVATE_DEPENDS) target_link_libraries(${arg_cpp_lib_NAME} PRIVATE ${arg_cpp_lib_PRIVATE_DEPENDS}) endif() if(arg_cpp_lib_INTERFACE_DEPENDS) target_link_libraries(${arg_cpp_lib_NAME} INTERFACE ${arg_cpp_lib_INTERFACE_DEPENDS}) endif()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
CMake/ystdlib-cpp-helpers.cmake(4 hunks)CMakeLists.txt(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: lint-check (ubuntu-24.04)
🔇 Additional comments (4)
CMakeLists.txt (1)
46-47: LGTM - Added standard CMake option for shared librariesThis is a standard CMake option that controls whether libraries are built as shared (.so/.dll) or static (.a/.lib) by default. The option is appropriately documented and placed in a logical location in the file.
CMake/ystdlib-cpp-helpers.cmake (3)
1-18: Well-implemented helper function for detecting header-only librariesThe function correctly identifies header-only libraries by removing header file extensions and checking if any source files remain. The implementation is clean and the function is well-documented.
28-44: LGTM - Updated function signature with new parametersThe function parameters have been appropriately expanded to support the new functionality. The documentation and parameter declarations are well aligned.
59-83: LGTM - Well-implemented conditional library creationThe implementation correctly differentiates between header-only libraries (which need to be INTERFACE) and libraries with source files. The include directories and compile features are set with appropriate visibility modifiers.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
CMake/ystdlib-cpp-helpers.cmake (2)
1-18: Well-structured header-only detection logicThe
check_if_header_only_libraryfunction is implemented with clear logic to determine if a library consists only of header files. It correctly identifies and filters out common header file extensions.For additional robustness, consider extending the pattern to include other possible header extensions like
.hxxor.h++, though this is not critical for most projects.- if(${src_file} MATCHES ".*\\.(h|hpp|inc)") + if(${src_file} MATCHES ".*\\.(h|hpp|inc|hxx|h\\+\\+)")
61-83: Library creation logic requires clarificationThe implementation correctly handles both header-only and regular libraries, but some aspects could be clearer:
- When creating a non-interface library, the empty string parameter to
add_library()deserves explanation:- add_library(${arg_cpp_lib_NAME} "") # Library type determined by BUILD_SHARED_LIBS + # Empty string makes library type determined by BUILD_SHARED_LIBS option + add_library(${arg_cpp_lib_NAME} "")
- Consider adding a comment explaining the different visibility levels for include directories:
target_include_directories( ${arg_cpp_lib_NAME} PUBLIC # Public visibility for non-header-only libraries to expose headers "$<BUILD_INTERFACE:${arg_cpp_lib_LIB_BUILD_INTERFACE}>" )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
CMake/ystdlib-cpp-helpers.cmake(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: lint-check (macos-15)
🔇 Additional comments (4)
CMake/ystdlib-cpp-helpers.cmake (4)
28-30: LGTM: Properly documented function parametersThe new parameters for the
cpp_libraryfunction are clearly documented, making the function's usage apparent to users.
41-43: LGTM: Parameters correctly added to multiValueArgsThe parameters are appropriately categorized as multi-value arguments, which is consistent with their purpose.
59-60: LGTM: Good use of helper functionExcellent approach to separate the header-only detection logic into a reusable function.
85-85: LGTM: Proper library dependency handlingCorrectly links the library with its dependencies, regardless of whether it's a header-only or regular library.
|
@coderabbitai |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
CMake/ystdlib-cpp-helpers.cmake (2)
63-86: Consider parameter validation for SOURCES and HEADERSThe conditional logic for creating either an interface or standard library works well. However, there's no validation to ensure
arg_cpp_lib_SOURCESorarg_cpp_lib_HEADERSare defined before use, similar to the check for LIB_BUILD_INTERFACE.+ # Check if required parameters are provided + if(NOT DEFINED arg_cpp_lib_SOURCES) + set(arg_cpp_lib_SOURCES "") + endif() + + if(NOT DEFINED arg_cpp_lib_HEADERS) + set(arg_cpp_lib_HEADERS "") + endif() + check_if_header_only_library(arg_cpp_lib_SOURCES _IS_INTERFACE_LIB)
74-79: Consider relocating headers to PUBLIC sourcesCurrently, headers are included in the PRIVATE section of target_sources. If these are API headers that should be visible to dependents, consider moving them to the PUBLIC section.
target_sources( ${arg_cpp_lib_NAME} + PUBLIC + ${arg_cpp_lib_HEADERS} PRIVATE - ${arg_cpp_lib_HEADERS} ${arg_cpp_lib_SOURCES} )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
CMake/ystdlib-cpp-helpers.cmake(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: lint-check (ubuntu-24.04)
- GitHub Check: lint-check (macos-15)
🔇 Additional comments (5)
CMake/ystdlib-cpp-helpers.cmake (5)
1-18: Well-implemented header-only library check functionThe
check_if_header_only_libraryfunction is well-structured and correctly identifies libraries that consist solely of header files by iterating through the source list and removing header files.
28-31: Good documentation of new parametersClear documentation of the newly added parameters enhances the function's usability.
42-45: Appropriate parameter registrationThe new parameters are properly registered in the multiValueArgs list, which is necessary for cmake_parse_arguments to process them correctly.
61-62: Good use of the new check functionAppropriately using the newly defined function to determine library type.
88-94: Proper linkage of dependenciesThe update correctly handles both public and private dependencies, which is essential for libraries with source files.
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainI need to look at the 🏁 Script executed: Length of output: 729 @Bill-hbrhbr Thank you for the feedback! I see the header-only detection logic checks for .h, .hpp, and .inc extensions. I understand that since you only lint .h, .hpp, and .cpp files in your project, there's no need to expand the detection scope further. The current implementation is appropriately focused on your project's needs. ✏️ Learnings added
🧠 Learnings used |
CMake/ystdlib-cpp-helpers.cmake
Outdated
| DEPENDS | ||
| INTERNAL_DEPENDS |
There was a problem hiding this comment.
Lets match the cmake naming so something like:
PUBLIC_LINK_LIBS (and PRIVATE...) or just PUBLIC_LIBS maybe.
There was a problem hiding this comment.
I've renamed everything to use very explicit names. I got the idea of INTERFACE_LIBRARIES from here (which is CMake official doc)
CMake/ystdlib-cpp-helpers.cmake
Outdated
| HEADERS | ||
| SOURCES |
There was a problem hiding this comment.
I think everything works even if we don't separate/distinguish headers and source files, so I think just SOURCES is fine. If it is important to separate them, then maybe we should concatenate them and pass both to check_if_header_only_library to avoid user error (currently if you put a cpp file in HEADERS I think things will break)?
There was a problem hiding this comment.
Not important to separate now, but will make a difference when we only want to install public headers into install library paths.
There was a problem hiding this comment.
On a second thought, I've decided to keep them split and renamed the params for more clarity. I've also enforced a check of the public header list.
|
TLDR of changes since last review (except for the ones directly resolving your comments):
|
davidlion
changed the title
2 participants
Description
Expanded and updated the arglist of
cpp_library():Added a helper function
check_if_header_only()for two functionalities:PUBLIC_HEADERSPRIVATE_SOURCESstatic/sharedlibrary targetinterfacelibrary targetChecklist
breaking change.
Validation performed
No tests needed.
Summary by CodeRabbit