Verify headers by steve-downey · Pull Request #75 · bemanproject/optional

steve-downey · 2024-11-10T20:27:48Z

Convert to an INTERFACE target rather than static library and use the new-ish cmake verify headers mechanism to test that the headers compile and are stand alone.

Convert to an interface library and use verify headers to test that headers are self-contained and compile correctly by themselves. Test install at end of CI and run both Asan and RWDI builds.

Add the verify headers target to the build presets for each compiler.

neatudarius

Interesting. I think we should add this idea to our tomorrow's agenda.

I'm what others think. TLDR: We should have a rule in the standard for header only libraries!

@JeffGarland ?

Add the EXPORT component for the header install components.

steve-downey · 2024-11-10T23:51:46Z

It's a relatively new Cmake feature, but getting rid of the static library is a win. College tour tomorrow, so I'll miss the meeting, but please do bring it up.

…

On Sun, Nov 10, 2024, 17:11 Darius Neațu ***@***.***> wrote: ***@***.**** approved this pull request. Interesting. I think we should add this idea to our tomorrow's agenda. I'm what others think. — Reply to this email directly, view it on GitHub <#75 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAVNZ5WCAX4THWO2FVKTE3LZ77KY7AVCNFSM6AAAAABRQRTOM2VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDIMRVHA3DKMZUGI> . You are receiving this because you authored the thread.Message ID: ***@***.***>

Add back FindPackage support.

Lint failed. Fixed indentation.

neatudarius · 2024-11-11T09:45:05Z

include/beman/optional26/CMakeLists.txt

