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.

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

Add Kokkos::all_libs alias target for compatibility with TriBITS/Trilinos #6157

Merged
merged 2 commits into from
May 25, 2023

Conversation

dalg24
Copy link
Member

@dalg24 dalg24 commented May 23, 2023

Proposed alternative to #6138

@bartlettroscoe is there a good reason Kokkos::all_libs cannot be an alias target?

@dalg24 dalg24 added the CMake label May 23, 2023
@bartlettroscoe
Copy link
Contributor

@bartlettroscoe is there a good reason Kokkos::all_libs cannot be an alias target?

@dalg24, you need both an ALIAS target and an exported IMPORTED target (the former is for users that pull in Kokkos with add_subdirectory(kokkos) and the latter is for users that pull in Kokkos using find_package(Kokkos)).

Copy link
Contributor

@bartlettroscoe bartlettroscoe left a comment

Choose a reason for hiding this comment

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

This does not provide a Kokkos::all_libs IMPORTED target in the generated KokkosConfig.cmake file (see for yourself).

@dalg24
Copy link
Member Author

dalg24 commented May 24, 2023

This does not provide a Kokkos::all_libs IMPORTED target in the generated KokkosConfig.cmake file (see for yourself).

I don't understand. The configured file KokkosConfigCommon.cmake which gets included from KokkosConfig.cmake does define the alias. What am I missing?

@masterleinad
Copy link
Contributor

I can confirm that both example/build_cmake_installed and example/build_cmake_in_tree work with Kokkos::all_libs using this pull request.

Copy link
Contributor

@bartlettroscoe bartlettroscoe left a comment

Choose a reason for hiding this comment

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

I looked over this too fast. This adds the Kokkos::all_libs target to both the current CMake project and the KokkosConfig.cmake file. But note this only addresses part of PR #6138. The other part is setting missing vars like Kokkos_ENABLE_CUDA_RELOCATABLE_DEVICE_CODE in the KokkosConfig.cmake file (see #6138 (comment)).

Copy link
Contributor

@masterleinad masterleinad left a comment

Choose a reason for hiding this comment

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

Maybe add a comment that we do this for Trilinos/TriBITS.

@dalg24
Copy link
Member Author

dalg24 commented May 24, 2023

The other part is setting missing vars like Kokkos_ENABLE_CUDA_RELOCATABLE_DEVICE_CODE in the KokkosConfig.cmake file (see #6138 (comment)).

This part has been (mostly) addressed by #6142

CMakeLists.txt Outdated Show resolved Hide resolved
cmake/KokkosConfigCommon.cmake.in Outdated Show resolved Hide resolved
@dalg24 dalg24 marked this pull request as ready for review May 25, 2023 03:18
Comment on lines +11 to +14
# CMake Error at <prefix>/lib/cmake/Kokkos/KokkosConfigCommon.cmake:10 (ADD_LIBRARY):
# ADD_LIBRARY cannot create ALIAS target "Kokkos::all_libs" because target
# "Kokkos::kokkos" is imported but not globally visible.
IF(CMAKE_VERSION VERSION_LESS "3.18")
Copy link
Member Author

Choose a reason for hiding this comment

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

How did I determine the cmake version?
When I submitted the original version, I had tested with my local setup that has v3.25.
I was able to reproduce the CI failures with cmake-3.16 (in the CUDA RDC build that does Kokkos_INSTALL_TESTING).
After reading https://discourse.cmake.org/t/alias-and-not-globally-visible-targets/6305 I tried 3.23.5 and it did not yield an error. Then I arbitrarily picked 3.18.0 which also worked followed by 3.17.0 that did not.

I expect this entry from the cmake 3.18 changelog is what resolved the issue.

Copy link
Contributor

@masterleinad masterleinad left a comment

Choose a reason for hiding this comment

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

Looks OK to me.

Copy link
Contributor

@cz4rs cz4rs left a 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.

@dalg24 dalg24 merged commit 2a5c949 into kokkos:develop May 25, 2023
28 checks passed
@dalg24 dalg24 deleted the kokkos_all_libs_alias_target branch May 25, 2023 12:50
@masterleinad masterleinad mentioned this pull request Jun 7, 2023
nliber pushed a commit to nliber/kokkos that referenced this pull request Jun 22, 2023
…ilinos (kokkos#6157)

* Add Kokkos::all_libs alias target for compatibility with Trilinos

* Add comments and fixup for cmake version less than 3.18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants