refactor(taskfile): Split main dependency Taskfile into category-specific modules; Migrate antlr-runtime and utfcpp to use utils:install-remote-cmake-lib.
#1377
Conversation
WalkthroughThis PR restructures the dependency management system by introducing modular YAML taskfiles for each third-party C++ library dependency, refactoring the main orchestration layer, and updating task dependencies to use fully-qualified references. The changes consolidate scattered per-component task definitions into a centralized, parallelized installation model with separate CMake settings management. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Rationale: Although ~25 install taskfiles follow a homogeneous pattern (reducing per-file complexity), the total file count (30+) and heterogeneous changes in orchestration files (main.yaml, utils.yaml) require systematic verification of URL correctness, SHA-256 checksums, CMake flags, and dependency ordering. Key areas of focus include: (1) validation that the restructured init task and per-component workflows (core, spider) execute in correct sequence; (2) confirmation that all library-specific CMake arguments and tarball sources are accurate; (3) verification that dependency chains (e.g., spdlog → fmt, msgpack-cxx → boost) resolve correctly through the new task orchestration model; (4) review of the refactored colon-prefixed task references and the impact on include scope. Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
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 |
Bill-hbrhbr
changed the title
Bill-hbrhbr
changed the title
Bill-hbrhbr
requested review from
LinZhihao-723,
davidlion,
kirkrodrigues and
sitaowang1998
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (1)
taskfile.yaml (1)
130-162: Restore the dependency on the Spider build task.
deps:spider:spider-depstops after installing Spider’s prerequisites; it never runs thespiderbuild that produces{{.G_SPIDER_BUILD_DIR}}/spider-build/src/spider/spider_schedulerandspider_worker. Thersyncin the package recipe will therefore fail with missing inputs. Please depend on the full Spider build (e.g.,deps:spider) so these binaries exist before packaging.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (8)
taskfile.yaml(1 hunks)taskfiles/deps/cmake-libs.yaml(1 hunks)taskfiles/deps/core.yaml(1 hunks)taskfiles/deps/log-viewer.yaml(1 hunks)taskfiles/deps/main.yaml(1 hunks)taskfiles/deps/misc-libs.yaml(1 hunks)taskfiles/deps/spider.yaml(1 hunks)taskfiles/deps/utils.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/misc-libs.yaml
📚 Learning: 2025-07-23T09:54:45.185Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1122
File: components/core/src/clp/clp/CMakeLists.txt:175-195
Timestamp: 2025-07-23T09:54:45.185Z
Learning: In the CLP project, when reviewing CMakeLists.txt changes that introduce new compression library dependencies (BZip2, LibLZMA, LZ4, ZLIB), the team prefers to address conditional linking improvements in separate PRs rather than expanding the scope of focused migration PRs like the LibArchive task-based installation migration.
Applied to files:
taskfiles/deps/cmake-libs.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). (6)
- GitHub Check: package-image
- GitHub Check: musllinux_1_2-x86_64-static-linked-bins
- GitHub Check: ubuntu-jammy-lint
- GitHub Check: musllinux_1_2-x86_64-dynamic-linked-bins
- GitHub Check: build-macos (macos-15, false)
- GitHub Check: build-macos (macos-15, true)
|
If I understand correctly the scenario is that:
Is it correct to say the fix in this PR is splitting ==== (Assuming my understanding above is correct.) ==== On a side note, in the future only request a single reviewer at a time to reduce confusion and prevent duplicate comments. The yscope-maintainers group automatically notifies a bunch of people, but you probably want to talk to people to figure out the order of reviews (if you want multiple) or who has time. |
|
@CodeRabbit Open a PR in |
|
Tip For best results, initiate chat on the files or code changes. @sitaowang1998: I want to clarify your request. Based on the comment chain, davidlion mentioned opening an issue to track this problem. However, you mentioned opening a PR. I can help you:
Would you like me to open an issue in the
Please confirm if you'd like me to proceed with creating an issue. |
|
@CodeRabbit. Sorry I meant opening an issue |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 53 @sitaowang1998: No problem! I've created the issue in the 🔗 Issue #89: y-scope/yscope-dev-utils#89 The issue includes:
The issue is now ready for tracking and future implementation. |
This is already done in #1331. This PR is just a refactor of the taskfile structure with some minor fixes. The main problem is that The problem of including unnecessary settings in combined cmake setting files is not a big issue, as it does not affect the correctness of the build system. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
taskfiles/deps/cmake-libs.yaml (1)
165-167: Replace bashisms in the MariaDB precondition.
sourceand[[ … ]]aren’t available in/bin/sh, so this guard fails even on Ubuntu. Use POSIX syntax so the task can run.- - >- - source /etc/os-release && [[ "$ID" == "ubuntu" ]] + - >- + [ -r /etc/os-release ] && . /etc/os-release && [ "$ID" = "ubuntu" ]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
taskfiles/deps/cmake-libs.yaml(1 hunks)taskfiles/deps/core.yaml(1 hunks)taskfiles/deps/misc-libs.yaml(1 hunks)taskfiles/deps/spider.yaml(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
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.
📚 Learning: 2025-07-23T09:54:45.185Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1122
File: components/core/src/clp/clp/CMakeLists.txt:175-195
Timestamp: 2025-07-23T09:54:45.185Z
Learning: In the CLP project, when reviewing CMakeLists.txt changes that introduce new compression library dependencies (BZip2, LibLZMA, LZ4, ZLIB), the team prefers to address conditional linking improvements in separate PRs rather than expanding the scope of focused migration PRs like the LibArchive task-based installation migration.
Applied to files:
taskfiles/deps/cmake-libs.yaml
📚 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/misc-libs.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). (17)
- GitHub Check: ubuntu-jammy-dynamic-linked-bins
- GitHub Check: ubuntu-jammy-static-linked-bins
- GitHub Check: ubuntu-jammy-lint
- GitHub Check: package-image
- GitHub Check: manylinux_2_28-x86_64-static-linked-bins
- GitHub Check: manylinux_2_28-x86_64-dynamic-linked-bins
- GitHub Check: centos-stream-9-dynamic-linked-bins
- GitHub Check: centos-stream-9-static-linked-bins
- GitHub Check: musllinux_1_2-x86_64-dynamic-linked-bins
- GitHub Check: musllinux_1_2-x86_64-static-linked-bins
- GitHub Check: rust-checks (ubuntu-24.04)
- GitHub Check: rust-checks (ubuntu-22.04)
- GitHub Check: lint-check (ubuntu-24.04)
- GitHub Check: build-macos (macos-15, false)
- GitHub Check: build-macos (macos-14, true)
- GitHub Check: build-macos (macos-14, false)
- GitHub Check: build-macos (macos-15, true)
Bill-hbrhbr
changed the title
antlr-runtime and utfcpp to use utils:install-remote-cmake-lib.
taskfiles/deps/cmake-libs.yaml
Outdated
| - "-DANTLR4_INSTALL=ON" | ||
| - "-DCMAKE_BUILD_TYPE=Release" |
There was a problem hiding this comment.
| - "-DANTLR4_INSTALL=ON" | |
| - "-DCMAKE_BUILD_TYPE=Release" | |
| - "-DANTLR4_INSTALL=ON" | |
| - "-DBUILD_TESTING=OFF" | |
| - "-DCMAKE_BUILD_TYPE=Release" |
There was a problem hiding this comment.
BUILD_TESTING does not exist in antlr4-runtime.
There was a problem hiding this comment.
BUILD_TESTING is a variable in CTest. If a project use enable_testing, we can set BUILD_TESTING to turn off the test build. See https://cmake.org/cmake/help/latest/variable/BUILD_TESTING.html.
There was a problem hiding this comment.
I've found a better flag: -DANTLR_BUILD_CPP_TESTS=OFF
taskfiles/deps/cmake-libs.yaml
Outdated
| - "-DCMAKE_BUILD_TYPE=Release" | ||
| - "-DCMAKE_INSTALL_MESSAGE=LAZY" | ||
| - "-DCMAKE_INSTALL_PREFIX={{.G_DEPS_CPP_DIR}}/{{.LIB_NAME}}-install" | ||
| - "-DZLIB_BUILD_EXAMPLES=OFF" |
There was a problem hiding this comment.
| - "-DZLIB_BUILD_EXAMPLES=OFF" | |
| - "-DZLIB_BUILD_EXAMPLES=OFF" | |
| - "-DZLIB_BUILD_TESTING=OFF" |
There was a problem hiding this comment.
ZLIB_BUILD_TESTING is not present in zlib-1.3.1
There was a problem hiding this comment.
In 1.3.1, zlib uses enable_testing. However, it does not build test if example build if off. Same with utf8, let's leave it here with a comment.
Co-authored-by: sitaowang1998 <sitaowang1998@outlook.com>
|
To provide some context:
Many libraries expose test-related flags, but most are already Policy options we can adopt going forward:
Thoughts or preferences? |
|
As a side note, I'm trying to avoid scope creep in this PR. We should do as much copy-paste and moving code around as possible. But I guess I did modify the install methods of two libraries ( |
For 1. I think we can only apply |
I prefer to stick with 3 since it's the clearest to reason about. |
This reverts commit 5a2150c.
Co-authored-by: sitaowang1998 <sitaowang1998@outlook.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
taskfiles/deps/cmake-libs.yaml (3)
220-238: Add test build flag.Based on past review comments, explicitly setting
-DENABLE_TESTS=OFFwas suggested to disable test building for mongocxx.Apply this diff to disable test building:
- "-DBUILD_SHARED_AND_STATIC_LIBS=ON" - "-DBUILD_SHARED_LIBS_WITH_STATIC_MONGOC=ON" - "-DCMAKE_BUILD_TYPE=Release" - "-DCMAKE_INSTALL_MESSAGE=LAZY" - "-DENABLE_EXAMPLES=OFF" + - "-DENABLE_TESTS=OFF" - "-DENABLE_UNINSTALL=OFF"
68-79: Add test build flag.Based on past review comments, explicitly setting
-DENABLE_DATE_TESTING=OFFwas suggested to disable test building for the date library.Apply this diff to disable test building:
CMAKE_GEN_ARGS: - "-DCMAKE_BUILD_TYPE=Release" - "-DCMAKE_INSTALL_MESSAGE=LAZY" + - "-DENABLE_DATE_TESTING=OFF"
295-316: Add test build flag.Based on past review comments, explicitly setting
-DSPDLOG_BUILD_TESTS=OFFwas suggested. The current configuration includesSPDLOG_BUILD_TESTS_HO=OFFbut is missing the mainSPDLOG_BUILD_TESTSflag.Apply this diff to disable test building:
- "-DCMAKE_POLICY_DEFAULT_CMP0074=NEW" - "-DSPDLOG_BUILD_EXAMPLE=OFF" + - "-DSPDLOG_BUILD_TESTS=OFF" - "-DSPDLOG_BUILD_TESTS_HO=OFF" - "-DSPDLOG_BUILD_SHARED=ON"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
taskfiles/deps/cmake-libs.yaml(1 hunks)taskfiles/deps/main.yaml(1 hunks)
🧰 Additional context used
🧠 Learnings (6)
📓 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.
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1122
File: components/core/src/clp/clp/CMakeLists.txt:175-195
Timestamp: 2025-07-23T09:54:45.185Z
Learning: In the CLP project, when reviewing CMakeLists.txt changes that introduce new compression library dependencies (BZip2, LibLZMA, LZ4, ZLIB), the team prefers to address conditional linking improvements in separate PRs rather than expanding the scope of focused migration PRs like the LibArchive task-based installation migration.
📚 Learning: 2025-07-23T09:54:45.185Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1122
File: components/core/src/clp/clp/CMakeLists.txt:175-195
Timestamp: 2025-07-23T09:54:45.185Z
Learning: In the CLP project, when reviewing CMakeLists.txt changes that introduce new compression library dependencies (BZip2, LibLZMA, LZ4, ZLIB), the team prefers to address conditional linking improvements in separate PRs rather than expanding the scope of focused migration PRs like the LibArchive task-based installation migration.
Applied to files:
taskfiles/deps/cmake-libs.yaml
📚 Learning: 2025-09-15T18:53:15.844Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1226
File: taskfiles/deps/main.yaml:522-527
Timestamp: 2025-09-15T18:53:15.844Z
Learning: When ystdlib is built with -Dystdlib_BUILD_TESTING=OFF in taskfiles/deps/main.yaml, it does not require Catch2 settings to be injected via CMAKE_GEN_ARGS since testing is disabled and Catch2 is not needed during the build process.
Applied to files:
taskfiles/deps/cmake-libs.yaml
📚 Learning: 2025-08-19T07:08:15.583Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1229
File: taskfiles/deps/main.yaml:494-504
Timestamp: 2025-08-19T07:08:15.583Z
Learning: In zlib v1.3.1's CMake build system, only ZLIB_BUILD_EXAMPLES is a valid option. Other ZLIB_BUILD_* flags (MINIZIP, SHARED, STATIC, TESTING) and ZLIB_INSTALL are not defined in the CMakeLists.txt and have no effect. Use BUILD_SHARED_LIBS=ON for shared library control and ZLIB_BUILD_EXAMPLES=OFF to disable examples.
Applied to files:
taskfiles/deps/cmake-libs.yaml
📚 Learning: 2025-08-06T07:32:28.462Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1156
File: taskfiles/deps/main.yaml:70-71
Timestamp: 2025-08-06T07:32:28.462Z
Learning: In LZ4's CMake build system (build/cmake/CMakeLists.txt), the option to build static libraries is `BUILD_STATIC_LIBS=ON`, not `LZ4_BUILD_STATIC=ON`. The correct options for building both shared and static LZ4 libraries are `-DBUILD_SHARED_LIBS=ON` and `-DBUILD_STATIC_LIBS=ON`.
Applied to files:
taskfiles/deps/cmake-libs.yaml
📚 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:
taskfiles/deps/cmake-libs.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). (2)
- GitHub Check: rust-checks (ubuntu-22.04)
- GitHub Check: rust-checks (ubuntu-24.04)
🔇 Additional comments (21)
taskfiles/deps/main.yaml (4)
3-22: LGTM! Well-organized includes structure.The categorization of includes (project targets, utils, component libraries) improves clarity and maintainability. The flattening of component library tasks is appropriate for direct access.
29-30: LGTM! Checksums directory properly exposed.Making
G_DEPS_CPP_CHECKSUMS_DIRa public variable supports the modular dependency structure and aligns with the checksum management workflow described in the PR objectives.
42-46: LGTM! Spider correctly added to default dependencies.The explicit inclusion of spider in the default task aligns with the PR objectives and addresses the discussion in past review comments about keeping spider as a default task.
48-54: LGTM! Init task properly consolidated.The init task correctly creates directories in hierarchical order (G_DEPS_DIR → G_DEPS_CPP_DIR → G_DEPS_CPP_CHECKSUMS_DIR → G_DEPS_CPP_CMAKE_SETTINGS_DIR). The
internal: trueandrun: "once"attributes are appropriate for this initialization task.taskfiles/deps/cmake-libs.yaml (17)
3-12: LGTM! Clear variable definitions and policy documentation.The library name constants promote consistency, and the NOTE about CMP0074 policy provides valuable context for when and why the policy is set to NEW.
29-48: LGTM! ANTLR runtime correctly configured.The task properly addresses the test build flag discussion from past comments by using
-DANTLR_BUILD_CPP_TESTS=OFF. TheCMAKE_SOURCE_DIR: "runtime/Cpp"correctly points to the ANTLR runtime subdirectory, and the CMP0135 policy ensures extracted files use current timestamps.
50-66: LGTM! Catch2 properly configured.The test framework is correctly configured with testing disabled (
-DCATCH_BUILD_TESTING=OFF) and unnecessary extras excluded. The C++20 requirement aligns with the project's standards.
81-95: LGTM! fmt library properly configured.The task correctly builds shared libraries and disables both documentation and tests with
-DFMT_DOC=OFFand-DFMT_TEST=OFF.
97-143: LGTM! LibLZMA correctly configured with dual builds.The dual-build structure (shared and static) is necessary for LibLZMA and is implemented correctly. The test flag (
-DBUILD_TESTING=OFF) and tool flags are properly set. The join pattern on lines 139-140 works correctly in this context, as confirmed in past review discussions.
145-162: LGTM! log-surgeon properly configured with dependencies.The task correctly declares dependencies on fmt and microsoft.gsl, injects their CMake settings via
-Cflags, and disables test building with-Dlog_surgeon_BUILD_TESTING=OFF.
181-204: LGTM! MariaDB connector properly configured with platform restrictions.The Linux-only platform restriction and Ubuntu precondition are appropriate for Spider dependencies. The unit tests are correctly disabled with
-DWITH_UNIT_TESTS=OFF.
206-218: LGTM! Microsoft.GSL correctly configured.The task properly disables tests with
-DGSL_TEST=OFFand uses the library name variable consistently.
262-278: LGTM! nlohmann_json properly configured.The task correctly disables tests with
-DJSON_BuildTests=OFF. The NOTE about using the GitHub-generated tarball is helpful context for developers facing firewall restrictions.
280-293: LGTM! simdjson correctly configured.The task properly builds both shared and static libraries. Per past review discussions, simdjson does not support a BUILD_TESTING flag, so the configuration is complete as-is.
318-329: LGTM! utfcpp correctly configured.The task uses v4.0.6 which, per past review discussions, does not support
UTF8CPP_ENABLE_TESTSflag. The configuration is appropriate for this version.
331-343: LGTM! yaml-cpp properly configured.The task correctly disables tests with
-DYAML_CPP_BUILD_TESTS=OFF.
345-359: LGTM! ystdlib properly configured.The task correctly depends on Boost, disables testing with
-Dystdlib_BUILD_TESTING=OFF, and sets position-independent code generation. Per previous learnings, with testing disabled, Catch2 is not required during the build.Based on learnings
361-377: LGTM! zlib correctly configured.The task properly disables examples with
-DZLIB_BUILD_EXAMPLES=OFF. Per previous learnings, zlib v1.3.1 usesenable_testingbut does not build tests when examples are disabled, so the configuration is complete. The custom install prefix is appropriate.Based on learnings
379-395: LGTM! zstd properly configured.The task correctly builds both shared and static libraries, disables programs and tests with appropriate flags, and specifies the correct
CMAKE_SOURCE_DIRfor zstd's build system.
14-27: No explicit ABSL_BUILD_TESTING flag required.Abseil’s CMakeLists defaults
ABSL_BUILD_TESTINGto OFF, so adding-DABSL_BUILD_TESTING=OFFis unnecessary.
164-179: No test-related CMake options found; no change required.
There was a problem hiding this comment.
Actionable comments posted: 9
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (26)
taskfiles/deps/installs/absl.yaml(1 hunks)taskfiles/deps/installs/all.yaml(1 hunks)taskfiles/deps/installs/antlr-jar.yaml(1 hunks)taskfiles/deps/installs/antlr-runtime.yaml(1 hunks)taskfiles/deps/installs/boost.yaml(1 hunks)taskfiles/deps/installs/catch2.yaml(1 hunks)taskfiles/deps/installs/date.yaml(1 hunks)taskfiles/deps/installs/fmt.yaml(1 hunks)taskfiles/deps/installs/liblzma.yaml(1 hunks)taskfiles/deps/installs/log-surgeon.yaml(1 hunks)taskfiles/deps/installs/lz4.yaml(1 hunks)taskfiles/deps/installs/mariadb-connector-cpp.yaml(1 hunks)taskfiles/deps/installs/microsoft.gsl.yaml(1 hunks)taskfiles/deps/installs/mongocxx.yaml(1 hunks)taskfiles/deps/installs/msgpack-cxx.yaml(1 hunks)taskfiles/deps/installs/nlohmann_json.yaml(1 hunks)taskfiles/deps/installs/simdjson.yaml(1 hunks)taskfiles/deps/installs/spdlog.yaml(1 hunks)taskfiles/deps/installs/sqlite3.yaml(1 hunks)taskfiles/deps/installs/utfcpp.yaml(1 hunks)taskfiles/deps/installs/yaml-cpp.yaml(1 hunks)taskfiles/deps/installs/ystdlib.yaml(1 hunks)taskfiles/deps/installs/zlib.yaml(1 hunks)taskfiles/deps/installs/zstd.yaml(1 hunks)taskfiles/deps/main.yaml(1 hunks)taskfiles/deps/utils.yaml(4 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 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.
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1122
File: components/core/src/clp/clp/CMakeLists.txt:175-195
Timestamp: 2025-07-23T09:54:45.185Z
Learning: In the CLP project, when reviewing CMakeLists.txt changes that introduce new compression library dependencies (BZip2, LibLZMA, LZ4, ZLIB), the team prefers to address conditional linking improvements in separate PRs rather than expanding the scope of focused migration PRs like the LibArchive task-based installation migration.
📚 Learning: 2025-09-15T18:53:15.844Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1226
File: taskfiles/deps/main.yaml:522-527
Timestamp: 2025-09-15T18:53:15.844Z
Learning: When ystdlib is built with -Dystdlib_BUILD_TESTING=OFF in taskfiles/deps/main.yaml, it does not require Catch2 settings to be injected via CMAKE_GEN_ARGS since testing is disabled and Catch2 is not needed during the build process.
Applied to files:
taskfiles/deps/installs/catch2.yamltaskfiles/deps/installs/ystdlib.yaml
📚 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/installs/antlr-runtime.yamltaskfiles/deps/installs/antlr-jar.yaml
📚 Learning: 2025-07-23T09:54:45.185Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1122
File: components/core/src/clp/clp/CMakeLists.txt:175-195
Timestamp: 2025-07-23T09:54:45.185Z
Learning: In the CLP project, when reviewing CMakeLists.txt changes that introduce new compression library dependencies (BZip2, LibLZMA, LZ4, ZLIB), the team prefers to address conditional linking improvements in separate PRs rather than expanding the scope of focused migration PRs like the LibArchive task-based installation migration.
Applied to files:
taskfiles/deps/installs/zlib.yamltaskfiles/deps/installs/lz4.yamltaskfiles/deps/installs/liblzma.yaml
📚 Learning: 2025-08-25T07:27:19.449Z
Learnt from: Bill-hbrhbr
PR: y-scope/clp#1262
File: docs/src/dev-guide/components-core/index.md:50-50
Timestamp: 2025-08-25T07:27:19.449Z
Learning: In the CLP project, nlohmann_json version 3.11.3 is installed through their dependency management process, but they don't enforce version constraints in CMake find_package calls - they control versions through installation rather than CMake version pinning.
Applied to files:
taskfiles/deps/installs/nlohmann_json.yaml
🪛 GitHub Actions: clp-lint
taskfiles/deps/installs/msgpack-cxx.yaml
[error] 31-31: empty-lines: too many blank lines (1 > 0) in YAML file.
🪛 GitHub Check: lint-check (ubuntu-24.04)
taskfiles/deps/installs/antlr-runtime.yaml
[failure] 25-25:
25:1 [empty-lines] too many blank lines (1 > 0)
taskfiles/deps/installs/mongocxx.yaml
[failure] 24-24:
24:1 [empty-lines] too many blank lines (1 > 0)
taskfiles/deps/installs/msgpack-cxx.yaml
[failure] 31-31:
31:1 [empty-lines] too many blank lines (1 > 0)
🪛 YAMLlint (1.37.1)
taskfiles/deps/installs/antlr-runtime.yaml
[error] 25-25: too many blank lines (1 > 0)
(empty-lines)
taskfiles/deps/installs/mongocxx.yaml
[error] 24-24: too many blank lines (1 > 0)
(empty-lines)
taskfiles/deps/installs/msgpack-cxx.yaml
[error] 31-31: too many blank lines (1 > 0)
(empty-lines)
⏰ 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). (17)
- GitHub Check: musllinux_1_2-x86_64-dynamic-linked-bins
- GitHub Check: musllinux_1_2-x86_64-static-linked-bins
- GitHub Check: centos-stream-9-dynamic-linked-bins
- GitHub Check: centos-stream-9-static-linked-bins
- GitHub Check: manylinux_2_28-x86_64-static-linked-bins
- GitHub Check: ubuntu-jammy-dynamic-linked-bins
- GitHub Check: manylinux_2_28-x86_64-dynamic-linked-bins
- GitHub Check: package-image
- GitHub Check: ubuntu-jammy-lint
- GitHub Check: ubuntu-jammy-static-linked-bins
- GitHub Check: build-macos (macos-14, true)
- GitHub Check: build-macos (macos-15, true)
- GitHub Check: build-macos (macos-15, false)
- GitHub Check: build-macos (macos-14, false)
- GitHub Check: rust-checks (ubuntu-22.04)
- GitHub Check: rust-checks (ubuntu-24.04)
- GitHub Check: rust-checks (macos-15)
🔇 Additional comments (14)
taskfiles/deps/utils.yaml (2)
54-64: LGTM! Well-structured dynamic source glob.The INSTALL_SOURCE_GLOB logic correctly constructs a Task glob pattern that includes the install task's own YAML file plus all dependency YAML files. This ensures proper source tracking and cache invalidation when dependencies change.
74-97: LGTM! Namespace alignment with colon-prefixed task references.The task dependencies and commands now use fully-qualified colon-prefixed references (e.g.,
:yscope-dev-utils:checksum:validate), which improves namespace clarity and aligns with the PR's modularization objectives.taskfiles/deps/main.yaml (1)
47-54: LGTM! Centralized initialization addresses concurrency concerns.Moving the
inittask to main.yaml centralizes directory creation and ensures it's invoked only by top-level dependency tasks. This aligns with the PR's objective to address the race condition where multiple targets (core, spider) were invokinginstall-deps-and-generate-settingsconcurrently with the sameCMAKE_SETTINGS_DIR.taskfiles/deps/installs/spdlog.yaml (1)
30-32: LGTM! Helpful compatibility note.The comment about spdlog-fmt version compatibility is valuable documentation for future maintainers who may need to upgrade either library.
taskfiles/deps/installs/catch2.yaml (1)
15-16: Verify C++20 requirement is necessary.The explicit C++20 requirement may constrain projects that need to build with earlier C++ standards. Ensure this is intentional and aligns with the project's minimum C++ standard requirements.
taskfiles/deps/installs/ystdlib.yaml (1)
19-19: LGTM! Testing disabled as expected.As per learnings, when ystdlib is built with
-Dystdlib_BUILD_TESTING=OFF, it does not require Catch2 settings since testing is disabled. This is correctly implemented here.Based on learnings
taskfiles/deps/installs/zstd.yaml (1)
10-21: zstd install config looks good.Flags match upstream CMake options; source dir and tarball details look correct.
taskfiles/deps/installs/all.yaml (1)
8-31: Mapping file looks consistent.Includes are coherent and align with per-dependency taskfiles.
taskfiles/deps/installs/absl.yaml (1)
10-18: absl install config LGTM.Reasonable defaults (Release, C++20); tarball URL/SHA provided.
taskfiles/deps/installs/microsoft.gsl.yaml (1)
14-16: LIB_NAME is correctly sourced. G_GSL_LIB_NAME is defined in taskfiles/deps/main.yaml as "Microsoft.GSL", matching the expected CMake package name.taskfiles/deps/installs/lz4.yaml (1)
11-16: Retain BUILD_STATIC_LIBS as valid LZ4 option
BUILD_STATIC_LIBS is a supported, LZ4-specific CMake switch that LZ4 honours alongside BUILD_SHARED_LIBS.taskfiles/deps/installs/mongocxx.yaml (1)
12-18: No changes required
BUILD_SHARED_AND_STATIC_LIBS and BUILD_SHARED_LIBS_WITH_STATIC_MONGOC are valid upstream CMake flags in mongo-cxx-driver ≥ 3.5 (including r4.1.1).taskfiles/deps/installs/log-surgeon.yaml (1)
3-13: Structure and deps orchestration look good.The deps loop and once semantics align with the modularised flow. No issues spotted with namespacing or invocation order.
taskfiles/deps/installs/sqlite3.yaml (1)
17-23: Cannot verify task parameter contract; submodule not available in sandbox.The yscope-dev-utils repository is a Git submodule (https://github.com/y-scope/yscope-dev-utils.git) that is not cloned in this environment. Without access to the actual
download-and-extract-ziptask definition, I cannot confirm whether bothCHECKSUM_FILEandFILE_SHA256are valid simultaneously, nor can I determine the task's validation logic.Your original concern remains valid: passing both an MD5-named checksum file and an inline SHA256 hash to the same task is potentially ambiguous and warrants clarification from the external task's documentation or source code.
| CHECKSUM_FILE: "{{.G_DEPS_CPP_CHECKSUMS_DIR}}/antlr-jar.md5" | ||
| OUTPUT_FILE: "{{.G_ANTLR_JAR_FILE}}" | ||
| run: "once" | ||
| deps: | ||
| - task: ":yscope-dev-utils:checksum:validate" | ||
| vars: | ||
| CHECKSUM_FILE: "{{.CHECKSUM_FILE}}" | ||
| INCLUDE_PATTERNS: | ||
| - "{{.OUTPUT_FILE}}" | ||
| cmds: | ||
| - task: ":yscope-dev-utils:remote:curl" | ||
| vars: | ||
| FILE_SHA256: "eae2dfa119a64327444672aff63e9ec35a20180dc5b8090b7a6ab85125df4d76" | ||
| OUTPUT_FILE: "{{.OUTPUT_FILE}}" | ||
| URL: "https://www.antlr.org/download/antlr-{{.G_ANTLR_VERSION}}-complete.jar" | ||
|
|
||
| # This command must be last | ||
| - task: ":yscope-dev-utils:checksum:compute" | ||
| vars: | ||
| CHECKSUM_FILE: "{{.CHECKSUM_FILE}}" | ||
| INCLUDE_PATTERNS: | ||
| - "{{.OUTPUT_FILE}}" |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Checksum algorithm consistency (optional).
You verify download with SHA256 (curl) but store/validate a .md5 file. Functionally fine, but consider standardising on a single algorithm for stored checksums to reduce confusion.
If you want to align, confirm which checksum format is standard across deps (md5 vs sha256) and update this file accordingly.
🤖 Prompt for AI Agents
In taskfiles/deps/installs/antlr-jar.yaml around lines 7 to 28 the download is
being verified with a SHA256 value but the checksum file used/validated is named
.md5, causing algorithm inconsistency; decide which checksum algorithm your repo
standard uses (md5 or sha256) and make the file and tasks consistent: if
standard is sha256, rename CHECKSUM_FILE to use a .sha256 suffix, update any
checksum compute/validate tasks to compute/validate SHA256 and rename
vars/comments accordingly; if standard is md5, replace the CURL/FILE_SHA256
value with the MD5 checksum and ensure the compute/validate tasks
produce/consume an MD5; update any related task names or variables so the stored
checksum file, computed checksum algorithm, and the expected value all match.
| TARBALL_SHA256: "9f18272a9b32b622835a3365f850dd1063d60f5045fb1e12ce475ae6e18a35bb" | ||
| TARBALL_URL: "https://github.com/antlr/antlr4/archive/refs/tags/\ | ||
| {{.G_ANTLR_VERSION}}.tar.gz" | ||
|
|
There was a problem hiding this comment.
Fix YAML lint: remove trailing blank line.
YAMLlint reports an extra blank line at EOF.
Apply:
- 📝 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.
🧰 Tools
🪛 GitHub Check: lint-check (ubuntu-24.04)
[failure] 25-25:
25:1 [empty-lines] too many blank lines (1 > 0)
🪛 YAMLlint (1.37.1)
[error] 25-25: too many blank lines (1 > 0)
(empty-lines)
🤖 Prompt for AI Agents
In taskfiles/deps/installs/antlr-runtime.yaml around line 25, remove the
trailing blank line at the end of the file so the file ends on the last YAML
line instead of an extra newline; ensure there are no extra empty lines or
whitespace at EOF and save the file to satisfy YAML lint.
| CMAKE_GEN_ARGS: | ||
| - "-C {{.G_DEPS_CPP_CMAKE_SETTINGS_DIR}}/{{.G_FMT_LIB_NAME}}.cmake" | ||
| - "-C {{.G_DEPS_CPP_CMAKE_SETTINGS_DIR}}/{{.G_GSL_LIB_NAME}}.cmake" | ||
| - "-DCMAKE_BUILD_TYPE=Release" | ||
| - "-DCMAKE_INSTALL_MESSAGE=LAZY" | ||
| - "-Dlog_surgeon_BUILD_TESTING=OFF" | ||
| DEPS: | ||
| ref: ".DEPS" |
There was a problem hiding this comment.
🧩 Analysis chain
Verify the exact CMake option name to disable tests.
-Dlog_surgeon_BUILD_TESTING=OFF may be case- or name-mismatched (projects often use LOG_SURGEON_BUILD_TESTING or LOG_SURGEON_BUILD_TESTS). Please confirm upstream CMakeLists.
🌐 Web query:
In y-scope/log-surgeon CMake, what is the correct option to disable tests (LOG_SURGEON_BUILD_TESTING, LOG_SURGEON_BUILD_TESTS, or log_surgeon_BUILD_TESTING)?
💡 Result:
Use LOG_SURGEON_BUILD_TESTING. CMake's standard is BUILD_TESTING (from include(CTest)) and projects conventionally provide a project-prefixed override like MYPROJECT_BUILD_TESTING (e.g. LOG_SURGEON_BUILD_TESTING). [1][2]
Sources:
[1] CTest / BUILD_TESTING behavior (CMake docs). [2] Modern CMake advice showing use of a project-prefixed override (MYPROJECT_BUILD_TESTING).
Correct the CMake option to use uppercase project name prefix.
The option should be -DLOG_SURGEON_BUILD_TESTING=OFF (not log_surgeon_BUILD_TESTING). CMake projects follow the convention MYPROJECT_BUILD_TESTING with an uppercase project prefix, as documented in CTest standards.
🤖 Prompt for AI Agents
In taskfiles/deps/installs/log-surgeon.yaml around lines 16 to 23, the CMake
option uses a lowercase project prefix ("-Dlog_surgeon_BUILD_TESTING=OFF");
change it to the uppercase project prefix per CMake/CTest conventions by
replacing that option with "-DLOG_SURGEON_BUILD_TESTING=OFF" so the setting is
applied correctly.
| INSTALL_TASK_NAME: "log-surgeon" | ||
| LIB_NAME: "log_surgeon" | ||
| TARBALL_SHA256: "69a99e0804a52c6b6397c5e7eabecc9bb4915d0145632c66fc63ad13678ff56a" | ||
| TARBALL_URL: "https://github.com/y-scope/log-surgeon/archive/a722d07.tar.gz" |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Prefer full commit SHA in the GitHub archive URL.
Short SHAs can be ambiguous and are less stable. Use the 40‑char commit SHA in TARBALL_URL to improve reproducibility. Keep TARBALL_SHA256 in sync.
🤖 Prompt for AI Agents
In taskfiles/deps/installs/log-surgeon.yaml around lines 24 to 27, the
TARBALL_URL uses a short 7‑char commit SHA ("a722d07"); replace it with the full
40‑character commit SHA in the GitHub archive URL (e.g.
.../archive/<40-char-commit>.tar.gz) to ensure reproducible fetches, and after
updating the URL update TARBALL_SHA256 to the SHA256 of the tarball produced
from that full commit so the checksum and URL remain in sync.
| # NOTE: The MariaDB connector is only required for building Spider, which is only supported on | ||
| # Ubuntu. `INSTALL_LAYOUT` is currently hardcoded to "DEB". To support other Linux distros: | ||
| # - Update the precondition accordingly. | ||
| # - Make `INSTALL_LAYOUT` configurable. | ||
| - >- | ||
| source /etc/os-release && [[ "$ID" == "ubuntu" ]] |
There was a problem hiding this comment.
Replace non-POSIX source in precondition.
source is a bashism; go-task runs preconditions with /bin/sh (dash on Ubuntu), so this fails and the whole install task never runs. Use POSIX . instead.
Consider:
- - >-
- source /etc/os-release && [[ "$ID" == "ubuntu" ]]
+ - >-
+ . /etc/os-release && [ "$ID" = "ubuntu" ]📝 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.
| # NOTE: The MariaDB connector is only required for building Spider, which is only supported on | |
| # Ubuntu. `INSTALL_LAYOUT` is currently hardcoded to "DEB". To support other Linux distros: | |
| # - Update the precondition accordingly. | |
| # - Make `INSTALL_LAYOUT` configurable. | |
| - >- | |
| source /etc/os-release && [[ "$ID" == "ubuntu" ]] | |
| # NOTE: The MariaDB connector is only required for building Spider, which is only supported on | |
| # Ubuntu. `INSTALL_LAYOUT` is currently hardcoded to "DEB". To support other Linux distros: | |
| # - Update the precondition accordingly. | |
| # - Make `INSTALL_LAYOUT` configurable. | |
| - >- | |
| . /etc/os-release && [ "$ID" = "ubuntu" ] |
🤖 Prompt for AI Agents
In taskfiles/deps/installs/mariadb-connector-cpp.yaml around lines 8 to 13, the
precondition uses the non-POSIX bash builtin `source /etc/os-release` which
fails under /bin/sh (dash) and prevents the task from running; replace `source`
with the POSIX-compliant `.` (dot) form so the line becomes `. /etc/os-release
&& [[ "$ID" == "ubuntu" ]]`, or if you need strict POSIX for the test also
replace the bash-specific `[[ ... ]]` with `test`/`[` (e.g. `. /etc/os-release
&& [ "$ID" = "ubuntu" ]`) so the precondition works under /bin/sh.
| TARBALL_SHA256: "19dff3cf834a3e09229260f22a0325820a7e30c78b294db91794dd934776b33a" | ||
| TARBALL_URL: "https://github.com/mongodb/mongo-cxx-driver/releases/download/{{.VERSION}}\ | ||
| /mongo-cxx-driver-{{.VERSION}}.tar.gz" | ||
|
|
There was a problem hiding this comment.
Fix YAML lint: remove trailing blank line.
There’s an extra blank line at end of file; YAMLlint flags it.
Apply:
- 📝 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.
🧰 Tools
🪛 GitHub Check: lint-check (ubuntu-24.04)
[failure] 24-24:
24:1 [empty-lines] too many blank lines (1 > 0)
🪛 YAMLlint (1.37.1)
[error] 24-24: too many blank lines (1 > 0)
(empty-lines)
🤖 Prompt for AI Agents
In taskfiles/deps/installs/mongocxx.yaml around line 24, there is an extra
trailing blank line at the end of the file causing YAML lint to fail; remove the
blank empty line so the file ends with a single newline (no empty line after the
final content), then save and re-run YAML lint.
| TARBALL_SHA256: "7504b7af7e7b9002ce529d4f941e1b7fb1fb435768780ce7da4abaac79bb156f" | ||
| TARBALL_URL: "https://github.com/msgpack/msgpack-c/releases/download/cpp-{{.VERSION}}\ | ||
| /msgpack-cxx-{{.VERSION}}.tar.gz" | ||
|
|
There was a problem hiding this comment.
Remove the trailing blank line.
The linter has flagged this extra blank line at the end of the file, which violates the YAML formatting rules.
Apply this diff:
/msgpack-cxx-{{.VERSION}}.tar.gz"
-📝 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.
🧰 Tools
🪛 GitHub Actions: clp-lint
[error] 31-31: empty-lines: too many blank lines (1 > 0) in YAML file.
🪛 GitHub Check: lint-check (ubuntu-24.04)
[failure] 31-31:
31:1 [empty-lines] too many blank lines (1 > 0)
🪛 YAMLlint (1.37.1)
[error] 31-31: too many blank lines (1 > 0)
(empty-lines)
🤖 Prompt for AI Agents
In taskfiles/deps/installs/msgpack-cxx.yaml around line 31, there is an extra
trailing blank line at the end of the file — remove that final empty line so the
file ends immediately after the last content line (no extra newline/blank line
after the last entry) to satisfy the linter.
| - "mkdir -p '{{.INSTALL_INCLUDE_DIR}}'" | ||
| - "rm -f '{{.INSTALL_SYMLINK}}'" | ||
| - "ln -s '{{.SRC_DIR}}' '{{.INSTALL_SYMLINK}}'" |
There was a problem hiding this comment.
Make the symlink update idempotent and resilient.
rm -f won’t remove a pre-existing directory; ln -s will then fail. Prefer ln -sfn to atomically replace an existing symlink or directory entry.
Apply this diff:
- - "mkdir -p '{{.INSTALL_INCLUDE_DIR}}'"
- - "rm -f '{{.INSTALL_SYMLINK}}'"
- - "ln -s '{{.SRC_DIR}}' '{{.INSTALL_SYMLINK}}'"
+ - "mkdir -p '{{.INSTALL_INCLUDE_DIR}}'"
+ - "ln -sfn '{{.SRC_DIR}}' '{{.INSTALL_SYMLINK}}'"📝 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.
| - "mkdir -p '{{.INSTALL_INCLUDE_DIR}}'" | |
| - "rm -f '{{.INSTALL_SYMLINK}}'" | |
| - "ln -s '{{.SRC_DIR}}' '{{.INSTALL_SYMLINK}}'" | |
| - "mkdir -p '{{.INSTALL_INCLUDE_DIR}}'" | |
| - "ln -sfn '{{.SRC_DIR}}' '{{.INSTALL_SYMLINK}}'" |
🤖 Prompt for AI Agents
In taskfiles/deps/installs/sqlite3.yaml around lines 23 to 25, the current
sequence uses "rm -f '{{.INSTALL_SYMLINK}}'" then "ln -s '{{.SRC_DIR}}'
'{{.INSTALL_SYMLINK}}'", which fails if INSTALL_SYMLINK is an existing
directory; replace that sequence with a single atomic symlink creation using "ln
-sfn '{{.SRC_DIR}}' '{{.INSTALL_SYMLINK}}'" so the link is created or replaced
idempotently and resiliently (keep the mkdir line unchanged).
| - |- | ||
| echo "set(CLP_SQLITE3_SOURCE_DIRECTORY \"{{.SRC_DIR}}\")" \ | ||
| > "{{.G_DEPS_CPP_CMAKE_SETTINGS_DIR}}/{{.LIB_NAME}}.cmake" | ||
| echo "set(CLP_SQLITE3_INCLUDE_DIRECTORY \"{{.INSTALL_INCLUDE_DIR}}\")" \ | ||
| >> "{{.G_DEPS_CPP_CMAKE_SETTINGS_DIR}}/{{.LIB_NAME}}.cmake" |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Ensure settings dir exists and simplify output with a heredoc.
Safeguard against missing {{.G_DEPS_CPP_CMAKE_SETTINGS_DIR}} and reduce quoting/backslashes.
- - |-
- echo "set(CLP_SQLITE3_SOURCE_DIRECTORY \"{{.SRC_DIR}}\")" \
- > "{{.G_DEPS_CPP_CMAKE_SETTINGS_DIR}}/{{.LIB_NAME}}.cmake"
- echo "set(CLP_SQLITE3_INCLUDE_DIRECTORY \"{{.INSTALL_INCLUDE_DIR}}\")" \
- >> "{{.G_DEPS_CPP_CMAKE_SETTINGS_DIR}}/{{.LIB_NAME}}.cmake"
+ - "mkdir -p '{{.G_DEPS_CPP_CMAKE_SETTINGS_DIR}}'"
+ - |-
+ cat > "{{.G_DEPS_CPP_CMAKE_SETTINGS_DIR}}/{{.LIB_NAME}}.cmake" <<EOF
+ set(CLP_SQLITE3_SOURCE_DIRECTORY "{{.SRC_DIR}}")
+ set(CLP_SQLITE3_INCLUDE_DIRECTORY "{{.INSTALL_INCLUDE_DIR}}")
+ EOF📝 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.
| - |- | |
| echo "set(CLP_SQLITE3_SOURCE_DIRECTORY \"{{.SRC_DIR}}\")" \ | |
| > "{{.G_DEPS_CPP_CMAKE_SETTINGS_DIR}}/{{.LIB_NAME}}.cmake" | |
| echo "set(CLP_SQLITE3_INCLUDE_DIRECTORY \"{{.INSTALL_INCLUDE_DIR}}\")" \ | |
| >> "{{.G_DEPS_CPP_CMAKE_SETTINGS_DIR}}/{{.LIB_NAME}}.cmake" | |
| - "mkdir -p '{{.G_DEPS_CPP_CMAKE_SETTINGS_DIR}}'" | |
| - |- | |
| cat > "{{.G_DEPS_CPP_CMAKE_SETTINGS_DIR}}/{{.LIB_NAME}}.cmake" <<EOF | |
| set(CLP_SQLITE3_SOURCE_DIRECTORY "{{.SRC_DIR}}") | |
| set(CLP_SQLITE3_INCLUDE_DIRECTORY "{{.INSTALL_INCLUDE_DIR}}") | |
| EOF |
🤖 Prompt for AI Agents
In taskfiles/deps/installs/sqlite3.yaml around lines 26 to 30, ensure the target
settings directory exists and replace the echo + quoting + backslashes with a
simple heredoc: create the directory with mkdir -p
"{{.G_DEPS_CPP_CMAKE_SETTINGS_DIR}}" before writing, then write both set(...)
lines to "{{.G_DEPS_CPP_CMAKE_SETTINGS_DIR}}/{{.LIB_NAME}}.cmake" using a single
here-document (cat > file <<'EOF' ... EOF) to simplify quoting and avoid
multiple redirections.
5 participants
Description
Background
Library install tasks in main.yaml are designed to be reusable, but they were only used to build the project
clp-coreviatask deps:core.With the introduction of a new project
task deps:spiderin #1318 , we quickly realized that the library installation utilities in both this repo and in yscope-dev-utils were designed in a way that prevents easily adding a second CPP project.Specifically, there were several problems:
task deps:coreis up-to-date.up-to-datestatus of individual lib installation.cmake-settingsdirectory, instead of one for each project:cmake-settingsfile that lists the union of dependencies required by bothcoreandspider.deps:coreanddeps:spider-depwrite into different top-level CMake configuration files (fixes #1330). #1331 fixes the race conditions that could occur when both projects are trying to combine cmake settings to the same file.Solution
Changes in yscope-dev-utils are required first, and then we use the new utilities to fix the race conditions above:
reset-dirandreset-filetasks and apply across exported Taskfiles. yscope-dev-utils#91To make the future bug fix logic clearer for review, this PR introduces a purely cosmetic refactorization of main.yaml first.
Later, when we finally bump the
yscope-dev-utilsversion, we will open subsequent PRs for actual fixes:Main structural changes
Previously, all dependency tasks were bundled inside a single
main.yaml. Now, we split the tasks into modular, categorized files:utils:install-remote-cmake-libdue to well-written CMake project structure.Miscellaneous Changes
Other than the structural copy-paste refactoring, this PR also includes several one big and several small adjustments.
Big adjustment:
utils:install-remote-cmake-libantlr-runtime: instead of extracting the downloaded tar and removing the external layers outside the Cpp directory, we directly specify the directory whereCMakelists.txtlives.utfcpp: being an interface-only library, is still installable. We now install it as a regular library.Small adjustments:
deps:coreanddeps:spider-depno longer resets the cmake settings directory, which is already done byyscope-dev-utils:cmake:install-deps-and-generate-settingsdeps:section ofdeps:<target>-all-parallelnow iterates over a list variable.utils:inittask has been moved intomain.yamland is now only invoked by the top-level deps tasks:deps:coredeps:spider:spider-deputils:andyscope-dev-utils:inside some taskfiles are now prefixed with a leading colon (:) to align with updated namespaces.Summary
Restructuring in this PR improves clarity, modularity, and maintainability, and lays the groundwork for properly supporting multiple independent CPP project targets in the future.
For the prototype of the final version, see:
Checklist
breaking change.
Validation performed
Summary by CodeRabbit