Detect more c++ standards and make sure useful message actually appears #635
Conversation
CMakeLists.txt
Outdated
| elseif("cxx_std_14" IN_LIST ROOT_COMPILE_FEATURES) | ||
| set(ROOT_CXX_STANDARD 14) | ||
| elseif("cxx_std_11" IN_LIST ROOT_COMPILE_FEATURES) | ||
| set(ROOT_CXX_STANDARD 11) |
There was a problem hiding this comment.
Is there a reason not to put these in order? 11, 14, 17, 20, else
There was a problem hiding this comment.
Mainly laziness. ;) I will order them
There was a problem hiding this comment.
Or simply a loop
foreach(std IN ITEMS 20 17 14 11)
if("cxx_std_${std}" IN_LIST ROOT_COMPILE_FEATURES)
set(ROOT_CXX_STANDARD ${std})
break()
endif()
endforeach()
CMakeLists.txt
Outdated
| set(ROOT_CXX_STANDARD 17) | ||
| elseif("cxx_std_20" IN_LIST ROOT_COMPILE_FEATURES) | ||
| set(ROOT_CXX_STANDARD 20) | ||
| else() | ||
| message(FATAL_ERROR "ROOT C++ could not be detected") | ||
| message(WARNING "ROOT c++ standard could not be detected") |
There was a problem hiding this comment.
This I don't understand, if the standard can't be detected then that's possibly going to cause some problems later on, no? And after 11 to 20 are included this shouldn't happen anyway? Didn't ROOT change not too long ago how they define the standard? That now would build fine but with maybe different standards in ROOT and podio.
There was a problem hiding this comment.
IIUC an undefined ROOT_CXX_STANDARD would be caught by this:
Lines 101 to 103 in 76990a9
Which would at least tell people what a potential problem could be rather, than a rather cryptic error about not being able to detect a c++ standard.
The ROOT_CXX_STANDARD is only available from 6.32 onwards. So we still need this determination. From 6.30 onwards c++17 is required. I can't remember now when they switched the default c++ standard to 17, but given that I got an email today that someone ran into this issue with 6.28, I suppose not too long ago.
There was a problem hiding this comment.
Ah yes I was thinking cxx_std had been changed but it was ROOT_CXX_STANDARD: #540.
BEGINRELEASENOTES
ENDRELEASENOTES
Previously, if the c++ standard was not 17 or 20 it was not detected, and we failed hard with
ROOT C++ could not be detected, which doesn't tell the user anything. Demoting that to a warning makes the actually useful message that we have appear.