Location via proxy:   [ UP ]  
[Report a bug]   [Manage cookies]                
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.

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

AVRO-3088: [C++] Export CMake package config file #3299

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

wgtmac
Copy link
Member

@wgtmac wgtmac commented Jan 22, 2025

What is the purpose of the change

  • Apply modern CMake practice to use target-oriented functions.
  • Export and install CMake package configuration file.

Verifying this change

I have manually verified that the exported config file can be used by downstream projects like below:

find_package(Avro CONFIG REQUIRED)
target_link_libraries(xxx PRIVATE Avro::avrocpp_shared)
target_link_libraries(xxx PRIVATE Avro::avrocpp_static)

Documentation

  • Does this pull request introduce a new feature? (yes / no)
  • If yes, how is the feature documented? (not applicable / docs / JavaDocs / not documented)

@github-actions github-actions bot added the C++ Pull Requests for C++ binding label Jan 22, 2025
@wgtmac
Copy link
Member Author

wgtmac commented Jan 22, 2025

@mitjap @mkmkme @thiru-mg Could you help review this? Thanks!

lang/c++/CMakeLists.txt Outdated Show resolved Hide resolved
lang/c++/CMakeLists.txt Outdated Show resolved Hide resolved
lang/c++/CMakeLists.txt Outdated Show resolved Hide resolved
wgtmac added a commit to wgtmac/iceberg-cpp that referenced this pull request Jan 23, 2025
lang/c++/CMakeLists.txt Outdated Show resolved Hide resolved
@Fokko Fokko requested a review from thiru-mg January 24, 2025 13:55
# Boost 1.70 and above provide a BoostConfig.cmake package configuration file.
# It guarantees that Boost::system target exists if found.
# See https://cmake.org/cmake/help/latest/policy/CMP0167.html
find_package (Boost 1.70 REQUIRED CONFIG COMPONENTS system)
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't set the Boost version in the Dockerfile: https://github.com/apache/avro/blob/main/share/docker/Dockerfile#L49

So that's good 👍

@Fokko Fokko requested review from mitjap and mkmkme January 26, 2025 12:44
endif ()

if (AVRO_BUILD_TESTS)
enable_testing()

macro (unittest name)
add_executable (${name} test/${name}.cc)
target_link_libraries (${name} avrocpp_s ${Boost_LIBRARIES} ${SNAPPY_LIBRARIES} ${ZLIB_LIBRARIES})
target_link_libraries (${name} avrocpp_s Boost::system ZLIB::ZLIB $<$<TARGET_EXISTS:Snappy::snappy>:Snappy::snappy>)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't we use $<TARGET_NAME_IF_EXISTS:Snappy::snappy> here as well instead of $<$<TARGET_EXISTS:Snappy::snappy>:Snappy::snappy>?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, that's better. I have switched to use TARGET_NAME_IF_EXISTS. At the same time, the minimum CMake version has been bumped to 3.12 to use it: https://cmake.org/cmake/help/v3.12/release/3.12.html?highlight=target_name_if_exists#generator-expressions

Copy link
Contributor

@mkmkme mkmkme left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C++ Pull Requests for C++ binding
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants