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

Remove CMAKE_CURRENT_BINARY_DIR path in rmm's target_include_directories #731

Merged

Conversation

trxcllnt
Copy link
Contributor

This PR removes the CMAKE_CURRENT_BINARY_DIR path in rmm's target_include_directories.

Best I can tell, the build generates a version_config.hpp file and places it there for installation later. It doesn't seem to be included by any actual RMM library headers.

This breaks ccache in a few different ways:

  • Building against different RMM build dirs:
    1. Build cuDF against RMM at rmm/build-dir-1 (note -I rmm/build-dir-1/include in cuDF compile commands).
    2. Rebuild cuDF against RMM at rmm/build-dir-2 (note -I rmm/build-dir-2/include in cuDF compile commands).
    3. ccache rebuilds all cuDF source files even though none of the RMM headers actually changed.
  • Build RMM as a dependency of cuDF retrieved via CPM (with CPM_SOURCE_CACHE):
    1. Project A calls CPMFindPackage(cudf ...), cuDF and RMM are downloaded and placed in CPM_SOURCE_CACHE.
    2. Project B calls CPMFindPackage(cudf ...), which uses cuDF and RMM source cached in CPM_SOURCE_CACHE.
    3. The ccache for cuDF source files cannot be shared between Project A and Project B, because the compile command for each cuDF object includes -I ProjectA/build/_deps/rmm-build/include and -I ProjectB/build/_deps/rmm-build/include, respectively.

@trxcllnt trxcllnt requested a review from a team as a code owner March 16, 2021 18:21
@github-actions github-actions bot added the CMake label Mar 16, 2021
@harrism
Copy link
Member

harrism commented Mar 16, 2021

As long as the generated version header is still installed in the include directory when installed, I'm cool with this.

@harrism harrism added improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Mar 16, 2021
@trxcllnt
Copy link
Contributor Author

@harrism yes, looks like it does:

image

@kkraus14
Copy link
Contributor

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 0f3ba03 into rapidsai:branch-0.19 Mar 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CMake improvement Improvement / enhancement to an existing function non-breaking Non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants