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

Add ability to specify library directories for target rpaths #295

Merged

Conversation

vyasr
Copy link
Contributor

@vyasr vyasr commented Oct 25, 2022

Description

Python extension modules built using scikit-build and rapids-cmake are typically designed around a model where a C++ library is built and the resulting Cython extension modules must link to it. While it is in principle possible to use both scikit-build and rapids-cmake with only Cython, that is not the case for any of the major RAPIDS packages. When the C++ component is built via scikit-build and installed into the final package, it must also be discoverable by the extension modules. Currently this is accomplished by propagating a variable RAPIDS_CYTHON_CREATED_MODULES from rapids_cython_create_modules that calling functions can use to get the list of targets whose RPATHs need updating, and setting these RPATHs is the responsibility of the caller. However, this approach is error-prone and frequently results in incorrect RPATH specifications.

This PR automates the process by having the calling code provide a set of directories where compiled libraries may be placed. The RPATHs of all extension modules built via rapids_cython_create_modules are then automatically updated to include those library directories.

Checklist

  • I am familiar with the Contributing Guidelines.
  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.
  • The cmake-format.json is up to date with these changes.
  • I have added new files under rapids-cmake/
    • I have added include guards (include_guard(GLOBAL))
    • I have added the associated docs/ rst file and update the api.rst

@vyasr vyasr added feature request New feature or request non-breaking Introduces a non-breaking change 3 - Ready for Review Ready for review by team labels Oct 25, 2022
@vyasr vyasr self-assigned this Oct 25, 2022
@vyasr vyasr requested a review from a team as a code owner October 25, 2022 20:22
Copy link
Contributor

@robertmaynard robertmaynard left a comment

Choose a reason for hiding this comment

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

Here are some minor changes.

I think this is a good starting place but we should change the separation of concerns.

I think we should have create_modules.cmake just add the ORIGIN rpath entry, and have a secondary function that you call providing a target, and a set of directories and it will add those as INSTALL_RPATHS.

So instead of having the logic look like:

set_lib_dirs( ..... ) # propagate stuff via side channels to create_modules
create_modules( )

We have

create_modules()
set_rpath_entries(<cython_target> <dirs>)

rapids-cmake/cython/create_modules.cmake Outdated Show resolved Hide resolved
rapids-cmake/cython/set_lib_dirs.cmake Outdated Show resolved Hide resolved
rapids-cmake/cython/set_lib_dirs.cmake Outdated Show resolved Hide resolved
@vyasr vyasr requested a review from robertmaynard October 26, 2022 21:27
Copy link
Contributor

@robertmaynard robertmaynard left a comment

Choose a reason for hiding this comment

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

Overall looks great, have a couple of minor suggestions.

rapids-cmake/cython/add_rpath_entries.cmake Outdated Show resolved Hide resolved
rapids-cmake/cython/add_rpath_entries.cmake Outdated Show resolved Hide resolved
@vyasr
Copy link
Contributor Author

vyasr commented Nov 1, 2022

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 4e8f8d2 into rapidsai:branch-22.12 Nov 1, 2022
@vyasr vyasr deleted the feature/rapids_cython_lib_rpath branch November 1, 2022 16:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 - Ready for Review Ready for review by team feature request New feature or request non-breaking Introduces a non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants