Skip to content

Feature/modernize cmake #33

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.

Sign up for GitHub

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

Closed
ClausKlein wants to merge 4 commits into from
Closed

Feature/modernize cmake #33

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
39d540b
Modernize cmake files
ClausKlein Nov 25, 2024
d744137
Update CMakeLists.txt
ClausKlein Dec 6, 2024
d462d55
Rebased and minor fixes after review
ClausKlein Dec 6, 2024
e7e5050
Do not set target_compile_features
ClausKlein Dec 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions .github/workflows/ci_tests.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,15 +104,15 @@ jobs:
- name: Build Release
run: |
cmake --build build --config Release --verbose
# TODO: cmake --build build --config Release --target all_verify_interface_header_sets
cmake --build build --config Release --target all_verify_interface_header_sets
cmake --install build --config Release --prefix /tmp/beman.inplace_vector
find /tmp/beman.inplace_vector -type f
- name: Test Release
run: ctest --test-dir build --build-config Release
- name: Build Debug
run: |
cmake --build build --config Debug --verbose
# TODO: cmake --build build --config Debug --target all_verify_interface_header_sets
cmake --build build --config Debug --target all_verify_interface_header_sets
cmake --install build --config Debug --prefix /tmp/beman.inplace_vector
find /tmp/beman.inplace_vector -type f
- name: Test Debug
Expand Down
2 changes: 2 additions & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
/build
/out
CMakeUserPresets.json
CMakeCache.txt
CMakeFiles/
Comment on lines +4 to +5
Copy link
Member

@camio camio Dec 6, 2024

Choose a reason for hiding this comment

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

Why these files?

Copy link
Contributor Author

@ClausKlein ClausKlein Dec 6, 2024

Choose a reason for hiding this comment

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

Garbage generated be in-source build which cmake .

Copy link
Member

@camio camio Dec 9, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@ClausKlein ClausKlein Dec 9, 2024

Choose a reason for hiding this comment

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

see #33 (comment)

Copy link
Member

@camio camio Dec 11, 2024

Choose a reason for hiding this comment

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

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

59 changes: 42 additions & 17 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
# SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
# cmake-format: on

cmake_minimum_required(VERSION 3.23)
cmake_minimum_required(VERSION 3.25...3.31)
Copy link
Member

@camio camio Dec 6, 2024

Choose a reason for hiding this comment

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

Why a range? Why this particular range?

Copy link
Contributor Author

@ClausKlein ClausKlein Dec 6, 2024

Choose a reason for hiding this comment

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

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

I would use 3.30...3.31 today.

Copy link
Member

@camio camio Dec 11, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@ClausKlein ClausKlein Dec 11, 2024

Choose a reason for hiding this comment

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

not possible!


project(
beman.inplace_vector
Expand All @@ -13,6 +13,27 @@ project(
LANGUAGES CXX
)

if(${CMAKE_SOURCE_DIR} STREQUAL ${CMAKE_BINARY_DIR})
message(FATAL_ERROR "In-source builds are not allowed!")
endif()
Comment on lines +16 to +18
Copy link
Member

@camio camio Dec 6, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Member

@wusatosi wusatosi Dec 6, 2024

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@ClausKlein ClausKlein Dec 6, 2024

Choose a reason for hiding this comment

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

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!

Copy link
Member

@wusatosi wusatosi Dec 6, 2024

Choose a reason for hiding this comment

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

@ClausKlein please stop falsely marking conversations as resolved.


# Require C++20, but let a parent project ask for something higher
if(DEFINED CMAKE_CXX_STANDARD)
if(CMAKE_CXX_STANDARD EQUAL 98 OR CMAKE_CXX_STANDARD LESS 20)
message(FATAL_ERROR "This project requires at least C++23")
endif()
else()
set(CMAKE_CXX_STANDARD 23)
endif()

# Always enforce the language constraint
set(CMAKE_CXX_STANDARD_REQUIRED ON)

# We don't need compiler extensions, but let a parent ask for them
if(NOT DEFINED CMAKE_CXX_EXTENSIONS)
set(CMAKE_CXX_EXTENSIONS OFF)
endif()

# [CMAKE.SKIP_EXAMPLES]
option(
BEMAN_EXEMPLAR_BUILD_EXAMPLES
Expand All @@ -27,38 +48,42 @@ option(
${PROJECT_IS_TOP_LEVEL}
)

set(CPACK_GENERATOR TGZ)
Copy link
Member

@camio camio Dec 6, 2024

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@ClausKlein ClausKlein Dec 6, 2024

Choose a reason for hiding this comment

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

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

Copy link
Member

@camio camio Dec 11, 2024

Choose a reason for hiding this comment

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

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


include(GNUInstallDirs)
include(CPack)

add_library(beman.inplace_vector INTERFACE)
# [CMAKE.LIBRARY_ALIAS]
add_library(beman::inplace_vector ALIAS beman.inplace_vector)

target_include_directories(
target_sources(
Copy link
Member

@camio camio Dec 6, 2024

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@ClausKlein ClausKlein Dec 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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"
)

Copy link
Member

@camio camio Dec 11, 2024

Choose a reason for hiding this comment

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

I understand that. See what is done in exemplar.

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"
)
set_target_properties(
beman.inplace_vector
INTERFACE
$<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/include>
$<INSTALL_INTERFACE:include>
PROPERTIES VERIFY_INTERFACE_HEADER_SETS ON
)

set(TARGET_PACKAGE_NAME ${PROJECT_NAME}-config)
set(TARGETS_EXPORT_NAME ${PROJECT_NAME}-targets)
set(INSTALL_CONFIGDIR ${CMAKE_INSTALL_LIBDIR}/cmake/${PROJECT_NAME})

# Install the InplaceVector library to the appropriate destination
install(
TARGETS beman.inplace_vector
EXPORT ${TARGETS_EXPORT_NAME}
DESTINATION
${CMAKE_INSTALL_LIBDIR}
)

# Install the header files to the appropriate destination
install(
DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}/
DESTINATION ${CMAKE_INSTALL_INCLUDEDIR}/${CMAKE_PROJECT_NAME}
FILES_MATCHING
PATTERN
"${CMAKE_CURRENT_SOURCE_DIR}/include/beman/inplace_vector/inplace_vector.hpp"
FILE_SET inplace_vector_public_headers
)

if(BEMAN_INPLACE_VECTOR_BUILD_TESTS)
include(CTest)
enable_testing()
add_subdirectory(tests/beman/inplace_vector)
endif()

Expand Down
2 changes: 1 addition & 1 deletion README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ TODO: tested platforms.

```text
# Configure build
$ cmake -S . -B build -DCMAKE_CXX_STANDARD=20
$ cmake -S . -B build -DCMAKE_CXX_STANDARD=23
-- The CXX compiler identification is GNU 11.4.0
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
Expand Down
Loading