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

Consolidate Au CMake targets #300

Merged
merged 4 commits into from
Oct 7, 2024
Merged

Conversation

chiphogg
Copy link
Contributor

@chiphogg chiphogg commented Oct 4, 2024

This breaks the 1:1 correspondence between target and test, so we split
up our helper accordingly into two.

We make au the single mega-target that holds everything that doesn't
depend on GTest. It turns out that there are two targets that do.
testing is for external users, and contains custom GMock matchers.
chrono_policy_validation is an internal-only library that makes it
easy to test that Au is like chrono where we want to be, and unlike
chrono otherwise. Each of these targets also depends on the au
mega-target.

Helps #257.

This breaks the 1:1 correspondence between target and test, so we split
up our helper accordingly into two.

We make `au` the single mega-target that holds everything that doesn't
depend on GTest.  It turns out that there are two targets that do.
`testing` is for external users, and contains custom GMock matchers.
`chrono_policy_validation` is an internal-only library that makes it
easy to test that Au is like chrono where we want to be, and unlike
chrono otherwise.  Each of these targets also depends on the `au`
mega-target.
@chiphogg chiphogg added the release notes: ⚙️ repo PR affecting the way the repository works label Oct 4, 2024
All the tests still passed locally from the previous build!  CMake is so
sloppy... I really miss bazel.
Tested locally with `au-docs-serve`
@chiphogg chiphogg marked this pull request as ready for review October 5, 2024 19:07
@chiphogg chiphogg requested a review from geoffviola October 5, 2024 19:07
@chiphogg
Copy link
Contributor Author

chiphogg commented Oct 5, 2024

Note to the reviewer:

This PR uses "checkpoint commits" to make it easier to verify correctness. Currently, there are 4. Start at the first commit, and use n and p to go to next and previous. Here is what the commits do:

  1. Bulk-update rules:
    1. Instead of header_only_library doing both the header and the test, we split it into two rules (by adding the new gtest_based_test), because tests and libraries aren't 1:1 anymore.
    2. We now have only three header_only_library instances: au (public, main); testing (public, optional); chrono_policy_validation (private).
    3. There are two aspects of this commit that make for easier diffing: (a) GTEST_SRCS is not renamed to SRCS yet, and (b) all of the HEADERS in au are on one single line, so that git isn't tempted to match them to their old rules.
    4. We get rid of GTEST_SRCS and GTEST_EXTRA_DEPS from the header-only libraries, because again, tests aren't 1:1 with library targets anymore.
    5. Every old header_only_library instance becomes a gtest_based_test
    6. _test gets appended to the name
    7. HEADERS goes away
    8. Old DEPS goes away
    9. New DEPS: delete any targets that went away, and add au (the new "mono-target") as a dep basically every time
  2. Do the rest of the change.
    1. The HEADERS go one per line
    2. GTEST_SRCS gets renamed to just SRCS
  3. Fix the breakage in the previous commit.
    1. Look. I ran all the tests before pushing. But CMake was so sloppy that it just reused the previously built executables! It's good that we're providing CMake support, but this just reinforces what a good thing it is that we're bazel-first.
  4. Update docs!

Copy link
Contributor

@geoffviola geoffviola left a comment

Choose a reason for hiding this comment

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

Looks good. I can see that the dependency on gmock has moved.

The following test still looks good.

docker run --rm \
ubuntu@sha256:2e863c44b718727c860746568e1d54afd13b2fa71b160f5cd9058fc436217b30 \
/bin/bash -c \
'apt update && apt install -y clang cmake git && git clone -b chiphogg/consolidate-cmake-libs#257 https://github.com/aurora-opensource/au.git && cd au && cmake -S . -B cmake/build -DCMAKE_VERIFY_INTERFACE_HEADER_SETS=TRUE && cmake --build cmake/build -j $(proc) --target all all_verify_interface_header_sets && ctest -j $(nproc) --test-dir cmake/build'

@chiphogg chiphogg merged commit fb03848 into main Oct 7, 2024
10 checks passed
@chiphogg chiphogg deleted the chiphogg/consolidate-cmake-libs#257 branch October 7, 2024 13:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release notes: ⚙️ repo PR affecting the way the repository works
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants