Do not force C++11 compilation target

RESTFramework
1

I’ve compiled my ROOT installation with C++17 support. Since the CMake setup of REST forces C++11 (via CMAKE_CXX_STANDARD), compilation of REST fails on my machine. I need to manually change the standard to 17.

I think it would be nice if we found a way to allow newer standards. Possible solution that come to mind:

  • set CMAKE_CXX_STANDARD to 17. If CMAKE_CXX_STANDARD_REQUIRED is set to NO, cmake will automatically use the newest possible standard that the compiler supports.
    → However, as far as I’m aware CMake doesn’t support an additional “still require standard Y as a minimum”, which means that it’d potentially try to compile the code with a compiler that doesn’t meet C++11. I can imagine some CentOS or Scientific Linux versions would thus fail with weird compiler errors.
  • explicitly check for flags the compiler supports using CheckCXXCompilerFlag — CMake 3.7.2 Documentation, starting from c++17, c++14 and finally fail if c++11 isn’t available.

Since I’m not that experienced with CMake I didn’t want to make any of the changes. :slight_smile:

2

I am not very familiar with cmake either. But CMAKE_CXX_STANDARD_REQUIRED=NO seems as a solution for most of the people? @lobis @nkx

Then if we get to the point that someone gets into problems with a particular system, we may add a FLAG in the CMakeLists.txt for that particular system?

3

I am also not very familiar with cmake but as I have read on the documentation I believe setting CMAKE_CXX_STANDARD_REQUIRED=NO would be a solution in this case (actually I believe you don’t really need to set it as “NO” as this is the default value). If this flag is set to “NO” cmake will find the latests compatible standard (including the ones more modern than the standard specified in CMAKE_CXX_STANDARD in this case 14 and 17).

So in my opinion we should remove the line set(CMAKE_CXX_STANDARD_REQUIRED ON) from the REST CMakeLists.txt file.

Some useful information: https://crascit.com/2015/03/28/enabling-cxx11-in-cmake/

1 Like
4

I have seen your commit.

But after reading the forum topic again

Should we not increase also CMAKE_CXX_STANDARD to 17? So that it will use the latest standard.

5

Yes I was thinking about this too. In my opinion we should use the 17 standard as there are some useful features, for example when working on verifying that a file exists I found the library std::filesystem that is only in C++ 17 so I could not use it for compatibility reasons. (This is just an example I am sure checking a file exists can be done in C++ 11 without linking to boost or anything complex but I did not find out how).

The obvious problem with using C++ 17 is that if the system doesn’t have it installed and we are using new features in this standard there would be an error but if you also agree I think we should use the latests standard.

Another thing we have to keep in mind is to also increase the minimum CMake version (right now is I believe to 3.8 would suffice) if we want to use the 17 standard.

If you agree with this I can push the commit.

6

I thought increasing to C++ 17 was kind of optional. In order to solve compilation problems in other machines, as reported in the post.

If CMAKE_CXX_STANDARD_REQUIRED is set to NO, it will compile with C++ 11 if 17 is not available.

I think we do not have to force to use 17, but if people starts to add features that are only supported in 17, we will see if someone complains in a future when compiling, and then decide.

Checking that a file exists can be done in many ways. In REST it can be done using the helper method REST_StringHelper::fileExists.

In any case, I see no problem that you push CMAKE_CXX_STANDARD 17, CMAKE_CXX_STANDARD_REQUIRED NO, and see what happens.

I already compiled with that options and got not issues.

7

We can optinally switch to 17 with preprocessor, while keeping the 11 code:

#if __cplusplus > 201402L
...(c++17 code)
#else
...(default c++11 code)
#endif

Is the old cmake not compatible wil c++17? What will the problem be? The pipeline failed due to such high cmake version

https://pandax.physics.sjtu.edu.cn:8443/pandax-iii/REST_v2/builds/3039

Honestly, I am not very motivated to keep up everything with the latest standard. I think we should make REST compilable under at least c++11(gcc 4.8.5), and under as lower cmake version as possible. This reduces installation problems/pre-requests under different systems.

Indeed we should not force either 11 or 17 compilation target. Maybe some 17 compiler cannot roll back to generate 11 target, and cause problem. I didn’t notice this problem because it works fine on my system (gcc 7.3.0, ubuntu 18.04).

8

The list of options for CMAKE_CXX_STANDARD (11, 14 or 17) depends on the cmake version, in order detect 17 as a valid option you must increase the cmake version.

I also agree with the lowest cmake version possible argument (which if we need c++17 it is pretty high). After having some problems compiling REST with the new CMakeLists.txt options (in fact I reverted the changes after failing to build on a system with root built using c++17 cmake flag) I also believe we should stick to c++11 compatiblity and while it is true one can use preprocessor directives to include c++17 code without problems I think we should avoid this as much as possible as it will increase complexity / maintainability (in my opinion).

On another topic, I have been trying to get the pipeline working on our GitLab with your .yml configuration. Have you thought on using the GitLab container registry? Did you configure the runners yourself (local runners) or are you using the Kubernetes cluster? I am thinking about enabling the CI/CD capabilities on our GitHub it seems doable but quite an effort, maybe it would make sense to use some shared platform for this?

Thanks.

9

So, it is perhaps better if you open a new topic? You may invoke Kaixiang in the new topic by using @nkx