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 support for a prefix in Cython module targets #198

Merged
merged 13 commits into from
May 27, 2022

Conversation

vyasr
Copy link
Contributor

@vyasr vyasr commented May 25, 2022

A package composed of subpackages may contain multiple Cython modules with the same filename at different paths i.e. in different parts of the package directory tree. Since we use the filenames to construct the CMake library targets, the resulting library target names will conflict unless there is a way to customize them. This PR adds that functionality. Note that because of the way that scikit-build's Cython is designed, we need to ensure that the generated shared library is named according to the original filename; the prefix should only be applied to the CMake target to avoid conflicts within CMake.

@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 May 25, 2022
@vyasr vyasr requested a review from a team as a code owner May 25, 2022 22:24
@vyasr vyasr self-assigned this May 25, 2022
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.

testing/cython/create_modules/CMakeLists.txt Outdated Show resolved Hide resolved
testing/cython/create_modules_with_prefix/CMakeLists.txt Outdated Show resolved Hide resolved
Co-authored-by: Robert Maynard <robertjmaynard@gmail.com>
RAPIDS.cmake Outdated Show resolved Hide resolved
@vyasr vyasr requested a review from robertmaynard May 27, 2022 14:45
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.

LGTM, merge whenever you want

@vyasr
Copy link
Contributor Author

vyasr commented May 27, 2022

Thanks @robertmaynard. I applied your suggestions and undid the changes to RAPIDS.cmake, so we should be good to merge unless you spot anything else on a final review.

@robertmaynard
Copy link
Contributor

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 8e42fec into rapidsai:branch-22.08 May 27, 2022
rapids-bot bot pushed a commit to rapidsai/cudf that referenced this pull request May 27, 2022
This PR changes the Python build system for cudf to use scikit-build and leverage CMake under the hood.

This PR depends on rapidsai/rapids-cmake#198. Once that PR is merged, I can update the pull of rapids-cmake into the cudf Python CMake build.

Authors:
  - Vyas Ramasubramani (https://github.com/vyasr)

Approvers:
  - Robert Maynard (https://github.com/robertmaynard)
  - AJ Schmidt (https://github.com/ajschmidt8)
  - Ashwin Srinath (https://github.com/shwina)

URL: #10919
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