Update install rules to install cmake into lib directory

[kkraus14]

Fixes #1292

Updates the cmake install rules to install in lib/thrust/cmake instead of include/thrust/cmake as the include folder is not searched in a normal find_package(Thrust) call.

[germasch]

I was going to suggest something like this, so I like it ;)

I think, however, there may be an issue with how thrust is packaged, including, I suspect, internally at Nvidia. That is, the directory structure of this repo is such that one can just copy thrust/ to somewhere/include/thrust/ and cmake or make install are never even used. It looks like this required some non-standard handling of things, including have the .cmake files under include/thrust/ and also, e.g., not having a standard generated version file, but rather one that parses the version from the thrust header (which is elegant, but different from what's usually done).

I do strongly support having the installed cmake files in a standard location where they'll be found. I just can't see a way of achieving this without getting inconsistencies between the installed and uninstalled version of thrust. Maybe a somewhat less confusing way would be to move the .cmake files out of thrust/cmake/, so in that case at least they wouldn't appear in two different places depending on how thrust is used, but only the installed version would have them, under lib/cmake/. But obviously, in that case it's important to make sure that the CUDA toolkit and whatever SDKs ship with these config files, not just whatever is under include/thrust.

[alliepiper]

Our internal CTK packaging scripts don't pick these files up at all at the moment (#1294), so this won't break anything there.

I agree that this is the way to go -- leave them where they are in the source tree to have some measure of support for the legacy "just copy the thrust dir" usecase, but put them somewhere discoverable for installations.

I'll give this a try sometime this week and hopefully get it into 1.11.0 if all goes well.

[alliepiper]

@kkraus14 @germasch I've updated this (and NVIDIA/cub#217) to work for both source tree and install tree, and added a test to validate the install tree package.

If you get a chance to try this out, please let me know if you find any issues.

[germasch]

@allisonvacanti: This PR works for me, thanks!

I kinda like it better to have the config files in lib/cmake/{thrust,cub} rather than lib/{thrust,cub}/cmake, probably because I usually see the former. But either way works. Like you, I guess, I don't like all the empty directories. This is a way to avoid that: (And it also reverses to lib/cmake, though that's not the main point.)

diff --git a/cmake/ThrustInstallRules.cmake b/cmake/ThrustInstallRules.cmake
index 31db9142..a69ec78d 100644
--- a/cmake/ThrustInstallRules.cmake
+++ b/cmake/ThrustInstallRules.cmake
@@ -8,13 +8,8 @@ install(DIRECTORY "${Thrust_SOURCE_DIR}/thrust"
     PATTERN "*.inl"
 )

-# Unfortunately this creates a ton of empty directories, see issue:
-# https://gitlab.kitware.com/cmake/cmake/-/issues/19189
-install(DIRECTORY "${Thrust_SOURCE_DIR}/thrust"
-  TYPE LIB
-  FILES_MATCHING
-    PATTERN "cmake/*.cmake"
-    PATTERN "cmake/*.md"
+install(DIRECTORY "${Thrust_SOURCE_DIR}/thrust/cmake/"
+  DESTINATION "lib/cmake/thrust"
 )

 # Depending on how Thrust is configured, CUB's CMake scripts may or may not be
@@ -29,11 +24,7 @@ if (THRUST_INSTALL_CUB_HEADERS)
       PATTERN "*.cuh"
   )

-  # Unfortunately this creates a ton of empty directories, see issue:
-  # https://gitlab.kitware.com/cmake/cmake/-/issues/19189
-  install(DIRECTORY "${Thrust_SOURCE_DIR}/dependencies/cub/cub"
-    TYPE LIB
-    FILES_MATCHING
-      PATTERN "cmake/*.cmake"
+  install(DIRECTORY "${Thrust_SOURCE_DIR}/dependencies/cub/cub/cmake/"
+    DESTINATION "lib/cmake/cub"
   )
 endif()

[alliepiper]

That sounds fine to me. I avoided that the first time around because it was clunky to make work with the TYPE LIB specification, which handles packaging usecases where the desired library path isn't [prefix]/lib.

It looks like CMake also provides the target library path as part of the GNUInstallDirs module.

I'll make the suggested change with the addition of the ${CMAKE_INSTALL_LIBDIR} variable.

I agree that switching the thrust/cmake nesting is cleaner, especially if we aren't bound by the TYPE LIB pattern.

[germasch]

That sounds great. I was thinking to use GNUInstallDirs to not hardcode the lib, but wasn't sure that it's worth the additional (slight) complication, but it is the right way of going about this.

[alliepiper]

It took a bit of refactoring to meet the GNUInstallDirs requirements, but this last push fixes up the install rules as proposed by @germasch. I'll merge this sometime this week before the 1.11.0 release.

[alliepiper]

Last push just adjusts the new submodule SHA. All set!