-
Notifications
You must be signed in to change notification settings - Fork 5
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
base: 33-module-for-beman-scope
Are you sure you want to change the base?
Prepare use of CMAKE_CXX_MODULE_STD #35
Conversation
CMakeLists.txt
Outdated
set(CMAKE_EXPERIMENTAL_CXX_IMPORT_STD | ||
"d0edc3af-4c50-42ea-a356-e2862fe7a444" | ||
) | ||
set(CMAKE_CXX_STANDARD 26) |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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
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... |
3572928
to
4b1e959
Compare
.github/workflows/ci_tests.yml
Outdated
@@ -166,10 +166,10 @@ jobs: | |||
matrix: | |||
compilers: | |||
- class: GNU | |||
version: 14 | |||
version: 15 |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
.github/workflows/ci_tests.yml
Outdated
@@ -80,7 +80,7 @@ jobs: | |||
- name: Install Ninja | |||
uses: lukka/get-cmake@latest | |||
with: | |||
cmakeVersion: "~3.25.0" | |||
cmakeVersion: "~4.0.0" |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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.
SYSTEM | ||
# FIXME: FIND_PACKAGE_ARGS 3.8.1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this?
b8efde2
to
27c2426
Compare
This works on CI see: https://github.com/ClausKlein/scope/actions/runs/15398118700 |
27c2426
to
34e9c54
Compare
gcc 15 is not distrbuted in any package managers... We can ignore it before we have our own infra to deploy them |
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
130cbc8
to
50efc36
Compare
50efc36
to
30117e8
Compare
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 |
I will remove the accidentally added presets (which need cmake 3.30), Too this is only valid for on # valid only for cmake version 4.0.x
set(CMAKE_EXPERIMENTAL_CXX_IMPORT_STD
"a9e1cf81-9932-4810-974b-6eccaf14e457"
) |
0d1895f
to
2b89401
Compare
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$ |
Uh oh!
There was an error while loading. Please reload this page.