Only install thrust when using a non 'system' version #9206
Conversation
Previously cudf presumed that no system version of Thrust matched the version cudf required, but with the merger of rapidsai/rmm#854 this isn't true.
robertmaynard
added
bug
|
rerun tests |
The cudf::Thrust target will hold the correct target_include_directory
This reverts commit 9a7fd15.
Codecov Report
@@ Coverage Diff @@
## branch-21.10 #9206 +/- ##
===============================================
Coverage ? 10.82%
===============================================
Files ? 115
Lines ? 19154
Branches ? 0
===============================================
Hits ? 2074
Misses ? 17080
Partials ? 0 Continue to review full report at Codecov.
|
| DESTINATION "${CMAKE_INSTALL_INCLUDEDIR}/libcudf/Thrust/dependencies/" | ||
| FILES_MATCHING | ||
| PATTERN "*.cuh") | ||
| if(Thrust_SOURCE_DIR) # only install thrust when we have an in-source version |
There was a problem hiding this comment.
Forgive me for being ignorant, but shouldn't all this stuff be encapsulated in Thrust's cmake target?
There was a problem hiding this comment.
Since cudf patches Thrust and therefore it isn't identical to the upstream version we wanted to ensure that won't be used by other projects accidentally. To do this we install Thrust inside of cudfs include directory.
Once we upstream the cudf patches of Thrust we can remove all of this
There was a problem hiding this comment.
I know Thrust has some machinery for injecting a custom namespace to ensure our custom thrust doesn't conflict with other versions. Do you think that's relevant here?
There was a problem hiding this comment.
Namespacing would solve an issue if downstream consumers of cudf need to also bring in a non-patched version of Thrust into a library calling cudf. I/we should sync with the thrust team, it might be best for cudf to always namespace our thrust inclusion going forward.
I would prefer to not use this PR to hash out the larger cudf/thrust issue as this resolves our currently broken CI pipelines.
|
@gpucibot merge |
Previously cudf presumed that no system version of Thrust matched the version cudf required, but with the merger
of rapidsai/rmm#854 this isn't true.