-
Notifications
You must be signed in to change notification settings - Fork 321
Add a test for <nv/target> to validate old dialect support. #7241
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.
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
base: main
Are you sure you want to change the base?
Conversation
| add_library( | ||
| "nvtarget_cpp${dialect}" | ||
| OBJECT | ||
| nvtargettest.cpp | ||
| ) | ||
| add_library( | ||
| "nvtarget_cuda${dialect}" | ||
| OBJECT | ||
| nvtargettest.cu | ||
| ) | ||
|
|
||
| target_include_directories( | ||
| "nvtarget_cpp${dialect}" | ||
| PRIVATE "${libcudacxx_SOURCE_DIR}/include" | ||
| ) | ||
| target_include_directories( | ||
| "nvtarget_cuda${dialect}" | ||
| PRIVATE "${libcudacxx_SOURCE_DIR}/include" | ||
| ) | ||
|
|
||
| set_target_properties( | ||
| "nvtarget_cpp${dialect}" | ||
| PROPERTIES | ||
| COMPILE_OPTIONS "${headertest_warning_levels_host}" | ||
| CXX_STANDARD "${dialect}" | ||
| CXX_STANDARD_REQUIRED ON | ||
| ) | ||
| set_target_properties( | ||
| "nvtarget_cuda${dialect}" | ||
| PROPERTIES | ||
| COMPILE_OPTIONS "${headertest_warning_levels_device}" | ||
| CUDA_STANDARD "${dialect}" | ||
| CUDA_STANDARD_REQUIRED ON | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't this be a better solution:
if ("cxx_std_${dialect}" IN_LIST CMAKE_CXX_COMPILE_FEATURES)
add_library("nvtarget_cpp${dialect}" OBJECT nvtargettest.cpp)
target_compile_features("nvtarget_cpp${dialect}" PRIVATE "cxx_std_${dialect}")
endif()
if ("cuda_std_${dialect}" IN_LIST CMAKE_CUDA_COMPILE_FEATURES)
add_library("nvtarget_cuda${dialect}" OBJECT nvtargettest.cu)
target_compile_features("nvtarget_cuda${dialect}" PRIVATE "cuda_std_${dialect}")
endif()It will check whether the compiler even supports the dialect first
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I'll swap it out for that, I'm operating off the assumption that we don't have to deal with incompatible compilers by now.
😬 CI Workflow Results🟥 Finished in 5h 54m: Pass: 99%/250 | Total: 6d 01h | Max: 5h 53m | Hits: 84%/346083See results here. |
| if (MSVC) | ||
| set( | ||
| nvtarget_dialects | ||
| 14 | ||
| 17 | ||
| 20 | ||
| ) | ||
| else() | ||
| set( | ||
| nvtarget_dialects | ||
| 11 | ||
| 14 | ||
| 17 | ||
| 20 | ||
| ) | ||
| endif() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe we want to run this in all configurations and C++20 is only available in some, so we should make sure this is supported
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would be solved by checking the available compiler features
Description
closes #7196
Checklist