Make building unit tests optional to simplify packaging #297
Conversation
|
|
ericriff
force-pushed
the
eriff/make-building-tests-optional
branch
from
e49062a
to
b128265
Compare
|
Thanks for making this PR! Conan support has been a longstanding wishlist item of mine, and it's really exciting to see folks from the community stepping up to help make it happen. 🚀 As I mentioned in the conversation for #215, we do have one target that legitimately depends on GTest. Not sure how we want to handle that here. As I also mentioned there, since we don't use CMake/conan on a day to day basis, there's a high prior probability of us just making weird choices out of ignorance. 🙂 LMK your thoughts on some of our best options. I hope I'll get a chance to look more closely at this either later today or sometime in the next couple days. |
There was a problem hiding this comment.
This looks good! I think this PR makes sense as the "add the first of two options" PR.
There is a merge conflict, because we've already landed the project CMake restructuring PR (#300). In order to make it as easy as possible to land this present PR, I made my own PR into your PR's branch: ericriff#1. My PR both resolves the merge conflict, and addresses my sole review comment. The instructions in the summary indicate how to use it. I believe that if you merge ericriff#1, we'll be able to land #297 basically as-is (although I admit I've never tried this strategy before).
After that, I'll put up a PR to add the second CMake option, which will let users opt out of all CMake dependencies. And then, I believe Au's CMake setup should be able to support a conan recipe, without any patches!
| @@ -32,28 +32,37 @@ project( | |||
| LANGUAGES CXX | |||
| ) | |||
|
|
|||
| option(ENABLE_TESTING "Build Unit Tests" ON) | |||
There was a problem hiding this comment.
Nit: let's add the AU_ prefix here. Note that I did this in ericriff#1, which you're welcome to merge into this PR according to the instructions in the summary. 🙂
| @@ -75,7 +75,7 @@ function(header_only_library) | |||
| ) | |||
|
|
|||
| # Add the test, if requested. | |||
| if (DEFINED ARG_GTEST_SRCS) | |||
| if (DEFINED ARG_GTEST_SRCS AND ENABLE_TESTING) | |||
There was a problem hiding this comment.
This line has a merge conflict with the new setup on main, because we broke this function apart into two separate functions: one for libraries, one for tests. Accordingly, there's no such thing as ARG_GTEST_SRCS anymore.
ericriff#1 resolves this merge conflict, by simply adding an early return when this variable is not true.
This brings in the external contributions from #297, merges in the latest main, and prepends an `AU_` prefix to the option. A future PR will add another option to let users opt out of the GTest dependency entirely, losing access to the `Au::testing` target (which wouldn't be any loss to people who don't use GTest). To test: ```sh cmake \ -S . \ -B cmake/build \ -DAU_ENABLE_TESTING=FALSE \ -DCMAKE_VERIFY_INTERFACE_HEADER_SETS=TRUE \ && cmake \ --build cmake/build \ --target all ``` Helps #257. --------- Co-authored-by: Riff, Eric <[email protected]>
I intent to package this library with Conan.
I don't want the conan package to depend on GTest, so instead of patching it out on the conan recipe I decided to propose this here instead.
I maintained the tests enabled by default, but a user can opt out of them with
-DENABLE_TESTING:BOOL=OFF.I don't think that gtest has any business on the installed targets. I disabled the
find_package()call on the generated config file if we're building unit tests, but there are other references to gtest on the generated files. That's unusual. I think GTest should be treated as an internal tool, used for testing, and do not get propagated downstream.-Eric