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

cmake: add USE_SYSTEM_* options #153

Merged
merged 1 commit into from
Jul 8, 2023

Conversation

ConnorBaker
Copy link
Contributor

@ConnorBaker ConnorBaker commented Jul 3, 2023

Similar to the work done in pytorch/pytorch#37137, this adds the following CMake options:

  • USE_SYSTEM_LIBS
  • USE_SYSTEM_GOOGLEBENCHMARK
  • USE_SYSTEM_GOOGLETEST

This is particularly useful in the context of Nix, where we can build these libraries once and then re-use them elsewhere to avoid rebuilding vendors dependencies.

@ConnorBaker
Copy link
Contributor Author

@malfet @Maratyszcza would either of you have the bandwidth to take a look at this?

@malfet
Copy link
Contributor

malfet commented Jul 6, 2023

@ConnorBaker Looks good to me, though I wonder if USE_SYSTEM_${LIBNANE} option is at all needed. I.e. what would be the realistic scenario, when one wants to use gtest from OS, but gmock from third_party folder.

CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
deps/clog/CMakeLists.txt Outdated Show resolved Hide resolved
@ConnorBaker
Copy link
Contributor Author

Thank you for the feedback! I've addressed comments and force-pushed the changes.

@malfet I think it makes sense to have USE_SYSTEM_LIBS to avoid needing to set multiple flags. With respect to the question about gmock: I thought gmock was brought in by find_package(GTest)? Or was this a question of how to support system gtest but third-party gmock?

@Maratyszcza are the names good now? Also (because I have multiple PRs open doing the same thing), would you recommend using that naming instead of GOOGLE_*?

@Maratyszcza
Copy link
Contributor

Maratyszcza commented Jul 10, 2023

@ConnorBaker I don't have a preference for GOOGLETEST vs GOOGLE_TEST, so long as the naming is consistent

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.

4 participants