Skip to content
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

Added Dependency Management for ReaderWriterQueue and Pistache #43

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Added Dependency Management for ReaderWriterQueue and Pistache #43

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0f5db7d
[cmake] Added ReaderWriterQueue dependency
nicolo-mn Jan 18, 2025
3cd05a5
[cmake] Added Pistache dependency and modified its target usages
nicolo-mn Jan 18, 2025
b8cee45
Update README.md
nicolo-mn Jan 18, 2025
06997a7
Minor fixes
nicolo-mn Jan 18, 2025
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
9 changes: 2 additions & 7 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,11 +90,6 @@ pkg_check_modules(rdmacm REQUIRED IMPORTED_TARGET librdmacm)
###
pkg_check_modules(ibverbs REQUIRED IMPORTED_TARGET libibverbs)

###
# pistache
###
pkg_check_modules(Pistache REQUIRED IMPORTED_TARGET libpistache)

###
# External dependencies
###
Expand Down Expand Up @@ -185,8 +180,8 @@ foreach(target ${targets})
set_target_properties(${target} PROPERTIES RUNTIME_OUTPUT_DIRECTORY bin)
endforeach()
target_link_libraries(executor PRIVATE dl)
target_include_directories(resource_manager SYSTEM PUBLIC $<TARGET_PROPERTY:PkgConfig::Pistache,INTERFACE_INCLUDE_DIRECTORIES>)
target_link_libraries(resource_manager PRIVATE PkgConfig::Pistache)
target_include_directories(resource_manager SYSTEM PUBLIC $<TARGET_PROPERTY:pistache_static,INTERFACE_INCLUDE_DIRECTORIES>)
target_link_libraries(resource_manager PRIVATE pistache_static)
Comment on lines +183 to +184
Copy link

Choose a reason for hiding this comment

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

💡 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_libraries to 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


###
# Benchmark apps
Expand Down
3 changes: 2 additions & 1 deletion README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,13 +47,14 @@ its `ugni` provider.
- C++ compiler with C++17 support.
- `libibverbs` with headers installed.
- `librdmacm` with headers installed.
- [pistache](https://github.com/pistacheio/pistache) - HTTP and REST framework.

Furthermore, we fetch and build the following dependencies during CMake build - unless
they are found already in the system.

- [spdlog](https://github.com/gabime/spdlog) 1.8
- [cereal](https://uscilab.github.io/cereal/) 1.3
- [readerwriterqueue](https://github.com/cameron314/readerwriterqueue) 1.0.6
- [pistache](https://github.com/pistacheio/pistache) 0.0.5
- [cxxopts](https://github.com/jarro2783/cxxopts)
- [googletest](https://github.com/google/googletest)

Expand Down
27 changes: 27 additions & 0 deletions cmake/dependencies.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,3 +64,30 @@ if(${RFAAS_WITH_TESTING})
FetchContent_MakeAvailable(googletest)
endif()

###
# readerwriterqueue
###
find_package(readerwriterqueue QUIET)
if(NOT readerwriterqueue_FOUND)
message(STATUS "Downloading and building readerwriterqueue dependency")
FetchContent_Declare(
readerwriterqueue
GIT_REPOSITORY https://github.com/cameron314/readerwriterqueue
GIT_TAG v1.0.6
)
FetchContent_MakeAvailable(readerwriterqueue)
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
)
FetchContent_MakeAvailable(libpistache)
endif()
Comment on lines +81 to +93
Copy link

Choose a reason for hiding this comment

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

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

Suggested change
###
# 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()