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

Use blasfeoConfig.cmake when building with BUILD_WITH_BLASFEO=OFF #22

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

traversaro
Copy link
Contributor

This PR is meant more as a proposal/discussion rather then a finished PR.

While removing the patch #18 from the conda-forge recipe of fatrop as 0.0.4 release of fatrop was released, I noticed the only remaining patch was this one.

Basically, blasfeo when installed installs a blasfeoConfig.cmake file, so find_package can be used directly instead of manually calling find_path and find_library and creating an imported target. This works fine in my tests, but if you have any setup with an external blasfeo you want to support, feel free to ask (for example, we could simply add /opt/blasfeo to the PATHS argument of find_package if useful).

fyi @nim65s as he could have an opinion on this based on nix packaging of fatrop.


# Set the path to the include directory
set_target_properties(blasfeo PROPERTIES INTERFACE_INCLUDE_DIRECTORIES ${BLASFEO_INCLUDE_DIR})
find_package(blasfeo REQUIRED)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
find_package(blasfeo REQUIRED)
find_package(blasfeo REQUIRED CONFIG)

If the goal is to use blasfeoConfig.cmake, we can make that explicit

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Personally I am not a big fan of this, but this is mostly personal taste/bikeshedding. If someone wants to use fatrop in a bigger CMake project via add_subdirectory/FetchContent, they may want to override the logic happening in find_package(blasfeo REQUIRED) with something custom, for whatever reason. This can be done by adding a Findblasfeo.cmake in the CMAKE_MODULE_PATH, but this will not work if downstream projects manually add CONFIG in their find_package calls. However, I do not feel strongly about this, so if mantainers perfer to have it it is ok for me to have it.

Copy link
Contributor

@nim65s nim65s left a comment

Choose a reason for hiding this comment

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

The previous version was working for me, but I agree that the proposed one would improve the packaging.
I can confirm that it works too for me :)

Maybe a little message(STATUS "Found blasfeo: ${blasfeo_DIR}") would be nice too.

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.

2 participants