Closed chiphogg closed 4 months ago
Another consideration is conditionally enabling the GoogleTest dependency, as was done for the branch with the initial CMake support. I would like to defer adding that complexity until we encounter a concrete use case that's blocked by it, so I can be sure I understand why we would want to do that.
We could add if statements with BUILD_TESTING. It might be a good option to provide users an escape hatch to drop some dependencies and targets. But it would be even better to get everything to work. 😄 I agree that can be considered in a future PR.
On the googletest side, I followed the docs. We use FetchContent, but fall back to find_package in case there's a system-installed version. This latter behaviour seems highly suspect to me, but the docs were adamant that this is typically the right move, especially for open source projects. See Section 39.8 ("Recommended Practices"). It feels like it would be nice to at least set a minimum version that we would accept from the find_package fallback, but we can probably defer that until we get an actual problem report. (And, honestly, maybe there's simply no use case for this anyway? People who want to develop the tests can use bazel, which for Au supports a zero-install setup. And people who just want to depend on Au via CMake don't really need to run the tests.)
Yeah, in general Ubuntu and cmake users are pretty loose on the dependency version requirements. This enables fewer downloaded libraries. But there will be less reproducibility. Also, it requires more testing. gtest like all software is under development 😄
There is a PACKAGE_FIND_VERSION_MIN_MAJOR
parameter in find_package.
CMAKE_VERIFY_INTERFACE_HEADER_SET
didn't seem to work as expected.
Well, I guess the docs might explain that:
New in version 3.24.
If you replace 3.23
with 3.24
in the min version command, does that help it work as intended?
If it does, then I'll make that change.
If you replace 3.23 with 3.24 in the min version command, does that help it work as intended?
I'm seeing the same issue. Here are the repro instructions.
docker run --rm \
ubuntu@sha256:2e863c44b718727c860746568e1d54afd13b2fa71b160f5cd9058fc436217b30 \
/bin/bash -c \
'apt update && apt install -y clang cmake git && git clone -b chiphogg/stdx-cmake#215 https://github.com/aurora-opensource/au.git && cd au && sed -i 's/23/24/g' CMakeLists.txt && cat CMakeLists.txt && cmake -S . -B cmake/build -DCMAKE_VERIFY_INTERFACE_HEADER_SET=TRUE && cmake --build cmake/build --target all all_verify_interface_header_sets test'
If you replace 3.23 with 3.24 in the min version command, does that help it work as intended?
I'm seeing the same issue. Here are the repro instructions.
docker run --rm \ ubuntu@sha256:2e863c44b718727c860746568e1d54afd13b2fa71b160f5cd9058fc436217b30 \ /bin/bash -c \ 'apt update && apt install -y clang cmake git && git clone -b chiphogg/stdx-cmake#215 https://github.com/aurora-opensource/au.git && cd au && sed -i 's/23/24/g' CMakeLists.txt && cat CMakeLists.txt && cmake -S . -B cmake/build -DCMAKE_VERIFY_INTERFACE_HEADER_SET=TRUE && cmake --build cmake/build --target all all_verify_interface_header_sets test'
This would appear to be a PEBKAC issue. (Both the K
and C
belonged to me.) See 67738aa for the fix.
Yep, it works. 🎉
docker run --rm \
ubuntu@sha256:2e863c44b718727c860746568e1d54afd13b2fa71b160f5cd9058fc436217b30 \
/bin/bash -c \
'apt update && apt install -y clang cmake git && git clone -b chiphogg/stdx-cmake#215 https://github.com/aurora-opensource/au.git && cd au && cmake -S . -B cmake/build -DCMAKE_VERIFY_INTERFACE_HEADER_SETS=TRUE && cmake --build cmake/build --target all all_verify_interface_header_sets test'
stdx
has no dependencies, so it's a good choice for our first target. We make anINTERFACE
target, usingFILE_SET HEADERS
for more modern handling. By trial and error, it seems like the right move is to set theBASE_DIRS
to the main project folder. The following other choices are supported by reading the docs (i.e., Professional CMake, 18th Edition), specifically, section 16.2.7 on File Sets:FILE_SET HEADERS
, we do not need to provideTYPE
.CMAKE_CURRENT_SOURCE_DIR
.We also take advantage of section 32.6, "File Sets Header Verification". The
CMAKE_VERIFY_INTERFACE_HEADER_SET
variable (which we also reflect in our docs) will automatically generate synthetic targets that make sure we got our include paths correct. We can "run" this "test" by building the targetall_verify_interface_header_sets
. This is how I figured out what to setBASE_DIRS
to.On the googletest side, I followed the docs. We use
FetchContent
, but fall back tofind_package
in case there's a system-installed version. This latter behaviour seems highly suspect to me, but the docs were adamant that this is typically the right move, especially for open source projects. See Section 39.8 ("Recommended Practices"). It feels like it would be nice to at least set a minimum version that we would accept from thefind_package
fallback, but we can probably defer that until we get an actual problem report. (And, honestly, maybe there's simply no use case for this anyway? People who want to develop the tests can usebazel
, which for Au supports a zero-install setup. And people who just want to depend on Au via CMake don't really need to run the tests.)Another consideration is conditionally enabling the GoogleTest dependency, as was done for the branch with the initial CMake support. I would like to defer adding that complexity until we encounter a concrete use case that's blocked by it, so I can be sure I understand why we would want to do that.
It turns out that we can "build-and-run-all-tests, incrementally" with a single CMake command that builds three named targets: all, "all header verifications", and test. I updated the instructions to include this.
Finally, it appears that running the tests can sometimes create a folder called
Testing
, so I added that to.gitignore
.Future PRs will add a bunch more targets.
Helps #215.