Skip to content
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

More tweaks after standard updates #31

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

More tweaks after standard updates #31

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
93f1a46
Root README: typo in links
neatudarius Sep 25, 2024
1fc7388
Remove extra whitespace
neatudarius Sep 25, 2024
9446152
Apply Beman standard: CMAKE.CONFIG
neatudarius Sep 25, 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
26 changes: 22 additions & 4 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,18 +4,22 @@
cmake_minimum_required(VERSION 3.23)

project(
beman.exemplar # CMake Project Name, which is also the name of the top-level
# targets (e.g., library, executable, etc.).
beman.exemplar
DESCRIPTION "A Beman library exemplar"
VERSION 0.0.0
LANGUAGES CXX)

include(CMakePackageConfigHelpers)
include(CTest)
include(FetchContent)
include(GNUInstallDirs)

set(TARGETS_EXPORT_NAME ${CMAKE_PROJECT_NAME}Targets)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd rather just inline this below instead of making a variable that's used only once.

set(INSTALL_CONFIGDIR ${CMAKE_INSTALL_LIBDIR}/cmake)

if(BUILD_TESTING)
enable_testing()

# Fetch GoogleTest
FetchContent_Declare(
googletest
GIT_REPOSITORY https://github.com/google/googletest.git
Expand All @@ -25,5 +29,19 @@ if(BUILD_TESTING)
endif()

add_subdirectory(src/beman/exemplar)

add_subdirectory(examples)

Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't all the library metadata (i.e., exemplar-config.cmake, etc.) go in the src/beman/exemplar subdir?

write_basic_package_version_file(
Copy link
Member Author

@neatudarius neatudarius Sep 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Inspect current behavior from https://github.com/beman-project/execution26/blob/main/CMakeLists.txt and https://github.com/beman-project/optional26/blob/main/CMakeLists.txt.

I tried to extract only the minimum required code to do the install.

Expected tree:

$ cmake --install build --prefix /opt/beman.exemplar
-- Install configuration: ""
-- Installing: /opt/beman.exemplar/lib/libbeman.exemplar.a
-- Installing: /opt/beman.exemplar/include
-- Installing: /opt/beman.exemplar/include/beman
-- Installing: /opt/beman.exemplar/include/beman/exemplar
-- Installing: /opt/beman.exemplar/include/beman/exemplar/identity.hpp
-- Installing: /opt/beman.exemplar/lib/cmake/beman.exemplarConfig.cmake
-- Installing: /opt/beman.exemplar/lib/cmake/beman.exemplarConfigVersion.cmake

$ tree /opt/beman.exemplar
/opt/beman.exemplar
├── include
│   └── beman
│       └── exemplar
│           └── identity.hpp
└── lib
    ├── cmake
    │   ├── beman.exemplarConfig.cmake
    │   └── beman.exemplarConfigVersion.cmake
    └── libbeman.exemplar.a

6 directories, 4 files

I would like your feedback and decide a standard pattern, which will be applied initially to beman.examplar, and eventually mirror it into other 2 repos.

Open questions:

  1. Are we OK to install only the next artifacts?
  • $export_path/include/beman/$short_name/* (all headers, in this repo - 1 file)
  • $export_path/lib/libbeman.short_name.a (1 file)
  • $export_path/lib/cmake/beman.short_name.Config[Version].cmake (2 files)
    What we should NOT export:
  • example binaries (we do that for optional26 , I'll fix it)
  • test binaries
  • scripts
  • docs

  1. @camio proposed to add a subdirectory for cmake configs - a.k.a. /opt/beman.exemplar/lib/cmake/beman.exemplarConfig.cmake -> /opt/beman.exemplar/lib/cmake/beman.exemplar/beman.exemplarConfig.cmake. Do people agree with that?

  2. @steve-downey , do we need these lines https://github.com/beman-project/optional26/blob/main/CMakeLists.txt#L36-L39?

Waiting for your input here. If any decision will be made here, we can put it into Beman standard.

CC: @camio @steve-downey @dietmarkuehl @bretbrownjr

Copy link
Member

Choose a reason for hiding this comment

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

[W]e should NOT export...docs

I'm okay with this for now. Eventually, we should export docs at installation time to the docs/beman.short_name/ directory, but we need to get our docs story figured out first.

@camio proposed to add a subdirectory for cmake configs

Yes, this is the convention for the ArchLinux distribution and probably others as well. See openjpeg2's contents for an example. Config Mode's Search Procedure includes a table indicating how *Config.cmake files are found.

Copy link
Contributor

Choose a reason for hiding this comment

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

Eventually we should just have a function like install_beman_library that wraps up all of the above. Packaging for Arch, packaging for vcpkg, packaging for Conan, etc., are all different, and it would be a mistake to hardcode logic for each library for each environment. That's an MxN complexity algorithm. We need an abstraction that gives us an M+N task.

${CMAKE_CURRENT_BINARY_DIR}/${CMAKE_PROJECT_NAME}ConfigVersion.cmake
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
${CMAKE_CURRENT_BINARY_DIR}/${CMAKE_PROJECT_NAME}ConfigVersion.cmake
${CMAKE_CURRENT_BINARY_DIR}/${CMAKE_PROJECT_NAME}-config-version.cmake

Just because lower.caseThenCamelCase.cmake is not how anyone would choose to name a file. See upstream docs on support for kebab-case filenames.

VERSION ${PROJECT_VERSION}
COMPATIBILITY AnyNewerVersion)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
COMPATIBILITY AnyNewerVersion)
COMPATIBILITY ExactVersion)

Unless we're doing something specific to guarantee some ABI compatibility across versions of a Beman project, we should be conservative about version matching. On the contrary, I fully expect API and ABI breaks across releases of Beman projects since they're by definition experimental and prone to design changes.

We could maybe revisit this choice on a case-by-case basis for projects that a "stable" status. But unless they're spelling their implementations with ABI tags and such, I wouldn't advise people bother.


configure_package_config_file(
"cmake/Config.cmake.in"
${CMAKE_CURRENT_BINARY_DIR}/${CMAKE_PROJECT_NAME}Config.cmake
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
${CMAKE_CURRENT_BINARY_DIR}/${CMAKE_PROJECT_NAME}Config.cmake
${CMAKE_CURRENT_BINARY_DIR}/${CMAKE_PROJECT_NAME}-config.cmake

See the comment about *ConfigVersion.cmake

INSTALL_DESTINATION ${INSTALL_CONFIGDIR})

install(
FILES ${CMAKE_CURRENT_BINARY_DIR}/${CMAKE_PROJECT_NAME}Config.cmake
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
FILES ${CMAKE_CURRENT_BINARY_DIR}/${CMAKE_PROJECT_NAME}Config.cmake
FILES ${CMAKE_CURRENT_BINARY_DIR}/${CMAKE_PROJECT_NAME}-config.cmake

${CMAKE_CURRENT_BINARY_DIR}/${CMAKE_PROJECT_NAME}ConfigVersion.cmake
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
${CMAKE_CURRENT_BINARY_DIR}/${CMAKE_PROJECT_NAME}ConfigVersion.cmake
${CMAKE_CURRENT_BINARY_DIR}/${CMAKE_PROJECT_NAME}-config-version.cmake

DESTINATION ${INSTALL_CONFIGDIR})
42 changes: 24 additions & 18 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ Implements: `std::identity` proposed in [Standard Library Concepts (P0898R3)](ht

### Usage: default projection in constrained algorithms

The following code snippet illustrates how we can achieve a default projection using `beman::exemplar::identity`:
The following code snippet illustrates how we can achieve a default projection using `beman::exemplar::identity`:


```cpp
Expand Down Expand Up @@ -73,7 +73,7 @@ int main()

```

Full runable examples can be found in `examples/` (e.g., [./examples/identity_as_default_projection.cpp.cpp](./examples/identity_as_default_projection.cpp.cpp)).
Full runable examples can be found in `examples/` (e.g., [./examples/identity_as_default_projection.cpp](./examples/identity_as_default_projection.cpp)).

## Building beman.exemplar

Expand Down Expand Up @@ -152,7 +152,8 @@ $ cmake -B build -S . -DCMAKE_CXX_STANDARD=20
-- Check for working CXX compiler: /usr/bin/c++ - skipped
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Configuring done (0.1s)
...
-- Configuring done (2.9s)
-- Generating done (0.0s)
-- Build files have been written to: /path/to/repo/build

Expand All @@ -167,17 +168,17 @@ $ cmake --build build
[ 50%] Building CXX object _deps/googletest-build/googletest/CMakeFiles/gtest_main.dir/src/gtest_main.cc.o
[ 60%] Linking CXX static library ../../../lib/libgtest_main.a
[ 60%] Built target gtest_main
[ 70%] Building CXX object src/beman/exemplar/tests/CMakeFiles/beman.exemplar.Test.dir/identity.t.cpp.o
[ 80%] Linking CXX executable beman.exemplar.Test
[ 80%] Built target beman.exemplar.Test
[ 90%] Building CXX object examples/CMakeFiles/identity_usage.dir/identity_usage.cpp.o
[100%] Linking CXX executable identity_usage
[100%] Built target identity_usage
[ 70%] Building CXX object src/beman/exemplar/CMakeFiles/beman.exemplar.tests.dir/identity.t.cpp.o
[ 80%] Linking CXX executable beman.exemplar.tests
[ 80%] Built target beman.exemplar.tests
[ 90%] Building CXX object examples/CMakeFiles/beman.exemplar.examples.identity_direct_usage.dir/identity_direct_usage.cpp.o
[100%] Linking CXX executable beman.exemplar.examples.identity_direct_usage
[100%] Built target beman.exemplar.examples.identity_direct_usage

# Run beman.exemplar tests.
$ ctest --test-dir build
Internal ctest changing into directory: /path/to/your/repo/build
Test project /path/to/your/repo/build
Internal ctest changing into directory: /path/to/repo/build
Test project /path/to/repo/build
Start 1: IdentityTest.call_identity_with_int
1/4 Test #1: IdentityTest.call_identity_with_int ........... Passed 0.00 sec
Start 2: IdentityTest.call_identity_with_custom_type
Expand All @@ -193,7 +194,7 @@ Total Test time (real) = 0.01 sec


# Run examples.
$ build/exemplar/beman.exemplar.examples.identity_direct_usage
$ build/examples/beman.exemplar.examples.identity_direct_usage
2024

```
Expand All @@ -207,11 +208,13 @@ $ build/exemplar/beman.exemplar.examples.identity_direct_usage
# Install build artifacts from `build` directory into `opt/beman.exemplar` path.
$ cmake --install build --prefix /opt/beman.exemplar
-- Install configuration: ""
-- Up-to-date: /opt/beman.exemplar/lib/libbeman.exemplar.a
-- Up-to-date: /opt/beman.exemplar/include
-- Up-to-date: /opt/beman.exemplar/include/beman
-- Up-to-date: /opt/beman.exemplar/include/beman/exemplar
-- Up-to-date: /opt/beman.exemplar/include/beman/exemplar/identity.hpp
-- Installing: /opt/beman.exemplar/lib/libbeman.exemplar.a
-- Installing: /opt/beman.exemplar/include
-- Installing: /opt/beman.exemplar/include/beman
-- Installing: /opt/beman.exemplar/include/beman/exemplar
-- Installing: /opt/beman.exemplar/include/beman/exemplar/identity.hpp
-- Installing: /opt/beman.exemplar/lib/cmake/beman.exemplarConfig.cmake
-- Installing: /opt/beman.exemplar/lib/cmake/beman.exemplarConfigVersion.cmake

# Check tree.
$ tree /opt/beman.exemplar
Expand All @@ -221,9 +224,12 @@ $ tree /opt/beman.exemplar
│   └── exemplar
│   └── identity.hpp
└── lib
├── cmake
│   ├── beman.exemplarConfig.cmake
│   └── beman.exemplarConfigVersion.cmake
Comment on lines +228 to +229
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
│   ├── beman.exemplarConfig.cmake
│   └── beman.exemplarConfigVersion.cmake
│   ├── beman.exemplar-config.cmake
│   └── beman.exemplar-config-version.cmake

Also, what happened to the file referenced by this bit in the config file?
include("${CMAKE_CURRENT_LIST_DIR}/@TARGETS_EXPORT_NAME@.cmake")

└── libbeman.exemplar.a

5 directories, 2 files
6 directories, 4 files
```

</details>
Expand Down
6 changes: 6 additions & 0 deletions cmake/Config.cmake.in
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
# SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception

@PACKAGE_INIT@

include("${CMAKE_CURRENT_LIST_DIR}/@TARGETS_EXPORT_NAME@.cmake")
check_required_components("@PROJECT_NAME@")