Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rework libcudf CMakeLists.txt to export targets for CPM #7107

Merged
merged 99 commits into from
Mar 2, 2021

Conversation

trxcllnt
Copy link
Contributor

@trxcllnt trxcllnt commented Jan 8, 2021

These changes rework libcudf's CMakeLists.txt files to export targets for inclusion by other CMake projects. I took inspiration from RMM's CMakeLists.txt. However I'm not a CMake expert, and likely missed/messed up a few things.

I also fixed those annoying -Wstrict-prototypes warnings we see compiling Cython files.

These changes are adequate to build libcudf as a dependency in another CMake project with CPM, for example:

function(find_and_configure_cudf VERSION)

    include(get_cpm)

    CPMAddPackage(NAME  cudf
        VERSION         ${VERSION}
        GIT_REPOSITORY  https://github.com/trxcllnt/cudf.git
        # Can also use a local path to your repo clone for testing
        # GIT_REPOSITORY  /home/ptaylor/dev/rapids/cudf
        GIT_TAG         fix/cmake-exports
        GIT_SHALLOW     TRUE
        SOURCE_SUBDIR   cpp
        OPTIONS         "BUILD_TESTS OFF"
                        "BUILD_BENCHMARKS OFF"
                        "ARROW_STATIC_LIB ON"
                        "JITIFY_USE_CACHE ON"
                        "CUDA_STATIC_RUNTIME ON"
                        "DISABLE_DEPRECATION_WARNING ON"
                        # control where generated JIT headers get placed
                        # so we don't cache-bust ccache every clean build
                        "CUDF_GENERATED_INCLUDE_DIR ${CPM_SOURCE_CACHE}/cudf-build"
    )
endfunction()

find_and_configure_cudf(${CUDF_VERSION})

add_target(${PROJECT_NAME} SHARED "${src_files}")
target_link_libraries(${PROJECT_NAME} rmm::rmm cudf::cudf)

… each library target, export library targets
@trxcllnt trxcllnt requested a review from kkraus14 January 8, 2021 22:51
@trxcllnt trxcllnt requested review from a team as code owners January 8, 2021 22:51
@trxcllnt
Copy link
Contributor Author

trxcllnt commented Mar 1, 2021

rerun tests

@kkraus14 kkraus14 removed the 5 - DO NOT MERGE Hold off on merging; see PR for details label Mar 1, 2021
@kkraus14
Copy link
Collaborator

kkraus14 commented Mar 1, 2021

rerun tests

@kkraus14 kkraus14 added 5 - Ready to Merge Testing and reviews complete, ready to merge and removed 2 - In Progress Currently a work in progress 4 - Needs Review Waiting for reviewer to review or respond labels Mar 1, 2021
@kkraus14
Copy link
Collaborator

kkraus14 commented Mar 2, 2021

rerun tests

1 similar comment
@kkraus14
Copy link
Collaborator

kkraus14 commented Mar 2, 2021

rerun tests

@kkraus14
Copy link
Collaborator

kkraus14 commented Mar 2, 2021

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 61091a0 into rapidsai:branch-0.19 Mar 2, 2021
@devavret devavret mentioned this pull request Mar 2, 2021
@ttnghia
Copy link
Contributor

ttnghia commented Mar 2, 2021

This PR is sooooo great! Thank you!!!

There is one more thing that I think we can improve it further. This PR still manually includes a list of .cu/.cpp files for the library. It would be much better if we can automate that by cmake. I'm not sure if we can do something similar to this: https://stackoverflow.com/questions/3201154/automatically-add-all-files-in-a-folder-to-a-target-using-cmake

@kkraus14
Copy link
Collaborator

kkraus14 commented Mar 2, 2021

This PR is sooooo great! Thank you!!!

There is one more thing that I think we can improve it further. This PR still manually includes a list of .cu/.cpp files for the library. It would be much better if we can automate that by cmake. I'm not sure if we can do something similar to this: https://stackoverflow.com/questions/3201154/automatically-add-all-files-in-a-folder-to-a-target-using-cmake

If we did this then CMake wouldn't be able to detect when changes are made, so CMake configuration would need to be run on every incremental build which is painful. We considered this and decided against doing this.

trxcllnt added a commit to trxcllnt/rapids-compose that referenced this pull request Mar 2, 2021
rapids-bot bot pushed a commit that referenced this pull request Mar 2, 2021
Fixing the Java bindings build after the removal of the libcudf sub-libraries in #7107.

Authors:
  - Jason Lowe (@jlowe)

Approvers:
  - Robert (Bobby) Evans (@revans2)
  - MithunR (@mythrocks)

URL: #7486
rapids-bot bot pushed a commit that referenced this pull request Mar 2, 2021
The refactor in #7107 introduced two subtle bugs that caused benchmarks to not build correctly.

1. The `benchmark_main` target needs to be explicitly placed on the final executable link line as it injects object files, and that doesn't work via transitive propagation.
2. The variable names `BUILD_BENCHMARKS` and `BUILD_TESTING` are shared between RMM and CUDF. When CUDF builds RMM via CPM it will force cache these variables to `OFF`. This means that if a developer try to run `cmake -DBUILD_BENCHMARKS=ON .` any subsequent execution of just `cmake .` will cause CUDF to forget it should have kept benchmarks enabled.

Authors:
  - Robert Maynard (@robertmaynard)

Approvers:
  - Keith Kraus (@kkraus14)

URL: #7485
@kkraus14 kkraus14 added breaking Breaking change and removed non-breaking Non-breaking change labels Mar 3, 2021
rapids-bot bot pushed a commit to rapidsai/cuspatial that referenced this pull request Mar 30, 2021
This PR includes many of the same changes as were made in [cudf/pull/7107](rapidsai/cudf#7107).

* Export `cuspatial::cuspatial` CMake alias targets.
* Fixes -Wall errors discovered when changing compile flags.
* Use `CPMFindPackage` to find `libcudf` installed on the system or build `libcudf` from source.

edit: Depends on rapidsai/cudf#7574 and rapidsai/cudf#7734

Authors:
  - Paul Taylor (@trxcllnt)

Approvers:
  - AJ Schmidt (@ajschmidt8)
  - Mark Harris (@harrism)
  - Keith Kraus (@kkraus14)

URL: #365
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
5 - Ready to Merge Testing and reviews complete, ready to merge breaking Breaking change CMake CMake build issue improvement Improvement / enhancement to an existing function libcudf Affects libcudf (C++/CUDA) code. Python Affects Python cuDF API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants