-
Couldn't load subscription status.
- Fork 83
refactor(deps): Document standard rules for writing lib install tasks and apply across all existing tasks: #1479
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
base: main
Are you sure you want to change the base?
Conversation
WalkthroughAdds a dependency-taskfiles README and refactors Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Developer
participant TaskfileRunner as Taskfile runner
participant Installer as install-remote-cmake-lib
participant Remote as Tarball host
participant Build as CMake/Build
Note over TaskfileRunner,Installer #D6EAF8: New centralized flow per dependency
Developer->>TaskfileRunner: invoke dependency task (per-lib)
TaskfileRunner->>Installer: call with LIB_NAME, TARBALL_URL, TARBALL_SHA256, CMAKE_GEN_ARGS, CMAKE_SOURCE_DIR
Installer->>Remote: fetch tarball (TARBALL_URL)
alt checksum present
Remote-->>Installer: tarball
Installer->>Installer: verify SHA256
else checksum removed/inlined
Remote-->>Installer: tarball
end
Installer->>Installer: extract to <lib>-extracted
Installer->>Build: cmake configure (CMAKE_GEN_ARGS)
Build->>Build: build (shared/static per args)
Build->>Installer: install artifacts
Installer-->>TaskfileRunner: success/failure
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related issues
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used🧠 Learnings (4)📓 Common learnings📚 Learning: 2025-05-28T18:33:30.155ZApplied to files:
📚 Learning: 2025-09-15T18:53:15.844ZApplied to files:
📚 Learning: 2025-08-19T07:08:15.583ZApplied to files:
🪛 LanguageTooltaskfiles/deps/README.md[grammar] ~12-~12: A determiner may be missing. (THE_SUPERLATIVE) [style] ~32-~32: Consider using a different verb to strengthen your wording. (SPEED_UP_ACCELERATE) [grammar] ~32-~32: The word ‘install’ is not a noun. (A_INSTALL) ⏰ 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). (3)
🔇 Additional comments (4)
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 |
|
give me a diff of each lib install task in the current version against the files in PR #1377, diff should include comments as well. |
|
@coderabbitai Diff should include the comments inside the tasks. not your own comments. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 114 🏁 Script executed: Length of output: 278 🏁 Script executed: Length of output: 62 🏁 Script executed: Length of output: 2174 🏁 Script executed: Length of output: 20857 🏁 Script executed: Length of output: 388 🏁 Script executed: Length of output: 42 🏁 Script executed: Length of output: 956 🏁 Script executed: Length of output: 217 🏁 Script executed: Length of output: 3833 🏁 Script executed: Length of output: 2294 🏁 Script executed: Length of output: 1601 🏁 Script executed: Length of output: 20921 🏁 Script executed: Length of output: 2121 🏁 Script executed: Length of output: 23007 @Bill-hbrhbr: Here are the diffs for each lib install task between PR #1377 and current PR #1479, with all comments inside the tasks preserved: The key structural differences across all tasks: PR #1377 structure (separate files): version: "3"
tasks:
default:
# task contentCurrent PR #1479 structure (inline in main.yaml): task-name:
# task contentMain changes:
Special cases:
All 23 complete lib install tasks have been compared with their full content including comments preserved in the diffs above. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 1397 🏁 Script executed: Length of output: 390 🏁 Script executed: Length of output: 186 🏁 Script executed: Length of output: 92 🏁 Script executed: Length of output: 305 🏁 Script executed: Length of output: 2128 🏁 Script executed: Length of output: 1150 🏁 Script executed: Length of output: 3315 🏁 Script executed: Length of output: 1431 🏁 Script executed: Length of output: 20928 🏁 Script executed: Length of output: 1386 🏁 Script executed: Length of output: 3492 🏁 Script executed: Length of output: 326 🏁 Script executed: Length of output: 7819 🏁 Script executed: Length of output: 24656 @Bill-hbrhbr Here are the diffs for each lib install task comparing the current version in PR #1479 against PR #1377: Summary of Key DifferencesThe diffs reveal the structural changes between the two approaches: PR #1377 (Modular Approach)
Current PR #1479 (Monolithic Approach)
Notable Content Differences
All 23 library install task diffs are shown in the output above. The main difference is the organizational structure, with PR #1377 splitting each library into its own file following a standard pattern, while the current PR keeps them all in main.yaml. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
taskfiles/deps/README.md(1 hunks)taskfiles/deps/main.yaml(14 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
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-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-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/main.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/main.yaml
🪛 LanguageTool
taskfiles/deps/README.md
[uncategorized] ~14-~14: Possible missing comma found.
Context: ...d parsing version numbers from download URLs unless the version is used elsewhere. ...
(AI_HYDRA_LEO_MISSING_COMMA)
[style] ~30-~30: Consider using a different verb to strengthen your wording.
Context: ...s, examples, docs or unused binaries to speed up the install process. - Prefer disablin...
(SPEED_UP_ACCELERATE)
[grammar] ~30-~30: The word ‘install’ is not a noun.
Context: ...docs or unused binaries to speed up the install process. - Prefer disabling lib-specif...
(A_INSTALL)
🪛 markdownlint-cli2 (0.18.1)
taskfiles/deps/README.md
37-37: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
38-38: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
🔇 Additional comments (6)
taskfiles/deps/main.yaml (6)
155-186: LGTM: antlr-jar and antlr-runtime refactoring is clean.The antlr-jar task correctly references the global
G_ANTLR_JAR_FILEvariable, and the antlr-runtime task is properly refactored to useinstall-remote-cmake-libwith appropriate CMake flags. The CMP0135 policy and comment explaining its purpose are well preserved.
296-313: Verify whether log-surgeon needsCMP0074=NEWpolicy.The task uses
-C {{.G_DEPS_CPP_CMAKE_SETTINGS_DIR}}/{{.G_FMT_LIB_NAME}}.cmakeand{{.G_GSL_LIB_NAME}}.cmakeconfig files for dependencies, which may internally use<PackageName>_ROOTvariables. Per the README, CMP0074 should be set to NEW when the component depends on another via<lib_name>_ROOT. Compare this to msgpack-cxx (line 401) and spdlog (line 454), which both setCMAKE_POLICY_DEFAULT_CMP0074=NEWwhen using similar config files.Does log-surgeon's CMakeLists.txt use
find_package(fmt)andfind_package(Microsoft.GSL)that would benefit from CMP0074? If so, add: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_POLICY_DEFAULT_CMP0074=NEW" - "-DCMAKE_BUILD_TYPE=Release"
530-546: Verify whether ystdlib needsCMP0074=NEWpolicy.The task uses
-C {{.G_DEPS_CPP_CMAKE_SETTINGS_DIR}}/{{.G_BOOST_LIB_NAME}}.cmaketo load Boost configuration, which is the same pattern as msgpack-cxx (line 401) and spdlog (line 454). Both of those tasks explicitly setCMAKE_POLICY_DEFAULT_CMP0074=NEW, but ystdlib does not.Confirm whether ystdlib's CMakeLists.txt uses
find_package(Boost)in a way that would benefit from CMP0074. If so, add it to CMAKE_GEN_ARGS:CMAKE_GEN_ARGS: - "-C {{.G_DEPS_CPP_CMAKE_SETTINGS_DIR}}/{{.G_BOOST_LIB_NAME}}.cmake" + - "-DCMAKE_POLICY_DEFAULT_CMP0074=NEW" - "-DCMAKE_BUILD_TYPE=Release"
465-514: LGTM: sqlite3 and utfcpp follow new naming conventions.Both tasks correctly use the
-extracteddirectory suffix as per README guidance. The direct CMake config file generation via heredoc is appropriate for these special cases that don't use standard CMake install paths.
548-564: LGTM: zlib CMake arguments are correct.Uses only valid zlib v1.3.1 configuration options (specifically
ZLIB_BUILD_EXAMPLES=OFFrather than invalidZLIB_BUILD_*variants). The specialCMAKE_INSTALL_PREFIXhandling is appropriate for zlib's structure.
241-256: Verify whether fmt should build both shared and static libraries.Per the README, tasks should "build both shared and static libraries when possible." The fmt task (line 248) only sets
BUILD_SHARED_LIBS=ON. Verify whether fmt's CMake supports static library builds and whether both variants are needed by downstream consumers.If fmt supports and should build static libraries, add a corresponding flag or consider a pattern similar to liblzma (which invokes the task twice with different
BUILD_SHARED_LIBSvalues).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code lgtm. The doc needs some more works.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. This PR just makes me think that we need some documents in yscope-dev-utils to explain how to use the tasks. Some logic, e.g. checksum validation should be in task dependency, is hard to understand at first glance. Do you think it worth the effort @kirkrodrigues ?
@davidlion In the near future, we could add some simple docs on how-to-use the checksum stuff. |
antlr-jar.antlr-runtimeto build viautils:install-remote-cmake-lib.CMAKE_GEN_ARGSof each task to conform to the newly set rules.spiderplatform check POSIX-compatibleDescription
Due to various
CMAKE_GEN_ARGSissues encountered while reviewing other PRs such as #1377 and #1414, we standardize CMake lib writing once and for all to cut down on future review/development time.Checklist
breaking change.
Validation performed
Summary by CodeRabbit
Summary by CodeRabbit
Release Notes
Documentation
Chores