Skip to content

Conversation

@ClausKlein
Copy link
Contributor

@ClausKlein ClausKlein commented Aug 25, 2022

to support new CMake feature for interface-libraries

this would fix #32

Copy link
Owner

@TheLartians TheLartians left a comment

Choose a reason for hiding this comment

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

Hey, thanks for the PR! TBH I don't quite understand what use-case is supports that we don't already have. Do you think you could write an example (or better - a test case) that would not work with the current behaviour?

@ClausKlein
Copy link
Contributor Author

ClausKlein commented Sep 3, 2022

The use case is that a header only project may check that every header is self contented and compiles.

Too it is possible to run-clang-tidy for each header, because CMake generate an object target for each header file in this set.

see https://discourse.cmake.org/t/verifying-header-sets-not-supported-for-header-only-interface-libraries/6139/8

and ClausKlein/asio@7701d95

@TheLartians
Copy link
Owner

So if I understand correctly from the docs, the advantage is that we don't need to specify the include destination explicitly but can now use the BASE_DIRS flag in the library definition? How about adding a test that relies on this feature?

@ClausKlein
Copy link
Contributor Author

ClausKlein commented Sep 15, 2022

So if I understand correctly from the docs, the advantage is that we don't need to specify the include destination explicitly but can now use the BASE_DIRS flag in the library definition? How about adding a test that relies on this feature?

No, the main advantages are

  • for an interface library you may check, that every header is self-contained
  • you may also check them with run-clang-tidy without writing tests.

see too aminya/project_options#143
and CMAKE_VERIFY_INTERFACE_HEADER_SETS

@TheLartians
Copy link
Owner

I still don't really understand the feature then, do you think you could add an independent test case that would fail with the current version, but pass using the suggested changes?

@ClausKlein
Copy link
Contributor Author

ClausKlein commented Sep 20, 2022

I still don't really understand the feature then, do you think you could add an independent test case that would fail with the current version, but pass using the suggested changes?

@TheLartians I push a version that is not usable with CMake v3.24.2 (pip install cmake)

How will you support to install a header only library using this new CMake feature?

@ClausKlein ClausKlein requested a review from TheLartians October 7, 2022 08:47
@ClausKlein
Copy link
Contributor Author

ClausKlein commented Oct 22, 2022

@TheLartians may you think about this MR again?

@ClausKlein
Copy link
Contributor Author

@TheLartians Q: Is the project not maintained anymore?

@TheLartians
Copy link
Owner

TheLartians commented Jan 28, 2023

Hey @ClausKlein I apologise for not being able to respond so far. I'm still planning to maintain this project, however I don't have any experience with FILE_SET HEADERS and don't yet understand what problem it solves. From a quick look at the readme it allows omitting the target_include_directories?

As before it would be great if you could avoid changing the test behaviour based on the CMake version. You can assume that tests will be performed with CMake > 3.23.0, but I still want to keep the existing test code that doesn't use the new feature. Perhaps you instead create another dependency test/file_set_dependency/CMakeLists.txt using file set instead?

@TheLartians TheLartians removed their request for review January 28, 2023 16:57
@ClausKlein
Copy link
Contributor Author

FYI: from import-cmake-c20-modules

Once you have built a compiler that supports p1689 and a version of CMake that supports C++20 modules and have run the tests to make sure things are set up correctly. To use modules in CMake you need to use target_sources and FILE_SETS. https://cmake.org/cmake/help/latest/command/target_sources.html

@TheLartians
Copy link
Owner

I see, so the files sets are required to support modules? In that case how about adding a separate test case that demonstrates this functionality by using modules instead of modifying the existing tests?

@ClausKlein
Copy link
Contributor Author

ClausKlein commented Mar 23, 2024

I can't use this cmake module as long as it does't support modern CMake FILE_SET!

@ClausKlein ClausKlein force-pushed the feature/test-fileset-headers branch 2 times, most recently from 147c5c0 to 29c7f17 Compare March 3, 2025 09:41
@ClausKlein ClausKlein force-pushed the feature/test-fileset-headers branch from e325e17 to a059986 Compare March 3, 2025 11:03
@ClausKlein ClausKlein force-pushed the feature/test-fileset-headers branch from db54ff5 to 3098d49 Compare March 3, 2025 17:38
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.

add support for CMAKE FILE_SET HEADERS

2 participants