fix(taskfile): Make deps:core and deps:spider-dep write into different top-level CMake configuration files (fixes #1330).#1331
Conversation
WalkthroughThe changes update internal build scripts and CMake configurations, altering which cmake-settings files are referenced for the core and spider components. The new approach points to component-specific settings files (core-all.cmake and spider-all.cmake) rather than the prior general all.cmake, but does not alter exported interfaces or error handling logic. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
taskfiles/deps/main.yaml (2)
44-46: Concurrency hazard: deleting the shared cmake-settings directory can still cause cross-task clobbering.If core and spider-dep run in parallel, either can remove the directory while the other is writing, causing intermittent failures. Only remove your component’s own “all” file (now unique) and keep the directory.
Apply:
- - "rm -rf '{{.G_DEPS_CPP_CMAKE_SETTINGS_DIR}}'" - - "mkdir -p '{{.G_DEPS_CPP_CMAKE_SETTINGS_DIR}}'" + - "mkdir -p '{{.G_DEPS_CPP_CMAKE_SETTINGS_DIR}}'" + - "rm -f '{{.G_DEPS_CPP_CMAKE_SETTINGS_DIR}}/core-all.cmake'"
120-122: Same race in spider-dep: avoid removing the shared cmake-settings directory.Limit cleanup to the component‑specific file.
Apply:
- - "rm -rf '{{.G_DEPS_CPP_CMAKE_SETTINGS_DIR}}'" - - "mkdir -p '{{.G_DEPS_CPP_CMAKE_SETTINGS_DIR}}'" + - "mkdir -p '{{.G_DEPS_CPP_CMAKE_SETTINGS_DIR}}'" + - "rm -f '{{.G_DEPS_CPP_CMAKE_SETTINGS_DIR}}/spider-all.cmake'"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
components/core/CMakeLists.txt(1 hunks)taskfiles/deps/main.yaml(3 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-05-28T18:33:30.155Z
Learnt from: anlowee
PR: y-scope/clp#925
File: taskfiles/deps/main.yaml:97-106
Timestamp: 2025-05-28T18:33:30.155Z
Learning: In the taskfiles dependency system (taskfiles/deps/main.yaml), echo commands are used to generate .cmake settings files that are consumed by the main CMake build process. These files set variables like ANTLR_RUNTIME_HEADER to point to dependency locations for use during compilation.
Applied to files:
taskfiles/deps/main.yaml
📚 Learning: 2025-08-18T05:43:07.868Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1184
File: components/core/cmake/Modules/FindLibLZMA.cmake:21-24
Timestamp: 2025-08-18T05:43:07.868Z
Learning: In the CLP project, all supplied `<lib>_ROOT` variables will live within the `CLP_CORE_DEPS_DIR` as part of their architectural design. This means that using CLP_CORE_DEPS_DIR for library discovery in custom Find modules is the intended approach, and prioritizing individual `<lib>_ROOT` variables over CLP_CORE_DEPS_DIR is not necessary.
Applied to files:
components/core/CMakeLists.txt
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (15)
- GitHub Check: ubuntu-jammy-static-linked-bins
- GitHub Check: package-image
- GitHub Check: ubuntu-jammy-lint
- GitHub Check: ubuntu-jammy-dynamic-linked-bins
- GitHub Check: manylinux_2_28-x86_64-static-linked-bins
- GitHub Check: centos-stream-9-static-linked-bins
- GitHub Check: manylinux_2_28-x86_64-dynamic-linked-bins
- GitHub Check: musllinux_1_2-x86_64-dynamic-linked-bins
- GitHub Check: centos-stream-9-dynamic-linked-bins
- GitHub Check: musllinux_1_2-x86_64-static-linked-bins
- GitHub Check: build (macos-15)
- GitHub Check: build-macos (macos-14, true)
- GitHub Check: build-macos (macos-15, true)
- GitHub Check: build-macos (macos-14, false)
- GitHub Check: lint-check (macos-15)
🔇 Additional comments (4)
taskfiles/deps/main.yaml (3)
49-51: Good: core writes to a component‑specific settings file.
109-113: Good: spider consumes spider-all.cmake via -C.
125-127: Good: spider-dep writes to its own settings file.components/core/CMakeLists.txt (1)
125-129: No stale all.cmake references remain – core-all.cmake usage is correct.
| CMAKE_SETTINGS_FILE: "{{.G_DEPS_CPP_CMAKE_SETTINGS_DIR}}/core-all.cmake" | ||
| DEP_TASK: "core-all-parallel" | ||
| - task: "utils:combine-cpp-checksum-files" |
There was a problem hiding this comment.
🧩 Analysis chain
Optional: verify no remaining references to the old shared file.
Run:
Also applies to: 125-127
🏁 Script executed:
#!/bin/bash
# Confirm all generation/consumption uses component-specific files.
rg -nP -C2 '\bcore-all\.cmake|\bspider-all\.cmake'
rg -nP -C2 '/cmake-settings/all\.cmake'Length of output: 1624
Replace remaining references to old shared CMake settings files
- taskfiles/deps/main.yaml (lines 49, 109, 125–127) still reference
core-all.cmakeandspider-all.cmake. - components/core/CMakeLists.txt (line 126) still includes
cmake-settings/core-all.cmake.
Update each to use the new component-specific CMake settings file.
🤖 Prompt for AI Agents
In taskfiles/deps/main.yaml around lines 49, 109, 125–127 and
components/core/CMakeLists.txt at line 126, update any references to the old
shared CMake settings files (core-all.cmake and spider-all.cmake) to point to
the new component-specific settings files (e.g., core.cmake or spider.cmake).
Concretely, change CMAKE_SETTINGS_FILE entries and any DEP_TASK or task names
that reference the shared files to use the corresponding component-specific
filename and update the include path in components/core/CMakeLists.txt from
cmake-settings/core-all.cmake to the new cmake-settings/core.cmake. Ensure all
remaining occurrences across the listed lines are replaced consistently.
There was a problem hiding this comment.
Overall idea for splitting into different meta-configs lgtm. The only thing is that shall we rename the setting files like:
core-all.cmake->core.cmakespider-all.cmake->spider.cmake
This is more concise, and the full path cmake-settings already indicates they're setting configs. The concern for doing this would be later if we need spider as a dependency for core, then spider.cmake can be confusing as it may need to serve for the config for core's spider dependency, not a meta-config. To solve that, we can put the dependency-generated cmake into an inner directory inside cmake-settings, so that only meta-config at the top-level.
@kirkrodrigues Does this make sense to u?
Do we just want to create that inner folder now? Maybe |
Sure. @sitaowang1998 What do u think? Can u try if this works? |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
taskfiles/deps/main.yaml (2)
44-51: Critical: deleting the shared cmake-settings directory reintroduces a parallelism race.
coreandspider-depbothrm -rfthe same directory. In parallel runs, one task can erase the other’s files. Scope each task to its own subdirectory and write concise filenames (core.cmake).Apply:
- - "rm -rf '{{.G_DEPS_CPP_CMAKE_SETTINGS_DIR}}'" - - "mkdir -p '{{.G_DEPS_CPP_CMAKE_SETTINGS_DIR}}'" + - "rm -rf '{{.G_DEPS_CPP_CMAKE_SETTINGS_DIR}}/core'" + - "mkdir -p '{{.G_DEPS_CPP_CMAKE_SETTINGS_DIR}}/core'" - task: "yscope-dev-utils:cmake:install-deps-and-generate-settings" vars: - CMAKE_SETTINGS_DIR: "{{.G_DEPS_CPP_CMAKE_SETTINGS_DIR}}" - CMAKE_SETTINGS_FILE: "{{.G_DEPS_CPP_CMAKE_SETTINGS_DIR}}/all-core.cmake" + CMAKE_SETTINGS_DIR: "{{.G_DEPS_CPP_CMAKE_SETTINGS_DIR}}/core" + CMAKE_SETTINGS_FILE: "{{.G_DEPS_CPP_CMAKE_SETTINGS_DIR}}/core/core.cmake" DEP_TASK: "core-all-parallel"This aligns with reviewer feedback to keep top-level files as meta-configs and reduce ambiguity going forward.
120-127: Mirror the isolation for spider-dep; stop deleting the shared dir.Prevent cross-task deletion and generate a scoped settings file.
- - "rm -rf '{{.G_DEPS_CPP_CMAKE_SETTINGS_DIR}}'" - - "mkdir -p '{{.G_DEPS_CPP_CMAKE_SETTINGS_DIR}}'" + - "rm -rf '{{.G_DEPS_CPP_CMAKE_SETTINGS_DIR}}/spider'" + - "mkdir -p '{{.G_DEPS_CPP_CMAKE_SETTINGS_DIR}}/spider'" - task: "yscope-dev-utils:cmake:install-deps-and-generate-settings" vars: - CMAKE_SETTINGS_DIR: "{{.G_DEPS_CPP_CMAKE_SETTINGS_DIR}}" - CMAKE_SETTINGS_FILE: "{{.G_DEPS_CPP_CMAKE_SETTINGS_DIR}}/all-spider.cmake" + CMAKE_SETTINGS_DIR: "{{.G_DEPS_CPP_CMAKE_SETTINGS_DIR}}/spider" + CMAKE_SETTINGS_FILE: "{{.G_DEPS_CPP_CMAKE_SETTINGS_DIR}}/spider/spider.cmake" DEP_TASK: "spider-all-parallel"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
components/core/CMakeLists.txt(1 hunks)taskfiles/deps/main.yaml(3 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1156
File: components/core/CMakeLists.txt:772-772
Timestamp: 2025-08-09T04:07:27.083Z
Learning: In the CLP project's CMakeLists.txt, when reviewing changes related to the ${zstd_TARGET} variable usage in test linking, the team is planning a refactoring PR to improve this mechanism. Guards for undefined target variables should be deferred to that separate PR rather than being added in focused dependency migration PRs.
📚 Learning: 2025-08-09T04:07:27.083Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1156
File: components/core/CMakeLists.txt:772-772
Timestamp: 2025-08-09T04:07:27.083Z
Learning: In the CLP project's CMakeLists.txt, when reviewing changes related to the ${zstd_TARGET} variable usage in test linking, the team is planning a refactoring PR to improve this mechanism. Guards for undefined target variables should be deferred to that separate PR rather than being added in focused dependency migration PRs.
Applied to files:
components/core/CMakeLists.txttaskfiles/deps/main.yaml
📚 Learning: 2025-08-18T05:43:07.868Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1184
File: components/core/cmake/Modules/FindLibLZMA.cmake:21-24
Timestamp: 2025-08-18T05:43:07.868Z
Learning: In the CLP project, all supplied `<lib>_ROOT` variables will live within the `CLP_CORE_DEPS_DIR` as part of their architectural design. This means that using CLP_CORE_DEPS_DIR for library discovery in custom Find modules is the intended approach, and prioritizing individual `<lib>_ROOT` variables over CLP_CORE_DEPS_DIR is not necessary.
Applied to files:
components/core/CMakeLists.txt
📚 Learning: 2025-05-28T18:33:30.155Z
Learnt from: anlowee
PR: y-scope/clp#925
File: taskfiles/deps/main.yaml:97-106
Timestamp: 2025-05-28T18:33:30.155Z
Learning: In the taskfiles dependency system (taskfiles/deps/main.yaml), echo commands are used to generate .cmake settings files that are consumed by the main CMake build process. These files set variables like ANTLR_RUNTIME_HEADER to point to dependency locations for use during compilation.
Applied to files:
taskfiles/deps/main.yaml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: package-image
- GitHub Check: ubuntu-jammy-lint
- GitHub Check: musllinux_1_2-x86_64-static-linked-bins
- GitHub Check: musllinux_1_2-x86_64-dynamic-linked-bins
- GitHub Check: centos-stream-9-static-linked-bins
- GitHub Check: manylinux_2_28-x86_64-static-linked-bins
- GitHub Check: centos-stream-9-dynamic-linked-bins
- GitHub Check: manylinux_2_28-x86_64-dynamic-linked-bins
🔇 Additional comments (1)
taskfiles/deps/main.yaml (1)
108-111: Point Spider to its isolated settings file.Match the proposed per-component subdir and concise filename.
- - "-C {{.G_DEPS_CPP_CMAKE_SETTINGS_DIR}}/all-spider.cmake" + - "-C {{.G_DEPS_CPP_CMAKE_SETTINGS_DIR}}/spider/spider.cmake"
| include("${CLP_CORE_DEPS_DIR}/cmake-settings/all-core.cmake" | ||
| OPTIONAL | ||
| RESULT_VARIABLE CLP_DEPS_SETTINGS_FILE_PATH | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Stop consuming settings from a shared dir; point to a component-specific subdirectory.
Parallel tasks still rm -rf the shared cmake-settings dir (see taskfiles), which can race and delete this file mid-build. Consume a namespaced file to eliminate cross‑component interference.
Apply:
- include("${CLP_CORE_DEPS_DIR}/cmake-settings/all-core.cmake"
+ include("${CLP_CORE_DEPS_DIR}/cmake-settings/core/core.cmake"
OPTIONAL
RESULT_VARIABLE CLP_DEPS_SETTINGS_FILE_PATH
)Follow-up: adjust the Taskfile to write core/core.cmake (and spider/spider.cmake) and stop deleting the whole directory. Based on learnings
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| include("${CLP_CORE_DEPS_DIR}/cmake-settings/all-core.cmake" | |
| OPTIONAL | |
| RESULT_VARIABLE CLP_DEPS_SETTINGS_FILE_PATH | |
| ) | |
| include("${CLP_CORE_DEPS_DIR}/cmake-settings/core/core.cmake" | |
| OPTIONAL | |
| RESULT_VARIABLE CLP_DEPS_SETTINGS_FILE_PATH | |
| ) |
🤖 Prompt for AI Agents
In components/core/CMakeLists.txt around lines 126 to 129, the CMake include is
pulling a shared cmake-settings file that can be removed by parallel tasks;
change the include to point at a component-specific namespaced file (e.g.
replace cmake-settings/all-core.cmake with cmake-settings/core/core.cmake) so
each component consumes its own settings file; update the RESULT_VARIABLE usage
unchanged and then update the Taskfile separately to write core/core.cmake (and
spider/spider.cmake) instead of deleting the entire cmake-settings directory.
deps:core and deps:spider writes to separate cmake setting files (fixes #1330).deps:core and deps:spider-dep write into different top-level CMake configuration files (fixes #1330).
LinZhihao-723
left a comment
There was a problem hiding this comment.
Directly modified the PR title.
…erent top-level CMake configuration files (fixes y-scope#1330). (y-scope#1331)
Description
Problem
Both
deps:coreanddeps:spider-depwrite tobuild/deps/cpp/cmake-setting/all.cmake. When running in parallel, the content ofall.cmakeis corrupted, and bothcoreanddeps:spiderfail to load the corrupted cmake file.Solution
This PR solves the problem by splitting the cmake setting file into two.
deps:corewrites tocore-all.cmakeanddeps:spider-depwrites tospider-all.cmake, avoiding concurrent writes to same file.coreanddeps:spidernow consume different cmake setting files.Checklist
breaking change.
Validation performed
taskno longer triggers errors.Summary by CodeRabbit