Feature/modernize cmake #33

ClausKlein · 2024-11-25T08:20:44Z

Use FILE_SET HEADERS for simplify installation.

Use VERIFY_INTERFACE_HEADER_SETS

Prevent use of include(CTest).

Prepare CPack to easy distribution.

depends on Add codespell to pre-commit #42

NOTE: This is first of a series of PR based on #14

This fix #35

DeveloperPaul123 · 2024-11-25T13:38:45Z

CMakeLists.txt

+        "$<$<COMPILE_FEATURES:cxx_std_23>:cxx_std_23>"
+        "$<$<NOT:$<COMPILE_FEATURES:cxx_std_23>>:cxx_std_20>"


Please remove these flags. For right now, I believe the Beman standard is to control the C++ standard version via CMAKE_CXX_STANDARD

CMAKE_CXX_STANDARD set it for build time, and it is not set.
But this exports your min required cxx_std on this machine.

see #35

inplace_vector targets a min standard of C++17 I believe.

Note from CI workflow: Needs C++ 20 as C++17 selectively disables ranges and concepts related functionalities!

CK is right here regarding the C++ version.

I am waiting for a pragmatic decision ...

I will mention this at the meeting tomorrow.

Mentioned... waiting for someone to come in and review....

This has been discussed at length elsewhere. We're going with the rule that depending on a Beman target should not change the compiler flags of the dependent. Doing so makes it impossible to build a large project with consistent flags.

If this library requires certain compiler + flag combinations, it should run a feature check at CMake time like what was done with iterator_interface and error out with a good message if there isn't a reasonable workaround.

see my example #33 (comment)

Note Some Professional CMake recommendations

include/beman/inplace_vector/inplace_vector.hpp

DeveloperPaul123

Just minor comments to address.

ClausKlein · 2024-11-26T09:07:41Z

@wusatosi This PR was requested from you. Any notes?

wusatosi · 2024-11-26T16:38:48Z

@wusatosi This PR was requested from you. Any notes?

Thanks! You made this a lot easier to review :P

Thank you for taking my feedback. I am not familiar with this part of CMake

I will leave the review up to @DeveloperPaul123 .

wusatosi

I confirm this PR fixes the install command and enables all_verify_interface_header_sets.

Nice job @ClausKlein , I will leave the details to @DeveloperPaul123 .

camio · 2024-12-06T01:38:22Z

CMakeLists.txt

 # cmake-format: on

-cmake_minimum_required(VERSION 3.23)
+set(CMAKE_SKIP_TEST_ALL_DEPENDENCY FALSE)


Why wouldn't we want the default here?

With this setting, it is possible to do make test without make all before.

I'd like to see how my suggestion to change the default in CMake plays out. If they're okay with changing the default, I'd be okay with leaving this line in as a temporary solution.

The discussion there has lead me the conclusion that it is not a good practice to use set(CMAKE_SKIP_TEST_ALL_DEPENDENCY FALSE). See this comment for a rationale summary.

That is your interpretation, not a fact!

camio · 2024-12-06T01:38:32Z

CMakeLists.txt

-cmake_minimum_required(VERSION 3.23)
+set(CMAKE_SKIP_TEST_ALL_DEPENDENCY FALSE)
+
+cmake_minimum_required(VERSION 3.25...3.31)


Why a range? Why this particular range?

To prevent policy warnings if you use a newer CMake version than v.3.25

I would use 3.30...3.31 today.

There aren't any policies that impact the CMake code in this repository so this isn't needed IIUC.

My preference is to keep the simple cmake_minimum_required(VERSION 3.23) and switch to the ... only in the case that we have a particular CMake policy warning we need to suppress, which hopefully is rare.

not possible!

camio · 2024-12-06T01:39:49Z

CMakeLists.txt

+if(${CMAKE_SOURCE_DIR} STREQUAL ${CMAKE_BINARY_DIR})
+    message(FATAL_ERROR "In-source builds are not allowed!")
+endif()


I don't think this check is worth the additional complexity. If people follow our build instructions (e.g. using presets), they should be fine. If this is a big stumbling block, then perhaps this kind of warning should be up-streamed to the Kitware people.

If this is not desired across beman, we should close bemanproject/exemplar#67 .

It is an open issue at CMake to deny in-source builds.

if you do cmake . -L in source_dir instead in binary_dir, please note the generated files in source tree!

@ClausKlein please stop falsely marking conversations as resolved.

camio · 2024-12-06T01:43:11Z

CMakeLists.txt

    ${PROJECT_IS_TOP_LEVEL}
 )

+set(CPACK_GENERATOR TGZ)


Why set this? Shouldn't the CMake invoker have the decision of what CPack generator to use?

This is no restriction, he user has still the choice to use cpack -G ZIP ...

I don't see a benefit of changing the default here, but I see a cost in complexity.

CMakeLists.txt

camio · 2024-12-06T01:45:32Z

CMakeLists.txt

 add_library(beman::inplace_vector ALIAS beman.inplace_vector)

-target_include_directories(
+target_sources(


I think this code would be better placed in src/beman/inplace_vector/CMakeLists.txt for consistency with beman.exemplar.

Not possible, the BASE_DIR is the current directory!

target_sources( beman.inplace_vector PUBLIC FILE_SET inplace_vector_public_headers TYPE HEADERS BASE_DIRS ${CMAKE_CURRENT_SOURCE_DIR}/include FILES "${CMAKE_CURRENT_SOURCE_DIR}/include/beman/inplace_vector/inplace_vector.hpp" )

I understand that. See what is done in exemplar.

camio · 2024-12-06T01:57:00Z

CMakeLists.txt

+        "$<$<COMPILE_FEATURES:cxx_std_23>:cxx_std_23>"
+        "$<$<NOT:$<COMPILE_FEATURES:cxx_std_23>>:cxx_std_20>"


This has been discussed at length elsewhere. We're going with the rule that depending on a Beman target should not change the compiler flags of the dependent. Doing so makes it impossible to build a large project with consistent flags.

If this library requires certain compiler + flag combinations, it should run a feature check at CMake time like what was done with iterator_interface and error out with a good message if there isn't a reasonable workaround.

camio · 2024-12-06T02:10:17Z

.gitignore

+CMakeCache.txt
+CMakeFiles/


Why these files?

Garbage generated be in-source build which cmake .

I don't think we should add to our .gitignore to cover cases where users do unsupported things.

Every line of code we add has an associated maintenance cost and an overhead for readers. I don't think, in this instance, the cost justifies the benefit.

see #33 (comment)

I'm still unconvinced adding these lines is worth the complexity.

Use FILE_SET HEADERS for simplify installation Set VERIFY_INTERFACE_HEADER_SETS property Prevent use of include(CTest) Prepare CPack to easy distribution Add all_verify_interface_header_sets to CI builds Prevent in-source builds Ignore cmake configure trash

Co-authored-by: David Sankel <[email protected]>

ClausKlein · 2024-12-11T16:27:05Z

This is a never ending strory, I Think, you will make it you way.

ClausKlein requested review from DeveloperPaul123, Hels15, camio and wusatosi as code owners November 25, 2024 08:20

ClausKlein mentioned this pull request Nov 25, 2024

Feature/add gcov target #31

Closed

1 task

ClausKlein force-pushed the feature/modernize-cmake branch from 122a417 to 39703ac Compare November 25, 2024 08:29

ClausKlein mentioned this pull request Nov 25, 2024

Feature/export cmake config package #14

Closed

DeveloperPaul123 reviewed Nov 25, 2024

View reviewed changes

include/beman/inplace_vector/inplace_vector.hpp Outdated Show resolved Hide resolved

DeveloperPaul123 requested changes Nov 25, 2024

View reviewed changes

ClausKlein requested a review from DeveloperPaul123 November 25, 2024 17:18

wusatosi approved these changes Nov 26, 2024

View reviewed changes

ClausKlein force-pushed the feature/modernize-cmake branch 4 times, most recently from ecd405a to b6eefec Compare November 27, 2024 06:27

ClausKlein requested a review from wusatosi November 27, 2024 06:33

ClausKlein force-pushed the feature/modernize-cmake branch from b6eefec to d06a5dc Compare November 27, 2024 20:10

camio reviewed Dec 6, 2024

View reviewed changes

ClausKlein and others added 3 commits December 6, 2024 07:56

Modernize cmake files

39d540b

Use FILE_SET HEADERS for simplify installation Set VERIFY_INTERFACE_HEADER_SETS property Prevent use of include(CTest) Prepare CPack to easy distribution Add all_verify_interface_header_sets to CI builds Prevent in-source builds Ignore cmake configure trash

Update CMakeLists.txt

d744137

Co-authored-by: David Sankel <[email protected]>

Rebased and minor fixes after review

d462d55

ClausKlein force-pushed the feature/modernize-cmake branch from e430e9b to d462d55 Compare December 6, 2024 07:10

ClausKlein requested a review from camio December 6, 2024 07:21

wusatosi mentioned this pull request Dec 6, 2024

Feature/add codespell to pre commit #49

Closed

This was referenced Dec 6, 2024

Feature/install cmake config package #32

Closed

Project requires at least C++20 bemanproject/optional#80

Closed

Do not set target_compile_features

e7e5050

camio mentioned this pull request Dec 11, 2024

Prevent “In-Source Builds” bemanproject/exemplar#67

Closed

ClausKlein closed this Dec 11, 2024

		"$<$<COMPILE_FEATURES:cxx_std_23>:cxx_std_23>"
		"$<$<NOT:$<COMPILE_FEATURES:cxx_std_23>>:cxx_std_20>"

		CMakeCache.txt
		CMakeFiles/

Feature/modernize cmake #33

Feature/modernize cmake #33

Uh oh!

Conversation

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

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ClausKlein Nov 25, 2024 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DeveloperPaul123 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!

ClausKlein commented Nov 26, 2024

Uh oh!

wusatosi commented Nov 26, 2024

Uh oh!

wusatosi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ClausKlein Dec 6, 2024 • 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!

ClausKlein commented Nov 25, 2024 •

edited

Loading

ClausKlein Nov 25, 2024 •

edited

Loading

DeveloperPaul123 left a comment •

edited

Loading

ClausKlein Dec 6, 2024 •

edited

Loading