Updated libcudftestutil CMake linking logic for 24.12 #1475
Conversation
github-actions
bot
added
cmake
| @@ -17,6 +17,25 @@ | |||
| ################################################################################################### | |||
| # - compiler function ----------------------------------------------------------------------------- | |||
|
|
|||
| add_library(cuspatial_test_common OBJECT test_common.cpp) | |||
There was a problem hiding this comment.
question: Why do we need a library with an empty source file that doesn't include anything?
There was a problem hiding this comment.
Two-fold:
- cmake doesn't allow adding an empty list of sources for a library.
- cudftestutil has been split into cudftestutil and cudftestutil_impl (cmake's INTERFACE sources), and they need to be linked to the executable. if we link to cudftestutil_impl for each test executable, we'll be paying the compilation cost repeatedly for each test compilation (due to source injection). instead, we create an object library called test_common and link to that, paying the compilation cost once.
There was a problem hiding this comment.
OK, thanks. Can you please document this in a comment?
There was a problem hiding this comment.
Thanks. Late question: why can't this empty library object just be created at the cudf level, rather than in cuspatial?
There was a problem hiding this comment.
cudftestutil depends on gtest and gmock. cudf doesn't intend to ship a gtest binary artifact any longer and we want users to have the freedom to choose whichever gtest versions they intend to use as long as it's source-compatible.
harrism
added
improvement
|
I'm going to queue a merge for this, which will trigger once tests pass. The last commit (before docs) passed, so this one should pass as well. |
|
/merge |
Description
This merge request fixes the CMake linking logic to cudftestutil for branch-24.12, allowing you to specify which version of GTest/GBench to use as long as the API is source compatible.
Follows up on: rapidsai/cudf#16839
Checklist