-
Notifications
You must be signed in to change notification settings - Fork 18
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
Implement Use CMake Header File Sets #30
Conversation
Request review from @bretbrownjr . |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR! I'm excited about the use of VERIFY_INTERFACE_HEADER_SETS
as that'll give us one more reason to avoid making static libraries when the code is header only (+CC @DeveloperPaul123)
It looks like gmock and gtest are being installed in addition to beman.exemplar (which they shouldn't). I observed a bunch of output like this in the ubuntu-gcc-werror output:
#18 [12/13] RUN cmake --install build --prefix /opt/beman.exemplar
#18 0.047 -- Install configuration: ""
#18 0.048 -- Installing: /opt/beman.exemplar/include
#18 0.048 -- Installing: /opt/beman.exemplar/include/gmock
#18 0.048 -- Installing: /opt/beman.exemplar/include/gmock/gmock-cardinalities.h
#18 0.049 -- Installing: /opt/beman.exemplar/include/gmock/gmock-matchers.h
#18 0.049 -- Installing: /opt/beman.exemplar/include/gmock/gmock-spec-builders.h
#18 0.050 -- Installing: /opt/beman.exemplar/include/gmock/gmock.h
Additionally, it looks like the header is being installed in lib/
instead of include/
:
#18 0.072 -- Installing: /opt/beman.exemplar/lib/beman/exemplar/identity.hpp
I think the way to fix this is to remove the |
I was gonna ask about this as well, I think this behavior exist prior to this PR. e. g. See: https://github.com/beman-project/exemplar/actions/runs/11016758495/job/30593042124#step:6:1 line ~400 |
I don't understand why this is happening. When we call |
What do you mean by "FILE_PATTERN"?
Not that I know of.
I don't think this change is user-visible. |
For FILE_PATTERN, I was trying to see if I can do away with defining the absolute path to the
Thanks for the feedback. |
Note that this is only the case in CMake |
3.29 docs for |
Ehhhh, this doesn't seem to be in the scope of this PR, I am not familiar with cmake so sorry I don't have anything to contribute for this. Does the rest of the pr look good? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good PR. I'm tempted to land it, as is but it's probably worth being a little picky on an "examplar" project that will be used for generating Beman projects going forward.
src/beman/exemplar/CMakeLists.txt
Outdated
DESTINATION ${CMAKE_INSTALL_LIBDIR}) | ||
|
||
install( | ||
DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}/../../../include/ | ||
DESTINATION ${CMAKE_INSTALL_INCLUDEDIR} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The CMake 3.24 docs seem to indicate we can leave off DESTINATION
because FILE_SET with the TYPE HEADERS will implicitly be installed into the INCLUDEDIR DESTINATION:
https://cmake.org/cmake/help/v3.24/command/install.html
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will test again tomorrow morning with this line removed, but I am pretty sure when I run CMake 3.26 locally without the destination argument it faults at install?
Edit: tested again, did not fault, I were wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I don't think removing this is correct. cmake --install
no longer output the identity.hpp
file, see latest CI run: https://github.com/beman-project/exemplar/actions/runs/11034273081/job/30647406013?pr=30#step:6:445 vs previous CI run: https://github.com/beman-project/exemplar/actions/runs/11024257637/job/30617095477#step:6:382 (Missing #18 0.073 -- Installing: /opt/beman.exemplar/include/beman/exemplar/identity.hpp
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem is that DESTINATION
is being applied to everything being installed. To make it apply only to the file set, it must come after FILE_SET HEADERS
. I tried this and it worked on cmake 3.30.1.
From the CMake docs:
The first
<artifact-option>...
group applies to target
:ref:Output Artifacts
that do not have a dedicated group specified
later in the same call.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👌 would update later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, right. Good catch @camio. It helps to indent fields like DESTINATION
so they are clearly "under" the FILE_SET
it's modifying. Or you can split the install()
command up into N more specific ones.
But I still swear that FILE_SET
should imply INCLUDEDIR
by default. Maybe @mathstuf can assist.
To be clear, I'm OK landing this PR with DESTINATION and following up on this detail later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, could @camio or @bretbrownjr provide more info on what this fixes? cmake --install ...
still installs gtest
aside from the project headers. Is this intended for having multiple source-file groups? Just curious.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, could @camio or @bretbrownjr provide more info on what [moving DESTINATION below FILE_SET] fixes?
It fixes the issue where libbeman.exemplar.a
was being placed in include/
instead of lib/
.
@@ -30,5 +30,6 @@ ENV CC="$cc" CXX="$cxx" CMAKE_GENERATOR="Ninja" CMAKE_EXPORT_COMPILE_COMMANDS=on | |||
RUN ls -lR src | |||
RUN cmake -B build -S . "$cmake_args" | |||
RUN cmake --build build --verbose | |||
RUN cmake --build build --target all_verify_interface_header_sets |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a big deal, but is the --verbose
flag worth adding for the all_verify_interface_header_sets
command? If we see a compiler error in a CI log, do we see enough detail to identify the relevant compilation commands?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure I can add the verbose flag. Logs are free.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#6 tries to remove the Dockerfiles. Maybe we should decide if we'll do that or not, as it may influence current PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have not checked failure case but I don't think verbose provide any useful information, should we still add the verbose flag?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can work on this later too but can you help me produce a case where the all_verify_interface_header_sets
fails? I can go check the log and see if verbose provide more information.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For instance, I would expect the following to work in a normally-formed CMake library but fail an all_verify_interface_header_sets
check.
// @file something.hpp
struct something::Widget {
// BUG: Uses std::vector but doesn't include <vector>
std::vector vec;
};
// @file something.cpp
#include <vector>
// something.hpp as expanded here gets vector defined for "free"
// by the preceeding #include, so something.cpp compiles fine
#include <something.hpp>
namespace {
size_t something::foobar(something::Widget const& widget) {
return widget.vec.size();
}
} // unnamed namespace
I just want to make sure the person looking at a failure log can tell:
- That
something.hpp
is being checked with a specific compile command- (Something about building a temporary like
$some_build_dir/something.hpp.cpp
probably)
- (Something about building a temporary like
- What the compile command is, because flags like header search paths can be relevant
- Exactly what the problem is
If we get that with --verbose
off, I'm happy. We can land a shorter log and everyone wins.
If we don't get it regardless of whether --verbose
is on or off, let's make an upstream CMake issue. I wouldn't expect tweaking this would be tricky or expensive for them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without verbose Flag
⋊> ~/D/test-cmake cmake --build build --target verify_interface_header_sets
[ 40%] Built target something_lib
[ 60%] Generating verify_something.hpp.cpp
[ 60%] Generating verify_something.hpp.cpp
[ 80%] Building CXX object CMakeFiles/verify_something.dir/verify_something.hpp.cpp.o
cc1plus: fatal error: /home/bradwu/Desktop/test-cmake/build/verify_something.hpp.cpp: No such file or directory
compilation terminated.
gmake[3]: *** [CMakeFiles/verify_something.dir/build.make:80: CMakeFiles/verify_something.dir/verify_something.hpp.cpp.o] Error 1
gmake[2]: *** [CMakeFiles/Makefile2:113: CMakeFiles/verify_something.dir/all] Error 2
gmake[1]: *** [CMakeFiles/Makefile2:146: CMakeFiles/verify_interface_header_sets.dir/rule] Error 2
gmake: *** [Makefile:150: verify_interface_header_sets] Error 2
With Verbose Flag
⋊> ~/D/test-cmake cmake --build build --target verify_interface_header_sets --verbose
/home/linuxbrew/.linuxbrew/Cellar/cmake/3.26.3/bin/cmake -S/home/bradwu/Desktop/test-cmake -B/home/bradwu/Desktop/test-cmake/build --check-build-system CMakeFiles/Makefile.cmake 0
/usr/bin/gmake -f CMakeFiles/Makefile2 verify_interface_header_sets
gmake[1]: Entering directory '/home/bradwu/Desktop/test-cmake/build'
/home/linuxbrew/.linuxbrew/Cellar/cmake/3.26.3/bin/cmake -S/home/bradwu/Desktop/test-cmake -B/home/bradwu/Desktop/test-cmake/build --check-build-system CMakeFiles/Makefile.cmake 0
/home/linuxbrew/.linuxbrew/Cellar/cmake/3.26.3/bin/cmake -E cmake_progress_start /home/bradwu/Desktop/test-cmake/build/CMakeFiles 5
/usr/bin/gmake -f CMakeFiles/Makefile2 CMakeFiles/verify_interface_header_sets.dir/all
gmake[2]: Entering directory '/home/bradwu/Desktop/test-cmake/build'
/usr/bin/gmake -f CMakeFiles/something_lib.dir/build.make CMakeFiles/something_lib.dir/depend
gmake[3]: Entering directory '/home/bradwu/Desktop/test-cmake/build'
cd /home/bradwu/Desktop/test-cmake/build && /home/linuxbrew/.linuxbrew/Cellar/cmake/3.26.3/bin/cmake -E cmake_depends "Unix Makefiles" /home/bradwu/Desktop/test-cmake /home/bradwu/Desktop/test-cmake /home/bradwu/Desktop/test-cmake/build /home/bradwu/Desktop/test-cmake/build /home/bradwu/Desktop/test-cmake/build/CMakeFiles/something_lib.dir/DependInfo.cmake --color=
gmake[3]: Leaving directory '/home/bradwu/Desktop/test-cmake/build'
/usr/bin/gmake -f CMakeFiles/something_lib.dir/build.make CMakeFiles/something_lib.dir/build
gmake[3]: Entering directory '/home/bradwu/Desktop/test-cmake/build'
gmake[3]: Nothing to be done for 'CMakeFiles/something_lib.dir/build'.
gmake[3]: Leaving directory '/home/bradwu/Desktop/test-cmake/build'
[ 40%] Built target something_lib
/usr/bin/gmake -f CMakeFiles/verify_something.dir/build.make CMakeFiles/verify_something.dir/depend
gmake[3]: Entering directory '/home/bradwu/Desktop/test-cmake/build'
[ 60%] Generating verify_something.hpp.cpp
/home/linuxbrew/.linuxbrew/Cellar/cmake/3.26.3/bin/cmake -E echo #include\ <something.hpp> > /home/bradwu/Desktop/test-cmake/build/verify_something.hpp.cpp
cd /home/bradwu/Desktop/test-cmake/build && /home/linuxbrew/.linuxbrew/Cellar/cmake/3.26.3/bin/cmake -E cmake_depends "Unix Makefiles" /home/bradwu/Desktop/test-cmake /home/bradwu/Desktop/test-cmake /home/bradwu/Desktop/test-cmake/build /home/bradwu/Desktop/test-cmake/build /home/bradwu/Desktop/test-cmake/build/CMakeFiles/verify_something.dir/DependInfo.cmake --color=
gmake[3]: Leaving directory '/home/bradwu/Desktop/test-cmake/build'
/usr/bin/gmake -f CMakeFiles/verify_something.dir/build.make CMakeFiles/verify_something.dir/build
gmake[3]: Entering directory '/home/bradwu/Desktop/test-cmake/build'
[ 60%] Generating verify_something.hpp.cpp
/home/linuxbrew/.linuxbrew/Cellar/cmake/3.26.3/bin/cmake -E echo #include\ <something.hpp> > /home/bradwu/Desktop/test-cmake/build/verify_something.hpp.cpp
[ 80%] Building CXX object CMakeFiles/verify_something.dir/verify_something.hpp.cpp.o
/usr/bin/c++ -I/home/bradwu/Desktop/test-cmake -std=gnu++17 -MD -MT CMakeFiles/verify_something.dir/verify_something.hpp.cpp.o -MF CMakeFiles/verify_something.dir/verify_something.hpp.cpp.o.d -o CMakeFiles/verify_something.dir/verify_something.hpp.cpp.o -c /home/bradwu/Desktop/test-cmake/build/verify_something.hpp.cpp
cc1plus: fatal error: /home/bradwu/Desktop/test-cmake/build/verify_something.hpp.cpp: No such file or directory
compilation terminated.
gmake[3]: *** [CMakeFiles/verify_something.dir/build.make:80: CMakeFiles/verify_something.dir/verify_something.hpp.cpp.o] Error 1
gmake[3]: Leaving directory '/home/bradwu/Desktop/test-cmake/build'
gmake[2]: *** [CMakeFiles/Makefile2:113: CMakeFiles/verify_something.dir/all] Error 2
gmake[2]: Leaving directory '/home/bradwu/Desktop/test-cmake/build'
gmake[1]: *** [CMakeFiles/Makefile2:146: CMakeFiles/verify_interface_header_sets.dir/rule] Error 2
gmake[1]: Leaving directory '/home/bradwu/Desktop/test-cmake/build'
gmake: *** [Makefile:150: verify_interface_header_sets] Error 2
Verbose doesn't provide more useful information, and honestly the error log is pretty bad. The error fails because it cannot find the verify_something.hpp.cpp
, even with verbose on nothing indicate the reason for failure to generate this file.
Tested on cmake version 3.26.3
.
Can someone test this on newer cmake
version? We should file an issue upstream if no more helpful error message is generated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested on cmake version 3.30.3
, same output, I will file a issue upstream, let's omit the verbose flag.
version 3.30.3 with verbose log
⋊> ~/D/test-cmake cmake --build build --verbose (base) 13:45:00
Change Dir: '/home/bradwu/Desktop/test-cmake/build'
Run Build Command(s): /home/linuxbrew/.linuxbrew/Cellar/cmake/3.30.3/bin/cmake -E env VERBOSE=1 /usr/bin/gmake -f Makefile
/home/linuxbrew/.linuxbrew/Cellar/cmake/3.30.3/bin/cmake -S/home/bradwu/Desktop/test-cmake -B/home/bradwu/Desktop/test-cmake/build --check-build-system CMakeFiles/Makefile.cmake 0
/home/linuxbrew/.linuxbrew/Cellar/cmake/3.30.3/bin/cmake -E cmake_progress_start /home/bradwu/Desktop/test-cmake/build/CMakeFiles /home/bradwu/Desktop/test-cmake/build//CMakeFiles/progress.marks
/usr/bin/gmake -f CMakeFiles/Makefile2 all
gmake[1]: Entering directory '/home/bradwu/Desktop/test-cmake/build'
/usr/bin/gmake -f CMakeFiles/something_lib.dir/build.make CMakeFiles/something_lib.dir/depend
gmake[2]: Entering directory '/home/bradwu/Desktop/test-cmake/build'
cd /home/bradwu/Desktop/test-cmake/build && /home/linuxbrew/.linuxbrew/Cellar/cmake/3.30.3/bin/cmake -E cmake_depends "Unix Makefiles" /home/bradwu/Desktop/test-cmake /home/bradwu/Desktop/test-cmake /home/bradwu/Desktop/test-cmake/build /home/bradwu/Desktop/test-cmake/build /home/bradwu/Desktop/test-cmake/build/CMakeFiles/something_lib.dir/DependInfo.cmake "--color="
Dependencies file "CMakeFiles/something_lib.dir/something.cpp.o.d" is newer than depends file "/home/bradwu/Desktop/test-cmake/build/CMakeFiles/something_lib.dir/compiler_depend.internal".
Consolidate compiler generated dependencies of target something_lib
gmake[2]: Leaving directory '/home/bradwu/Desktop/test-cmake/build'
/usr/bin/gmake -f CMakeFiles/something_lib.dir/build.make CMakeFiles/something_lib.dir/build
gmake[2]: Entering directory '/home/bradwu/Desktop/test-cmake/build'
gmake[2]: Nothing to be done for 'CMakeFiles/something_lib.dir/build'.
gmake[2]: Leaving directory '/home/bradwu/Desktop/test-cmake/build'
[ 40%] Built target something_lib
/usr/bin/gmake -f CMakeFiles/verify_something.dir/build.make CMakeFiles/verify_something.dir/depend
gmake[2]: Entering directory '/home/bradwu/Desktop/test-cmake/build'
[ 60%] Generating verify_something.hpp.cpp
/home/linuxbrew/.linuxbrew/Cellar/cmake/3.30.3/bin/cmake -E echo #include\ <something.hpp> > /home/bradwu/Desktop/test-cmake/build/verify_something.hpp.cpp
cd /home/bradwu/Desktop/test-cmake/build && /home/linuxbrew/.linuxbrew/Cellar/cmake/3.30.3/bin/cmake -E cmake_depends "Unix Makefiles" /home/bradwu/Desktop/test-cmake /home/bradwu/Desktop/test-cmake /home/bradwu/Desktop/test-cmake/build /home/bradwu/Desktop/test-cmake/build /home/bradwu/Desktop/test-cmake/build/CMakeFiles/verify_something.dir/DependInfo.cmake "--color="
gmake[2]: Leaving directory '/home/bradwu/Desktop/test-cmake/build'
/usr/bin/gmake -f CMakeFiles/verify_something.dir/build.make CMakeFiles/verify_something.dir/build
gmake[2]: Entering directory '/home/bradwu/Desktop/test-cmake/build'
[ 60%] Generating verify_something.hpp.cpp
/home/linuxbrew/.linuxbrew/Cellar/cmake/3.30.3/bin/cmake -E echo #include\ <something.hpp> > /home/bradwu/Desktop/test-cmake/build/verify_something.hpp.cpp
[ 80%] Building CXX object CMakeFiles/verify_something.dir/verify_something.hpp.cpp.o
/usr/bin/c++ -I/home/bradwu/Desktop/test-cmake -std=gnu++17 -MD -MT CMakeFiles/verify_something.dir/verify_something.hpp.cpp.o -MF CMakeFiles/verify_something.dir/verify_something.hpp.cpp.o.d -o CMakeFiles/verify_something.dir/verify_something.hpp.cpp.o -c /home/bradwu/Desktop/test-cmake/build/verify_something.hpp.cpp
cc1plus: fatal error: /home/bradwu/Desktop/test-cmake/build/verify_something.hpp.cpp: No such file or directory
compilation terminated.
gmake[2]: *** [CMakeFiles/verify_something.dir/build.make:80: CMakeFiles/verify_something.dir/verify_something.hpp.cpp.o] Error 1
gmake[2]: Leaving directory '/home/bradwu/Desktop/test-cmake/build'
gmake[1]: *** [CMakeFiles/Makefile2:113: CMakeFiles/verify_something.dir/all] Error 2
gmake[1]: Leaving directory '/home/bradwu/Desktop/test-cmake/build'
gmake: *** [Makefile:91: all] Error 2
This reverts commit f4ff6ac.
I agree (and thanks @DeveloperPaul123 for pointing out that the behavior we want is in new versions of CMake!). I'll created #32 to fix this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM from a file set perspective.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR looks good to land now.
This PR closes #21 .
all_verify_interface_header_sets
on builds).Please note that I am not familiar with cmake, so please double check my work.
Questions: