[REVIEW] Refine CMakeLists.txt to make it easy to be imported by external projects #541

gigony · 2020-09-03T23:57:01Z

Change CMAKE_SOURCE_DIR to CMAKE_CURRENT_SOURCE_DIR
Change CMAKE_BINARY_DIR to CMAKE_CURRENT_BINARY_DIR
Add 'project' statement to suppress warning messages
Use generator expression to refer the include folder depending on the case (build or install)
Ignore error if thrust is already patched

fix #540

GPUtester · 2020-09-03T23:57:03Z

Can one of the admins verify this patch?

GPUtester · 2020-09-03T23:57:03Z

Can one of the admins verify this patch?

GPUtester · 2020-09-03T23:57:03Z

Can one of the admins verify this patch?

harrism · 2020-09-04T01:56:09Z

OK to test

GPUtester · 2020-09-04T01:56:30Z

Please update the changelog in order to start CI tests.

View the gpuCI docs here.

harrism · 2020-09-04T01:56:43Z

@gigony in order for our CI tests to run, you will need to add an entry to CHANGELOG.md.

harrism · 2020-09-04T02:08:31Z

@germasch since I know you are helping modernize RMM's cmake, can you take a look at this PR?

germasch

Looks good to me (the warnings about missing project had annoyed me as well ;)

harrism · 2020-09-04T03:48:06Z

Looks good to me (the warnings about missing project had annoyed me as well ;)

Me too, but I had wrongly assumed they stemmed from the underlying googletest cmake!

- Change CMAKE_SOURCE_DIR to CMAKE_CURRENT_SOURCE_DIR - Change CMAKE_BINARY_DIR to CMAKE_CURRENT_BINARY_DIR - Add 'project' statement to suppress warning messages

- Use generator expression to refer the include folder depending on the case (build or install)

germasch · 2020-09-04T11:38:43Z

BTW, @gigony: Since you're starting to do INSTALL_INTERFACE stuff (which looks correct), I should let you know that I'd been working on making rmm installable together with a cmake config that allows it easily be used from another project via find_package. This has been kinda on hold since the cmake requirement was supposed to be upped to 3.17, but this hasn't happened. In any case, due to how external dependencies are currently handled, that's a bit of a non-trivial undertaking, but I'd be more than happy to discuss the past path forward if that's something you're interested in.

gigony · 2020-09-04T19:15:15Z

BTW, @gigony: Since you're starting to do INSTALL_INTERFACE stuff (which looks correct), I should let you know that I'd been working on making rmm installable together with a cmake config that allows it easily be used from another project via find_package. This has been kinda on hold since the cmake requirement was supposed to be upped to 3.17, but this hasn't happened. In any case, due to how external dependencies are currently handled, that's a bit of a non-trivial undertaking, but I'd be more than happy to discuss the past path forward if that's something you're interested in.

Thanks for sharing the information! I am happy to discuss it though merging this pull request would satisfy my need in the project I am currently working on.

