Feature/export cmake config package #14

ClausKlein · 2024-11-14T09:24:02Z

Added VERIFY_INTERFACE_HEADER_SETS too.

(and fixed compiler errors found with this check)

Add a test to check if the exported config package is usable

may be tested with the generic cmake workflow presets:

bash-5.2$ cmake --workflow debug
bash-5.2$ cmake --workflow release

ClausKlein · 2024-11-14T09:30:44Z

see too #15
cmake-package-configuration-files

CMakeLists.txt

.cmake-format

wusatosi

Note the lint failure.

I need help reviewing cmake updates here, @camio @neatudarius

There is quite a lot of collateral change in this PR.

.cmake-format

wusatosi · 2024-11-14T21:20:42Z

CMakeLists.txt


 project(
-    beman.inplace_vector
+    beman_inplace_vector


This does not conform to our naming tradition, why changing it to beman_inplace_vector?

We have used beman.short_name for almost all other projects.

Exemplar, utf_view, iterator_interface.

I changed it because I needed it for cmake export config package

And it was used like this in execution26 project.

For TBD see #14 (comment)

That will end in a very strange export package, not very common:

bash-5.2$ tree /usr/local/lib/cmake/boost_ boost_atomic-1.86.0/ boost_graph_parallel-1.86.0/ boost_math_tr1l-1.86.0/ boost_system-1.86.0/ boost_charconv-1.86.0/ boost_headers-1.86.0/ boost_nowide-1.86.0/ boost_test_exec_monitor-1.86.0/ boost_chrono-1.86.0/ boost_iostreams-1.86.0/ boost_prg_exec_monitor-1.86.0/ boost_thread-1.86.0/ boost_container-1.86.0/ boost_json-1.86.0/ boost_process-1.86.0/ boost_timer-1.86.0/ boost_context-1.86.0/ boost_locale-1.86.0/ boost_program_options-1.86.0/ boost_type_erasure-1.86.0/ boost_contract-1.86.0/ boost_log-1.86.0/ boost_random-1.86.0/ boost_unit_test_framework-1.86.0/ boost_coroutine-1.86.0/ boost_log_setup-1.86.0/ boost_regex-1.86.0/ boost_url-1.86.0/ boost_date_time-1.86.0/ boost_math_c99-1.86.0/ boost_serialization-1.86.0/ boost_wave-1.86.0/ boost_exception-1.86.0/ boost_math_c99f-1.86.0/ boost_stacktrace_addr2line-1.86.0/ boost_wserialization-1.86.0/ boost_fiber-1.86.0/ boost_math_c99l-1.86.0/ boost_stacktrace_basic-1.86.0/ boost_filesystem-1.86.0/ boost_math_tr1-1.86.0/ boost_stacktrace_from_exception-1.86.0/ boost_graph-1.86.0/ boost_math_tr1f-1.86.0/ boost_stacktrace_noop-1.86.0/ bash-5.2$ tree /usr/local/lib/cmake/boost_atomic-1.86.0 /usr/local/lib/cmake/boost_atomic-1.86.0 |-- boost_atomic-config-version.cmake |-- boost_atomic-config.cmake |-- libboost_atomic-variant-mt-shared.cmake `-- libboost_atomic-variant-mt-static.cmake 1 directory, 4 files bash-5.2$ tree stagedir/lib/cmake/ stagedir/lib/cmake/ `-- beman_inplace_vector |-- beman_inplace_vector-config-version.cmake |-- beman_inplace_vector-config.cmake `-- beman_inplace_vector-targets.cmake 2 directories, 3 files bash-5.2$

and it may be used like this:

#--------------------------------------------------------------------------------------- # search required packages and libs #--------------------------------------------------------------------------------------- find_package( Boost 1.86 CONFIG COMPONENTS filesystem asio headers REQUIRED )

I think you are right here. This was recommended in the weekly sync too because this format works better with compiler explorer. You can keep this.

I would also agree that using _ is more idiomatic than . within a project/target name.

The most important point in this PR!

So you agree this change?

Hey, according to: bemanproject/beman#69

We cannot update the project/ target name.

include/beman/inplace_vector/inplace_vector.hpp

CMakeLists.txt

src/beman/inplace_vector/tests/CMakeLists.txt

wusatosi

Thank you for your contribution.

I am not qualified to review CMake files so I cannot single-handly merge/ approve this.

Also, can we defer presets and CI updates to another PR?

I would suggest you limit the scope of your contribution in this PR to be:

CMakeLists.txt

You should split other changes into two PR sets:

CI Improvement:
- macos + windows CI test
Introduce presets (I will hold off to this before we figure out a standard preset set)

Just a note:
Currently we have a infrastructure syncing issue as we have a lot of duplicate work going on on shared infrastructure like CI config, CMake files and CMake presets.

We are going to figure out a way to have a central source of truth/ reference on infrastructure and all the projects will sync infrastructure code off that.

My bet is that will either be in a standalone repository or in exemplar, I will advise you to contribute to that repo on infrastructure related improvements. I believe it have a higher priority on review as well.

If your improvements are accepted in exemplar, it would be easier for me who has less experience to reference that PR and give you a green light.

I don't want to incur more duplicated work so I would hold-off contributing here before we figure out this infrastructure syncing issue.

Again I appreciate your contribution, you seem to know a lot of CMake and its best practices. I would advise you to come join our weekly sync meeting to get your improvement suggestions discussed and accepted quicker.

DeveloperPaul123 · 2024-11-19T22:34:26Z

@ClausKlein As @wusatosi said, thank you for your contribution, but I would agree that it would be helpful to split this PR into smaller ones. Additionally, River is correct in that there is continued discussion around presets and the CI set up.

ClausKlein · 2024-11-20T12:56:29Z

OK, I will try to split it.

But first, it has to work for me on all OS.

Your current CI does not check the exported config packages and could't find it!

Now it works: https://github.com/ClausKlein/inplace_vector/actions/runs/11947491926/job/33303527373#step:9:1

Fix windows build Add CI for macos and windows

ClausKlein · 2024-11-21T06:27:04Z

You should split other changes into two PR sets:

CI Improvement:

macos + windows CI test

In my experience splitting CI workflows files in OS specific parts makes some task like install toolchain really simpler!
Introduce presets

Here and in execution26 the I found no preset.
So I created the simples preset possible: a generic one with only tow workflows debug and release which works on each OS and with each toolchain. Just as demo and as prove of concept.

The preset used in examplar will be more and more complex and unreadable!
It needs a workflow for each compiler and each build config type that should be supported.

Just a note:
Currently we have an infrastructure syncing issue as we have a lot of duplicate work going on on shared infrastructure like > CI config, CMake files and CMake presets.

ClausKlein · 2024-11-22T23:43:36Z

Updated prove of concept for cmake workflow presets:

bash-5.2$ cmake --workflow gcov
Executing workflow step 1 of 2: configure preset "gcov"

Preset CMake variables:

  CMAKE_BUILD_TYPE="Coverage"
  CMAKE_CONFIGURATION_TYPES="Debug;RelWithDebInfo;Coverage;Asan;Lsan;Usan"
  CMAKE_CXX_COMPILER="/usr/local/opt/llvm/bin/clang++"
  CMAKE_CXX_EXTENSIONS:BOOL="FALSE"
  CMAKE_CXX_FLAGS="-Wall -Wextra -Wpedantic"
  CMAKE_CXX_FLAGS_COVERAGE="-g --coverage"
  CMAKE_CXX_STANDARD="23"
  CMAKE_CXX_STANDARD_REQUIRED:BOOL="TRUE"
  CMAKE_C_COMPILER="/usr/local/opt/llvm/bin/clang"
  CMAKE_C_FLAGS="-Wall -Wextra -Wpedantic"
  CMAKE_C_FLAGS_COVERAGE="-g --coverage"
  CMAKE_EXPORT_COMPILE_COMMANDS:BOOL="TRUE"
  CMAKE_INSTALL_PREFIX:PATH="/Users/clausklein/Workspace/cpp/beman-project/inplace_vector/stagedir"
  CMAKE_LD_FLAGS_COVERAGE="--coverage"
  CMAKE_PREFIX_PATH:STRING="/Users/clausklein/Workspace/cpp/beman-project/inplace_vector/stagedir"
  CMAKE_SKIP_TEST_ALL_DEPENDENCY:BOOL="FALSE"
  CMAKE_TOOLCHAIN_FILE:STRING="/Users/clausklein/Workspace/cpp/beman-project/inplace_vector/cmake/toolchains/default.cmake"
  GCOV_EXECUTABLE="llvm-cov gcov"

-- Examples to be built: fibonacci
-- Configuring done (0.3s)
-- Generating done (0.1s)
-- Build files have been written to: /Users/clausklein/Workspace/cpp/beman-project/inplace_vector/build

Executing workflow step 2 of 2: build preset "gcov"

[2/4] Running tests...
Test project /Users/clausklein/Workspace/cpp/beman-project/inplace_vector/build
    Start 1: beman.inplace_vector.test
1/2 Test #1: beman.inplace_vector.test ........   Passed    0.15 sec
    Start 2: fibonacci
2/2 Test #2: fibonacci ........................   Passed    0.05 sec

100% tests passed, 0 tests failed out of 2

Total Test time (real) =   0.21 sec
[4/4] Running gcovr to process coverage results
(INFO) - MainThread - Reading coverage data...
(INFO) - MainThread - Writing coverage report...
lines: 92.6% (100 out of 108)
functions: 100.0% (37 out of 37)
branches: 42.6% (23 out of 54)
bash-5.2$ cmake --workflow --list-presets
Available workflow presets:

  "asan"
  "lsan"
  "usan"
  "gcov"
  "debug"
  "release"

same `generic presets` on every build host available using `CXX` environment variable:

bash-5.2$ cmake --list-presets
Available configure presets:

  "debug"   - Debug Build
  "release" - Release Build
  "gcov"    - Coverage Build
  "asan"    - AddressSanitizer Build
  "lsan"    - LeakSanitizer Build
  "usan"    - UndefinedSanitizer Build
bash-5.2$

gcov, asan, lsan, and usan presets added

with clang-19 and gcc-14 on ubuntu-24.04

wusatosi · 2024-11-24T06:08:46Z

[ OS per workflow ]

I honestly have no problem with both approaches, the reason I am trying to solve all configuration into one file in exemplar is so it is clear what platform gets what kind of compile flags exceptions vs what testing matrix is universal in one file. And it's also easier for downstream to copy paste that single file to create a new matrix (so there could be multiple domains of CI testing files). I don't want all my testing configs scattered around multiple files.

[ Preset design ]

I can definitely see where you are coming from. And I do agree with you to an extent. Please leave this as a comment on the exemplar repo or the discourse thread.

wusatosi

Hey since I kept seeing updates from this PR I decided to jump in and check again.

The CMake related change looks good, again I cannot single handedly review them but I've made my best effort to go through them and to make them obvious to appropriate reviewers.

I again advise you to move the following changes out of this PR:

CI changes
Preset addition
coverage info generation

Again those improvements are badly needed. But please keep them separate into smaller PR so we can review your changes quicker. e. g. If you submit a PR that only introduce CI/ code coverage, I probably could have it merged without request help from others in beman.

I suggest you keep only the following files in this PR:

main CMake file
examples/ and include/ makefile
CMake file needed to generate package.

I value your effort and your time so I do really want to have your changes merged. So I again request you to break the PR down to smaller constituents, so we can have them reviewed ASAP.

I suggest you open the code coverage related change in another PR first, as that seems to be the smallest impacting change. I would be comfortable reviewing the change, merge it and sync your improvements upstream to exemplar.

Our next weekly sync happens on Monday 12PM EST, I sincerely invite you to come to join us. I think your suggestion for preset is a good alternative to what we are trying to do right now. You seem to have great knowledge about CMake, it would be fantastic to talk through what you would suggest generally in the meeting and gain a consensus with us on what would be desired. The information on how to join us should be available on discourse, you can message @ leads on discourse if you have questions subscribing to the weekly sync calendar, I believe you should be able to join the meeting with only a link.

wusatosi · 2024-11-24T05:44:26Z

CMakeLists.txt

 # [CMAKE.LIBRARY_ALIAS]
-add_library(beman::inplace_vector ALIAS beman.inplace_vector)
+add_library(beman::beman_inplace_vector ALIAS beman_inplace_vector)


Non conforming with [CMAKE.LIBRARY_ALIAS], https://github.com/bemanproject/beman/blob/main/docs/BEMAN_STANDARD.md#cmake . Please discard this change.

see #14 (comment)

wusatosi · 2024-11-24T05:49:43Z

CMakeLists.txt

+block()
+    # NOTE: copied from execution26:
+    # FIXME: but not yet used? CK
+    # set(TARGET_NAME inplace_vector)


Wdym by this?

That a long odyssey:

This block was copied from execution26. But the original version use CMAKE_PROJECT_NAME and that is wrong!

If you import this project with FETCH_CONTENT the PROJECT_NAME must be used.

CMakeLists.txt

wusatosi · 2024-11-24T05:54:16Z

examples/CMakeLists.txt

@@ -1,8 +1,17 @@
 # SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception

+cmake_minimum_required(VERSION 3.25...3.31)
+
+project(beman_inplace_vector_example LANGUAGES CXX)


Why do we want a separate project here?

I guess this could demonstrate using the project as a package?

Need help to confirm if this is needed.

see tests ...

wusatosi · 2024-11-24T05:54:45Z

examples/CMakeLists.txt

@@ -12,6 +21,7 @@ foreach(example ${ALL_EXAMPLES})
    )
    target_link_libraries(
        beman.inplace_vector.examples.${example}
-        beman.inplace_vector
+        beman::beman_inplace_vector


See above comment.

Use: beman::inplace_vector or beman_inplace_vector

cmake/CMakeGenericPresets.json

cmake/CMakeLinuxPresets.json

cmake/CMakeWindowsPresets.json

cmake/gcovr.cfg.in

cmake/toolchains/default.cmake

ClausKlein · 2024-11-24T09:14:39Z

tests/beman/inplace_vector/CMakeLists.txt

+    add_test(
+        NAME find-package-test
+        COMMAND
+            ${CMAKE_CTEST_COMMAND} --verbose --output-on-failure -C Debug
+            --build-and-test "${PROJECT_SOURCE_DIR}/examples"
+            "${CMAKE_CURRENT_BINARY_DIR}/find-package-test" --build-generator
+            ${CMAKE_GENERATOR} --build-makeprogram ${CMAKE_MAKE_PROGRAM}
+            --build-options "-DCMAKE_CXX_COMPILER=${CMAKE_CXX_COMPILER}"
+            "-DCMAKE_CXX_STANDARD=${CMAKE_CXX_STANDARD}"
+            "-DCMAKE_BUILD_TYPE=Debug"
+            "-DCMAKE_PREFIX_PATH=${CMAKE_PREFIX_PATH}"
+    )
+endif()


NOTE: This add a test that check, if the our examples standalone project can be build with our exported cmake config package.

ClausKlein · 2024-11-25T08:52:41Z

Closed.
Will be replaced by a series of PR started with PR #33 ....

ClausKlein · 2024-11-25T08:53:58Z

to be continuers with other PR ...

ClausKlein commented Nov 14, 2024

View reviewed changes

CMakeLists.txt Outdated Show resolved Hide resolved

ClausKlein commented Nov 14, 2024

View reviewed changes

.cmake-format Show resolved Hide resolved

wusatosi requested changes Nov 14, 2024

View reviewed changes

wusatosi requested review from camio and neatudarius November 14, 2024 21:38

github-actions bot reviewed Nov 17, 2024

View reviewed changes

ClausKlein requested a review from wusatosi November 19, 2024 08:18

ClausKlein force-pushed the feature/export_cmake_config_package branch from 82e3e27 to 071bde5 Compare November 19, 2024 10:07

ClausKlein requested review from Hels15 and DeveloperPaul123 as code owners November 19, 2024 10:07

wusatosi reviewed Nov 19, 2024

View reviewed changes

ClausKlein added 2 commits November 21, 2024 05:35

Export cmake config package

e9b94ce

Fix windows build Add CI for macos and windows

Add find-package-test using examples

67a3b6a

ClausKlein force-pushed the feature/export_cmake_config_package branch from 071bde5 to 67a3b6a Compare November 21, 2024 05:17

ClausKlein added 3 commits November 21, 2024 06:33

Add an implementation note

25b04eb

Set CMake minimum version to 3.25

3a030e0

Need to set CMAKE_PREFIX_PATH on Linux CI

1923125

ClausKlein added 5 commits November 22, 2024 23:14

Use Ninja Multi-Config generator

f9c6d1a

Use environment to get compiler on CI

75935e9

Add find-package-test onyl for Debug builds on CI

ee3d65b

Run examples as test too

a28d0a8

Add missing depends to test for gcovr run

0ee9ae7

ClausKlein marked this pull request as draft November 22, 2024 23:49

Set gcov program from environment on CI

7b88578

ClausKlein force-pushed the feature/export_cmake_config_package branch from df55f5e to 7e118c2 Compare November 23, 2024 20:46

Add missing Windows presets

9b16321

gcov, asan, lsan, and usan presets added

ClausKlein force-pushed the feature/export_cmake_config_package branch from 6bffc75 to 9b16321 Compare November 23, 2024 21:11

Test cmake presets on Linux CI too

4ec37c3

with clang-19 and gcc-14 on ubuntu-24.04

ClausKlein force-pushed the feature/export_cmake_config_package branch from 0b08043 to 4ec37c3 Compare November 24, 2024 03:30

ClausKlein added 2 commits November 24, 2024 04:40

Fix gcov config options

821293a

Try to check gcov on linux CI

a75dccb

wusatosi requested changes Nov 24, 2024

View reviewed changes

ClausKlein commented Nov 24, 2024

View reviewed changes

cleanup

e7071c7

This was referenced Nov 24, 2024

Feature/add gcov target #31

Closed

Feature/modernize cmake #33

Closed

ClausKlein closed this Nov 25, 2024

ClausKlein mentioned this pull request Nov 27, 2024

Feature/install cmake config package #32

Closed

2 tasks

wusatosi mentioned this pull request Nov 27, 2024

Update library prefix to beman_ bemanproject/beman#69

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/export cmake config package #14

Feature/export cmake config package #14

ClausKlein commented Nov 14, 2024 •

edited

Loading

ClausKlein commented Nov 14, 2024 •

edited

Loading

wusatosi left a comment •

edited

Loading

wusatosi Nov 14, 2024 •

edited

Loading

ClausKlein Nov 15, 2024 •

edited

Loading

ClausKlein Nov 17, 2024

ClausKlein Nov 17, 2024

wusatosi Nov 19, 2024

DeveloperPaul123 Nov 19, 2024

ClausKlein Nov 24, 2024

wusatosi Nov 27, 2024

wusatosi left a comment •

edited

Loading

DeveloperPaul123 commented Nov 19, 2024

ClausKlein commented Nov 20, 2024 •

edited

Loading

ClausKlein commented Nov 21, 2024

ClausKlein commented Nov 22, 2024

wusatosi commented Nov 24, 2024

wusatosi left a comment •

edited

Loading

wusatosi Nov 24, 2024

ClausKlein Nov 24, 2024

wusatosi Nov 24, 2024

ClausKlein Nov 24, 2024

wusatosi Nov 24, 2024

ClausKlein Nov 24, 2024

wusatosi Nov 24, 2024

ClausKlein Nov 24, 2024 •

edited

Loading

ClausKlein commented Nov 25, 2024 •

edited

Loading

ClausKlein commented Nov 25, 2024

Feature/export cmake config package #14

Feature/export cmake config package #14

Conversation

ClausKlein commented Nov 14, 2024 • edited Loading

Added VERIFY_INTERFACE_HEADER_SETS too.

Add a test to check if the exported config package is usable

ClausKlein commented Nov 14, 2024 • edited Loading

wusatosi left a comment • edited Loading

Choose a reason for hiding this comment

wusatosi Nov 14, 2024 • edited Loading

Choose a reason for hiding this comment

ClausKlein Nov 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

The most important point in this PR!

Choose a reason for hiding this comment

wusatosi left a comment • edited Loading

Choose a reason for hiding this comment

DeveloperPaul123 commented Nov 19, 2024

ClausKlein commented Nov 20, 2024 • edited Loading

ClausKlein commented Nov 21, 2024

ClausKlein commented Nov 22, 2024

same generic presets on every build host available using CXX environment variable:

wusatosi commented Nov 24, 2024

wusatosi left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

That a long odyssey:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ClausKlein Nov 24, 2024 • edited Loading

Choose a reason for hiding this comment

ClausKlein commented Nov 25, 2024 • edited Loading

ClausKlein commented Nov 25, 2024

ClausKlein commented Nov 14, 2024 •

edited

Loading

ClausKlein commented Nov 14, 2024 •

edited

Loading

wusatosi left a comment •

edited

Loading

wusatosi Nov 14, 2024 •

edited

Loading

ClausKlein Nov 15, 2024 •

edited

Loading

wusatosi left a comment •

edited

Loading

ClausKlein commented Nov 20, 2024 •

edited

Loading

same `generic presets` on every build host available using `CXX` environment variable:

wusatosi left a comment •

edited

Loading

ClausKlein Nov 24, 2024 •

edited

Loading

ClausKlein commented Nov 25, 2024 •

edited

Loading