+install(
+    EXPORT beman_optional26_export
+    DESTINATION ${CMAKE_INSTALL_LIBDIR}/cmake/beman/optional26/
+    NAMESPACE Beman::Optional26::


Suggested change

NAMESPACE Beman::Optional26::

NAMESPACE beman::optional26::

?

neatudarius · 2024-11-11T09:46:04Z

CMakeLists.txt

+
 # Create the library target and named header set for beman_optional26
-add_library(beman_optional26 STATIC)
+add_library(beman_optional26 INTERFACE)


Suggested change

add_library(beman_optional26 INTERFACE)

add_library(beman.optional26 INTERFACE)

The standard says it should use . after beman. Would you like to apply these updates in your PR?

I can fix this up as part of the PR. Need to merge in other changes in any case.

That would be great. But feel free to speed-up/skip steps today if you need something in main for Poland.

Note: that will end in very strange behaviour! Normally I use something like this:

find_package(Boost CONFIG QUIET) if(TARGET Boost::headers AND TARGET fmt::fmt) add_executable(to_string to_string.cpp) target_link_libraries(to_string fmt::fmt Boost::headers) endif()

This is creating the target that gets the named header file_set added to it? It's up here to avoid ordering problems between subdirectories.

neatudarius · 2024-11-12T17:42:52Z

@steve-downey , please proceed with this PR. We discussed yesterday in the sync and it's OK to propose this standard cmake practice in the Beman standard (same document, a new CMAKE.$NAME rule). Can you add this recommendation?

If you cannot help with that, I can do it after Poland week (in 2-3 weeks).

steve-downey · 2024-11-15T16:58:50Z

@steve-downey , please proceed with this PR. We discussed yesterday in the sync and it's OK to propose this standard cmake practice in the Beman standard (same document, a new CMAKE.$NAME rule). Can you add this recommendation?

If you cannot help with that, I can do it after Poland week (in 2-3 weeks).

Names of the header set, install component, or all of the above?
The header set name needs to be non-colliding to allow add subdir composition. If it's not clear in the standard that the goal is to allow project composition, we should do so, just to clarify that if you find something that conflicts with composition, fixing it is allowed over rote adherence to the standard (not an issue here...)
The component name is part of the interface to the project, so it's probably even more important to standardise that. In particular it's likely to affect how building within vcpkg and conan, etc work.

neatudarius · 2024-11-15T17:06:57Z

@steve-downey , please proceed with this PR. We discussed yesterday in the sync and it's OK to propose this standard cmake practice in the Beman standard (same document, a new CMAKE.$NAME rule). Can you add this recommendation?
If you cannot help with that, I can do it after Poland week (in 2-3 weeks).

Names of the header set, install component, or all of the above? The header set name needs to be non-colliding to allow add subdir composition. If it's not clear in the standard that the goal is to allow project composition, we should do so, just to clarify that if you find something that conflicts with composition, fixing it is allowed over rote adherence to the standard (not an issue here...) The component name is part of the interface to the project, so it's probably even more important to standardise that. In particular it's likely to affect how building within vcpkg and conan, etc work.

It sounds like it's a good idea to put in standard all of the above. I would approve that, and David was also happy about your PR. It's true that we had only 5 people in the last sync. If you want to get more feedback, do a discourse thread, but I would go directly with the PR in BEMAN_STANDARD.md.

ClausKlein · 2024-11-17T05:13:31Z

this PR ends in this cmake export packages which is not usable!

bash-5.2$ tree .install/clang-19/lib/cmake/
.install/clang-19/lib/cmake/
`-- beman
    `-- optional26
        |-- BemanOptional26Config.cmake
        `-- beman_optional26.cmake

3 directories, 2 files
bash-5.2$

Also the packagename-version.cmake is missing.

Usage:

cmake_minimum_required(VERSION 3.27...3.31)

project(beman_optional26_example VERSION 0.0.0 LANGUAGES CXX)

set(BEMAN_OPTIONAL26_LIBRARY "Beman::Optional26::beman_optional26")

if(PROJECT_IS_TOP_LEVEL)
    find_package(Beman::Optional26 0.0.0 EXACT REQUIRED)
endif()

Result:

cmake -S . -G Ninja -B .build -D CMAKE_PREFIX_PATH=../.install/clang-19
CMake Error at CMakeLists.txt:15 (find_package):
  By not providing "FindBeman::Optional26.cmake" in CMAKE_MODULE_PATH this
  project has asked CMake to find a package configuration file provided by
  "Beman::Optional26", but CMake did not find one.

  Could not find a package configuration file provided by "Beman::Optional26"
  (requested version 0.0.0) with any of the following names:

    Beman::Optional26Config.cmake
    beman::optional26-config.cmake

  Add the installation prefix of "Beman::Optional26" to CMAKE_PREFIX_PATH or
  set "Beman::Optional26_DIR" to a directory containing one of the above
  files.  If "Beman::Optional26" provides a separate development package or
  SDK, be sure it has been installed.


-- Configuring incomplete, errors occurred!
bash-5.2$

ClausKlein · 2024-11-17T05:34:56Z

A header only library is architecture and compiler independent.

Why install under prefix/clang-19?

ClausKlein · 2024-11-17T05:37:40Z

bash-5.2$ cmake -S . -G Ninja -B .build -D CMAKE_PREFIX_PATH=../.install/clang-19
CMake Error at CMakeLists.txt:13 (find_package):
  Could not find a configuration file for package "beman_optional26" that
  exactly matches requested version "0.0.0".

  The following configuration files were considered but not accepted:

    /Users/clausklein/Workspace/cpp/beman-project/optional26/.install/clang-19/lib/cmake/beman_optional26/beman_optional26-config.cmake, version: unknown

-- Configuring incomplete, errors occurred!
bash-5.2$

steve-downey · 2024-11-17T08:11:52Z

A header only library is architecture and compiler independent.

Why install under prefix/clang-19?

Habit, I suppose? Easier to always disambiguate than sort out of its strictly necessary.
Although if there are any other components of the project installed, not having collisions is useful.
In my normal usage, this install location is transient, and just used to produce a deb that's installed later.

steve-downey · 2024-12-06T19:58:48Z

I'll work on the installation bugs and catch up with the Beman Standard in follow on PRs.

steve-downey added 3 commits November 10, 2024 14:48

Convert to INTERFACE and verify headers

426ca35

Convert to an interface library and use verify headers to test that headers are self-contained and compile correctly by themselves. Test install at end of CI and run both Asan and RWDI builds.

Add verify to build presets

5919e9a

Add the verify headers target to the build presets for each compiler.

Clean up empty CML.txt files

74bd833

steve-downey requested review from camio and neatudarius as code owners November 10, 2024 20:27

Reformat CML.txt to pass lint

b337a9c

neatudarius approved these changes Nov 10, 2024

View reviewed changes

neatudarius mentioned this pull request Nov 10, 2024

Apply the Beman Standard: cmake related rules bemanproject/iterator_interface#21

Merged

9 tasks

Add back cmake export file

b7cb332

Add the EXPORT component for the header install components.

steve-downey added 2 commits November 10, 2024 20:35

Export a Config.cmake file for FindPackage

35f363a

Add back FindPackage support.

Reformat CML.txt

5691c2d

Lint failed. Fixed indentation.

neatudarius reviewed Nov 11, 2024

View reviewed changes

steve-downey mentioned this pull request Nov 16, 2024

Add support for apple OSX builds #79

Closed

1 task

Merge branch 'main' into verify_headers

af29583

steve-downey merged commit 5cffcdb into bemanproject:main Dec 6, 2024

wusatosi mentioned this pull request Feb 6, 2025

Make project header only bemanproject/exemplar#113

Closed

	add_library(beman_optional26 INTERFACE)
	add_library(beman.optional26 INTERFACE)

Conversation

steve-downey commented Nov 10, 2024

Uh oh!

neatudarius left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

steve-downey commented Nov 10, 2024 via email

Uh oh!

neatudarius Nov 11, 2024

Choose a reason for hiding this comment

Uh oh!

neatudarius Nov 11, 2024

Choose a reason for hiding this comment

Uh oh!

steve-downey Nov 15, 2024

Choose a reason for hiding this comment

Uh oh!

neatudarius Nov 15, 2024

Choose a reason for hiding this comment

Uh oh!

ClausKlein Nov 17, 2024

Choose a reason for hiding this comment

Uh oh!

steve-downey Nov 17, 2024

Choose a reason for hiding this comment

Uh oh!

neatudarius commented Nov 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

steve-downey commented Nov 15, 2024

Uh oh!

neatudarius commented Nov 15, 2024

Uh oh!

ClausKlein commented Nov 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ClausKlein commented Nov 17, 2024

Uh oh!

ClausKlein commented Nov 17, 2024

Uh oh!

steve-downey commented Nov 17, 2024

Uh oh!

steve-downey commented Dec 6, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

neatudarius left a comment •

edited

Loading

neatudarius commented Nov 12, 2024 •

edited

Loading

ClausKlein commented Nov 17, 2024 •

edited

Loading