Fix rtcx embed target-level dependencies under the Makefiles generator - #23106
Conversation
The rtcx embed custom command depended on fragment object libraries only via $<TARGET_OBJECTS:...> generator expressions. With the Unix Makefiles generator this produces file-level dependencies without any target-level ordering, so a parallel build races and fails with: No rule to make target 'CMakeFiles/cudf_fragments_transform_kernel_20.dir/src/transform/jit/kernel.fatbin', needed by 'rtcx_embed/cudf_fragments.hpp'. Ninja resolves this through its global build graph, which is why CI is unaffected. Record the object-library target names alongside the object genexes and pass them to the custom command DEPENDS, which makes CMake emit proper target-level dependencies.
github-actions
Bot
added
libcudf
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis change extends CMake embed dependency tracking in cpp/librtcx/embed.cmake. When rtcx_embed_blob() matches a $<TARGET_OBJECTS:...> file reference, it now also records the target name into a new EMBED_TARGET_DEP_NAMES property. rtcx_embed() reads this property and appends it to the custom command's DEPENDS clause. ChangesEmbed Build Dependency Tracking
Estimated code review effort: 1 (Trivial) | ~5 minutes Related Issues: None specified Related PRs: None specified Suggested labels: cmake, build Suggested reviewers: None specified Poem: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cpp/librtcx/embed.cmake (1)
167-181: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: dedupe accumulated target dep names/objects.
If multiple blob entries reference the same object-library target within one embed target,
EMBED_TARGET_DEPS/EMBED_TARGET_DEP_NAMESwill accumulate duplicate entries. Harmless functionally, butlist(REMOVE_DUPLICATES ...)before use inrtcx_embed()would keep generatedDEPENDSlists tidy.♻️ Optional dedup suggestion
get_property( EMBED_TARGET_DEP_NAMES TARGET ${TARGET}__embed_props PROPERTY EMBED_TARGET_DEP_NAMES ) + if(EMBED_TARGET_DEP_NAMES) + list(REMOVE_DUPLICATES EMBED_TARGET_DEP_NAMES) + endif()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cpp/librtcx/embed.cmake` around lines 167 - 181, Deduplicate the accumulated object-library target dependencies in rtcx_embed() so repeated $<TARGET_OBJECTS:...> entries from the same target do not add duplicate values to EMBED_TARGET_DEPS and EMBED_TARGET_DEP_NAMES. Add list(REMOVE_DUPLICATES ...) before these properties are consumed, using the existing EMBED_TARGET_DEPS, EMBED_TARGET_DEP_NAMES, and rtcx_embed() symbols to keep the generated DEPENDS lists tidy without changing behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@cpp/librtcx/embed.cmake`:
- Around line 167-181: Deduplicate the accumulated object-library target
dependencies in rtcx_embed() so repeated $<TARGET_OBJECTS:...> entries from the
same target do not add duplicate values to EMBED_TARGET_DEPS and
EMBED_TARGET_DEP_NAMES. Add list(REMOVE_DUPLICATES ...) before these properties
are consumed, using the existing EMBED_TARGET_DEPS, EMBED_TARGET_DEP_NAMES, and
rtcx_embed() symbols to keep the generated DEPENDS lists tidy without changing
behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 3a8e35ae-fc9a-4397-93b6-9f00de06adc8
📒 Files selected for processing (1)
cpp/librtcx/embed.cmake
|
/okay to test c7bc602 |
bdice
left a comment
bdice
left a comment
There was a problem hiding this comment.
This is fine with me if you observe it fixes the problem, but maybe wait to merge until another CMake expert can review.
|
/merge |
3 participants
Description
Since #22680, a from-scratch
./build.shwith the Unix Makefiles generator fails with:The
rtcx_embed()custom command depends on the fragment object libraries only via$<TARGET_OBJECTS:...>generator expressions. With the Makefiles generator this produces file-level prerequisites with no build rule and no target-level ordering —cudf_fragments.dir/allonly depended oncudf_fragments__jit_embed_run, not on the 21cudf_fragments_transform_kernel_Nobject libraries — so a parallel make races ahead of the fatbin compilations and dies. Ninja resolves the same dependency through its global build graph, which is why CI never hit this.The fix records the object-library target names in a new
EMBED_TARGET_DEP_NAMESproperty alongside the existing$<TARGET_OBJECTS:...>genexes, and passes those names to the custom commandDEPENDS. Naming a real target there makes CMake emit a proper target-level dependency, so the Makefiles generator builds all fragment fatbins before running the embed step.Verified locally with CMake 4.3.4 + Unix Makefiles: after this change
Makefile2contains target-level deps fromcudf_fragments.dir/allon everycudf_fragments_transform_kernel_N.dir/all, and a clean-state-j32build ofcudf_fragments(and a full./build.sh) succeeds where it previously failed.Checklist