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

[BUG]: CCCL_INCLUDE_DIR variables incorrect in CMake output #526

Closed
1 task done
cliffburdick opened this issue Oct 6, 2023 · 12 comments
Closed
1 task done

[BUG]: CCCL_INCLUDE_DIR variables incorrect in CMake output #526

cliffburdick opened this issue Oct 6, 2023 · 12 comments
Assignees
Labels
bug Something isn't working right.

Comments

@cliffburdick
Copy link
Contributor

Is this a duplicate?

Type of Bug

Something else

Component

Not sure

Describe the bug

When adding CCCL through CPM the following variables are created:

-- CCCL_BINARY_DIR=/repro/proj/build/_deps/cccl-build
-- CCCL_DIR=/repro/proj/build/_deps/cccl-src/lib/cmake/cccl
-- CCCL_ENABLE_DEFAULT_THRUST_TARGET=ON
-- CCCL_ENABLE_INSTALL_RULES=OFF
-- CCCL_INCLUDE_DIR=/include
-- CCCL_IS_TOP_LEVEL=OFF
-- CCCL_SOURCE_DIR=/repro/proj/build/_deps/cccl-src
-- CCCL_THRUST_DEVICE_SYSTEM=CUDA
-- CCCL_THRUST_HOST_SYSTEM=CPP
-- CCCL_VERSION=v2.2.0

Notably projects that have stricter warnings than CCCL projects enabled cannot use:
target_link_libraries(your_executable PRIVATE CCCL::CCCL)

as suggested here because target_link_libraries does not allow a SYSTEM modifier for a transitive target. Instead, we must use the variables created above manually. The problem is that CCCL_INCLUDE_DIR should be an absolute path pointing to where CCCL resides. You can see in the output above /include is wrong, so CCCL_INCLUDE_DIR cannot be used. Instead as a workaround we have to do something like:

target_include_directories(target SYSTEM INTERFACE
                            $<BUILD_INTERFACE:${CCCL_SOURCE_DIR}/libcudacxx>
                            $<BUILD_INTERFACE:${CCCL_SOURCE_DIR}/cub>
                            $<BUILD_INTERFACE:${CCCL_SOURCE_DIR}/thrust>)

How to Reproduce

Follow the example CMake instructions and print the variables:

https://github.com/NVIDIA/cccl/tree/main/examples/example_project

Expected behavior

In the example above CCCL_INCLUDE_DIR should be:

$<BUILD_INTERFACE:${CCCL_SOURCE_DIR}/

Reproduction link

No response

Operating System

No response

nvidia-smi output

No response

NVCC version

No response

@cliffburdick cliffburdick added the bug Something isn't working right. label Oct 6, 2023
@github-project-automation github-project-automation bot moved this to Todo in CCCL Oct 6, 2023
@github-actions github-actions bot added the needs triage Issues that require the team's attention label Oct 6, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Oct 6, 2023

Hi @cliffburdick!

Thanks for submitting this issue - the CCCL team has been notified and we'll get back to you as soon as we can!
In the mean time, feel free to add any relevant information to this issue.

@jrhemstad
Copy link
Collaborator

@allisonvacanti or @robertmaynard could you take a look at this? I believe this is a result of the fact that we have to include CCCL headers as user includes instead of system includes.

@jrhemstad jrhemstad assigned alliepiper and unassigned miscco Oct 6, 2023
@jrhemstad jrhemstad removed the needs triage Issues that require the team's attention label Oct 6, 2023
@alliepiper
Copy link
Collaborator

This is strange -- we don't define CCCL_INCLUDE_DIR at all. It doesn't make sense since there are no CCCL headers -- the CCCL packages just glue together the Thrust/CUB/libcudacxx targets. I'm not sure how that variable is getting defined at all. Maybe CPM is doing this?

Since there aren't any CCCL headers, there is no CCCL include path, and you can just omit that from your targets. I believe this can be closed as NAB -- will that work for you, @cliffburdick?

@cliffburdick
Copy link
Contributor Author

This is strange -- we don't define CCCL_INCLUDE_DIR at all. It doesn't make sense since there are no CCCL headers -- the CCCL packages just glue together the Thrust/CUB/libcudacxx targets. I'm not sure how that variable is getting defined at all. Maybe CPM is doing this?

Since there aren't any CCCL headers, there is no CCCL include path, and you can just omit that from your targets. I believe this can be closed as NAB -- will that work for you, @cliffburdick?

I think it's likely CPM. Do you think CCCL should export some variables people can use? The way I have it right now is a bit clunky since I'm hard-coding all the things CCCL includes, but if it ever adds another library I have to modify my CMakeLists. Maybe it should create a string that people can use?

@jrhemstad
Copy link
Collaborator

jrhemstad commented Oct 6, 2023

Do you think CCCL should export some variables people can use?

I'd be supportive of this. Obviously, the ideal solution would be to just use the CCCL::CCCL target, but I understand the limitations with warnings and inability to use a system include path today.

Can CCCL_INCLUDE_DIR be a list of multiple directories? All the headers aren't in one place, so we need to add multiple paths to the include path.

@cliffburdick
Copy link
Contributor Author

I think you can provide CCCL_INCLUDE_DIRS that's a list of all the include dirs, but @allisonvacanti or @robertmaynard would need to verify.

@robertmaynard
Copy link
Contributor

CPM does not set any variables with the pattern <project>_INCLUDE_DIR. That would break one of the core rules of CPM which is that the primary output of a CPM call are targets and not variables.

Likewise variables like CCCL_IS_TOP_LEVEL are not cache variables which tell me that CPM couldn't have added the project, since that variable wouldn't be exposed.

While CCCL could provide a CCCL_INCLUDE_DIRS that won't resolve anything if we follow CMake find_package recommendations that _INCLUDE_DIRS variables are supposed to be non-cache. Therefore wouldn't be exposed by CPM.

@cliffburdick
Copy link
Contributor Author

@robertmaynard what is the recommendation for projects that follow a stricter set of warnings than CCCL and therefor cannot use their targets?

@robertmaynard
Copy link
Contributor

@robertmaynard what is the recommendation for projects that follow a stricter set of warnings than CCCL and therefor cannot use their targets?

By default all find_package targets have the includes marked as SYSTEM. What you are running into here is a complex issue due to the implicit CTK includes that nvcc has.

  • nvcc has an implicit user include of the CTK includes, which includes a version of the CCCL.
  • System includes take lower priority independent of command line order compared to user includes
  • Therefore the nvcc implicit user include of CCCL will always take priority over a find_package version of the CCCL

So to workaround the above issues the CCCL have functions like thrust_create_target to construct targets that have user includes instead of system includes. If you try to use the SYSTEM hammer you will most likely fail to actually use CCCL headers and instead fall back to the CTK version.

The most consistent way to remove these warnings would be to patch CCCL ( maybe with the gcc pragma that says that this is a system header ). You could also look into futzing around with SYSTEM include order ( specifically SYSTEM BEFORE on user CCCL and SYSTEM AFTER on CTK ) that might also be an option. In those cases you can get the include directories from the CCCL targets via get_target_property.

Note:
In general when using CPM you can also specify the SYSTEM option which will mark any git cloned dependency as system. This will keep the includes 'consistent' across how CPM finds a dependency.

@jrhemstad
Copy link
Collaborator

To summarize, the reason @cliffburdick wants to add CCCL headers SYSTEM includes (i.e., -isystem=) is to avoid warnings from CCCL headers.

As @robertmaynard mentioned, there are complex reasons with the way nvcc implicitly includes CTK headers that prevent using SYSTEM for CCCL headers today.

However, while looking into this issue, I've found that there is another solution to ensuring CCCL headers don't emit warnings that does not require using -isystem. I wrote it up here: #527

This would mean users like @cliffburdick could just link against the CCCL::CCCL target without issue and not need to worry about workarounds like using CCCL_INCLUDE_DIRS manually.

@cliffburdick if you agree, then I think we could close this issue as "Not Planned" and defer to #527 as the solution to the problem you ultimately care about.

@cliffburdick
Copy link
Contributor Author

Yes, thanks.

@jrhemstad
Copy link
Collaborator

Closing in favor of #527. Linking against the exposed cmake targets like CCCL::CCCL will continue to be the primary way to add CCCL to cmake projects. That is currently blocked by the fact that CCCL headers can emit warnings in user code, which will be addressed by #527.

@jrhemstad jrhemstad closed this as not planned Won't fix, can't repro, duplicate, stale Oct 9, 2023
@github-project-automation github-project-automation bot moved this from Todo to Done in CCCL Oct 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working right.
Projects
Archived in project
Development

No branches or pull requests

5 participants