Fix find_package case for pre-installed Ginkgo#100
Conversation
- Use find_package(Ginkgo) instead of find_package(ginkgo) to match the upstream GinkgoConfig.cmake naming on case-sensitive filesystems - Guard install(IMPORTED_RUNTIME_ARTIFACTS) with if(NOT Ginkgo_FOUND) since bare targets (ginkgo, ginkgo_device, ...) only exist when Ginkgo is built from source via FetchContent, not when using a pre-installed package
|
Thanks! I am also currently experimenting distributing ginkgo via wheels or similar. I think we can have a look at pytorch or jax. |
The install(DIRECTORY) used an absolute DESTINATION (${Python_SITELIB})
which bypasses CMAKE_INSTALL_PREFIX. Since py-build-cmake uses a staging
directory as the install prefix to collect files for the wheel, the
absolute path wrote files directly to the real site-packages instead of
the staging area, resulting in wheels missing the compiled .so binding.
- Use relative DESTINATION (${PY_BUILD_CMAKE_MODULE_NAME}) so files are
installed under CMAKE_INSTALL_PREFIX where py-build-cmake picks them up
- Add trailing / to source DIRECTORY to install contents, not the
directory itself (avoids pyGinkgo/pyGinkgo/ nesting)
Nice! I just managed to get conda packages for ginkgo working with both cuda and cpu builds, with and without mpi! If you want to test it, there's a recipe here: https://github.com/rho-novatron/staged-recipes/tree/ginkgo . The |
There was a problem hiding this comment.
Pull request overview
Fixes CMake configuration and install logic so a pre-installed Ginkgo (e.g., conda) is discoverable on case-sensitive systems and wheel builds install into the staging prefix correctly.
Changes:
- Switch
find_packagecall tofind_package(Ginkgo QUIET)and update the corresponding_FOUNDvariable usage. - Update install rules to use a relative
DESTINATIONand avoid nesting by adding a trailing/to the installed directory. - Guard installation of Ginkgo runtime artifacts so it only runs when Ginkgo is fetched/built from source.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| install(DIRECTORY ${CMAKE_LIBRARY_OUTPUT_DIRECTORY}/ | ||
| DESTINATION ${PY_BUILD_CMAKE_MODULE_NAME}) |
There was a problem hiding this comment.
PY_BUILD_CMAKE_MODULE_NAME is used as the install() destination, but it isn’t defined anywhere in this project (only provided by py-build-cmake). This breaks the documented plain-CMake workflow (cmake --install .) because the install destination becomes empty and files get installed into the install prefix root. Consider adding a fallback (e.g., default to ${PROJECT_NAME} or ${Python_SITELIB}/${PROJECT_NAME} when PY_BUILD_CMAKE_MODULE_NAME is not defined), or make the destination configurable via an option/cache variable.
| message(STATUS "Install path: ${Python_SITELIB}") | ||
| install(DIRECTORY ${CMAKE_LIBRARY_OUTPUT_DIRECTORY} | ||
| DESTINATION ${Python_SITELIB}) | ||
| install( | ||
| IMPORTED_RUNTIME_ARTIFACTS | ||
| ginkgo | ||
| ginkgo_device | ||
| ginkgo_hip | ||
| ginkgo_cuda | ||
| ginkgo_omp | ||
| ginkgo_dpcpp | ||
| ginkgo_reference | ||
| DESTINATION | ||
| ${Python_SITELIB}/${PROJECT_NAME}) | ||
| install(DIRECTORY ${CMAKE_LIBRARY_OUTPUT_DIRECTORY}/ |
There was a problem hiding this comment.
The status message still prints Install path: ${Python_SITELIB}, but the install destination was changed to ${PY_BUILD_CMAKE_MODULE_NAME}. This is now misleading when diagnosing packaging/install issues; it should report the actual computed install destination (including any fallback logic) rather than the old Python_SITELIB value.
There was a problem hiding this comment.
@copilot apply changes based on this feedback
| set(CMAKE_INSTALL_RPATH ${Python_SITELIB}/${PROJECT_NAME}) | ||
|
|
There was a problem hiding this comment.
CMAKE_INSTALL_RPATH is still set using the absolute ${Python_SITELIB}/${PROJECT_NAME} path, even though installation is now intended to go through a staging prefix for wheel builds. This can embed a build-environment-specific absolute RPATH into the extension, which is problematic for wheel relocatability. Consider setting an RPATH relative to the installed module location (e.g., $ORIGIN) or deriving it from the actual install destination instead of Python_SITELIB.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Problem
Two CMake issues prevented using a pre-installed Ginkgo (e.g. from conda) and broke wheel packaging:
Case mismatch in
find_package:find_package(ginkgo QUIET)(lowercase) fails to find a pre-installed Ginkgo on case-sensitive filesystems (Linux). Ginkgo upstream installs its CMake config asGinkgoConfig.cmakeinlib/cmake/Ginkgo/, but CMake'sfind_package(ginkgo)searches forginkgoConfig.cmake— a case mismatch.Absolute install destination breaks wheel packaging:
install(DIRECTORY ... DESTINATION ${Python_SITELIB})used an absolute path, which bypassesCMAKE_INSTALL_PREFIX. Since py-build-cmake uses a staging directory as the install prefix to collect files for the wheel, the absolute destination wrote files directly to site-packages — the compiled.sobinding was never included in the wheel.Additionally,
install(IMPORTED_RUNTIME_ARTIFACTS ginkgo ...)references bare targets that only exist when Ginkgo is built from source via FetchContent. When using a pre-installed Ginkgo, these targets don't exist and the build fails.Changes
find_package(Ginkgo QUIET)instead offind_package(ginkgo QUIET)to matchGinkgoConfig.cmakeinstall(IMPORTED_RUNTIME_ARTIFACTS ...)withif(NOT Ginkgo_FOUND)— Ginkgo shared libraries only need to be bundled when built from sourceDESTINATION(${PY_BUILD_CMAKE_MODULE_NAME}) instead of absolute${Python_SITELIB}so files are installed into the staging area where py-build-cmake picks them up for the wheel/to sourceDIRECTORYto install contents, not the directory itself (avoidspyGinkgo/pyGinkgo/nesting)Testing
Verified that:
pip wheel .produces a wheel containing the compiled.sobinding (~823KB vs 12KB before)pip install <wheel>followed byimport pyGinkgosucceeds