Ensure cuco export set is installed in cmake build

[jlowe]

Fixes #11146

Adds the cuco dependency information to the install exports for cudf. Verified this change fixes the spark-rapids-jni build.

[jbrennan333]

+1 this looks good to me

[vyasr]

This makes sense, but I am confused why the below rapids_export_package is not having the same effect. The error is definitely occurring for you when BUILD_SHARED_LIBS=OFF, right? So the rapids_export_package call should be happening?

[jlowe]

Verified via CMakeCache file that BUILD_SHARED_LIBS is indeed OFF. No idea why the original code isn't installing the cuco cmake files as it should.

[vyasr]

I'm fine with this change since it seems like we should be doing it anyway (we should probably remove the manual rapids_export_package logic below since it's redundant now, at least in theory), but I'm just going to play with the build a little to try to understand why this is happening to make sure that we have the right fix and aren't breaking the shared lib use case in some surprising way.

[robertmaynard]

This should only be added when BUILD_SHARED_LIBS is a false value.

[jlowe]

If the cuco cmake files are only installed when building static libraries, how does rapids_find_package(cudf) work in the shared library case? Or is cuco not listed as a dependency of libcudf in that case?

[vyasr]

Also curious about this.

[robertmaynard]

It isn't listed as a dependency in those cases as the headers aren't exposed as part of the public API.

We only need to expose it for static linking.

[vyasr]

I experimented this morning but wasn't able to track down the underlying issue here, or why this wasn't necessary prior to #11139. Chatted with @jlowe offline and we're going to wait for @robertmaynard to take a look on Monday to confirm that there aren't concerns that we're missing here before we merge.

[robertmaynard]

I experimented this morning but wasn't able to track down the underlying issue here, or why this wasn't necessary prior to #11139. Chatted with @jlowe offline and we're going to wait for @robertmaynard to take a look on Monday to confirm that there aren't concerns that we're missing here before we merge.

The reason we some new logic is that rapids_cpm_cuco doesn't install the headers when it isn't part of an INSTALL_EXPORT_SET ( https://github.com/rapidsai/rapids-cmake/blob/branch-22.08/rapids-cmake/cpm/cuco.cmake#L77 ). Previously get_cucollections would install the files when BUILD_SHARED_LIBS was disabled ( via EXCLUDE_FROM_ALL ).

We need to remove cuco from the foreach(METADATA_KIND IN LISTS METADATA_KINDS) logic in the root CMakeLists.txt, and only have INSTALL_EXPORT_SET set for cuco when BUILD_SHARED_LIBS is OFF.

[codecov]

Codecov Report

❗ No coverage uploaded for pull request base (branch-22.08@93d912b). Click here to learn what that means.
The diff coverage is n/a.

@@               Coverage Diff               @@
##             branch-22.08   #11147   +/-   ##
===============================================
  Coverage                ?   86.33%           
===============================================
  Files                   ?      144           
  Lines                   ?    22751           
  Branches                ?        0           
===============================================
  Hits                    ?    19641           
  Misses                  ?     3110           
  Partials                ?        0           

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 93d912b...3943c7a. Read the comment docs.

[vyasr]

The reason we some new logic is that rapids_cpm_cuco doesn't install the headers when it isn't part of an INSTALL_EXPORT_SET ( https://github.com/rapidsai/rapids-cmake/blob/branch-22.08/rapids-cmake/cpm/cuco.cmake#L77 ). Previously get_cucollections would install the files when BUILD_SHARED_LIBS was disabled ( via EXCLUDE_FROM_ALL ).

I tried changing EXCLUDE_FROM_ALL manually in the definition of rapids_cpm_cuco to test this possibility, and it had no effect. In hindsight, perhaps my changes were being overwritten by new pulls of rapids-cmake. I was using the container for the Spark JNI and maybe that triggers a re-pull of rapids-cmake every time, erasing any local changes?

When EXCLUDE_FROM_ALL is set, is it still be possible to install cuco if a user manually did make cuco (or something analogous, I don't know if a corresponding target is automatically created for dependencies)?

[robertmaynard]

When EXCLUDE_FROM_ALL is set, is it still be possible to install cuco if a user manually did make cuco (or something analogous, I don't know if a corresponding target is automatically created for dependencies)?

When EXCLUDE_FROM_ALL is defined to True it won't be part of the make install / ninja install target, and requesting installation of the specifc files in other ways is considered undefined behavior.

[robertmaynard]

I tried changing EXCLUDE_FROM_ALL manually in the definition of rapids_cpm_cuco to test this possibility, and it had no effect. In hindsight, perhaps my changes were being overwritten by new pulls of rapids-cmake.

I expect that is exactly what is happening.

[razajafri]

@gpucibot merge