Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Allow CMake to configure with nvc++ #14370

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Allow CMake to configure with nvc++ #14370

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1d6a6d9
Build DAVIX with -noswitcherror when using nvc++
bernhardmgruber Nov 28, 2023
7396b43
Detect __cplusplus with nvc++
bernhardmgruber Nov 29, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions builtins/davix/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't that be upstreamed in the DAVIX repository?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably. Here is a PR: cern-fts/davix#110

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Merged upstream, but no new release of davix yet.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

# DAVIX uses the flag -fstack-protector-all, which nvc++ does not understand
set(DAVIX_CXX_FLAGS "${DAVIX_CXX_FLAGS} -noswitcherror")
endif()

ExternalProject_Add(DAVIX
URL ${DAVIX_URL}/davix-${DAVIX_VERSION}.tar.gz
Expand Down
8 changes: 6 additions & 2 deletions cmake/modules/RootConfiguration.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Member

@pcanal pcanal Jan 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is try_compile not working for NVHPC ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@pcanal pcanal Jan 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

else()
try_compile(has__cplusplus "${CMAKE_BINARY_DIR}" SOURCES "${CMAKE_SOURCE_DIR}/config/__cplusplus.cxx"
OUTPUT_VARIABLE __cplusplus_PPout)
string(REGEX MATCH "__cplusplus=([0-9]+)" __cplusplus "${__cplusplus_PPout}")
endif()
string(REGEX MATCH "__cplusplus[=| ]([0-9]+)" __cplusplus "${__cplusplus_PPout}")
set(__cplusplus ${CMAKE_MATCH_1}L)

configure_file(${PROJECT_SOURCE_DIR}/config/RConfigure.in ginclude/RConfigure.h NEWLINE_STYLE UNIX)
Expand Down
2 changes: 1 addition & 1 deletion config/__cplusplus.cxx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
#define _STRINGIFY(x) #x
#define STRINGIFY(x) _STRINGIFY(x)

// `#pragma message` is supported in well-known compilers including gcc, clang, icc, and MSVC
// `#pragma message` is supported in well-known compilers including gcc, clang, icc, and MSVC. But not nvc++.
#pragma message("__cplusplus=" STRINGIFY(__cplusplus))

int main(void)
Expand Down
Loading