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

Jump to bottom

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

Merged
merged 8 commits into from
Feb 1, 2025
Merged

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

merged 8 commits into from
Feb 1, 2025

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 wgtmac mentioned this pull request Jan 22, 2025
Merged
@wgtmac wgtmac force-pushed the cmake_config branch 2 times, most recently from 65235c0 to 69b9bd8 Compare January 22, 2025 16:11
@wgtmac
Copy link
Member Author

wgtmac commented Jan 22, 2025

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

mitjap
mitjap reviewed Jan 22, 2025
View reviewed changes
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
@wgtmac
Adopt change from apache/avro#3299
923bf15
mkmkme
mkmkme reviewed Jan 23, 2025
View reviewed changes
lang/c++/CMakeLists.txt Outdated Show resolved Hide resolved
@Fokko Fokko requested a review from thiru-mg January 24, 2025 13:55
Fokko
Fokko reviewed Jan 24, 2025
View reviewed changes
lang/c++/CMakeLists.txt
# 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
mkmkme
mkmkme reviewed Jan 27, 2025
View reviewed changes
lang/c++/CMakeLists.txt Outdated
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

mkmkme
mkmkme approved these changes Jan 31, 2025
View reviewed changes
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!

@wgtmac
Copy link
Member Author

wgtmac commented Feb 1, 2025 •
edited
Loading

Thanks for the review! @mkmkme @martin-g

Now I have rebased to the main branch and resolved the conflict.

@wgtmac
Copy link
Member Author

wgtmac commented Feb 1, 2025

@thiru-mg @martin-g @Fokko Could you help merge this? iceberg-cpp is depending on my fork which is unavailable after rebasing: https://github.com/apache/iceberg-cpp/blob/c0bf8ce28e4879f674f9fe39160e0a1bae6845f4/cmake_modules/IcebergThirdpartyToolchain.cmake#L153-L154

@thiru-mg thiru-mg merged commit c75e6d5 into apache:main Feb 1, 2025
4 checks passed
@wgtmac
Copy link
Member Author

wgtmac commented Feb 1, 2025

Thanks for merging it! @thiru-mg

@Fokko
Copy link
Contributor

Fokko commented Feb 3, 2025

I see that @thiru-mg beat met to it (which is a good thing), thanks for adding this @wgtmac 🙌

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Fokko Fokko Fokko left review comments

@martin-g martin-g martin-g left review comments

@thiru-mg thiru-mg thiru-mg approved these changes

@mkmkme mkmkme mkmkme approved these changes

@mitjap mitjap Awaiting requested review from mitjap

Assignees
No one assigned
Labels
C++ Pull Requests for C++ binding
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@wgtmac @Fokko @martin-g @mitjap @mkmkme @thiru-mg