Skip to content

Prepare use of CMAKE_CXX_MODULE_STD #35

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.

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

Draft
wants to merge 7 commits into
base: 33-module-for-beman-scope
Choose a base branch
from

Conversation

ClausKlein
Copy link

@ClausKlein ClausKlein commented May 31, 2025

/Users/clausklein/Workspace/cpp/cxx23/scope
bash-5.2$ !CXX
CXX=g++-15 cmake -S . -B build -D CMAKE_CXX_STANDARD=20 -D CMAKE_CXX_MODULE_STD=0 --fresh 
-- The CXX compiler identification is GNU 15.1.0
-- Checking whether CXX compiler has -isysroot
-- Checking whether CXX compiler has -isysroot - yes
-- Checking whether CXX compiler supports OSX deployment target flag
-- Checking whether CXX compiler supports OSX deployment target flag - yes
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Check for working CXX compiler: /usr/local/bin/g++-15 - skipped
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- CMAKE_CXX_STANDARD="20" ; CMAKE_CXX_SCAN_FOR_MODULES="1" ; CMAKE_CXX_MODULE_STD="0" ; CMAKE_CXX_COMPILER_IMPORT_STD=""
-- Compiler is: GNU version: 15.1.0
-- CMake is: 4.0.2 modules scan: 1
Cloning into 'catch2-src'...
HEAD is now at 2b60af89 v3.8.1
-- Performing Test HAVE_FLAG__ffile_prefix_map__Users_clausklein_Workspace_cpp_cxx23_scope_build__deps_catch2_src__
-- Performing Test HAVE_FLAG__ffile_prefix_map__Users_clausklein_Workspace_cpp_cxx23_scope_build__deps_catch2_src__ - Success
Examples to be built: scope_example;unique_resource;unique_resource-file
-- Configuring done (16.5s)
-- Generating done (0.1s)
-- Build files have been written to: /Users/clausklein/Workspace/cpp/cxx23/scope/build
bash-5.2$ CXX=g++-15 cmake -S . -B build -D CMAKE_CXX_STANDARD=23 -D CMAKE_CXX_MODULE_STD=1 --fresh 
-- The CXX compiler identification is GNU 15.1.0
-- Checking whether CXX compiler has -isysroot
-- Checking whether CXX compiler has -isysroot - yes
-- Checking whether CXX compiler supports OSX deployment target flag
-- Checking whether CXX compiler supports OSX deployment target flag - yes
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Check for working CXX compiler: /usr/local/bin/g++-15 - skipped
-- Detecting CXX compile features
CMake Warning (dev) at /usr/local/share/cmake/Modules/Compiler/CMakeCommonCompilerMacros.cmake:248 (cmake_language):
  CMake's support for `import std;` in C++23 and newer is experimental.  It
  is meant only for experimentation and feedback to CMake developers.
Call Stack (most recent call first):
  /usr/local/share/cmake/Modules/CMakeDetermineCompilerSupport.cmake:113 (cmake_create_cxx_import_std)
  /usr/local/share/cmake/Modules/CMakeTestCXXCompiler.cmake:83 (CMAKE_DETERMINE_COMPILER_SUPPORT)
  CMakeLists.txt:17 (project)
This warning is for project developers.  Use -Wno-dev to suppress it.

-- Detecting CXX compile features - done
-- CMAKE_CXX_STANDARD="23" ; CMAKE_CXX_SCAN_FOR_MODULES="1" ; CMAKE_CXX_MODULE_STD="1" ; CMAKE_CXX_COMPILER_IMPORT_STD="23;26"
-- Compiler is: GNU version: 15.1.0
-- CMake is: 4.0.2 modules scan: 1
Cloning into 'catch2-src'...
HEAD is now at 2b60af89 v3.8.1
-- Performing Test HAVE_FLAG__ffile_prefix_map__Users_clausklein_Workspace_cpp_cxx23_scope_build__deps_catch2_src__
-- Performing Test HAVE_FLAG__ffile_prefix_map__Users_clausklein_Workspace_cpp_cxx23_scope_build__deps_catch2_src__ - Success
Examples to be built: scope_example;unique_resource;unique_resource-file
-- Configuring done (8.6s)
-- Generating done (0.1s)
-- Build files have been written to: /Users/clausklein/Workspace/cpp/cxx23/scope/build

bash-5.2$ cmake --version
cmake version 4.0.2

CMake suite maintained and supported by Kitware (kitware.com/cmake).
bash-5.2$ 

CMakeLists.txt Outdated
set(CMAKE_EXPERIMENTAL_CXX_IMPORT_STD
"d0edc3af-4c50-42ea-a356-e2862fe7a444"
)
set(CMAKE_CXX_STANDARD 26)
Copy link
Member

Choose a reason for hiding this comment

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

In discussions at c++now about this we made the decision that we weren't setting the standard in our cmake so as to not override the user options. If the library requires a certain level well that needs to be check in header and cause compile failure. That's the better approach since a user might just lift this code and not use our cmake at all

CMakeLists.txt Outdated
@@ -1,6 +1,14 @@
# SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
# TODO(CK): set(CMAKE_CXX_MODULE_STD 1)
if(CMAKE_CXX_MODULE_STD)
set(CMAKE_EXPERIMENTAL_CXX_IMPORT_STD
Copy link
Member

Choose a reason for hiding this comment

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

In case it isn't clear from the other PR, I removed all aspirations of using import std; for the moment -- I'm really only trying to get to import beman.scope; right now. For which, the claim is non of this is needed. That said happy to experiment here...

Copy link
Member

Choose a reason for hiding this comment

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

After pulling your branch locally -- I commented out all of this section and it still works with clang-20 and g++15

@JeffGarland
Copy link
Member

Regardless of my commentary above -- I've confirmed this actually works on g++15 and clang++20. And we have modules lift-off? Let's see how far we can take this...

@JeffGarland JeffGarland requested a review from nickelpro May 31, 2025 12:10
@ClausKlein ClausKlein marked this pull request as draft May 31, 2025 14:25
@ClausKlein ClausKlein force-pushed the feature/use-cxx-module branch from 3572928 to 4b1e959 Compare May 31, 2025 14:34
@@ -166,10 +166,10 @@ jobs:
matrix:
compilers:
- class: GNU
version: 14
version: 15
Copy link
Member

Choose a reason for hiding this comment

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

so I pulled this change into my branch, but it looks like we need a change to the test container because it's unable to find gcc-15

Copy link
Member

@wusatosi wusatosi May 31, 2025

Choose a reason for hiding this comment

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

We don't have testing container up for gcc-15 yet.
No package managers for ubuntu 24.04 ships it for now.
This is high priority to me, but if anyone want to work on it please do.
See: bemanproject/infra#18

@@ -80,7 +80,7 @@ jobs:
- name: Install Ninja
uses: lukka/get-cmake@latest
with:
cmakeVersion: "~3.25.0"
cmakeVersion: "~4.0.0"
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason we use inconsistent CMake version here

Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason we use inconsistent CMake version here

we need a newer version of cmake than 3.25 for modules -- needs to be 3.28 at least to work with 'named modules'. 4.x might well be required to support import std;

CMakeLists.txt Outdated
set(CMAKE_CXX_STANDARD_REQUIRED OFF)
endif()

cmake_minimum_required(VERSION 3.28...4.0)
Copy link
Member

Choose a reason for hiding this comment

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

as previously discussed, no upper bound for cmake version requirement.

Comment on lines +9 to +10
SYSTEM
# FIXME: FIND_PACKAGE_ARGS 3.8.1
Copy link
Member

Choose a reason for hiding this comment

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

What is this?

@ClausKlein ClausKlein force-pushed the feature/use-cxx-module branch 2 times, most recently from b8efde2 to 27c2426 Compare May 31, 2025 19:08
@ClausKlein
Copy link
Author

ClausKlein commented Jun 1, 2025

@ClausKlein ClausKlein marked this pull request as ready for review June 1, 2025 03:05
@ClausKlein ClausKlein force-pushed the feature/use-cxx-module branch from 27c2426 to 34e9c54 Compare June 1, 2025 04:02
@wusatosi
Copy link
Member

wusatosi commented Jun 1, 2025

This works on CI except for g++-15: https://github.com/ClausKlein/scope/actions/runs/15366704399

gcc 15 is not distrbuted in any package managers... We can ignore it before we have our own infra to deploy them

@ClausKlein ClausKlein marked this pull request as draft June 1, 2025 21:16
@ClausKlein
Copy link
Author

ClausKlein commented Jun 1, 2025

I have no time to rebase 3 times a day!

But I can wait until you finish your exploration.

We need at least cmake v3.28!

We need to set CMAKE_CXX_SCAN_FOR_MODULES too!

Fix compiler program name for apple-clang

Prevent unintended define of HAS_STDLIB_MODULES

Make it more readable

The final cleanup

Prepare run-clang-tidy too
@ClausKlein ClausKlein force-pushed the feature/use-cxx-module branch from 130cbc8 to 50efc36 Compare June 3, 2025 02:41
@ClausKlein ClausKlein force-pushed the feature/use-cxx-module branch from 50efc36 to 30117e8 Compare June 3, 2025 02:43
@JeffGarland
Copy link
Member

So here's what I'd like to do with this PR. Can we split it into 2? The first one would be for a new issue to update presets and clang-tidy. That way we can just approve that stuff and merge it to main. It's only barely related to the modules work.

The second part would be for experimental import std support. Since that seems like it's in an extremely experimental state w.r.t. compilers and cmake I'm thinking we should keep that that on a branch and document that if people want to play with it they should use that branch. That's breaking some new ground, but it might be a nice way to push the envelope while keeping main focused on what works now. Thoughts?

@ClausKlein
Copy link
Author

I will remove the accidentally added presets (which need cmake 3.30),
but is seem we need to use cmake_minimum_required(VERSION 4.0)

Too this is only valid for on CMake minor version:

# valid only for cmake version 4.0.x
        set(CMAKE_EXPERIMENTAL_CXX_IMPORT_STD
            "a9e1cf81-9932-4810-974b-6eccaf14e457"
        )

@ClausKlein ClausKlein force-pushed the feature/use-cxx-module branch from 0d1895f to 2b89401 Compare June 6, 2025 11:26
@ClausKlein
Copy link
Author

ClausKlein commented Jun 6, 2025

But I have still problems with build tests on OSX:

bash-5.2$ ninja -C build examples/all 
ninja: Entering directory `build'
ninja: warning: build log version is too old; starting over
[19/19] Linking CXX executable examples/unique_resource

bash-5.2$ ninja -C build all_verify_interface_header_sets 
ninja: Entering directory `build'
[1/1] Building CXX object CMakeFiles/beman.scope_verify_interface_header_sets.dir/beman.scope_verify_interface_header_sets/beman/scope/scope.hpp.cxx.o

bash-5.2$ ninja -C build all -k 1
ninja: Entering directory `build'
[237/237] Linking CXX executable tests/test.module
FAILED: tests/test.module tests/test.module-b12d07c_tests.cmake /Users/clausklein/Workspace/cpp/cxx23/scope/build/tests/test.module-b12d07c_tests.cmake 
: && /usr/local/bin/g++-15  -Wl,-search_paths_first -Wl,-headerpad_max_install_names  tests/CMakeFiles/test.module.dir/module.test.cpp.o -o tests/test.module  _deps/catch2-build/src/libCatch2Main.a  libbeman.scope.a  _deps/catch2-build/src/libCatch2.a && cd /Users/clausklein/Workspace/cpp/cxx23/scope/build/tests && /usr/local/bin/cmake -D TEST_TARGET=test.module -D TEST_EXECUTABLE=/Users/clausklein/Workspace/cpp/cxx23/scope/build/tests/test.module -D TEST_EXECUTOR= -D TEST_WORKING_DIR=/Users/clausklein/Workspace/cpp/cxx23/scope/build/tests -D TEST_SPEC= -D TEST_EXTRA_ARGS= -D "TEST_PROPERTIES=SKIP_RETURN_CODE;4" -D TEST_PREFIX= -D TEST_SUFFIX= -D TEST_LIST=test.module_TESTS -D TEST_REPORTER= -D TEST_OUTPUT_DIR= -D TEST_OUTPUT_PREFIX= -D TEST_OUTPUT_SUFFIX= -D TEST_DL_PATHS= -D TEST_DL_FRAMEWORK_PATHS= -D CTEST_FILE=/Users/clausklein/Workspace/cpp/cxx23/scope/build/tests/test.module-b12d07c_tests.cmake -D ADD_TAGS_AS_LABELS=FALSE -P /Users/clausklein/Workspace/cpp/cxx23/scope/build/_deps/catch2-src/extras/CatchAddTests.cmake
Undefined symbols for architecture x86_64:
  "initializer for module std", referenced from:
      __static_initialization_and_destruction_1() in module.test.cpp.o
ld: symbol(s) not found for architecture x86_64
collect2: error: ld returned 1 exit status
ninja: build stopped: subcommand failed.
bash-5.2$ 

@ClausKlein ClausKlein requested a review from JeffGarland June 6, 2025 11:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants