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

Add CMake target for every remaining library #263

Merged
merged 3 commits into from
Jul 11, 2024

Conversation

chiphogg
Copy link
Contributor

This should, in principle, enable a full Au installation via CMake!

I took a more expansive view of which libraries are "for public
consumption" relative to bazel. The old division was based on a strong
expectation that users would always want to include one or at most a
very few files, before we fully appreciated the benefits of file-by-file
inclusion for build speed. I'll bring bazel in line in a follow up PR.

In making these CMake targets, I discovered two small irregularities in
their bazel counterparts --- namely, a missing dependency, and a bizarre
copts line that I think came from a stray default vim snippet. I
fixed them in this same PR.

Helps #215. In order to resolve it fully, we'll need to tidy up our
documentation posture w.r.t. CMake.

This should, in principle, enable a full Au installation via CMake!

I took a more expansive view of which libraries are "for public
consumption" relative to bazel.  The old division was based on a strong
expectation that users would always want to include one or at most a
very few files, before we fully appreciated the benefits of file-by-file
inclusion for build speed.  I'll bring bazel in line in a follow up PR.

In making these CMake targets, I discovered two small irregularities in
their bazel counterparts --- namely, a missing dependency, and a bizarre
`copts` line that I think came from a stray default vim snippet.  I
fixed them in this same PR.

Helps #215.  In order to resolve it fully, we'll need to tidy up our
documentation posture w.r.t. CMake.
@chiphogg chiphogg added the release notes: ⚙️ repo PR affecting the way the repository works label Jul 10, 2024
@chiphogg chiphogg requested a review from geoffviola July 10, 2024 21:33
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.

Great!

I tested it with this command

docker run --rm \
ubuntu@sha256:2e863c44b718727c860746568e1d54afd13b2fa71b160f5cd9058fc436217b30 \
/bin/bash -c \
'apt update && apt install -y clang cmake git && git clone -b chiphogg/cmake-other-libs#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 -j $(proc) --target all all_verify_interface_header_sets && ctest -j $(nproc) --test-dir cmake/build'

As a side note, omitting test from cmake --build cmake/build -j $(proc) --target all all_verify_interface_header_sets means that the tests don't run. For whatever reason, -j works on the build step, but not the test step. Running ctest alone allows the tests to run in parallel.

quantity_point
unit_of_measure
unit_symbol
GTEST_SRCS
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
GTEST_SRCS
GTEST_SRCS
prefix_test.cc
GTEST_EXTRA_DEPS

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice catch 😅

#

header_only_library(
NAME au
Copy link
Contributor

Choose a reason for hiding this comment

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

I found it a little weird that the indentation is 3 spaces. It's not actually a problem, but I'm more familiar with 2 spaces. At some point, we may want to use a formatter like https://github.com/cheshirekow/cmake_format.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For some reason, 3 spaces was what my editor chose. Super weird! Fixed by switching to 2.

@geoffviola
Copy link
Contributor

Did we want to support no_wconversion_test.cc in CMake?

@chiphogg
Copy link
Contributor Author

Did we want to support no_wconversion_test.cc in CMake?

Sorry, I should have mentioned: I decided to skip that one, as well as error_examples.cc. I think we get all the value we need from those out of bazel. If I'm wrong about that, we can add them later.

chiphogg added 2 commits July 11, 2024 12:53
Not sure why it was 3 before, but that was a weird value.
@chiphogg chiphogg merged commit fc050fa into main Jul 11, 2024
10 checks passed
@chiphogg chiphogg deleted the chiphogg/cmake-other-libs#215 branch July 11, 2024 17:10
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