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

Moves target_link_libraries() and target_compile_features() from cmak… #193

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

jonathonl
Copy link
Contributor

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.

…e config to lists file and adds find_package() for BLAS and LAPACK.
@jonathonl
Copy link
Contributor Author

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

@tdegeus
Copy link
Member

tdegeus commented Apr 3, 2021

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
Copy link
Member

tdegeus commented Apr 3, 2021

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

@jonathonl
Copy link
Contributor Author

jonathonl commented Apr 3, 2021

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cmake error Cannot specify link libraries for target "xtensor-blas" which is not built
2 participants