-
Notifications
You must be signed in to change notification settings - Fork 155
fix: CMake package and imported library target names #568
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.
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
base: main
Are you sure you want to change the base?
Conversation
Summary of ChangesHello @ta7mid, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refines the CMake build system for reflect-cpp to enhance compatibility with updated versions of external dependencies, specifically libbson (2.0.0+) and Apache Arrow (21.0.0+). It addresses breaking changes in how these libraries expose their CMake packages and targets, ensuring reflect-cpp can correctly find and link against them. The changes also streamline the selection between static and shared library linkage based on the project's build type, improving overall build robustness. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request addresses several issues with dependency handling in CMake for libbson and Apache Arrow. The changes correctly adapt to new package and target names in recent versions of these libraries and simplify the logic for linking static or shared variants. My main concern is with the version range specified for libbson, which is too restrictive and would prevent using versions newer than 2.0.0. I've left specific suggestions to address this in the code.
|
@ta7mid thanks for the PR. However, it appears that this has broken the Conan build. Could you take another look? |
|
@liuzicheng1987 Can you check if it works now? |
|
@ta7mid , there still appear to be the same issue with Conan... |
|
It's probably fixed now. |
and refactor
This PR addresses the following issues encountered/noted when creating a Homebrew formula for reflect-cpp:
CMakeLists.txt, reflect-cpp links witharrow::arrowwhenREFLECTCPP_USE_VCPKGis false, but no such target is provided by the CMake config packages installed with Apache Arrow ≥21.0.0, regardless of whether vcpkg is used.target_link_libraries(reflectcpp PUBLIC $<IF:$<TARGET_EXISTS:…>…,…>)are not evaluated and get copied verbatim into the exportedreflectcpp-exports.cmake.