Rework folder structure and cmake files

[ericriff]

This is a rough PR to illustrate what I meant here #215 (comment)

The diff is huge because I moved pretty much every source file to follow a more conventional layout.

1. All headers are now on include/*
2. All tests are now on tests/*

Two options were added to the top level CMakeLists:

1. AU_BUILD_TESTS: Allows the consumer to disable building tests which greatly improves build times when au is a subproject / being packaged. This also allows the consumer to completely ditch the dependency on GTest if wanted (E.g. for packaging).
2. AU_USE_VENDORED_GTEST: Allows the consumer to use a version of GTest already present on the system. This not only makes this lib more flexible, but it greatly speeds up rebuilding from scratch.

An alias library Au::au was added. This is a CMake best practice and provides uniform usability of this library in all scenarios. Without it a project that includes au as a submodule would have to link to the au target whereas the projects that include FetchContent or a local install would link to the Au::au target. Now on every scenario, the consumer links to Au::au.

I've tested both scenarios:

Local install

A developer build au from sources and installs it

cmake -S . -B _build -DAU_USE_VENDORED_GTEST:BOOL=OFF
cmake --build _build -j16
cmake --install _build --prefix _install

In this case I've used the system gtest, all tests passed

ctest --test-dir _build

Then another project can consume it by just pointing to the install dir (if a --prefix was used, it is not needed for system libs)

cmake_minimum_required(VERSION 3.20)
project(au-consumer LANGUAGES CXX)
find_package(Au REQUIRED CONFIG)
add_executable(au-consumer test.cpp)
target_link_libraries(au-consumer PRIVATE Au::au)
target_compile_features(au-consumer PRIVATE cxx_std_14)

built with

cmake -S . -B _build -DCMAKE_PREFIX_PATH=/home/eriff/repos/au/_install
cmake --build _build

Using a submodule

Alternatively some project could include au as a submodule. The CMake for this project just replaces the find_package call with add_subdirectory(). In this case I've completely disabled the unit tests to illustrate a real world scenario

cmake_minimum_required(VERSION 3.20)
project(au-consumer LANGUAGES CXX)\

set(AU_BUILD_TESTS OFF)

add_subdirectory(au-vendored)
add_executable(au-consumer test_package.cpp)
target_link_libraries(au-consumer PRIVATE Au::au)
target_compile_features(au-consumer PRIVATE cxx_std_14)

build with

cmake -S . -B _build
cmake --build _build

Disclaimer

Once again -- this is a rough, illustrative PR. I'm sure it broke the whole bazel build since I didn't even try to make it build there. I'm not familiar with it.

[chiphogg]

Wow --- thanks for taking the time to share your ideas in a form that's precise enough to execute! 🙂 Yes, as predicted, all the builds broken, but that's not a problem as I understand that this PR is intended as a communication tool.

I had some feedback on some of the parts, but I've run out of time/energy for tonight. I'll plan to both give that feedback later, and use this PR to help me out in playing around to make the structure more Conan-friendly.

And thanks for including detailed test plans in your PR description, which will surely be invaluable with those tasks!

[chiphogg]

OK, thanks for your patience. 🙂 It's a really exciting time at Aurora right now, so I haven't had much time/energy on weeknights to respond. But now let me provide the promised feedback for these ideas.

First, a general remark, not related to the summary. As I confessed earlier, I'm extremely inexperienced at using CMake, so my knowledge of best practices is quite weak. That said, what I did learn, I learned by studying the CMake 3.29 version (i.e., 19th Edition) of Professional CMake, by Craig Scott. So a good mental model for what to expect from Au's CMake is something like "cutting edge best practices, naively applied". 😁

I'll go into some specific examples below, wherever it's relevant to things you mentioned in the summary. One example that isn't in the summary is the FILE_SET usage, which this PR would remove. I see that you're using target_include_directories() instead, which we don't currently have anywhere. Although I'm hard pressed to explain exactly why it's better, I've heard that FILE_SET is more the modern approach, and it replaces any need for target_include_directories() (see this comment from Craig Scott). So I think we probably want to keep FILE_SET as-is.

Now, on to the summary!

The diff is huge because I moved pretty much every source file to follow a more conventional layout.

1. All headers are now on include/*
2. All tests are now on tests/*

I'd prefer to keep the layout the same. I'll go into some detail explaining why.

My ideal way to construct a library is to compose it out of individual, heavily unit tested library targets. I want each target to be a cohesive "unit", including the test: it should be as easy as possible to jump back and forth among the .hh, the .cc (where applicable), and the _test.cc files. The best way to make this easy is to put the files right next to each other. This also makes it prominently visible whether a file has a test.

When libraries get installed to the system, of course, those three components get scattered to the winds. .hh files go in an include/ folder, .cc files get compiled and the object files go in a library folder, and test files are simply omitted. That's fine --- the way we define the CMake targets and install commands makes it easy for each file to get where it needs to go. That's CMake's job.

By contrast, I've always found developing in the more "traditional" layout extremely confusing. After all, recall that I'm going in there with the mindset of looking for the individual pieces of a unified-whole library target... and I find myself having to hunt through different folder hierarchies to get them. Worse, because it's so hard to find "the corresponding" unit test for a target, there often isn't one! Each test file may cover many targets. This means that (a) when I sit down to write a test, I have no idea where to put it, and (b) it's far too easy for a completely untested library to hide. For (b), yes, coverage tools can help and are needed anyway, but I think it's ideally "both/and" rather than "either/or".

mp-units is a good example of where I've felt this way. It's a very well written library that has excellent test coverage. But the test files are far from the headers, and most headers don't have one corresponding test flie, so I find there's a lot of friction every time I sit down to write a test. Even if I simply pick a place to put the test, I can't be confident that the next person (including "me 6 months later") would pick the same test file to put a similar test. This makes me worry that logically similar tests would get diffused throughout the test folder over time.

A couple of side remarks:

• Obviously, it's fine, and even very good to have test files that aren't 1:1 with individual targets: we need these higher level integration and/or end-to-end tests. But the converse is not true: I never want to see a library target that doesn't have a test file.
• Also noting that Au is not perfect in this regard. 🙂 We do have some files in separate test/ folders, in cases where we're using folders and globs for targets. Maybe we shouldn't be using globs 😅 --- although they're far less problematic for bazel than for CMake. However, even in this case, every individual header does have a corresponding test file, AFAIK.

This is pure speculation, but I suspect that we can trace the evolution of the traditional layout to a time when it wasn't as easy for something like CMake to distribute headers, object files, and tests automatically --- a time when automated tests were thought of as "nice-to-have", or in many cases not thought of at all. By contrast, monorepo projects (such as the one that birthed Au) never had a separate "install" step, and generally grew up in an era where automated testing and high coverage were simply assumed. In this context, it seems very natural to me to group all the files for a target in one place (including the test file).

I hope this lengthy section doesn't come across as a rant. 🙂 I simply felt that, if I'm departing from established common practice, it behooves me to explain why.

Two options were added to the top level CMakeLists:

1. AU_BUILD_TESTS: Allows the consumer to disable building tests which greatly improves build times when au is a subproject / being packaged. This also allows the consumer to completely ditch the dependency on GTest if wanted (E.g. for packaging).
2. AU_USE_VENDORED_GTEST: Allows the consumer to use a version of GTest already present on the system. This not only makes this lib more flexible, but it greatly speeds up rebuilding from scratch.

Yes, It does feel like 2 new options is the right amount. One should be to disable building the tests. The other should be to opt out of taking a googletest dependency at all, thereby losing access to the Au::testing library target.

That said, I don't think we actually need AU_USE_VENDORED_GTEST. According to Section 39.5.1 ("Try find_package() Before FetchContent") of the Professional CMake book: adding FIND_PACKAGE_ARGS to a FetchContent_Declare command tells CMake that it's OK to use find_package() to satisfy the dependency, and in fact, by default, this is the first thing that CMake should try. If the user has a satisfactory googletest version installed, but Au uses FetchContent for googletest instead, it seems to me that this would be a bug. Did you see this happening earlier?

An alias library Au::au was added. This is a CMake best practice and provides uniform usability of this library in all scenarios. Without it a project that includes au as a submodule would have to link to the au target whereas the projects that include FetchContent or a local install would link to the Au::au target. Now on every scenario, the consumer links to Au::au.

I believe we already do this; see:

au/cmake/HeaderOnlyLibrary.cmake

Line 65 in 030298c

add_library(Au::${ARG_NAME} ALIAS ${ARG_NAME})

I've tested both scenarios:

(...@chiphogg snipped contents because they're available in the summary...)

Awesome! Having these concrete test cases, which we can adapt to whatever solution we end up with, is a huge boon --- much appreciated.

Disclaimer

Once again -- this is a rough, illustrative PR. I'm sure it broke the whole bazel build since I didn't even try to make it build there. I'm not familiar with it.

Thank you for putting in all this effort to explain what you were saying! Let's take stock of where we're at, and where we hope to end up. In order to support conan, here are the steps I see:

1. Tweak the library structure.
   1. That's the main job that this present PR Rework folder structure and cmake files #298 was doing, but Rework folder structure and cmake files #298 is discussion-only.
   2. I created Consolidate Au CMake targets #300 to do this, and just now took it out of draft.
2. Add the GTest-related variables.
   1. I'm planning to use Make building unit tests optional to simplify packaging #297 for this, mainly because it's a great start, and because I want you to formally get credit by showing up in the git logs for this project. 🙂 I may end up pushing some commits to that branch before landing, if that turns out to be the most natural approach.
   2. Roughly, I think the plan will be to get Consolidate Au CMake targets #300 fixed up and landed; then, merge the new main into Make building unit tests optional to simplify packaging #297; then, figure out exactly what CMake options we should add and how they should behave.

LMK what you think of this plan, and thanks again for helping make Au more broadly accessible!

[chiphogg]

I wanted to update my thoughts here, because I ran across this YouTube video:

CppCon 2019: Robert Schumacher “Don't Package Your Libraries, Write Packagable Libraries! (Part 2)”

Above, I argued that modern CMake obviates the need to separate headers, source files, and tests in a project's structure, for purposes of making it easy to install and/or deploy. After all, defining CMake targets and their options is how we define where the files should go.

I think that's probably still true, if we're using modern CMake. But what the above talk made me appreciate is that CMake isn't all that's out there --- there may be other, private build systems that I've never even heard of. If we structure our library according to how it's likely to be installed on any system, then we maximize the number of build systems that people can actually use with our library.

I still think it's better on balance to keep tests integrated with their code, but I wanted to record for posterity this compelling counter-argument that I had encountered for the first time.