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

Gate CMake tests behind AU_ENABLE_TESTING option #303

Merged
merged 6 commits into from
Oct 21, 2024

Conversation

chiphogg
Copy link
Contributor

@chiphogg chiphogg commented Oct 20, 2024

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:

cmake \
  -S . \
  -B cmake/build \
  -DAU_ENABLE_TESTING=FALSE \
  -DCMAKE_VERIFY_INTERFACE_HEADER_SETS=TRUE \
&& cmake \
  --build cmake/build \
  --target all

Helps #257.

@chiphogg chiphogg marked this pull request as ready for review October 20, 2024 21:09
@chiphogg chiphogg requested a review from geoffviola October 20, 2024 21:09
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.

It looks like targets in au/code/au/CMakeLists.txt like chrono_policy_validation still depend on GTest::gtest. Is that expected?

I see an error when I run

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

@chiphogg
Copy link
Contributor Author

It looks like targets in au/code/au/CMakeLists.txt like chrono_policy_validation still depend on GTest::gtest. Is that expected?

I see an error when I run

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

Nice catch!

The current plan of record includes making two options: one for running the tests, and one for including the gtest dependency at all. In the end, if the latter is false, then we will force the former to be false as well.

This present PR just adds the first option. The main reason it exists is so that we can land @ericriff's contributions to main, and give him credit. But his PR predates the fuller understanding reflected in our plan of record: it conflates the two options, and assumes that users skipping the tests would also want to remove the GTest dependency. I fixed it up in the latest commit.

Of course, this reveals a hole in our CI testing: we didn't add tests for this new option. I generally prefer to do this in the same PR, but editing .github will add another reviewer to this PR, and I think it makes more sense to do this holistically once both options have been added. I filed #304 so we won't lose track of this.

@chiphogg chiphogg requested a review from geoffviola October 21, 2024 15:06
@chiphogg
Copy link
Contributor Author

(See the updated PR summary for a testing command.)

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.

LGTM.

Also, we probably don't need -DCMAKE_VERIFY_INTERFACE_HEADER_SETS=TRUE when we run cmake with -DAU_ENABLE_TESTING=FALSE.

@chiphogg chiphogg merged commit 4f4f1fc into main Oct 21, 2024
10 checks passed
@chiphogg chiphogg deleted the chiphogg/update_vars#257 branch December 2, 2024 18:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants