[PATCH v1 1/2] cmake: support cmake options like Seastar_CXX_DIALECT=c++20

[Kefu Chai]

before this change, we CMAKE_CXX_STANDARD is parsed from
Seastar_CXX_DIALECT using
string(SUBSTRING ${Seastar_CXX_DIALECT} 5 2 STANDARD)
this works fine with options like -std=gnu++17 where the
C++ standard starts at the 5th character, but this solution
trying to parse C++ standard has couple issues:

* we also support -std=c++17. the C++ standard part starts
at the 3rd character.
* some of us might want to use c++2a or gnu++2a, but "2a" is
not one of the CXX_STANDARD understood by CMake, per
https://cmake.org/cmake/help/latest/prop_tgt/CXX_STANDARD.html
* per https://gcc.gnu.org/onlinedocs/gcc/C-Dialect-Options.html,
> The name 'gnu++2a' is deprecated.

after this change, the command line is parsed using regex. and
show a warning message if the configured standard is deprecated
by GCC. in future, we might want to stop parsing the command gcc's
"-std" option once we have the luxury of using CMake 3.8 (for
CXX_STANDARD=17) or CMake 3.12 (for CXX_STANDARD=20). see
https://cmake.org/cmake/help/latest/prop_tgt/CXX_STANDARD.html

Signed-off-by: Kefu Chai <tcha...@gmail.com>
---
CMakeLists.txt | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/CMakeLists.txt b/CMakeLists.txt
index 3bc6613f..0eb19393 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -193,8 +193,13 @@ set (Seastar_CXX_DIALECT
"Compile Seastar according to the named C++ standard.")
if (Seastar_CXX_DIALECT)
# extract CXX standard (14/17/...) from Seastar_CXX_DIALECT
- string(SUBSTRING ${Seastar_CXX_DIALECT} 5 2 STANDARD)
- set(CMAKE_CXX_STANDARD ${STANDARD})
+ set(cxx_1z 17)
+ set(cxx_2a 20)
+ string (REGEX MATCH "[0-9].*$" CMAKE_CXX_STANDARD "${Seastar_CXX_DIALECT}")
+ if (CMAKE_CXX_STANDARD MATCHES "[^0-9]")
+ set (CMAKE_CXX_STANDARD ${cxx_${CMAKE_CXX_STANDARD}})
+ message (WARNING "${Seastar_CXX_DIALECT} is deprecated. Please use ${CMAKE_CXX_STANDARD} instead.")
+ endif ()
set(CMAKE_CXX_STANDARD_REQUIRED ON)
endif ()

--
2.36.1

[Kefu Chai]

as we should not have an empty "Seastar_CXX_DIALECT", if the upper
project reset it to an empty string, we'd have following compiling
failure:

c++: error: unrecognized command-line option ‘-std=’

Signed-off-by: Kefu Chai <tcha...@gmail.com>
---

CMakeLists.txt | 18 ++++++++----------
1 file changed, 8 insertions(+), 10 deletions(-)

diff --git a/CMakeLists.txt b/CMakeLists.txt
index 0eb19393..15ef481c 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -191,17 +191,15 @@ set (Seastar_CXX_DIALECT
CACHE
STRING

"Compile Seastar according to the named C++ standard.")

-if (Seastar_CXX_DIALECT)
- # extract CXX standard (14/17/...) from Seastar_CXX_DIALECT
- set(cxx_1z 17)
- set(cxx_2a 20)
- string (REGEX MATCH "[0-9].*$" CMAKE_CXX_STANDARD "${Seastar_CXX_DIALECT}")
- if (CMAKE_CXX_STANDARD MATCHES "[^0-9]")
- set (CMAKE_CXX_STANDARD ${cxx_${CMAKE_CXX_STANDARD}})
- message (WARNING "${Seastar_CXX_DIALECT} is deprecated. Please use ${CMAKE_CXX_STANDARD} instead.")
- endif ()
- set(CMAKE_CXX_STANDARD_REQUIRED ON)
+# extract CXX standard (14/17/...) from Seastar_CXX_DIALECT
+set(cxx_1z 17)
+set(cxx_2a 20)
+string (REGEX MATCH "[0-9].*$" CMAKE_CXX_STANDARD "${Seastar_CXX_DIALECT}")
+if (CMAKE_CXX_STANDARD MATCHES "[^0-9]")

+ set (CMAKE_CXX_STANDARD ${cxx_${CMAKE_CXX_STANDARD}})
+ message (WARNING "${Seastar_CXX_DIALECT} is deprecated. Please use ${CMAKE_CXX_STANDARD} instead.")

endif ()
+set(CMAKE_CXX_STANDARD_REQUIRED ON)

set (Seastar_CXX_FLAGS
""
--
2.36.1

[Avi Kivity]

On 17/05/2022 04.38, Kefu Chai wrote:
> before this change, we CMAKE_CXX_STANDARD is parsed from
> Seastar_CXX_DIALECT using
> string(SUBSTRING ${Seastar_CXX_DIALECT} 5 2 STANDARD)
> this works fine with options like -std=gnu++17 where the
> C++ standard starts at the 5th character, but this solution
> trying to parse C++ standard has couple issues:
>
> * we also support -std=c++17. the C++ standard part starts
> at the 3rd character.
> * some of us might want to use c++2a or gnu++2a, but "2a" is
> not one of the CXX_STANDARD understood by CMake, per
> https://cmake.org/cmake/help/latest/prop_tgt/CXX_STANDARD.html
> * per https://gcc.gnu.org/onlinedocs/gcc/C-Dialect-Options.html,
> > The name 'gnu++2a' is deprecated.
>
> after this change, the command line is parsed using regex. and
> show a warning message if the configured standard is deprecated
> by GCC. in future, we might want to stop parsing the command gcc's
> "-std" option once we have the luxury of using CMake 3.8 (for
> CXX_STANDARD=17) or CMake 3.12 (for CXX_STANDARD=20). see
> https://cmake.org/cmake/help/latest/prop_tgt/CXX_STANDARD.html

What are our options of dropping Seastar_CXX_DIALECT in favor of
CXX_STANDARD? I prefer to use standard cmake practice (though I dislike
cmake itself). But I'm worried it won't let us use c++2b before c++23 is
available, or that gnu++2b won't be supported. The latter is not really
important these days, gcc is deprecating extensions and the non-gnu
language is perfectly fine.


The patch itself looks fine, but better to remove all the code around
Seastar_CXX_DIALECT instead. Everyone will be happier.

[tcha...@gmail.com]

On Tuesday, May 17, 2022 at 5:02:54 PM UTC+8 Avi Kivity wrote:

On 17/05/2022 04.38, Kefu Chai wrote:
> before this change, we CMAKE_CXX_STANDARD is parsed from
> Seastar_CXX_DIALECT using
> string(SUBSTRING ${Seastar_CXX_DIALECT} 5 2 STANDARD)
> this works fine with options like -std=gnu++17 where the
> C++ standard starts at the 5th character, but this solution
> trying to parse C++ standard has couple issues:
>
> * we also support -std=c++17. the C++ standard part starts
> at the 3rd character.
> * some of us might want to use c++2a or gnu++2a, but "2a" is
> not one of the CXX_STANDARD understood by CMake, per
> https://cmake.org/cmake/help/latest/prop_tgt/CXX_STANDARD.html
> * per https://gcc.gnu.org/onlinedocs/gcc/C-Dialect-Options.html,
> > The name 'gnu++2a' is deprecated.
>
> after this change, the command line is parsed using regex. and
> show a warning message if the configured standard is deprecated
> by GCC. in future, we might want to stop parsing the command gcc's
> "-std" option once we have the luxury of using CMake 3.8 (for
> CXX_STANDARD=17) or CMake 3.12 (for CXX_STANDARD=20). see
> https://cmake.org/cmake/help/latest/prop_tgt/CXX_STANDARD.html


What are our options of dropping Seastar_CXX_DIALECT in favor of
CXX_STANDARD? I prefer to use standard cmake practice (though I dislike

i think it depends on the minimum cmake version specified by cmake_minimum_required(), in other words, what distros we want to support.

 

cmake itself). But I'm worried it won't let us use c++2b before c++23 is
available, or that gnu++2b won't be supported. The latter is not really

gnu++2b is an alias of gnu++23. which is used if 

- CXX_STANDARD is 23, and

- CMAKE_CXX_EXTENSIONS is ON (otherwise -std=c++23 is used)

the change to support C++23 in CMake was introduced in https://github.com/Kitware/CMake/commit/9fbbebe3d03f2576a8c5d6284c6f122b99e232a9 , which was included by cmake v3.20.3 . following are CMake versions in some popular distros:

- fedora 34 includes cmake v3.20.5. see https://fedora.pkgs.org/34/fedora-updates-x86_64/cmake-3.20.5-1.fc34.i686.rpm.html

- ubuntu jammy includes cmake v3.22.1. see https://packages.ubuntu.com/jammy/cmake

- centos 8 includes cmake v3.20.2 . see https://centos.pkgs.org/8/centos-appstream-aarch64/cmake-data-3.20.2-4.el8.noarch.rpm.html

- arch packages cmake v3.23.1. see https://archlinux.org/packages/extra/x86_64/cmake/

 

so considering the 

important these days, gcc is deprecating extensions and the non-gnu
language is perfectly fine.


The patch itself looks fine, but better to remove all the code around
Seastar_CXX_DIALECT instead. Everyone will be happier.

i am all for dropping Seastar_CXX_DIALECT in favor of CMAKE_CXX_STANDARD. the only hurdle is that we would have to bump up the version in "cmake_minimum_required (VERSION 3.5)" with minimum impacts. a possible solution for those who don't have access to a recent CMake could be to update `CMakeLists.txt` so:

- make CMAKE_CXX_STANDARD a cached user variable with default value of "17"

- if CMAKE_CXX_STANDARD is lower than 17 or greater than 23, error out.

- check for CMAKE_VERSION, if the specified CMAKE_CXX_STANDARD is not supported by cmake, error out with an error message proposing user to set CMAKE_CXX_FLAGS using cmake command line.

in addition to the cmake change, we also can improve the `configure.py` script, so it checks and sets CMAKE_CXX_FLAGS if necessary on behalf of user .

so if user has cmake v3.20.3 and up, "cmake -DCMAKE_CXX_STANDARD=20" would just work fine. for those who are stuck with older cmake, they can still use configure.py for setting things up.

what do you think?

[Avi Kivity]

It's likely (but maybe I'm optimistic) that a distro that support c++2b/2c will also have a matching cmake. If someone builds their own gcc, they can build their own cmake.

 

cmake itself). But I'm worried it won't let us use c++2b before c++23 is
available, or that gnu++2b won't be supported. The latter is not really

gnu++2b is an alias of gnu++23. which is used if 

- CXX_STANDARD is 23, and

- CMAKE_CXX_EXTENSIONS is ON (otherwise -std=c++23 is used)

the change to support C++23 in CMake was introduced in https://github.com/Kitware/CMake/commit/9fbbebe3d03f2576a8c5d6284c6f122b99e232a9 , which was included by cmake v3.20.3 . following are CMake versions in some popular distros:

- fedora 34 includes cmake v3.20.5. see https://fedora.pkgs.org/34/fedora-updates-x86_64/cmake-3.20.5-1.fc34.i686.rpm.html

- ubuntu jammy includes cmake v3.22.1. see https://packages.ubuntu.com/jammy/cmake

- centos 8 includes cmake v3.20.2 . see https://centos.pkgs.org/8/centos-appstream-aarch64/cmake-data-3.20.2-4.el8.noarch.rpm.html

- arch packages cmake v3.23.1. see https://archlinux.org/packages/extra/x86_64/cmake/

 

so considering the 

important these days, gcc is deprecating extensions and the non-gnu
language is perfectly fine.


The patch itself looks fine, but better to remove all the code around
Seastar_CXX_DIALECT instead. Everyone will be happier.

i am all for dropping Seastar_CXX_DIALECT in favor of CMAKE_CXX_STANDARD. the only hurdle is that we would have to bump up the version in "cmake_minimum_required (VERSION 3.5)" with minimum impacts. a possible solution for those who don't have access to a recent CMake could be to update `CMakeLists.txt` so:

- make CMAKE_CXX_STANDARD a cached user variable with default value of "17"

- if CMAKE_CXX_STANDARD is lower than 17 or greater than 23, error out.

- check for CMAKE_VERSION, if the specified CMAKE_CXX_STANDARD is not supported by cmake, error out with an error message proposing user to set CMAKE_CXX_FLAGS using cmake command line.

in addition to the cmake change, we also can improve the `configure.py` script, so it checks and sets CMAKE_CXX_FLAGS if necessary on behalf of user .

so if user has cmake v3.20.3 and up, "cmake -DCMAKE_CXX_STANDARD=20" would just work fine. for those who are stuck with older cmake, they can still use configure.py for setting things up.

what do you think?

I think it's reasonable. We should avoid duplicating cmake functionality, this is all fallout from the configure.py -> cmake move.

 

> Signed-off-by: Kefu Chai <tcha...@gmail.com>
> ---
> CMakeLists.txt | 9 +++++++--
> 1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/CMakeLists.txt b/CMakeLists.txt
> index 3bc6613f..0eb19393 100644
> --- a/CMakeLists.txt
> +++ b/CMakeLists.txt
> @@ -193,8 +193,13 @@ set (Seastar_CXX_DIALECT
> "Compile Seastar according to the named C++ standard.")
> if (Seastar_CXX_DIALECT)
> # extract CXX standard (14/17/...) from Seastar_CXX_DIALECT
> - string(SUBSTRING ${Seastar_CXX_DIALECT} 5 2 STANDARD)
> - set(CMAKE_CXX_STANDARD ${STANDARD})
> + set(cxx_1z 17)
> + set(cxx_2a 20)
> + string (REGEX MATCH "[0-9].*$" CMAKE_CXX_STANDARD "${Seastar_CXX_DIALECT}")
> + if (CMAKE_CXX_STANDARD MATCHES "[^0-9]")
> + set (CMAKE_CXX_STANDARD ${cxx_${CMAKE_CXX_STANDARD}})
> + message (WARNING "${Seastar_CXX_DIALECT} is deprecated. Please use ${CMAKE_CXX_STANDARD} instead.")
> + endif ()
> set(CMAKE_CXX_STANDARD_REQUIRED ON)
> endif ()
>

--
You received this message because you are subscribed to the Google Groups "seastar-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to seastar-dev...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/seastar-dev/a2e52ee1-d86c-412d-92d3-48d275a530b5n%40googlegroups.com.

[kefu chai]

OK. will create a more "ambitious" changeset to implement the proposed changes.

--
Regards
Kefu Chai