In the recent revision of this pull request, I have addressed another issue (4. in #540) found while importing rmm in my project. Please help review it.
Thanks!

germasch · 2020-09-04T19:31:59Z

Thanks for sharing the information! I am happy to discuss it though merging this pull request would satisfy my need in the project I am currently working on.

Okay, that's good, so we're basically working on orthogonal things, you want to use rmm via add_subdirectory(), and me via find_package(), both of which are worthwhile supporting.

In the recent revision of this pull request, I have addressed another issue (4. in #540) found while importing rmm in my project. Please help review it.

Whereas I'm not too familiar with what exactly happens there, I think it'll be fixed by #538 as that gets rid of the need to patch in the first place.

harrism

LGTM but I would like @kkraus14 to take a look.

kkraus14 · 2020-09-08T18:28:32Z

CMakeLists.txt

@@ -155,8 +155,8 @@ SET_TARGET_PROPERTIES(rmm PROPERTIES BUILD_RPATH "\$ORIGIN")
 target_include_directories(rmm PUBLIC
  "${THRUST_INCLUDE_DIR}"


Should really move to using a target for this instead of CMake variables. This path likely won't resolve properly on the importing machine.

@gigony's goal here is to have a consuming project use rmm via add_subdirectory(rmm). For that purpose, I think this will work, since the consuming project will be built at the same time, so the include path will still be there.

You're right that this isn't going to work for a installed version of rmm -- which is my goal (in future PRs). thrust is going to be particularly hasslesome for this, since there may could be three ways of using thrust (from the cudatoolkit, installed elsewhere externally, or included via FetchContent. And in addition, thrust doesn't quite support the usual find_package() way of pulling it in :(

But for now, I believe this PR supports @gigony's use case and shouldn't break anything.

And in addition, thrust doesn't quite support the usual find_package() way of pulling it in :(

I believe this was changed as of Thrust 1.9.10, where there's now a nicer CMake target and the like: https://github.com/thrust/thrust/blob/main/thrust/cmake/README.md

Yeah, I've seen that, but the "not quite the usual" thing is the thrust_create_target(...) (which does exist for good reasons, though).

I think we'll be able to work with it, but the thing about it is that a find_package(rmm) would pull in thrust and would have to do a thrust_create_target() internally, not exactly knowing what thrust variant the consuming application actually wants. It might not be a problem in the typical use case, but I think I can dream up issues where the application also makes a thrust target and one ends up pulling in thrust twice via different target names and with different configurations...

CMakeLists.txt

@rlratzel

…t cugraph as an external project and other tech debt removal (#1367) This PR makes cuGraph's cmake files more consistent with other RAPIDS libs by matching the minimum required cmake version, adding `project()` statements to cugraph's thirdparty modules, and using `CMAKE_CURRENT_SOURCE_DIR` appropriately so paths are relative to the CMakeLists.txt file rather than the top-level cmake dir of the project (since that may not be the cugraph cpp dir in the case of cugraph being used as an external project by another application). This also adds a `CUDA_ARCHITECTURES=OFF` setting to suppress the warning printed for each test target. This setting may be replaced/changed once the findcudatoolkit feature is used in a future cmake version. This also removes the Arrow and GTest cmake files since Arrow is not a direct dependency and those files were not being used, and GTest is now a build requirement in the conda dev environment and does not need to be built from source (the conda dev env files have been updated accordingly). This PR also addresses much of #1075 , but not completely since gunrock is still using `ExternalProject` due to (I think) updates that need to be made to their cmake files to support this. This was tested by observing a successful clean build, however it was **not** tested by creating a separate cmake application to simulate cugraph being used as a 3rd party package. Note: the changes in this PR were modeled after rapidsai/rmm#541 closes #1137 closes #1266 Authors: - Rick Ratzel (@rlratzel) Approvers: - Chuck Hastings (@ChuckHastings) - AJ Schmidt (@ajschmidt8) - Brad Rees (@BradReesWork) URL: #1367

gigony requested a review from a team as a code owner September 3, 2020 23:57

gigony changed the title ~~Update CMakeLists.txt for gtest/gbenchmark #540~~ Update CMakeLists.txt for gtest/gbenchmark Sep 3, 2020

germasch approved these changes Sep 4, 2020

View reviewed changes

gigony added 2 commits September 3, 2020 22:45

Update CMakeLists.txt for gtest/gbenchmark

97880e3

- Change CMAKE_SOURCE_DIR to CMAKE_CURRENT_SOURCE_DIR - Change CMAKE_BINARY_DIR to CMAKE_CURRENT_BINARY_DIR - Add 'project' statement to suppress warning messages

Refer to correct include paths

3414426

- Use generator expression to refer the include folder depending on the case (build or install)

gigony force-pushed the refine_cmake branch from 50eb479 to fff2e76 Compare September 4, 2020 05:49

gigony changed the title ~~Update CMakeLists.txt for gtest/gbenchmark~~ Refine CMakeLists.txt to make it easy to be imported by external projects Sep 4, 2020

gigony force-pushed the refine_cmake branch from fff2e76 to 7d98b4c Compare September 4, 2020 18:51

gigony added 2 commits September 4, 2020 15:21

Ignore error if thrust is already patched

5a29149

Update CHANGELOG for rapidsai#541

c13a5e0

gigony force-pushed the refine_cmake branch from 7d98b4c to c13a5e0 Compare September 4, 2020 22:22

harrism approved these changes Sep 8, 2020

View reviewed changes

harrism added CMake 3 - Ready for review Ready for review by team labels Sep 8, 2020

harrism changed the title ~~Refine CMakeLists.txt to make it easy to be imported by external projects~~ [REVIEW] Refine CMakeLists.txt to make it easy to be imported by external projects Sep 8, 2020

harrism requested a review from kkraus14 September 8, 2020 02:44

kkraus14 reviewed Sep 8, 2020

View reviewed changes

CMakeLists.txt Outdated Show resolved Hide resolved

gigony and others added 2 commits September 8, 2020 13:08

Merge branch 'branch-0.16' into refine_cmake

5ff361e

Merge branch 'branch-0.16' into refine_cmake

08bfe0e

kkraus14 approved these changes Sep 8, 2020

View reviewed changes

kkraus14 added 5 - Ready to Merge Testing and reviews complete, ready to merge and removed 3 - Ready for review Ready for review by team labels Sep 8, 2020

harrism merged commit 6cf7323 into rapidsai:branch-0.16 Sep 8, 2020

This was referenced Sep 9, 2020

[REVIEW] Fix CMake warnings in external projects rapidsai/cudf#6176

Merged

[REVIEW] Fix CMake warnings in external projects rapidsai/cuspatial#289

Merged

rlratzel mentioned this pull request Feb 1, 2021

Update CMakeLists.txt files for consistency with RAPIDS and to support cugraph as an external project and other tech debt removal rapidsai/cugraph#1367

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[REVIEW] Refine CMakeLists.txt to make it easy to be imported by external projects #541

[REVIEW] Refine CMakeLists.txt to make it easy to be imported by external projects #541

gigony commented Sep 3, 2020 •

edited

Loading

GPUtester commented Sep 3, 2020

GPUtester commented Sep 3, 2020

GPUtester commented Sep 3, 2020

harrism commented Sep 4, 2020

GPUtester commented Sep 4, 2020

harrism commented Sep 4, 2020

harrism commented Sep 4, 2020

germasch left a comment

harrism commented Sep 4, 2020

germasch commented Sep 4, 2020

gigony commented Sep 4, 2020

germasch commented Sep 4, 2020

harrism left a comment

kkraus14 Sep 8, 2020

germasch Sep 8, 2020

kkraus14 Sep 8, 2020

germasch Sep 8, 2020

		@@ -155,8 +155,8 @@ SET_TARGET_PROPERTIES(rmm PROPERTIES BUILD_RPATH "\$ORIGIN")
		target_include_directories(rmm PUBLIC
		"${THRUST_INCLUDE_DIR}"

[REVIEW] Refine CMakeLists.txt to make it easy to be imported by external projects #541

[REVIEW] Refine CMakeLists.txt to make it easy to be imported by external projects #541

Conversation

gigony commented Sep 3, 2020 • edited Loading

GPUtester commented Sep 3, 2020

GPUtester commented Sep 3, 2020

GPUtester commented Sep 3, 2020

harrism commented Sep 4, 2020

GPUtester commented Sep 4, 2020

harrism commented Sep 4, 2020

harrism commented Sep 4, 2020

germasch left a comment

Choose a reason for hiding this comment

harrism commented Sep 4, 2020

germasch commented Sep 4, 2020

gigony commented Sep 4, 2020

germasch commented Sep 4, 2020

harrism left a comment

Choose a reason for hiding this comment

kkraus14 Sep 8, 2020

Choose a reason for hiding this comment

germasch Sep 8, 2020

Choose a reason for hiding this comment

kkraus14 Sep 8, 2020

Choose a reason for hiding this comment

germasch Sep 8, 2020

Choose a reason for hiding this comment

gigony commented Sep 3, 2020 •

edited

Loading