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

[FIX] Add Treelite_BINARY_DIR include to cuml++ build interface include paths #4018

Merged

Conversation

trxcllnt
Copy link
Collaborator

This PR fixes an issue causing cuML source builds to fail when CPM adds Treelite.

Treelite's generated include path isn't in their INTERFACE_INCLUDE_DIRECTORIES, so this path isn't added to the cuml++ target's include dirs either. The fix is to add Treelite's generated include path to the cuml++ target.

In file included from _deps/cuml-src/cpp/src/decisiontree/decisiontree_impl.cuh:20,
                 from _deps/cuml-src/cpp/src/decisiontree/decisiontree.cu:22:
_deps/treelite-src/include/treelite/tree.h:11:10: fatal error: treelite/version.h: No such file or directory
   11 | #include <treelite/version.h>

@trxcllnt trxcllnt requested a review from a team as a code owner June 30, 2021 21:52
@hcho3
Copy link
Contributor

hcho3 commented Jun 30, 2021

@trxcllnt Should I update CMakeLists.txt of Treelite to change INTERFACE_INCLUDE_DIRECTORIES ?

@codecov-commenter
Copy link

Codecov Report

❗ No coverage uploaded for pull request base (branch-21.08@0744a2f). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@               Coverage Diff               @@
##             branch-21.08    #4018   +/-   ##
===============================================
  Coverage                ?   85.47%           
===============================================
  Files                   ?      230           
  Lines                   ?    18130           
  Branches                ?        0           
===============================================
  Hits                    ?    15496           
  Misses                  ?     2634           
  Partials                ?        0           
Flag Coverage Δ
dask 48.15% <0.00%> (?)
non-dask 77.75% <0.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.


Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0744a2f...77bc060. Read the comment docs.

@trxcllnt
Copy link
Collaborator Author

trxcllnt commented Jul 1, 2021

@hcho3 yeah, I think we'd need to add this inside this loop:

  target_include_directories(${lib} PUBLIC
    $<BUILD_INTERFACE:${PROJECT_BINARY_DIR}/include>)

@ajschmidt8 ajschmidt8 added bug Something isn't working non-breaking Non-breaking change labels Jul 1, 2021
@dantegd
Copy link
Member

dantegd commented Jul 1, 2021

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 56d0794 into rapidsai:branch-21.08 Jul 1, 2021
rapids-bot bot pushed a commit that referenced this pull request Jul 2, 2021
Follow-up to #4018, but this time it actually works.

Makes `${Treelite_SOURCE_DIR}/include` part of the build interface so users consuming an installed `cuml++` don't get an include path to the build dir.

Authors:
  - Paul Taylor (https://github.com/trxcllnt)

Approvers:
  - Dante Gama Dessavre (https://github.com/dantegd)

URL: #4023
vimarsh6739 pushed a commit to vimarsh6739/cuml that referenced this pull request Oct 9, 2023
…aths (rapidsai#4018)

This PR fixes an issue causing cuML source builds to fail when CPM adds Treelite.

Treelite's generated include path isn't in their `INTERFACE_INCLUDE_DIRECTORIES`, so this path isn't added to the `cuml++` target's include dirs either. The fix is to add Treelite's generated include path to the `cuml++` target.

```shell
In file included from _deps/cuml-src/cpp/src/decisiontree/decisiontree_impl.cuh:20,
                 from _deps/cuml-src/cpp/src/decisiontree/decisiontree.cu:22:
_deps/treelite-src/include/treelite/tree.h:11:10: fatal error: treelite/version.h: No such file or directory
   11 | #include <treelite/version.h>
```

Authors:
  - Paul Taylor (https://github.com/trxcllnt)

Approvers:
  - Dante Gama Dessavre (https://github.com/dantegd)

URL: rapidsai#4018
vimarsh6739 pushed a commit to vimarsh6739/cuml that referenced this pull request Oct 9, 2023
Follow-up to rapidsai#4018, but this time it actually works.

Makes `${Treelite_SOURCE_DIR}/include` part of the build interface so users consuming an installed `cuml++` don't get an include path to the build dir.

Authors:
  - Paul Taylor (https://github.com/trxcllnt)

Approvers:
  - Dante Gama Dessavre (https://github.com/dantegd)

URL: rapidsai#4023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working CMake CUDA/C++ non-breaking Non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants