xtensor-blas icon indicating copy to clipboard operation
xtensor-blas copied to clipboard

Moves target_link_libraries() and target_compile_features() from cmak…

Open jonathonl opened this issue 3 years ago • 4 comments

This fixes #169 and correctly determines BLAS and LAPACK location.

Both target_compile_features() and target_link_libraries() are meant to be used in CMakeLists.txt file (not the config). CMake will then export this information to the targets file.

The find_dependency() calls in the config file are supposed to be matched with find_package() calls in the CMakeLists.txt file.

jonathonl avatar Apr 02 '21 21:04 jonathonl

Is there a reason why BLAS is not available during the appveyor test? Should BLAS and LAPACK be optional?

jonathonl avatar Apr 02 '21 21:04 jonathonl

Thanks for the PR.

I think that your suggestion is not what is desired for general shipping of a library. If I’m not mistaken you will now encode the install-time dependencies in the config file. If install time is elsewhere (e.g. conda-build) the library will be left with irrelevant paths.

Instead, what is the current behaviour is that the dependencies are ‘searched’ when calling find_package(xtensor-blas) and are thus the correct ones for you current environment (and/or system)

tdegeus avatar Apr 03 '21 06:04 tdegeus

A second this is that what would fix #169 is to get rid of more modern convenience functions. As specified in the issue, replacing:

target_link_libraries(xtensor-blas INTERFACE ${BLAS_LIBRARIES} ${LAPACK_LIBRARIES})

by

set_target_properties(xtensor-blas PROPERTIES 
    INTERFACE_LINK_LIBRARIES "${BLAS_LIBRARIES};${LAPACK_LIBRARIES}"
)

Which retains the functionality fully but also works for CMake versions that do not yet have the convenience function target_link_libraries

tdegeus avatar Apr 03 '21 12:04 tdegeus

I see. This is not an issue with conda as they set placeholder prefix paths when building and then do a string replace when users run conda install (see https://stackoverflow.com/a/65136313/1034772). This is how it works for the conda-forge shrinkwrap package, which is a header-only library that depends on three other libraries.

But I agree that this approach would likely be a problem if you want to support other package managers beyond conda. Both deb and rpm packages would contain the build paths instead of the install paths.

I updated the PR to remove the find_package and target_link_library calls from CMakeLists.txt. INTERFACE_LINK_LIBRARIES is now set in the config file. I left target_compile_features (which has been available since v3.1) in CMakeLists.txt since this will be exported to the targets file.

jonathonl avatar Apr 03 '21 13:04 jonathonl