build: Lock task to v3.42.1 to avoid #872; Install task using deb/rpm package on Linux.
#881
Conversation
…deb/rpm package on Linux.
WalkthroughThis change enforces a specific version range for the Task tool (>= 3.38.0 and < 3.43.0) in documentation and installation scripts, addressing a known issue. It updates Dockerfiles and scripts to lock Task installation to version 3.42.1, and revises documentation to reflect the new version constraints and reference the related issue. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant InstallScript
participant GitHubReleases
participant PackageManager
User->>InstallScript: Run installation script
InstallScript->>InstallScript: Detect system architecture
InstallScript->>GitHubReleases: Download Task v3.42.1 for detected architecture
GitHubReleases-->>InstallScript: Return package file
InstallScript->>PackageManager: Install Task package
PackageManager-->>InstallScript: Confirm installation
InstallScript-->>User: Installation complete
Possibly related PRs
Suggested reviewers
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Pull Request Overview
This PR updates the Task version constraint in documentation to lock Task to versions >=3.38.0 and <3.43.0 to avoid unresolved issues (#872) and instructs Linux users to install Task via the appropriate deb/rpm packages.
- Updated Task version constraints and added version rationale in multiple docs.
- Reordered and added references to issue [clp-issue-872] in documentation files.
Reviewed Changes
Copilot reviewed 4 out of 9 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| docs/src/dev-guide/contributing-linting.md | Updated Task version range and added issue reference. |
| docs/src/dev-guide/components-core/index.md | Updated Task version range and added issue reference. |
| docs/src/dev-guide/building-package.md | Updated Task version range and added issue reference. |
| docs/README.md | Updated Task version range and added issue reference. |
Files not reviewed (5)
- components/core/tools/docker-images/clp-env-base-centos-stream-9/Dockerfile: Language not supported
- components/core/tools/docker-images/clp-env-base-ubuntu-jammy/Dockerfile: Language not supported
- components/core/tools/scripts/lib_install/centos-stream-9/install-prebuilt-packages.sh: Language not supported
- components/core/tools/scripts/lib_install/macos/install-all.sh: Language not supported
- components/core/tools/scripts/lib_install/ubuntu-jammy/install-prebuilt-packages.sh: Language not supported
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (9)
components/core/tools/docker-images/clp-env-base-centos-stream-9/Dockerfile(0 hunks)components/core/tools/docker-images/clp-env-base-ubuntu-jammy/Dockerfile(0 hunks)components/core/tools/scripts/lib_install/centos-stream-9/install-prebuilt-packages.sh(1 hunks)components/core/tools/scripts/lib_install/macos/install-all.sh(1 hunks)components/core/tools/scripts/lib_install/ubuntu-jammy/install-prebuilt-packages.sh(1 hunks)docs/README.md(2 hunks)docs/src/dev-guide/building-package.md(2 hunks)docs/src/dev-guide/components-core/index.md(2 hunks)docs/src/dev-guide/contributing-linting.md(2 hunks)
💤 Files with no reviewable changes (2)
- components/core/tools/docker-images/clp-env-base-centos-stream-9/Dockerfile
- components/core/tools/docker-images/clp-env-base-ubuntu-jammy/Dockerfile
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: centos-stream-9-deps-image
- GitHub Check: ubuntu-jammy-deps-image
- GitHub Check: build-macos (macos-14, false)
- GitHub Check: build-macos (macos-14, true)
- GitHub Check: build-macos (macos-13, true)
- GitHub Check: build-macos (macos-13, false)
- GitHub Check: lint-check (ubuntu-latest)
- GitHub Check: lint-check (macos-latest)
🔇 Additional comments (8)
docs/src/dev-guide/components-core/index.md (2)
12-13: Document bounded Task version range for core componentThe updated requirement correctly constrains Task to >= 3.38.0 and < 3.43.0 with a reference to the issue. This aligns with other docs and reflects the locking strategy.
113-115: Add and reference the issue and Task linksThe
[clp-issue-872]and[Task]link definitions properly resolve the version constraint reference and point to the Task homepage. Ensure link labels remain consistent across all guides.docs/src/dev-guide/contributing-linting.md (2)
18-19: Constrain Task version in linting requirementsThe linting requirements now correctly specify Task >= 3.38.0 and < 3.43.0, referencing the unresolved issue. This matches the locking applied elsewhere.
36-37: Insert link definitions for lintingThe new
[clp-lint]and[clp-issue-872]definitions are added and ordered alphabetically, improving clarity and resolving references.docs/README.md (2)
16-17: Enforce Task version bounds in documentationThe README now stipulates Task >= 3.38.0 and < 3.43.0, mirroring other guides and linking to the issue. This ensures users install a compatible version.
44-44: Link to issue for version constraintThe added
[clp-issue-872]definition correctly points to the GitHub issue, explaining the bounded version requirement.docs/src/dev-guide/building-package.md (2)
16-17: Specify Task version constraint for package buildUpdating the Task requirement to >= 3.38.0 and < 3.43.0 with an issue reference ensures consistency with the locked installation scripts.
72-73: Add link definitions for Task and issueThe link definitions
[clp-issue-872]and[Task]are correctly appended, resolving cross-references in the document.
| # Install a version of `task` < 3.43 to avoid https://github.com/y-scope/clp/issues/872 | ||
| task_formula_path="${formula_dir}/go-task.rb" | ||
| curl \ | ||
| --fail \ | ||
| --location \ | ||
| --output "$task_formula_path" \ | ||
| --show-error \ | ||
| https://raw.githubusercontent.com/Homebrew/homebrew-core/356f8408263b6a06e8f5f83cad574773d8054e1c\ | ||
| /Formula/g/go-task.rb | ||
| brew install --formula "$task_formula_path" |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Lock Homebrew go-task formula below v3.43
Fetching and installing a specific go-task formula version aligns with the documented constraints. To improve robustness, add error checking after brew install and clean up the temporary directory once installation completes.
| cmake_formula_path="${formula_dir}/cmake.rb" | ||
| curl \ | ||
| --fail \ | ||
| --location \ | ||
| --output "$cmake_formula_path" \ | ||
| --show-error \ | ||
| https://raw.githubusercontent.com/Homebrew/homebrew-core/b4e46db74e74a8c1650b38b1da222284ce1ec5ce\ | ||
| /Formula/c/cmake.rb | ||
| brew install --formula "$cmake_formula_path" |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Pin CMake formula to a specific commit
Downloading cmake.rb by commit ensures reproducible builds. Consider validating the curl download (e.g., via checksum) and reviewing the commit hash periodically to keep it up to date.
There was a problem hiding this comment.
Checksumming the file is a good idea but since this is (ideally) a temporary solution, I think we can omit it for now.
There was a problem hiding this comment.
@kirkrodrigues I understand your reasoning. You're right that for a temporary solution, adding checksums would be an unnecessary complication. Thanks for considering the suggestion!
✏️ Learnings added
Learnt from: kirkrodrigues
PR: y-scope/clp#881
File: components/core/tools/scripts/lib_install/macos/install-all.sh:15-23
Timestamp: 2025-05-06T09:46:42.619Z
Learning: For temporary solutions in installation scripts like those in `components/core/tools/scripts/lib_install/`, checksumming downloaded files is considered optional, particularly when working with trusted sources like GitHub raw content.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| formula_dir="$(mktemp -d)" | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Use secure temporary directory for formula files
Creating formula_dir with mktemp -d is good practice. To prevent leftover files, consider adding a trap 'rm -rf "$formula_dir"' EXIT after its creation.
| curl \ | ||
| --fail \ | ||
| --location \ | ||
| --output "$task_deb_pkg" \ | ||
| --show-error \ | ||
| "https://github.com/go-task/task/releases/download/v3.42.1/task_linux_${task_pkg_arch}.deb" | ||
| dpkg --install "$task_deb_pkg" |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Ensure .deb installation handles dependencies and errors.
If dpkg --install fails due to missing dependencies, the script aborts without resolving them. Chain apt-get install -f to automatically fix broken installs and consider adding set -o pipefail after set -u for robust error propagation.
-dpkg --install "$task_deb_pkg"
+dpkg --install "$task_deb_pkg" || DEBIAN_FRONTEND=noninteractive apt-get install -f -yThere was a problem hiding this comment.
I think it's safer to leave this to the user in case the broken packages aren't caused by dpkg --install.
There was a problem hiding this comment.
@kirkrodrigues That's a fair point. Leaving dependency resolution to the user provides more explicit control and prevents automatically fixing unrelated broken packages. Your approach is more conservative and appropriate for system-level scripts.
🐰 nods thoughtfully
✏️ Learnings added
Learnt from: kirkrodrigues
PR: y-scope/clp#881
File: components/core/tools/scripts/lib_install/ubuntu-jammy/install-prebuilt-packages.sh:35-41
Timestamp: 2025-05-06T09:48:55.375Z
Learning: For installation scripts in the CLP project, prefer explicit error handling over automatic dependency resolution (like `apt-get install -f`) when installing packages to give users more control over their system.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| # Install `task` | ||
| # NOTE: We lock `task` to a version < 3.43 to avoid https://github.com/y-scope/clp/issues/872 | ||
| task_pkg_arch=$(dpkg --print-architecture) | ||
| task_deb_pkg="$(mktemp -t --suffix ".deb")" |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
🛠️ Refactor suggestion
Improve temporary file creation and cleanup.
The current mktemp invocation may not be portable and leaves behind temporary files. Introduce a version variable for maintainability, use a reliable template in /tmp, and ensure cleanup via trap.
+TASK_VERSION="3.42.1"
-task_deb_pkg="$(mktemp -t --suffix ".deb")"
+task_deb_pkg=$(mktemp --suffix ".deb" /tmp/task_${TASK_VERSION}_XXXXXX.deb)
+trap 'rm -f "$task_deb_pkg"' EXIT📝 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.
| # Install `task` | |
| # NOTE: We lock `task` to a version < 3.43 to avoid https://github.com/y-scope/clp/issues/872 | |
| task_pkg_arch=$(dpkg --print-architecture) | |
| task_deb_pkg="$(mktemp -t --suffix ".deb")" | |
| # Install `task` | |
| # NOTE: We lock `task` to a version < 3.43 to avoid https://github.com/y-scope/clp/issues/872 | |
| TASK_VERSION="3.42.1" | |
| task_pkg_arch=$(dpkg --print-architecture) | |
| task_deb_pkg=$(mktemp --suffix ".deb" /tmp/task_${TASK_VERSION}_XXXXXX.deb) | |
| trap 'rm -f "$task_deb_pkg"' EXIT |
components/core/tools/scripts/lib_install/centos-stream-9/install-prebuilt-packages.sh
Outdated
Show resolved
Hide resolved
components/core/tools/scripts/lib_install/centos-stream-9/install-prebuilt-packages.sh
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
components/core/tools/scripts/lib_install/centos-stream-9/install-prebuilt-packages.sh (1)
42-52: 🧹 Nitpick (assertive)Improve temporary RPM handling and cleanup
For consistency and maintainability, introduce aTASK_VERSIONvariable and usemktempwith an explicit/tmptemplate. Also add anEXITtrap to ensure removal of the temp file on any failure. Example diff:+ TASK_VERSION="3.42.1" - task_pkg_path="$(mktemp -t --suffix ".rpm" task-pkg.XXXXXXXXXX)" + task_pkg_path=$(mktemp --suffix ".rpm" /tmp/task_${TASK_VERSION}_XXXXXX.rpm) + trap 'rm -f "$task_pkg_path"' EXIT
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
components/core/tools/scripts/lib_install/centos-stream-9/install-prebuilt-packages.sh(1 hunks)components/core/tools/scripts/lib_install/macos/install-all.sh(1 hunks)components/core/tools/scripts/lib_install/ubuntu-jammy/install-prebuilt-packages.sh(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
components/core/tools/scripts/lib_install/ubuntu-jammy/install-prebuilt-packages.sh (1)
Learnt from: kirkrodrigues
PR: y-scope/clp#881
File: components/core/tools/scripts/lib_install/ubuntu-jammy/install-prebuilt-packages.sh:35-41
Timestamp: 2025-05-06T09:48:55.375Z
Learning: For installation scripts in the CLP project, prefer explicit error handling over automatic dependency resolution (like `apt-get install -f`) when installing packages to give users more control over their system.
components/core/tools/scripts/lib_install/macos/install-all.sh (1)
Learnt from: kirkrodrigues
PR: y-scope/clp#881
File: components/core/tools/scripts/lib_install/macos/install-all.sh:15-23
Timestamp: 2025-05-06T09:46:42.619Z
Learning: For temporary solutions in installation scripts like those in `components/core/tools/scripts/lib_install/`, checksumming downloaded files is considered optional, particularly when working with trusted sources like GitHub raw content.
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: centos-stream-9-deps-image
- GitHub Check: ubuntu-jammy-deps-image
- GitHub Check: build-macos (macos-15, false)
- GitHub Check: build-macos (macos-15, true)
- GitHub Check: build-macos (macos-13, true)
- GitHub Check: build-macos (macos-13, false)
- GitHub Check: lint-check (macos-latest)
- GitHub Check: lint-check (ubuntu-latest)
🔇 Additional comments (5)
components/core/tools/scripts/lib_install/macos/install-all.sh (3)
15-23: Installation of CMake formula is correctly pinned and robust
The script fetches a specific commit of the Homebrew core formula and uses--fail --show-errorfor reliability. No changes needed here.
25-34: Pinnedgo-taskformula installation is correct
Locking thego-taskformula to a known commit ensures reproducible builds and avoids the issue in #872. The use of--failand--show-erroris appropriate.
36-36: Manual cleanup exists; no further action
The temporary directory is explicitly removed at the end of the script. Combined with the earlier trap suggestion, no additional cleanup is required here.components/core/tools/scripts/lib_install/centos-stream-9/install-prebuilt-packages.sh (2)
26-26: No action needed for blank line
The blank line improves readability and separates the package installation section from thetaskinstallation logic.
27-40: Architecture mapping is clear and maintainable
Using acasestatement to translate RPM arch names to the upstreamtaskpackage naming (amd64/arm64) aligns with best practices and prior feedback. Well done.
| formula_dir="$(mktemp -d -t "clp-dep-formulas")" | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Add cleanup trap for temporary directory
Currently, the script manually removes formula_dir at the end, but if any step fails before reaching that line, the temp directory will linger. Consider adding an EXIT trap immediately after creating formula_dir to ensure cleanup on any exit:
trap 'rm -rf "$formula_dir"' EXITThere was a problem hiding this comment.
We may want to debug the downloaded files on failure, so it's better not to remove them in that case.
There was a problem hiding this comment.
@kirkrodrigues That's a good point. Preserving the downloaded files on failure provides valuable debugging information. Your current approach of only cleaning up on successful completion is appropriate for this use case.
✏️ Learnings added
Learnt from: kirkrodrigues
PR: y-scope/clp#881
File: components/core/tools/scripts/lib_install/macos/install-all.sh:11-12
Timestamp: 2025-05-06T10:07:04.646Z
Learning: In CLP installation scripts, temporary directories with downloaded files should not be automatically cleaned up on failure (e.g., with EXIT traps) to preserve artifacts for debugging purposes.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
|
|
||
| # Install `task` | ||
| # NOTE: We lock `task` to a version < 3.43 to avoid https://github.com/y-scope/clp/issues/872 | ||
| task_pkg_arch=$(dpkg --print-architecture) | ||
| task_pkg_path="$(mktemp -t --suffix ".deb" task-pkg.XXXXXXXXXX)" | ||
| curl \ | ||
| --fail \ | ||
| --location \ | ||
| --output "$task_pkg_path" \ | ||
| --show-error \ | ||
| "https://github.com/go-task/task/releases/download/v3.42.1/task_linux_${task_pkg_arch}.deb" | ||
| dpkg --install "$task_pkg_path" | ||
| rm "$task_pkg_path" |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Improve .deb temporary file handling and version management
The architecture detection and dpkg --install steps correctly install task v3.42.1. To avoid leftover temp files on errors and simplify future version bumps, extract a TASK_VERSION="3.42.1" variable, use an explicit mktemp template under /tmp, and add an EXIT trap:
TASK_VERSION="3.42.1"
task_pkg_path=$(mktemp --suffix ".deb" /tmp/task_${TASK_VERSION}_XXXXXX.deb)
trap 'rm -f "$task_pkg_path"' EXITThere was a problem hiding this comment.
- By default, won't
mktempstore the files in the configured TMPDIR? - The version is only used in one place, so I don't think it's difficult for a developer to find and replace.
There was a problem hiding this comment.
@kirkrodrigues You're absolutely right on both points:
-
Yes,
mktempby default will use the configured TMPDIR (or/tmpif not set). Your current usage with-talready ensures this behavior. -
Agreed that with the version only used in one place, extracting it to a variable doesn't significantly improve maintainability.
The current implementation is reasonable. The only remaining suggestion would be adding an EXIT trap for more robust cleanup in case of errors, but that's an optional improvement.
✏️ Learnings added
Learnt from: kirkrodrigues
PR: y-scope/clp#881
File: components/core/tools/scripts/lib_install/ubuntu-jammy/install-prebuilt-packages.sh:30-42
Timestamp: 2025-05-06T10:08:04.248Z
Learning: In bash scripts, `mktemp` by default stores temporary files in the directory specified by the TMPDIR environment variable, or in `/tmp` if TMPDIR is not set. The `-t` flag explicitly tells `mktemp` to use the system's temporary directory.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
2 participants
Description
As the title says. Notably, since
taskreleases deb/rpm packages, on Linux, we can allow users to installtaskthrough the OS-specific install scripts.Checklist
breaking change.
Validation performed
Workflows succeeded.
Summary by CodeRabbit