Conversation
- removed `rapidjson` - added vcpkg zlib - added vcpkg gmp - added vcpkg catch2 - added vcpkg prometheus-cpp
There was a problem hiding this comment.
Pull Request Overview
This PR removes the rapidjson Git submodule from the project and updates the build system to use rapidjson as an external package dependency instead. This change modernizes the dependency management approach by moving away from Git submodules to package-based dependencies.
- Removed rapidjson Git submodule and its configuration
- Updated CMakeLists.txt to find and link rapidjson as an external package
- Commented out rapidjson submodule entry in .gitmodules instead of removing it completely
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| external/rapidjson | Removes the rapidjson submodule directory |
| CMakeLists.txt | Adds package finding logic and updates library linking for rapidjson |
| .gitmodules | Comments out the rapidjson submodule configuration |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| if (NOT TARGET rapidjson) | ||
| find_package(RapidJSON REQUIRED) | ||
| if(NOT TARGET rapidjson) | ||
| message(FATAL_ERROR "RapidJSON not found!") | ||
| endif() | ||
| endif() |
There was a problem hiding this comment.
The logic is inconsistent: find_package(RapidJSON REQUIRED) should create a target, but the code checks for rapidjson (lowercase) while the package name is RapidJSON (camelcase). The target name created by find_package may be different. Consider using RapidJSON::RapidJSON or check the actual target name created by the RapidJSON CMake package.
| "$<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}>/external/rapidjson/include" | ||
| "$<INSTALL_INTERFACE:${CMAKE_INSTALL_INCLUDEDIR}>") | ||
|
|
||
| target_link_libraries(abieos rapidjson) |
There was a problem hiding this comment.
The target name rapidjson may not exist. RapidJSON typically creates targets like RapidJSON::RapidJSON. Verify the correct target name from the RapidJSON package documentation or use rapidjson::rapidjson if that's the expected target.
| "$<INSTALL_INTERFACE:${CMAKE_INSTALL_INCLUDEDIR}>") | ||
|
|
||
| target_link_libraries(abieos_module ${CMAKE_THREAD_LIBS_INIT}) | ||
| target_link_libraries(abieos_module ${CMAKE_THREAD_LIBS_INIT} rapidjson) |
There was a problem hiding this comment.
Same issue as above - the target name rapidjson may not be correct. Should use the proper target name created by the RapidJSON package.
dtaghavi
left a comment
dtaghavi
left a comment
There was a problem hiding this comment.
Approved but if what CoPilot said is true we could adjust that. I imagine it doesn't matter since it worked for you in testing.
No description provided.