Add the first target and test #258
Conversation
`stdx` has no dependencies, so it's a good choice for our first target.
We make an `INTERFACE` target, using `FILE_SET HEADERS` for more modern
handling. By trial and error, it seems like the right move is to set
the `BASE_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:
- When we say `FILE_SET HEADERS`, we do not need to provide `TYPE`.
- Providing relative paths is correct, because they will be taken to be
relative to `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 target `all_verify_interface_header_sets`. This is how I
figured out what to set `BASE_DIRS` to.
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.)
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.
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. |
There was a problem hiding this comment.
The changes look good.
The tests run well with this
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 && cmake --build cmake/build --target all test'
Note that image uses cmake version 3.28.3.
CMAKE_VERIFY_INTERFACE_HEADER_SET didn't seem to work as expected.
I saw the following during the configure step.
CMake Warning:
Manually-specified variables were not used by the project:
CMAKE_VERIFY_INTERFACE_HEADER_SET
Then, I saw the following during the build/run step
gmake: *** No rule to make target 'all_verify_interface_header_sets'. Stop.
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 |
Well, I guess the docs might explain that:
If you replace If it does, then I'll make that change. |
I'm seeing the same issue. Here are the repro instructions. |
I tested this out by actually installing googletest system-wide. It turned out to be version 1.11. When I wiped the build folder and re-installed, there was still a `_deps` folder that had googletest in it. Then when I set the new version arg to 1.10, the install was a lot shorter and there was no `_deps`.
This improves the actually-working of the software.
This would appear to be a PEBKAC issue. (Both the |
|
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' |
stdxhas no dependencies, so it's a good choice for our first target.We make an
INTERFACEtarget, usingFILE_SET HEADERSfor more modernhandling. By trial and error, it seems like the right move is to set
the
BASE_DIRSto the main project folder. The following other choicesare 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.relative to
CMAKE_CURRENT_SOURCE_DIR.We also take advantage of section 32.6, "File Sets Header Verification".
The
CMAKE_VERIFY_INTERFACE_HEADER_SETvariable (which we also reflectin our docs) will automatically generate synthetic targets that make
sure we got our include paths correct. We can "run" this "test" by
building the target
all_verify_interface_header_sets. This is how Ifigured out what to set
BASE_DIRSto.On the googletest side, I followed the docs. We use
FetchContent, butfall back to
find_packagein 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_packagefallback, but we can probably defer thatuntil 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. Andpeople 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.