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

FetchContent Improvements #373

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

Conversation

milianw
Copy link
Contributor

@milianw milianw commented Sep 29, 2023

Summary

This patch series makes it possible to consume more library from the system, by leveraging the new FIND_PACKAGE_ARGS facility for FetchContent.

With the way msgpack is now included, I can now finally compile this repo without any extra patches.

Test plan

It compiles :)

Fixes: #359

This allows us to use FIND_PACKAGE_ARGS for FetchContent with
the SYSTEM argument.
Warnings in thirdparty dependencies are not very interesting for
the development of an application. By setting the SYSTEM arg we
will include the dependency header via `-isystem` instead.
Straightforward adaption to leverage `find_package` on those systems
that have a tomlplusplus that's new enough for our purposes.
This is also relatively straight forward, except for the fact
that we don't get the glog unittest targets when the dependency
is found. Thus the changeset is slightly larger than one might
expect otherwise.
The version bump was done so that I can test it with the package
I have available on my system. Previously, the embedded msgpack
code didn't compile there. Now I simply can use the system provided
msgpack here which is much better.
This code is copied from [1] which is Apache-2.0 licensed and
owned by Meta, so I hope this is fine licensing wise.

[1]: https://github.com/facebookarchive/caffe2/blob/157ff81a12cf48eb4b0a2ad854c4ae205ad8aa07/cmake/Modules/FindRocksDB.cmake
Due to the custom build system integration for RocksDB we cannot
simply use FIND_PACKAGE_ARGS. Instead, we first do a `find_package`
and only if that fails do we fallback to the old rocksdb integration.

To better facilitate this, I took the liberty to also modernize
the CMake code a bit. Consumers of the lib just need to link
against the new `rocksdb::librocksdb` alias target to get the
required library and include directory.
Copy link
Contributor

@JakeHillion JakeHillion left a comment

Choose a reason for hiding this comment

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

First comments: looks good to me except for CMake 3.25. Arch and a few other distros have it, but the CentOS we use internally and the Ubuntu used in the CI don't. We have an issue tracking this as the SYSTEM parameter should definitely be added when it's more widespread (#114).

Appreciate the efforts! This doesn't build on my work machine with the version put back to 3.20 as is (mostly our fault) so I need a bit of time to get to the bottom of that.

@milianw
Copy link
Contributor Author

milianw commented Sep 30, 2023

@JakeHillion while SYSTEM requires CMake 3.25, removing that won't bring a substantial advantaged since FIND_PACKAGE_ARGS requires CMake 3.24 - see https://cmake.org/cmake/help/latest/module/FetchContent.html From what I gather, that version is still too new for you?

If so, we need to take a different approach, one more akin to what I did for RocksDB in 5a6ef38 - Are you OK with that?

@JakeHillion
Copy link
Contributor

Sorry for not picking up on that. The approach taken with RocksDB seems good (if !find_package then fetchcontent). Could you also create an issue for changing this to FIND_PACKAGE_ARGS when 3.24 is widely available, and link to it from a comment in the CMake in each case? Then hopefully we'll remember to switch. Thanks again for cleaning this up and making it easier to build OI externally.

Copy link
Contributor

@JakeHillion JakeHillion left a comment

Choose a reason for hiding this comment

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

Thanks for looking into this. if not find_package(...) then fetch_content(...) seems a good approach to me and should keep us compatible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

issues with CMake dependency handling
3 participants