Allow CMake to configure with nvc++ #14370
Conversation
bernhardmgruber
requested review from
bellenot and
Axel-Naumann
as code owners
|
Can one of the admins verify this patch? |
|
@phsft-bot build |
|
Starting build on |
|
I'm not sure this is a good idea: We know that |
|
Build failed on ROOT-performance-centos8-multicore/soversion. Failing tests: |
Isn't one of those user the NVidia engineer(s) that needs to build ROOT to fix the compiler? |
| @@ -20,6 +20,10 @@ list(APPEND DAVIX_LIBRARIES ${DAVIX_PREFIX}/lib/${CMAKE_STATIC_LIBRARY_PREFIX}da | |||
| list(APPEND DAVIX_LIBRARIES ${DAVIX_PREFIX}/src/DAVIX-build/deps/curl-install/usr/lib/${CMAKE_STATIC_LIBRARY_PREFIX}curl${CMAKE_STATIC_LIBRARY_SUFFIX}) | |||
|
|
|||
| string(REPLACE "-Werror " "" DAVIX_CXX_FLAGS "${CMAKE_CXX_FLAGS} ") | |||
| if (CMAKE_CXX_COMPILER_ID STREQUAL "NVHPC") | |||
There was a problem hiding this comment.
Shouldn't that be upstreamed in the DAVIX repository?
There was a problem hiding this comment.
Probably. Here is a PR: cern-fts/davix#110
There was a problem hiding this comment.
Merged upstream, but no new release of davix yet.
There was a problem hiding this comment.
A version of Davix with this fix has been release, and the ROOT builtin davix was updated to it.
The first commit of this PR is not needed anymore.
| @@ -560,9 +560,13 @@ if(PYTHON_VERSION_STRING_Development_Other) | |||
| endif() | |||
|
|
|||
| #---RConfigure.h--------------------------------------------------------------------------------------------- | |||
| try_compile(has__cplusplus "${CMAKE_BINARY_DIR}" SOURCES "${CMAKE_SOURCE_DIR}/config/__cplusplus.cxx" | |||
| if (CMAKE_CXX_COMPILER_ID STREQUAL "NVHPC") | |||
| execute_process(COMMAND ${CMAKE_CXX_COMPILER} -dM -E /dev/null OUTPUT_VARIABLE __cplusplus_PPout) | |||
There was a problem hiding this comment.
Why is try_compile not working for NVHPC ?
There was a problem hiding this comment.
Using try_compile would work as well:
try_compile(has__cplusplus "${CMAKE_BINARY_DIR}" SOURCES "${CMAKE_SOURCE_DIR}/config/__cplusplus.cxx"
COMPILE_DEFINITIONS -dM -E OUTPUT_VARIABLE __cplusplus_PPout)
But it would be irritating, since the source file is not actually read and compiled by the compiler. I cannot put /dev/null here, because that does not exist on Windows :) The importance is to compile with -dM -E, which asks the compiler to dump all predefined macros. The pragma message in __cplusplus.cxx is not supported by nvc++, so nothing is printed during compilation.
There was a problem hiding this comment.
Ah so semantically we probably need:
try_compile(has__cplusplus ...)
if ( has__cplusplus is empty )
go the slower route
endif()
alternatively if it was actually resulted in a different code (but it likely is not)
has_pragma_message) = test (or aquire somehow) that compiler support pragma message
if (has_pragma_test)
use try_compiler
else()
use the slower route
endif()
i.e. this fix is not a priori dependent of a specific compiler.
They can fix their compiler by building upstream LLVM, I don't see the point. |
Humm ... so we are sure they is no other bad interactions? But anyway furthermore, if we put stuff that is 'needed' (i.e. handling genuine difference not bugs in the compiler) then we will be ready when they are. |
We don't know because we never get to that part: My memory is that to this date, we never had a working |
Test Results 9 files 9 suites 1d 21h 50m 56s ⏱️ Results for commit 7396b43. |
I fully agree here. Just because nvc++ cannot compile ROOT now does not mean we should make it even harder to get there. Especially, since I already invested the time to find out what's needed in cmake. And they are actually fairly small, so I am also not proposing a maintenance nightmare. |
IMHO it sends the wrong signal that |
A fair point but I fail to see how it has an consequence for this PR. As is the master does not reject ie. Not merging this PR would not send the signal that The 'right' way to express the point you make is: |
This PR is a successor to #14139, allowing CMake to configure ROOT when nvc++ is used as C++ compiler.