-
Notifications
You must be signed in to change notification settings - Fork 407
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
Export relevant kokkos options #6142
Export relevant kokkos options #6142
Conversation
To elaborate a bit, I am working towards properly exporting variables such as diff --git a/cmake/KokkosConfigCommon.cmake.in b/cmake/KokkosConfigCommon.cmake.in
index 63949e561..597aff5b0 100644
--- a/cmake/KokkosConfigCommon.cmake.in
+++ b/cmake/KokkosConfigCommon.cmake.in
@@ -10,6 +10,9 @@ SET(Kokkos_CXX_STANDARD @KOKKOS_CXX_STANDARD@)
FOREACH(DEV ${Kokkos_DEVICES})
SET(Kokkos_ENABLE_${DEV} ON)
ENDFOREACH()
+FOREACH(OPT ${Kokkos_OPTIONS})
+ SET(Kokkos_ENABLE_${OPT} ON)
+ENDFOREACH()
IF(Kokkos_ENABLE_CUDA)
SET(Kokkos_CUDA_ARCHITECTURES @KOKKOS_CUDA_ARCHITECTURES@) I just don't want us to export all sort of garbage variables. |
I'm wondering if we should rather whitelist options to export. |
Both have pros and cons. If no one feel too strongly about it I would just go with the blacklist. We might accidentally export more than we intended but we will fix it. |
I would prefer if we already do the
part in this pull request so that we have more uniform behavior already between building as part of |
I thought the "what" we export was a bit orthogonal to our exporting variables but I added to this PR in the hope we reach consensus. |
Ping |
Kokkos_ENABLE_BENCHMARKS | ||
Kokkos_ENABLE_EXAMPLES | ||
Kokkos_ENABLE_TESTS | ||
Kokkos_ENABLE_HEADER_SELF_CONTAINMENT_TESTS | ||
Kokkos_ENABLE_COMPILER_WARNINGS | ||
) |
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 would probably add DEPRECATION_WARNINGS
if you want to Kokkos_ENABLE_CUDA_RELOCATABLE_CODE
anyway.
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.
The deprecation warnings are different. Although I am not convinced it is really useful, in contrast to the ones I listed, it is something that is observable downstream.
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.
Sure. I can see that KOKKOS_ENABLE_DEPRECATED_CODE_*
might be relevant but DEPRECATION_WARNINGS
could also be an impl variable.
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 think exporting the options is the important change here. We can also work on the list of non-exported options elsewhere.
8fa3b9d
to
a77990a
Compare
a77990a
to
3f565bb
Compare
One argument for whitelist would be readability: it's probably more important to see which options we allow to be exported versus things we don't. |
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.
Looks good to me, no strong preference on whitelist vs blacklist.
Honor
Kokkos_OPTIONS_NOT_TO_EXPORT
list (that is used when building as part of Trilinos) and add a few options that it makes no sense to export to the list.For instance, no one care whether I enabled self containment tests when finding an install of Kokkos.
We might want to clean further but I think it is already a step in the right direction.
Here are the changes on my local
KokkosConfigCommon.cmake