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

Link libdl as interface library #5179

Merged
merged 2 commits into from
Jul 7, 2022

Conversation

masterleinad
Copy link
Contributor

Fixes #4824. Basically, this just uses -ldl instead of the full library path for linking but we still try to find the library and add the include path for dlfcn.h.
An alternative to changing ${NAME}_FOUND_LIBRARIES for interface libraries, would be to not provide the library name in the initial call but add ${CMAKE_DL_LIBS} via target_link_libraries later.
I opted for the current approach since it feels less of a workaround, and interface libraries are not supposed to depend on hardcoded library paths anyway.

@masterleinad masterleinad added this to In progress in Kokkos Release 3.7 -- 2022 Target Date via automation Jul 5, 2022
@masterleinad masterleinad added this to the Tentative 3.7 Release milestone Jul 5, 2022
Copy link
Contributor

@nmm0 nmm0 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. I think it's better to link the library like you did rather than adding yet another code path in the cmake

@dalg24
Copy link
Member

dalg24 commented Jul 6, 2022

I don't fully understand how specifying the INTERFACE dependency type helped. Are we abusing the current find TPL implementation? Why would it not yield the same link flag as w/o it?

@@ -1 +1 @@
KOKKOS_FIND_IMPORTED(LIBDL HEADER dlfcn.h LIBRARY dl)
KOKKOS_FIND_IMPORTED(LIBDL HEADER dlfcn.h INTERFACE LIBRARIES ${CMAKE_DL_LIBS})
Copy link
Member

Choose a reason for hiding this comment

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

Why LIBRARY -> LIBRARIES?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This required fewer changes. KOKKOS_CREATE_IMPORTED_TPL (called by KOKKOS_FIND_IMPORTED) treats LIBRARY(referenced as TPL_LIBRARY) as IMPORTED_LOCATION but uses TARGET_LINK_LIBRARIES for LIBRARIES (referenced as TPL_LINK_LIBRARIES). In particular, we currently error out if KOKKOS_CREATE_IMPORTED_TPL called for an interface library gets a LIBRARY argument passed.

@masterleinad
Copy link
Contributor Author

I don't fully understand how specifying the INTERFACE dependency type helped.

Without INTERFACE, we try to create an imported target that needs an imported location (otherwise you get a lot of CMake warnings) and this is precisely what we try to avoid here, also see

MACRO(kokkos_create_imported_tpl NAME)
CMAKE_PARSE_ARGUMENTS(TPL
"INTERFACE"
"LIBRARY"
"LINK_LIBRARIES;INCLUDES;COMPILE_DEFINITIONS;COMPILE_OPTIONS;LINK_OPTIONS"
${ARGN})
IF (KOKKOS_HAS_TRILINOS)
#TODO: we need to set a bunch of cache variables here
ELSEIF (TPL_INTERFACE)
ADD_LIBRARY(${NAME} INTERFACE)
#Give this an importy-looking name
ADD_LIBRARY(Kokkos::${NAME} ALIAS ${NAME})
IF (TPL_LIBRARY)
MESSAGE(SEND_ERROR "TPL Interface library ${NAME} should not have an IMPORTED_LOCATION")
ENDIF()
#Things have to go in quoted in case we have multiple list entries
IF(TPL_LINK_LIBRARIES)
TARGET_LINK_LIBRARIES(${NAME} INTERFACE ${TPL_LINK_LIBRARIES})
ENDIF()
IF(TPL_INCLUDES)
TARGET_INCLUDE_DIRECTORIES(${NAME} INTERFACE ${TPL_INCLUDES})
ENDIF()
IF(TPL_COMPILE_DEFINITIONS)
TARGET_COMPILE_DEFINITIONS(${NAME} INTERFACE ${TPL_COMPILE_DEFINITIONS})
ENDIF()
IF(TPL_COMPILE_OPTIONS)
TARGET_COMPILE_OPTIONS(${NAME} INTERFACE ${TPL_COMPILE_OPTIONS})
ENDIF()
IF(TPL_LINK_OPTIONS)
TARGET_LINK_LIBRARIES(${NAME} INTERFACE ${TPL_LINK_OPTIONS})
ENDIF()
ELSE()
ADD_LIBRARY(${NAME} UNKNOWN IMPORTED)
IF(TPL_LIBRARY)
SET_TARGET_PROPERTIES(${NAME} PROPERTIES
IMPORTED_LOCATION ${TPL_LIBRARY})
ENDIF()
#Things have to go in quoted in case we have multiple list entries
IF(TPL_LINK_LIBRARIES)
SET_TARGET_PROPERTIES(${NAME} PROPERTIES
INTERFACE_LINK_LIBRARIES "${TPL_LINK_LIBRARIES}")
ENDIF()
IF(TPL_INCLUDES)
SET_TARGET_PROPERTIES(${NAME} PROPERTIES
INTERFACE_INCLUDE_DIRECTORIES "${TPL_INCLUDES}")
ENDIF()
IF(TPL_COMPILE_DEFINITIONS)
SET_TARGET_PROPERTIES(${NAME} PROPERTIES
INTERFACE_COMPILE_DEFINITIONS "${TPL_COMPILE_DEFINITIONS}")
ENDIF()
IF(TPL_COMPILE_OPTIONS)
SET_TARGET_PROPERTIES(${NAME} PROPERTIES
INTERFACE_COMPILE_OPTIONS "${TPL_COMPILE_OPTIONS}")
ENDIF()
IF(TPL_LINK_OPTIONS)
SET_TARGET_PROPERTIES(${NAME} PROPERTIES
INTERFACE_LINK_LIBRARIES "${TPL_LINK_OPTIONS}")
ENDIF()
ENDIF()
ENDMACRO()

@cz4rs
Copy link
Contributor

cz4rs commented Jul 6, 2022

This required fewer changes. KOKKOS_CREATE_IMPORTED_TPL (called by KOKKOS_FIND_IMPORTED) treats LIBRARY(referenced as TPL_LIBRARY) as IMPORTED_LOCATION but uses TARGET_LINK_LIBRARIES for LIBRARIES (referenced as TPL_LINK_LIBRARIES). In particular, we currently error out if KOKKOS_CREATE_IMPORTED_TPL called for an interface library gets a LIBRARY argument passed.

Ok, so the difference between passing LIBRARY and LIBRARIES is somewhat surprising. The documentation for the macro doesn't really help:

# ``LIBRARY <name>``
#
# If specified, this gives the name of the library to look for
#
# ``LIBRARIES <name1> <name2> ...``
#
# If specified, this gives a list of libraries to find for the package

And if I didn't look at the implementation, I would intuitively expect something like:

  FOREACH(LIB ${TPL_LIBRARIES})
    do-the-same-stuff-we-do-for-single-library()
  ENDFOREACH()

@crtrott crtrott added the Blocks Promotion Overview issue for release-blocking bugs label Jul 6, 2022
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 9beba32 into kokkos:develop Jul 7, 2022
Kokkos Release 3.7 -- 2022 Target Date automation moved this from In progress to Done Jul 7, 2022
petscbot pushed a commit to petsc/petsc that referenced this pull request May 16, 2023
Profiling is useful for us and downstream users and we should turn it on when possible

The Kokkos issue raised at kokkos/kokkos#4824 seems to have been fixed
by kokkos/kokkos#5179

This MR effectively reverts !4890
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Blocks Promotion Overview issue for release-blocking bugs
Development

Successfully merging this pull request may close these issues.

None yet

5 participants