fix: resolve CCCL headers from the CUDA 13 include path - #496
Open
janickm wants to merge 2 commits into
Open
Conversation
`cccl_headers` comes from the generic `<component>_headers` in BUILD.dctk_comp, which always sets `includes` to `<component>/include`. CUDA 13 moved the CCCL headers one level deeper, into `include/cccl/`, so that path resolves `<cccl/thrust/...>` but not `<thrust/...>`: fatal error: thrust/complex.h: No such file or directory The per-library targets in BUILD.cccl already handle the move, but `cccl_headers` is not generated from that fragment. It also reaches `cuda_headers`, which aggregates every component's headers target, so both fail the same way. BUILD.cccl now declares `cccl_headers` itself, with the same `if_cuda_toolkit_version_ge` the neighbouring targets use, and BUILD.dctk_comp's generic target is suppressed for that component: the two fragments are concatenated into one BUILD file, so only one of them may declare it. Keeping the version knowledge in the fragment that already has it avoids teaching the generic template about CCCL's layout. The new target is confined to deliverable repos. A local toolkit expands BUILD.cccl into `@cuda` itself, where the component name is "cuda" and BUILD.lctk_cuda already declares `cuda_headers`. Fixes bazel-contrib#489
Builds a kernel including `<thrust/complex.h>` against `cccl_headers` and against the `cuda_headers` aggregate that wraps it. Without the preceding commit both fail on a CUDA 13 toolkit with "thrust/complex.h: No such file or directory". One declared version, so CUDA_REDIST_VERSION_OVERRIDE sweeps the case across the CUDA versions in the matrix and covers both header layouts. Declaring two versions here would instead exercise the multi-version toolkit, where `if_cuda_toolkit_version_ge` reads the maximum declared version rather than the selected one -- a separate problem, and not what this is testing. The targets pass nvcc's own `-std=c++17`, rather than an `-Xcompiler` host flag, because thrust refuses to compile below it and some CI hosts default to an older standard. It has to be nvcc's flag so that it governs the device dialect too; a host-only flag leaves nvcc compiling the CCCL headers as C++14.
Collaborator
|
I will delay this after #501 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #489.
Problem
cccl_headersis generated from the generic<component>_headersinBUILD.dctk_comp, which always setsincludesto<component>/include. CUDA 13 moved the CCCL headers one level deeper, intoinclude/cccl/, so that path resolves<cccl/thrust/...>but not<thrust/...>:The per-library targets in
BUILD.ccclalready handle the move, butcccl_headersis not generated from that fragment. It also reachescuda_headers, which aggregates every component's headers target, so both fail the same way.Fix
Per your suggestion in the issue:
BUILD.ccclnow declarescccl_headersitself, using the sameif_cuda_toolkit_version_geas the neighbouringthrust/cub/libcudacxxtargets, and the generic target inBUILD.dctk_compis suppressed for that component. The two fragments are concatenated into one BUILD file, so only one of them may declare it — the suppression is required, not cosmetic. It follows the shape of_component_owns_cuda_repo_aliasfrom #488.Keeping the version knowledge in the fragment that already has it avoids teaching the generic template about CCCL's layout.
One wrinkle worth flagging: the new target is confined to deliverable repos via
if_local_cuda_toolkit. A local toolkit expandsBUILD.ccclinto@cudaitself, where%{component_name}iscudaandBUILD.lctk_cudaalready declarescuda_headers— without the guard,toolchain_rootfails with a duplicatecuda_headers.Verification
Non-cccl component repos generate byte-identical BUILD files, and exactly one
cccl_headersis declared:cccl_headersresolves tocccl/include/ccclcccl/includeThe new integration case builds a kernel including
<thrust/complex.h>through bothcccl_headersand thecuda_headersaggregate. It declares a single CUDA version, soCUDA_REDIST_VERSION_OVERRIDEsweeps it across the versions in the matrix and covers both layouts. I confirmed it fails on unpatched main and passes here.The targets pass nvcc's own
-std=c++17, not an-Xcompilerhost flag: thrust refuses to compile below C++17, some CI hosts default lower, and a host-only flag leaves nvcc compiling the CCCL headers as C++14 (which fails on MSVC with "inline specifier allowed on function declarations only").Full matrix green on a branch in my fork — all 10
Test Example Buildjobs including the three Windows ones, plusTest Utilitiesandpre-commit. Also ranbazel test //tests/...(82 pass), the examples including//rdc:all, andtest_all.sh.Separate, not fixed here
While testing this across two declared versions I hit a different instance of the max-version problem: every component repo loads
@cuda//:defs.bzl, whosectk_versionis stamped from the maximum declared version, soif_cuda_toolkit_version_gereturns the wrong answer inside a non-maximum component repo. On current main,@cuda_cccl_..._12_8_1//:thrustalready resolvescccl/include/ccclwhen 13.x is also declared.That is pre-existing and affects
thrust,cubandlibcudacxxequally, and it cannot be fixed in the fragments, sinceif_cuda_toolkit_version_geis a load-time function reading a stamped constant. It needs each component repo to know its own version, which theversionattribute already carries. I kept it out of this PR and will file it separately unless you would rather it were folded in.