rapidsai / rapids-cmake

https://docs.rapids.ai/api/rapids-cmake/stable/
Apache License 2.0
27 stars 44 forks source link

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

Closed vyasr closed 2 months ago

vyasr commented 2 months ago

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

robertmaynard commented 2 months ago

Changes look correct to me.

vyasr commented 2 months ago

We've now confirmed that this fix works on https://github.com/rapidsai/cudf/pull/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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

/merge