-
Notifications
You must be signed in to change notification settings - Fork 56
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
base: master
Are you sure you want to change the base?
Conversation
…e config to lists file and adds find_package() for BLAS and LAPACK.
Is there a reason why BLAS is not available during the appveyor test? Should BLAS and LAPACK be optional? |
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 |
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 |
…ng INTERFACE_LINK_LIBRARIES in config.
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 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. |
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.