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

Correctly set the install location for nvcomp when using the proprietary binary #597

Merged
merged 9 commits into from
May 6, 2024

Conversation

vyasr
Copy link
Contributor

@vyasr vyasr commented Apr 26, 2024

Description

When we download the nvcomp binaries, we have to ensure that those binaries are also using the location determined by rapids_cmake_install_lib_dir.

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 bug Something isn't working non-breaking Introduces a non-breaking change labels Apr 26, 2024
@vyasr vyasr self-assigned this Apr 26, 2024
@robertmaynard
Copy link
Contributor

Changes look correct to me.

@vyasr vyasr marked this pull request as ready for review April 30, 2024 00:17
@vyasr vyasr requested a review from a team as a code owner April 30, 2024 00:17
@vyasr
Copy link
Contributor Author

vyasr commented Apr 30, 2024

We've now confirmed that this fix works on rapidsai/cudf#15483. However, it looks like it's incomplete. The cmake files that the nvcomp package contains include the lib directory, but since we've patched it to be lib64 (in cases where that's what rapids_cmake_install_lib_dir returns) the cmake is now incorrect. I think we'll have to do a manual modification of the nvcomp-release-targets.cmake file to replace ${_IMPORT_PREFIX}/lib/ with ${_IMPORT_PREFIX}/${lib_dir}/. @robertmaynard does this seem too hacky to you?

@robertmaynard
Copy link
Contributor

We've now confirmed that this fix works on rapidsai/cudf#15483. However, it looks like it's incomplete. The cmake files that the nvcomp package contains include the lib directory, but since we've patched it to be lib64 (in cases where that's what rapids_cmake_install_lib_dir returns) the cmake is now incorrect. I think we'll have to do a manual modification of the nvcomp-release-targets.cmake file to replace ${_IMPORT_PREFIX}/lib/ with ${_IMPORT_PREFIX}/${lib_dir}/. @robertmaynard does this seem too hacky to you?

Looks required for us to have a consistent install location when doing pip packages. So we will need to do it.
I would like to see the logic be placed in someplace like cmake/detail so that we can re-use it when needed

@vyasr
Copy link
Contributor Author

vyasr commented Apr 30, 2024

I've pushed out something that I think will work. Once I see it working with the cudf PR, I'll try and pull it out into a more generalized utility. @robertmaynard any suggestions on what you would want that to look like?

@vyasr
Copy link
Contributor Author

vyasr commented Apr 30, 2024

The Python wheel builds in the cudf repo worked, so I think this is functionally ready! So I just need to address Robert's request above.

@robertmaynard
Copy link
Contributor

I've pushed out something that I think will work. Once I see it working with the cudf PR, I'll try and pull it out into a more generalized utility. @robertmaynard any suggestions on what you would want that to look like?

We should have a test that verifies that transformation has occurred.

@vyasr
Copy link
Contributor Author

vyasr commented May 6, 2024

/merge

@rapids-bot rapids-bot bot merged commit 591e654 into rapidsai:branch-24.06 May 6, 2024
15 checks passed
@vyasr vyasr deleted the fix/nvcomp_lib_dir branch May 6, 2024 22:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working non-breaking Introduces a non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants