Added Dependency Management for ReaderWriterQueue and Pistache #43
Conversation
WalkthroughThe pull request introduces changes to the project's dependency management and build configuration. The modifications focus on updating the Pistache library usage from a package config method to a static library approach. Additionally, new dependencies like Changes
Sequence DiagramsequenceDiagram
participant CM as CMake
participant RWQ as ReaderWriterQueue
participant Pistache as Pistache Library
participant GT as GoogleTest
CM->>RWQ: Fetch dependency v1.0.6
CM->>Pistache: Fetch library v0.0.5
CM->>GT: Conditionally fetch if testing enabled
CM->>CM: Configure build targets
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: CHILL
Plan: Pro
📒 Files selected for processing (2)
CMakeLists.txt(1 hunks)cmake/dependencies.cmake(1 hunks)
🔇 Additional comments (2)
cmake/dependencies.cmake (1)
67-79: Verify if ReaderWriterQueue requires any build options.The dependency configuration follows the project's pattern and uses a pinned version which is good. However, let's verify if any build options (like tests, examples) should be disabled.
✅ Verification successful
No build options needed for ReaderWriterQueue
The library is header-only with a minimal CMake configuration that doesn't expose any build options. The current dependency configuration is correct as is.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if ReaderWriterQueue has CMake options we should configure curl -s https://raw.githubusercontent.com/cameron314/readerwriterqueue/v1.0.6/CMakeLists.txt | grep -i "option("Length of output: 114
Script:
#!/bin/bash # First, let's check the repository structure at v1.0.6 curl -s "https://api.github.com/repos/cameron314/readerwriterqueue/git/trees/v1.0.6?recursive=1" | jq -r '.tree[].path' | grep -i 'cmake\|CMakeLists.txt' # Alternatively, let's check the raw content directly curl -sI https://raw.githubusercontent.com/cameron314/readerwriterqueue/v1.0.6/CMakeLists.txtLength of output: 1136
Script:
#!/bin/bash # Fetch and display CMakeLists.txt content curl -s https://raw.githubusercontent.com/cameron314/readerwriterqueue/v1.0.6/CMakeLists.txt # Also check for any other potential configuration files curl -s "https://api.github.com/repos/cameron314/readerwriterqueue/git/trees/v1.0.6?recursive=1" | jq -r '.tree[].path' | grep -i '\.cmake$'Length of output: 616
CMakeLists.txt (1)
Line range hint
71-77: Verify if PkgConfig is still needed.Since Pistache is now managed through FetchContent, let's verify if PkgConfig is still required by other dependencies.
✅ Verification successful
PkgConfig is required and should be kept
PkgConfig is still needed for configuring required dependencies:
- librdmacm (RDMA Connection Manager)
- libibverbs (InfiniBand Verbs)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check remaining PkgConfig usage rg "pkg_check_modules" --type cmakeLength of output: 189
| ### | ||
| # pistache | ||
| ### | ||
| find_package(libpistache 0.0.5 EXACT QUIET) | ||
| if(NOT libpistache_FOUND) | ||
| message(STATUS "Downloading and building libpistache dependency") | ||
| FetchContent_Declare( | ||
| libpistache | ||
| GIT_REPOSITORY https://github.com/pistacheio/pistache | ||
| GIT_TAG 0.0.5 | ||
| ) | ||
| FetchContent_MakeAvailable(libpistache) | ||
| endif() |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add CMAKE_ARGS to disable unnecessary Pistache features.
While using a pinned version that matches Debian 12 stable is good, we should also disable unnecessary Pistache features to minimize build time and dependencies.
Add these build options:
FetchContent_Declare(
libpistache
GIT_REPOSITORY https://github.com/pistacheio/pistache
GIT_TAG 0.0.5
+ CMAKE_ARGS
+ -DPISTACHE_BUILD_TESTS=OFF
+ -DPISTACHE_BUILD_EXAMPLES=OFF
+ -DPISTACHE_BUILD_DOCS=OFF
)📝 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.
| ### | |
| # pistache | |
| ### | |
| find_package(libpistache 0.0.5 EXACT QUIET) | |
| if(NOT libpistache_FOUND) | |
| message(STATUS "Downloading and building libpistache dependency") | |
| FetchContent_Declare( | |
| libpistache | |
| GIT_REPOSITORY https://github.com/pistacheio/pistache | |
| GIT_TAG 0.0.5 | |
| ) | |
| FetchContent_MakeAvailable(libpistache) | |
| endif() | |
| ### | |
| # pistache | |
| ### | |
| find_package(libpistache 0.0.5 EXACT QUIET) | |
| if(NOT libpistache_FOUND) | |
| message(STATUS "Downloading and building libpistache dependency") | |
| FetchContent_Declare( | |
| libpistache | |
| GIT_REPOSITORY https://github.com/pistacheio/pistache | |
| GIT_TAG 0.0.5 | |
| CMAKE_ARGS | |
| -DPISTACHE_BUILD_TESTS=OFF | |
| -DPISTACHE_BUILD_EXAMPLES=OFF | |
| -DPISTACHE_BUILD_DOCS=OFF | |
| ) | |
| FetchContent_MakeAvailable(libpistache) | |
| endif() |
| target_include_directories(resource_manager SYSTEM PUBLIC $<TARGET_PROPERTY:pistache_static,INTERFACE_INCLUDE_DIRECTORIES>) | ||
| target_link_libraries(resource_manager PRIVATE pistache_static) |
There was a problem hiding this comment.
💡 Codebase verification
Missing required dependencies for Pistache
The current configuration is incomplete. Based on Pistache's requirements, the following changes are needed:
- Add
find_package(OpenSSL REQUIRED COMPONENTS SSL Crypto) - Update
target_link_librariesto include required dependencies:target_link_libraries(resource_manager PRIVATE pistache_static Threads::Threads OpenSSL::SSL OpenSSL::Crypto )
🔗 Analysis chain
Verify Pistache dependencies are properly linked.
The switch to static linking is good, but we should verify that all required Pistache dependencies are properly linked.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check Pistache dependencies
ast-grep --pattern 'target_link_libraries($$$pistache_static$$$)' | grep -B2 -A2 'target_link_libraries'
# Verify if Pistache requires additional system libraries
curl -s https://raw.githubusercontent.com/pistacheio/pistache/0.0.5/CMakeLists.txt | grep -i "find_package"
Length of output: 330
Script:
#!/bin/bash
# Check for required dependencies in CMakeLists.txt
echo "=== Checking for required dependencies ==="
rg "find_package\((Threads|OpenSSL)" -A 2
echo -e "\n=== Checking resource_manager linking ==="
rg "target_link_libraries.*resource_manager.*" -B 2 -A 2
Length of output: 679
|
@nicolo-mn Thanks for the PR; it looks very nice! I wouldn't say it's your first :-) Two questions:
|
This PR addresses Issue #14.
As requested, I tested the code from PR #41, but I encountered build issues: the libraries were fetched without updating the target usages, and fetching Pistache from the master branch introduced a dependency on
libcurl4-openssl-dev.To resolve these issues, I updated the target usages and switched to fetching a specific tagged release of Pistache instead of the master branch. The selected tag corresponds to the version available in the Debian 12 stable repository, as the latest (very recent) version introduces an additional dependency, as mentioned above. I also updated the README and moved the management of both Pistache and ReaderWriterQueue from
CMakeLists.txttocmake/dependencies.cmakealongside the other fetched dependencies.I built and executed the project on a Debian 12 (stable) machine with SoftRoCE to ensure all dependencies are fetched, included, and linked correctly.
This is my first PR for this project so please be patient with me, and I’ll be happy to make any additional changes or fixes as needed.
Summary by CodeRabbit
Release Notes
readerwriterqueuelibrary version 1.0.6pistachelibrary to version 0.0.5