feat(docker): Add container image containing the CLP package, for Docker Compose integration (resolves #1164). #1166
Conversation
WalkthroughAdds a new clp-package Docker image (Dockerfile, build script, task, docs), refactors the runtime image build action to support multiple image types via dynamic metadata, updates workflows to use the new action and adds a package-image job in clp-artifact-build. Adjusts apt package lists and sets npm cache path in the run-on-image action. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Developer/CI
participant WF as clp-artifact-build Workflow
participant Job as package-image Job
participant RunImg as run-on-image Action
participant Task as task docker-images:package
participant BuildAct as clp-build-runtime-image Action
participant Docker as docker/build-push-action
Dev->>WF: Push/PR triggers
WF->>WF: Filter relevant changes
WF->>Job: Start when deps satisfied
Job->>RunImg: Execute in ubuntu-jammy image
RunImg->>Task: task docker-images:package
Task->>Docker: Build clp-package:dev (local)
RunImg-->>Job: Exit
Job->>BuildAct: image_type=package
BuildAct->>BuildAct: Compute DOCKERFILE, IMAGE_NAME
BuildAct->>Docker: Build & push ghcr.io/.../clp-package tags
Docker-->>Job: Image published
sequenceDiagram
autonumber
actor Caller as Workflow/Job
participant Action as clp-build-runtime-image
participant Meta as docker/metadata-action
participant Builder as docker/build-push-action
Caller->>Action: inputs: image_type, platform_id?, platform_version_id?
Action->>Action: Compute metadata (DOCKERFILE, IMAGE_NAME)
Action->>Meta: name=IMAGE_NAME
Meta-->>Action: tags, labels
Action->>Builder: file=DOCKERFILE, tags, labels
Builder-->>Caller: Build/push result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
✨ Finishing Touches🧪 Generate unit tests
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. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
i understand the added dependencies are not strictly "core" related but i believe some others here (e.g. unzip) are in a similar situation. we might want to split the package specific dependencies in anther refactoring PR, though i personally don't see too much benefit yet. any concerns / other ideas?
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🔭 Outside diff range comments (3)
components/core/tools/scripts/lib_install/ubuntu-jammy/install-prebuilt-packages.sh (1)
9-10: Consider clearing APT cache to shrink layer size
apt-get update+installleaves/var/lib/apt/listspopulated. If this script is also used inside build stages, a final
apt-get clean && rm -rf /var/lib/apt/lists/*would avoid ~25 MB of dead weight..github/actions/run-on-image/action.yaml (1)
46-52: Ensure npm cache dir is writable inside rootless container
--env npm_config_cache=/tmp/.npmis a good move, but when running as an arbitrary UID the/tmp/.npmdirectory may be owned by root from a previous layer. To avoid EACCES, pre-create it with the correct permissions (e.g.,docker run … --tmpfs /tmp:rw,size=100m)..github/workflows/clp-execution-image-build.yaml (1)
46-48: Filter list still references removed fileThe
paths-filterstep keeps the oldclp-execution-image-build/action.yaml. Update it to the new generic action, otherwise the job won’t trigger when that file changes.- - ".github/actions/clp-execution-image-build/action.yaml" + - ".github/actions/clp-image-build/action.yaml"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (9)
.github/actions/clp-image-build/action.yaml(2 hunks).github/actions/run-on-image/action.yaml(1 hunks).github/workflows/clp-execution-image-build.yaml(2 hunks).github/workflows/clp-package-image-build.yaml(1 hunks)components/core/tools/scripts/lib_install/ubuntu-jammy/install-prebuilt-packages.sh(1 hunks)taskfile.yaml(1 hunks)tools/docker-images/clp-package-ubuntu-jammy/Dockerfile(1 hunks)tools/docker-images/clp-package-ubuntu-jammy/build.sh(1 hunks)tools/docker-images/clp-package-ubuntu-jammy/setup-scripts/install-prebuilt-packages.sh(1 hunks)
🧰 Additional context used
🧠 Learnings (14)
📓 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.
Learnt from: jackluo923
PR: y-scope/clp#1054
File: components/core/tools/docker-images/clp-env-base-musllinux_1_2-x86/build.sh:18-24
Timestamp: 2025-07-01T14:52:02.418Z
Learning: In the CLP project, consistency across platform build scripts is prioritized over defensive programming when it comes to git remote handling. All build.sh files in docker-images directories should follow the same pattern for git metadata injection.
Learnt from: jackluo923
PR: y-scope/clp#1054
File: components/core/tools/docker-images/clp-env-base-musllinux_1_2-x86/build.sh:18-24
Timestamp: 2025-07-01T14:52:02.418Z
Learning: In the CLP project, consistency across platform build scripts is prioritized over defensive programming when it comes to git remote handling. All build.sh files in docker-images directories should follow the same pattern for git metadata injection.
Learnt from: quinntaylormitchell
PR: y-scope/clp#918
File: .github/workflows/clp-execution-image-build.yaml:77-97
Timestamp: 2025-05-26T16:03:05.519Z
Learning: In .github/workflows/clp-execution-image-build.yaml, the ubuntu-jammy-execution-image and ubuntu-noble-execution-image jobs are intentionally kept separate (rather than using a matrix strategy) to make it easier to remove individual platform versions when they reach end of life, such as when jammy eventually becomes obsolete.
Learnt from: jackluo923
PR: y-scope/clp#1054
File: components/core/tools/docker-images/clp-env-base-musllinux_1_2-aarch64/build.sh:3-5
Timestamp: 2025-07-07T17:43:04.349Z
Learning: In CLP project build scripts (specifically build.sh files in docker-images directories), maintain consistency with the established pattern of using separate `set -eu` and `set -o pipefail` commands rather than combining them into `set -euo pipefail`, to ensure uniform script structure across all platform build scripts.
Learnt from: anlowee
PR: y-scope/clp#925
File: .github/workflows/clp-s-antlr-generation.yaml:24-27
Timestamp: 2025-05-27T20:04:51.498Z
Learning: The clp codebase uses commit SHAs instead of version tags for GitHub Actions (like actions/checkout) as an established pattern across workflow files.
Learnt from: jackluo923
PR: y-scope/clp#1054
File: components/core/tools/scripts/lib_install/musllinux_1_2/install-prebuilt-packages.sh:6-15
Timestamp: 2025-07-01T14:52:15.217Z
Learning: For installation scripts in the CLP project, maintain consistency in command patterns across platforms rather than applying platform-specific optimizations. When a platform follows a pattern of separate update and install commands (like `apt-get update && apt-get install` or `apk update && apk add`), preserve this pattern for uniform script structure.
Learnt from: jackluo923
PR: y-scope/clp#1054
File: components/core/tools/scripts/lib_install/musllinux_1_2/install-prebuilt-packages.sh:6-15
Timestamp: 2025-07-01T14:52:15.217Z
Learning: For installation scripts in the CLP project, maintain consistency in command patterns across platforms rather than applying platform-specific optimizations. When a platform follows a pattern of separate update and install commands (like `apt-get update && apt-get install` or `apk update && apk add`), preserve this pattern for uniform script structure.
Learnt from: jackluo923
PR: y-scope/clp#1054
File: components/core/tools/scripts/lib_install/musllinux_1_2/install-prebuilt-packages.sh:6-15
Timestamp: 2025-07-01T14:52:15.217Z
Learning: For installation scripts in the CLP project, maintain consistency in command patterns across platforms rather than applying platform-specific optimizations. When a platform follows a pattern of separate update and install commands (like `apt-get update && apt-get install` or `apk update && apk add`), preserve this pattern for uniform script structure.
Learnt from: jackluo923
PR: y-scope/clp#1054
File: components/core/tools/scripts/lib_install/musllinux_1_2/install-prebuilt-packages.sh:6-15
Timestamp: 2025-07-01T14:52:15.217Z
Learning: For installation scripts in the CLP project, maintain consistency in command patterns across different platforms (e.g., using separate update and install commands like `apk update && apk add`, `apt update && apt install`, `yum update && yum install`) rather than platform-specific optimizations, to ensure uniform script structure and readability.
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.408Z
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.
📚 Learning: in clp installation scripts, consistency across platform scripts is prioritized over defensive progr...
Learnt from: jackluo923
PR: y-scope/clp#1054
File: components/core/tools/scripts/lib_install/musllinux_1_2/install-prebuilt-packages.sh:27-32
Timestamp: 2025-07-07T17:41:15.655Z
Learning: In CLP installation scripts, consistency across platform scripts is prioritized over defensive programming improvements. For example, when extracting Task binaries with tar in `install-prebuilt-packages.sh`, the extraction pattern should remain consistent with other platform scripts rather than adding defensive flags like `--strip-components=1` to handle potential tarball layout changes.
Applied to files:
components/core/tools/scripts/lib_install/ubuntu-jammy/install-prebuilt-packages.shtools/docker-images/clp-package-ubuntu-jammy/Dockerfiletools/docker-images/clp-package-ubuntu-jammy/build.shtools/docker-images/clp-package-ubuntu-jammy/setup-scripts/install-prebuilt-packages.sh
📚 Learning: for installation scripts in the clp project, maintain consistency in command patterns across platfor...
Learnt from: jackluo923
PR: y-scope/clp#1054
File: components/core/tools/scripts/lib_install/musllinux_1_2/install-prebuilt-packages.sh:6-15
Timestamp: 2025-07-01T14:52:15.217Z
Learning: For installation scripts in the CLP project, maintain consistency in command patterns across platforms rather than applying platform-specific optimizations. When a platform follows a pattern of separate update and install commands (like `apt-get update && apt-get install` or `apk update && apk add`), preserve this pattern for uniform script structure.
Applied to files:
components/core/tools/scripts/lib_install/ubuntu-jammy/install-prebuilt-packages.shtools/docker-images/clp-package-ubuntu-jammy/Dockerfiletools/docker-images/clp-package-ubuntu-jammy/build.shtools/docker-images/clp-package-ubuntu-jammy/setup-scripts/install-prebuilt-packages.sh
📚 Learning: for installation scripts in the clp project, prefer explicit error handling over automatic dependenc...
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.408Z
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.
Applied to files:
components/core/tools/scripts/lib_install/ubuntu-jammy/install-prebuilt-packages.shtools/docker-images/clp-package-ubuntu-jammy/Dockerfiletools/docker-images/clp-package-ubuntu-jammy/build.shtools/docker-images/clp-package-ubuntu-jammy/setup-scripts/install-prebuilt-packages.sh
📚 Learning: for installation scripts in the clp project, maintain consistency in command patterns across differe...
Learnt from: jackluo923
PR: y-scope/clp#1054
File: components/core/tools/scripts/lib_install/musllinux_1_2/install-prebuilt-packages.sh:6-15
Timestamp: 2025-07-01T14:52:15.217Z
Learning: For installation scripts in the CLP project, maintain consistency in command patterns across different platforms (e.g., using separate update and install commands like `apk update && apk add`, `apt update && apt install`, `yum update && yum install`) rather than platform-specific optimizations, to ensure uniform script structure and readability.
Applied to files:
components/core/tools/scripts/lib_install/ubuntu-jammy/install-prebuilt-packages.shtools/docker-images/clp-package-ubuntu-jammy/Dockerfiletools/docker-images/clp-package-ubuntu-jammy/build.shtools/docker-images/clp-package-ubuntu-jammy/setup-scripts/install-prebuilt-packages.sh
📚 Learning: in clp installation scripts within `components/core/tools/scripts/lib_install/`, maintain consistenc...
Learnt from: jackluo923
PR: y-scope/clp#1054
File: components/core/tools/scripts/lib_install/musllinux_1_2/install-packages-from-source.sh:6-8
Timestamp: 2025-07-01T14:51:19.172Z
Learning: In CLP installation scripts within `components/core/tools/scripts/lib_install/`, maintain consistency with existing variable declaration patterns across platforms rather than adding individual improvements like `readonly` declarations.
Applied to files:
components/core/tools/scripts/lib_install/ubuntu-jammy/install-prebuilt-packages.shtools/docker-images/clp-package-ubuntu-jammy/build.shtools/docker-images/clp-package-ubuntu-jammy/setup-scripts/install-prebuilt-packages.sh
📚 Learning: in .github/workflows/clp-execution-image-build.yaml, the ubuntu-jammy-execution-image and ubuntu-nob...
Learnt from: quinntaylormitchell
PR: y-scope/clp#918
File: .github/workflows/clp-execution-image-build.yaml:77-97
Timestamp: 2025-05-26T16:03:05.519Z
Learning: In .github/workflows/clp-execution-image-build.yaml, the ubuntu-jammy-execution-image and ubuntu-noble-execution-image jobs are intentionally kept separate (rather than using a matrix strategy) to make it easier to remove individual platform versions when they reach end of life, such as when jammy eventually becomes obsolete.
Applied to files:
.github/workflows/clp-execution-image-build.yamltools/docker-images/clp-package-ubuntu-jammy/Dockerfiletaskfile.yamltools/docker-images/clp-package-ubuntu-jammy/build.sh.github/workflows/clp-package-image-build.yaml.github/actions/clp-image-build/action.yaml
📚 Learning: the clp codebase uses commit shas instead of version tags for github actions (like actions/checkout)...
Learnt from: anlowee
PR: y-scope/clp#925
File: .github/workflows/clp-s-antlr-generation.yaml:24-27
Timestamp: 2025-05-27T20:04:51.498Z
Learning: The clp codebase uses commit SHAs instead of version tags for GitHub Actions (like actions/checkout) as an established pattern across workflow files.
Applied to files:
.github/workflows/clp-execution-image-build.yaml.github/workflows/clp-package-image-build.yaml.github/actions/clp-image-build/action.yaml
📚 Learning: in the clp project, consistency across platform build scripts is prioritized over defensive programm...
Learnt from: jackluo923
PR: y-scope/clp#1054
File: components/core/tools/docker-images/clp-env-base-musllinux_1_2-x86/build.sh:18-24
Timestamp: 2025-07-01T14:52:02.418Z
Learning: In the CLP project, consistency across platform build scripts is prioritized over defensive programming when it comes to git remote handling. All build.sh files in docker-images directories should follow the same pattern for git metadata injection.
Applied to files:
.github/workflows/clp-execution-image-build.yamltools/docker-images/clp-package-ubuntu-jammy/Dockerfiletaskfile.yamltools/docker-images/clp-package-ubuntu-jammy/build.sh.github/workflows/clp-package-image-build.yaml.github/actions/clp-image-build/action.yaml
📚 Learning: in clp project build scripts (specifically build.sh files in docker-images directories), maintain co...
Learnt from: jackluo923
PR: y-scope/clp#1054
File: components/core/tools/docker-images/clp-env-base-musllinux_1_2-aarch64/build.sh:3-5
Timestamp: 2025-07-07T17:43:04.349Z
Learning: In CLP project build scripts (specifically build.sh files in docker-images directories), maintain consistency with the established pattern of using separate `set -eu` and `set -o pipefail` commands rather than combining them into `set -euo pipefail`, to ensure uniform script structure across all platform build scripts.
Applied to files:
.github/workflows/clp-execution-image-build.yamltools/docker-images/clp-package-ubuntu-jammy/Dockerfiletools/docker-images/clp-package-ubuntu-jammy/build.sh.github/actions/clp-image-build/action.yamltools/docker-images/clp-package-ubuntu-jammy/setup-scripts/install-prebuilt-packages.sh
📚 Learning: for the clp-core debian package creation script, strict version format validation is considered unne...
Learnt from: jackluo923
PR: y-scope/clp#718
File: components/core/tools/scripts/utils/create-debian-package.py:41-41
Timestamp: 2025-02-12T22:24:17.723Z
Learning: For the clp-core Debian package creation script, strict version format validation is considered unnecessary complexity and should be avoided.
Applied to files:
tools/docker-images/clp-package-ubuntu-jammy/Dockerfile
📚 Learning: in the clp project, when reviewing cmakelists.txt changes that introduce new compression library dep...
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:
tools/docker-images/clp-package-ubuntu-jammy/Dockerfiletaskfile.yaml.github/workflows/clp-package-image-build.yaml
📚 Learning: in the taskfiles dependency system (taskfiles/deps/main.yaml), echo commands are used to generate .c...
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:
taskfile.yaml
📚 Learning: when reviewing wrapper scripts in `components/clp-package-utils/clp_package_utils/scripts/`, note th...
Learnt from: haiqi96
PR: y-scope/clp#594
File: components/clp-package-utils/clp_package_utils/scripts/del_archives.py:56-65
Timestamp: 2024-11-18T16:49:20.248Z
Learning: When reviewing wrapper scripts in `components/clp-package-utils/clp_package_utils/scripts/`, note that it's preferred to keep error handling simple without adding extra complexity.
Applied to files:
tools/docker-images/clp-package-ubuntu-jammy/build.shtools/docker-images/clp-package-ubuntu-jammy/setup-scripts/install-prebuilt-packages.sh
🪛 Checkov (3.2.334)
tools/docker-images/clp-package-ubuntu-jammy/Dockerfile
[LOW] 1-22: Ensure that HEALTHCHECK instructions have been added to container images
(CKV_DOCKER_2)
[LOW] 1-22: Ensure that a user for the container has been created
(CKV_DOCKER_3)
🪛 Hadolint (2.12.0)
tools/docker-images/clp-package-ubuntu-jammy/Dockerfile
[info] 9-9: Multiple consecutive RUN instructions. Consider consolidation.
(DL3059)
🪛 Shellcheck (0.10.0)
tools/docker-images/clp-package-ubuntu-jammy/build.sh
[warning] 25-25: Quote to prevent word splitting/globbing, or split robustly with mapfile or read -a.
(SC2206)
🪛 actionlint (1.7.7)
.github/workflows/clp-package-image-build.yaml
22-22: shellcheck reported issue in this script: SC2046:warning:1:7: Quote this to prevent word splitting
(shellcheck)
22-22: shellcheck reported issue in this script: SC2046:warning:1:16: Quote this to prevent word splitting
(shellcheck)
56-56: shellcheck reported issue in this script: SC2046:warning:1:7: Quote this to prevent word splitting
(shellcheck)
56-56: shellcheck reported issue in this script: SC2046:warning:1:16: Quote this to prevent word splitting
(shellcheck)
84-84: shellcheck reported issue in this script: SC2046:warning:1:7: Quote this to prevent word splitting
(shellcheck)
84-84: shellcheck reported issue in this script: SC2046:warning:1:16: Quote this to prevent word splitting
(shellcheck)
90-90: property "filter-relevant-changes" is not defined in object type {ubuntu-jammy-deps-image: {outputs: {}; result: string}}
(expression)
🔇 Additional comments (3)
components/core/tools/scripts/lib_install/ubuntu-jammy/install-prebuilt-packages.sh (1)
24-31: Addition ofpython3-devandrsynclooks correct, but please verify cross-platform consistencyThese two packages are now part of the Ubuntu-Jammy pre-built list, yet they are still absent from the analogous install scripts for musllinux 1.2, noble, etc. Given the project’s emphasis on keeping the per-platform scripts in sync (see learnings from jackluo923), ensure the other platform flavours don’t silently diverge.
Would you like a quick grep script to confirm the package lists across all
install-prebuilt-packages.shvariants?taskfile.yaml (1)
165-175:docker-image-packagetask LGTMTask definition follows existing style (localised
SRC_DIR, depends onpackage, full sources glob). No concerns.tools/docker-images/clp-package-ubuntu-jammy/setup-scripts/install-prebuilt-packages.sh (1)
9-19: Runtime image might needpython3-devfor native pip buildsThe package image omits
python3-dev, unlike the build-time script. If any wheels with C-extensions are installed at runtime (e.g., viapip installinside the container), compilation will fail. Confirm that all required native wheels are pre-built; otherwise add the header/devel package.
| images: "${{inputs.image_registry}}/${{steps.sanitization.outputs.REPOSITORY}}\ | ||
| /clp-execution-x86-${{inputs.platform_id}}-${{inputs.platform_version_id}}" | ||
| /clp-${{inputs.image_type}}-x86-${{inputs.platform_id}}-${{inputs.platform_version_id}}" | ||
|
|
There was a problem hiding this comment.
Hard-coded x86 causes wrong tags for ARM images
The docker/metadata-action line still bakes in -x86- even though the action is now multi-purpose.
Attempts to publish arm64 images will end up mis-tagged.
- /clp-${{inputs.image_type}}-x86-${{inputs.platform_id}}-${{inputs.platform_version_id}}"
+ /clp-${{inputs.image_type}}-${{inputs.platform_id}}-${{inputs.platform_version_id}}"(Alternatively inject ${{env.ARCH_NAME}} when multi-arch builds land.)
📝 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.
| images: "${{inputs.image_registry}}/${{steps.sanitization.outputs.REPOSITORY}}\ | |
| /clp-execution-x86-${{inputs.platform_id}}-${{inputs.platform_version_id}}" | |
| /clp-${{inputs.image_type}}-x86-${{inputs.platform_id}}-${{inputs.platform_version_id}}" | |
| images: "${{inputs.image_registry}}/${{steps.sanitization.outputs.REPOSITORY}}\ | |
| /clp-${{inputs.image_type}}-${{inputs.platform_id}}-${{inputs.platform_version_id}}" |
🤖 Prompt for AI Agents
In .github/actions/clp-image-build/action.yaml around lines 58 to 60, the image
tag hardcodes "x86" which causes incorrect tags for ARM images. Replace the
fixed "x86" string with a dynamic variable that reflects the actual
architecture, such as using an input or environment variable like
`${{inputs.arch}}` or `${{env.ARCH_NAME}}` to correctly tag images for different
architectures.
| - name: "Work around actions/runner-images/issues/6775" | ||
| run: "chown $(id -u):$(id -g) -R ." | ||
| shell: "bash" | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Quote $(id -u) and $(id -g) to silence ShellCheck SC2046
The workaround lines can trigger word-splitting warnings; quoting fixes it.
- run: "chown $(id -u):$(id -g) -R ."
+ run: "chown $(id -u):$(id -g) -R ."(Only quotes needed around the whole expression if re-templated; keep consistent across all three occurrences.)
Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 actionlint (1.7.7)
22-22: shellcheck reported issue in this script: SC2046:warning:1:7: Quote this to prevent word splitting
(shellcheck)
22-22: shellcheck reported issue in this script: SC2046:warning:1:16: Quote this to prevent word splitting
(shellcheck)
🤖 Prompt for AI Agents
In .github/workflows/clp-package-image-build.yaml around lines 21 to 24, the
shell command uses unquoted command substitutions $(id -u) and $(id -g), which
can cause word-splitting warnings from ShellCheck SC2046. Fix this by adding
double quotes around each command substitution, changing them to "$(id -u)" and
"$(id -g)" to properly quote the expressions and prevent word splitting.
| ubuntu-jammy-package-image: | ||
| name: "ubuntu-jammy-package-image" | ||
| needs: "ubuntu-jammy-deps-image" | ||
| runs-on: "ubuntu-24.04" | ||
| env: |
There was a problem hiding this comment.
Job cannot reference filter-relevant-changes outputs because it isn’t in needs
ubuntu-jammy-package-image uses
needs.filter-relevant-changes.outputs.ubuntu_jammy_image_changed
but only lists ubuntu-jammy-deps-image in needs.
Workflow evaluation will fail at runtime.
-needs: "ubuntu-jammy-deps-image"
+needs:
+ - "ubuntu-jammy-deps-image"
+ - "filter-relevant-changes"📝 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.
| ubuntu-jammy-package-image: | |
| name: "ubuntu-jammy-package-image" | |
| needs: "ubuntu-jammy-deps-image" | |
| runs-on: "ubuntu-24.04" | |
| env: | |
| ubuntu-jammy-package-image: | |
| name: "ubuntu-jammy-package-image" | |
| needs: | |
| - "ubuntu-jammy-deps-image" | |
| - "filter-relevant-changes" | |
| runs-on: "ubuntu-24.04" | |
| env: |
🤖 Prompt for AI Agents
In .github/workflows/clp-package-image-build.yaml around lines 71 to 75, the job
ubuntu-jammy-package-image references outputs from filter-relevant-changes in
its needs but does not list filter-relevant-changes in its needs array. To fix
this, add filter-relevant-changes to the needs list for
ubuntu-jammy-package-image so it can access the required outputs and avoid
runtime evaluation errors.
| if command -v git >/dev/null && git -C "$script_dir" rev-parse --is-inside-work-tree >/dev/null ; | ||
| then | ||
| build_cmd+=( | ||
| --label "org.opencontainers.image.revision=$(git -C "$script_dir" rev-parse HEAD)" | ||
| --label "org.opencontainers.image.source=$(git -C "$script_dir" remote get-url origin)" | ||
| ) |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Use repository root for Git labels to stay consistent with other image builders
Current call runs git -C "$script_dir", so the commit hash points at this directory, not the repo’s HEAD.
Down-stream tooling (e.g., SBOM, provenance scanners) expects the top-level commit SHAs—mirroring the pattern in existing clp-env-base-* scripts.
- --label "org.opencontainers.image.revision=$(git -C "$script_dir" rev-parse HEAD)"
- --label "org.opencontainers.image.source=$(git -C "$script_dir" remote get-url origin)"
+ --label "org.opencontainers.image.revision=$(git -C "$repo_root" rev-parse HEAD)"
+ --label "org.opencontainers.image.source=$(git -C "$repo_root" remote get-url origin)"🤖 Prompt for AI Agents
In tools/docker-images/clp-package-ubuntu-jammy/build.sh around lines 30 to 35,
the git commands use "$script_dir" which points to the current directory, not
the repository root. Update the git commands to use the repository root
directory by running `git rev-parse --show-toplevel` to get the root path and
then use `git -C` with that root path for the commit hash and remote URL labels.
This ensures the labels reference the repo's HEAD commit consistently with other
image builders.
| ENV PATH="/opt/clp/sbin:${PATH}" | ||
| ENV PATH="/opt/clp/bin:${PATH}" | ||
| ENV PYTHONPATH="/opt/clp/lib/python3/site-packages" | ||
| ENV CLP_HOME="/opt/clp" |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Combine PATH modifications into a single ENV and preserve original order
Multiple ENV PATH= lines duplicate state and create extra layers. A single declaration is clearer:
-ENV PATH="/opt/clp/sbin:${PATH}"
-ENV PATH="/opt/clp/bin:${PATH}"
+ENV PATH="/opt/clp/sbin:/opt/clp/bin:${PATH}"🧰 Tools
🪛 Checkov (3.2.334)
[LOW] 1-22: Ensure that HEALTHCHECK instructions have been added to container images
(CKV_DOCKER_2)
[LOW] 1-22: Ensure that a user for the container has been created
(CKV_DOCKER_3)
🤖 Prompt for AI Agents
In tools/docker-images/clp-package-ubuntu-jammy/Dockerfile at lines 19 to 22,
multiple ENV instructions modify PATH separately, causing redundant layers and
potential order issues. Combine the two PATH modifications into a single ENV
statement that prepends both /opt/clp/sbin and /opt/clp/bin to the existing PATH
in the correct order, then keep the PYTHONPATH and CLP_HOME ENV declarations as
separate lines.
There was a problem hiding this comment.
wouldn't splitting into multiple lines be more VCS friendly?
There was a problem hiding this comment.
@junhaoliao You raise an excellent point about VCS friendliness! You're absolutely right that separate ENV lines make it much easier to track changes in version control - when someone adds or removes a path component, it shows up as a clean line addition/removal rather than a modification to an entire concatenated string.
The trade-off is:
- Multiple ENV lines: Better for VCS diffs and change tracking
- Single ENV line: Fewer Docker layers and slightly more efficient
Given that this is a new Dockerfile and you're prioritizing maintainability and clear change tracking, keeping the separate ENV lines is a perfectly reasonable choice. The Docker layer efficiency gain is minimal compared to the VCS clarity benefit you mentioned.
I'll defer to your preference on this one! 🐰
🧠 Learnings used
Learnt from: quinntaylormitchell
PR: y-scope/clp#918
File: .github/workflows/clp-execution-image-build.yaml:77-97
Timestamp: 2025-05-26T16:03:05.519Z
Learning: In .github/workflows/clp-execution-image-build.yaml, the ubuntu-jammy-execution-image and ubuntu-noble-execution-image jobs are intentionally kept separate (rather than using a matrix strategy) to make it easier to remove individual platform versions when they reach end of life, such as when jammy eventually becomes obsolete.
Learnt from: jackluo923
PR: y-scope/clp#1054
File: components/core/tools/docker-images/clp-env-base-musllinux_1_2-x86/build.sh:18-24
Timestamp: 2025-07-01T14:52:02.418Z
Learning: In the CLP project, consistency across platform build scripts is prioritized over defensive programming when it comes to git remote handling. All build.sh files in docker-images directories should follow the same pattern for git metadata injection.
Learnt from: jackluo923
PR: y-scope/clp#1054
File: components/core/tools/docker-images/clp-env-base-musllinux_1_2-x86/build.sh:18-24
Timestamp: 2025-07-01T14:52:02.418Z
Learning: In the CLP project, consistency across platform build scripts is prioritized over defensive programming when it comes to git remote handling. All build.sh files in docker-images directories should follow the same pattern for git metadata injection.
Learnt from: jackluo923
PR: y-scope/clp#1054
File: components/core/tools/docker-images/clp-env-base-musllinux_1_2-aarch64/build.sh:3-5
Timestamp: 2025-07-07T17:43:04.349Z
Learning: In CLP project build scripts (specifically build.sh files in docker-images directories), maintain consistency with the established pattern of using separate `set -eu` and `set -o pipefail` commands rather than combining them into `set -euo pipefail`, to ensure uniform script structure across all platform build scripts.
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.408Z
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.
Learnt from: jackluo923
PR: y-scope/clp#1054
File: components/core/tools/scripts/lib_install/musllinux_1_2/install-prebuilt-packages.sh:6-15
Timestamp: 2025-07-01T14:52:15.217Z
Learning: For installation scripts in the CLP project, maintain consistency in command patterns across platforms rather than applying platform-specific optimizations. When a platform follows a pattern of separate update and install commands (like `apt-get update && apt-get install` or `apk update && apk add`), preserve this pattern for uniform script structure.
Learnt from: jackluo923
PR: y-scope/clp#1054
File: components/core/tools/scripts/lib_install/musllinux_1_2/install-prebuilt-packages.sh:6-15
Timestamp: 2025-07-01T14:52:15.217Z
Learning: For installation scripts in the CLP project, maintain consistency in command patterns across platforms rather than applying platform-specific optimizations. When a platform follows a pattern of separate update and install commands (like `apt-get update && apt-get install` or `apk update && apk add`), preserve this pattern for uniform script structure.
Learnt from: jackluo923
PR: y-scope/clp#1054
File: components/core/tools/scripts/lib_install/musllinux_1_2/install-prebuilt-packages.sh:6-15
Timestamp: 2025-07-01T14:52:15.217Z
Learning: For installation scripts in the CLP project, maintain consistency in command patterns across platforms rather than applying platform-specific optimizations. When a platform follows a pattern of separate update and install commands (like `apt-get update && apt-get install` or `apk update && apk add`), preserve this pattern for uniform script structure.
Learnt from: jackluo923
PR: y-scope/clp#718
File: components/core/tools/scripts/utils/create-debian-package.py:41-41
Timestamp: 2025-02-12T22:24:17.723Z
Learning: For the clp-core Debian package creation script, strict version format validation is considered unnecessary complexity and should be avoided.
Learnt from: jackluo923
PR: y-scope/clp#1054
File: components/core/tools/scripts/lib_install/musllinux_1_2/install-prebuilt-packages.sh:6-15
Timestamp: 2025-07-01T14:52:15.217Z
Learning: For installation scripts in the CLP project, maintain consistency in command patterns across different platforms (e.g., using separate update and install commands like `apk update && apk add`, `apt update && apt install`, `yum update && yum install`) rather than platform-specific optimizations, to ensure uniform script structure and readability.
Learnt from: jackluo923
PR: y-scope/clp#1054
File: components/core/tools/scripts/lib_install/musllinux_1_2/install-packages-from-source.sh:6-8
Timestamp: 2025-07-01T14:51:19.172Z
Learning: In CLP installation scripts within `components/core/tools/scripts/lib_install/`, maintain consistency with existing variable declaration patterns across platforms rather than adding individual improvements like `readonly` declarations.
Learnt from: jackluo923
PR: y-scope/clp#1054
File: components/core/tools/scripts/lib_install/musllinux_1_2/install-packages-from-source.sh:6-8
Timestamp: 2025-07-01T14:51:19.172Z
Learning: In CLP installation scripts within `components/core/tools/scripts/lib_install/`, maintain consistency with existing variable declaration patterns across platforms rather than adding individual improvements like `readonly` declarations.
Learnt from: jackluo923
PR: y-scope/clp#1054
File: components/core/tools/scripts/lib_install/musllinux_1_2/install-prebuilt-packages.sh:27-32
Timestamp: 2025-07-07T17:41:15.655Z
Learning: In CLP installation scripts, consistency across platform scripts is prioritized over defensive programming improvements. For example, when extracting Task binaries with tar in `install-prebuilt-packages.sh`, the extraction pattern should remain consistent with other platform scripts rather than adding defensive flags like `--strip-components=1` to handle potential tarball layout changes.
Learnt from: haiqi96
PR: y-scope/clp#594
File: components/clp-package-utils/clp_package_utils/scripts/del_archives.py:56-65
Timestamp: 2024-11-18T16:49:20.248Z
Learning: When reviewing wrapper scripts in `components/clp-package-utils/clp_package_utils/scripts/`, note that it's preferred to keep error handling simple without adding extra complexity.
Learnt from: junhaoliao
PR: y-scope/clp#1078
File: components/webui/server/src/plugins/MongoSocketIoServer/index.ts:105-108
Timestamp: 2025-07-08T17:20:02.560Z
Learning: The y-scope/clp codebase has a 100-character line length limit that takes precedence over other style preferences like template literals when they conflict.
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.
Learnt from: quinntaylormitchell
PR: y-scope/clp#968
File: docs/src/user-guide/quick-start/overview.md:73-109
Timestamp: 2025-06-18T20:39:05.899Z
Learning: The CLP project team prefers to use video content to demonstrate detailed procedural steps (like tarball extraction) rather than including every step in the written documentation, keeping the docs focused on conceptual guidance.
Learnt from: anlowee
PR: y-scope/clp#925
File: .github/workflows/clp-s-antlr-generation.yaml:24-27
Timestamp: 2025-05-27T20:04:51.498Z
Learning: The clp codebase uses commit SHAs instead of version tags for GitHub Actions (like actions/checkout) as an established pattern across workflow files.
tools/docker-images/clp-package-ubuntu-jammy/setup-scripts/install-prebuilt-packages.sh
Outdated
Show resolved
Hide resolved
| workflow_dispatch: | ||
|
|
||
| env: | ||
| DEPS_IMAGE_NAME_PREFIX: "clp-core-dependencies-x86-" |
There was a problem hiding this comment.
(Similar to the comment at https://github.com/y-scope/clp/pull/1166/files#r2256441706)
I see how it can be awkward to build the package with a "core" dependencies container, but i haven't seen any quicker way to build the package. if there's no significant benefit in differentiating between "core" and "package" dependencies, shall we just unify them (simply rename the existing build env containers).
There was a problem hiding this comment.
Yeah, I think we can rename the existing containers and make them sufficient for building the entire CLP package (since the requirements for building the package are not a lot more than what's required for building core). That said, let's defer this to another PR to avoid delaying this one.
There was a problem hiding this comment.
@coderabbitai create an issue to track the effort of creating a universal build env image
There was a problem hiding this comment.
🧩 Analysis chain
@junhaoliao I've created a GitHub issue to track the effort of creating a universal build environment image. The issue captures the context from your discussion with kirkrodrigues about unifying the "core" and "package" dependencies containers, includes references to this PR and the specific comment, and outlines the proposed solution with clear acceptance criteria.
🏁 Script executed:
#!/bin/bash
# Create GitHub issue to track universal build environment image effort
gh issue create \
--title "Unify core and package dependencies into universal build environment image" \
--body "## Context
Currently, we have separate \"core\" dependencies containers that are being used to build both core components and the full CLP package. This creates an awkward situation where package builds rely on \"core\" dependency containers.
## Problem
As discussed in PR #1166 (https://github.com/y-scope/clp/pull/1166#discussion_r2256457011), the current naming and separation between \"core\" and \"package\" dependencies doesn't provide significant benefits and creates confusion.
## Proposed Solution
Rename and unify the existing build environment containers to create a universal build environment image that can:
- Build CLP core components
- Build the complete CLP package
- Provide all necessary dependencies for the entire build process
## References
- Original discussion: https://github.com/y-scope/clp/pull/1166#discussion_r2256457011
- PR #1166: https://github.com/y-scope/clp/pull/1166
## Acceptance Criteria
- [ ] Rename existing core dependencies containers to reflect their universal nature
- [ ] Ensure the unified image contains all dependencies needed for both core and package builds
- [ ] Update all workflows and documentation to reference the new unified image name
- [ ] Verify that both core and package builds work correctly with the unified image
---
Requested by: @junhaoliao" \
--assignee junhaoliao
Length of output: 1491
@junhaoliao Perfect! I've successfully created issue #1266 to track the effort of creating a universal build environment image. The issue is now available at #1266 and has been assigned to you as requested.
The issue captures all the relevant context from your discussion with kirkrodrigues about unifying the "core" and "package" dependencies containers, includes proper references to this PR and the specific comment thread, and provides clear acceptance criteria for completing the work.
🧠 Learnings used
Learnt from: quinntaylormitchell
PR: y-scope/clp#918
File: .github/workflows/clp-execution-image-build.yaml:77-97
Timestamp: 2025-05-26T16:03:05.519Z
Learning: In .github/workflows/clp-execution-image-build.yaml, the ubuntu-jammy-execution-image and ubuntu-noble-execution-image jobs are intentionally kept separate (rather than using a matrix strategy) to make it easier to remove individual platform versions when they reach end of life, such as when jammy eventually becomes obsolete.
junhaoliao
changed the title
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
docs/src/dev-guide/building-package.md(1 hunks)docs/src/dev-guide/tooling-containers.md(1 hunks)docs/src/dev-guide/tooling-gh-workflows.md(1 hunks)
🧰 Additional context used
🧠 Learnings (12)
📓 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.
Learnt from: jackluo923
PR: y-scope/clp#1054
File: components/core/tools/docker-images/clp-env-base-musllinux_1_2-x86/build.sh:18-24
Timestamp: 2025-07-01T14:52:02.418Z
Learning: In the CLP project, consistency across platform build scripts is prioritized over defensive programming when it comes to git remote handling. All build.sh files in docker-images directories should follow the same pattern for git metadata injection.
Learnt from: jackluo923
PR: y-scope/clp#1054
File: components/core/tools/docker-images/clp-env-base-musllinux_1_2-x86/build.sh:18-24
Timestamp: 2025-07-01T14:52:02.418Z
Learning: In the CLP project, consistency across platform build scripts is prioritized over defensive programming when it comes to git remote handling. All build.sh files in docker-images directories should follow the same pattern for git metadata injection.
Learnt from: quinntaylormitchell
PR: y-scope/clp#918
File: .github/workflows/clp-execution-image-build.yaml:77-97
Timestamp: 2025-05-26T16:03:05.519Z
Learning: In .github/workflows/clp-execution-image-build.yaml, the ubuntu-jammy-execution-image and ubuntu-noble-execution-image jobs are intentionally kept separate (rather than using a matrix strategy) to make it easier to remove individual platform versions when they reach end of life, such as when jammy eventually becomes obsolete.
Learnt from: jackluo923
PR: y-scope/clp#1054
File: components/core/tools/docker-images/clp-env-base-musllinux_1_2-aarch64/build.sh:3-5
Timestamp: 2025-07-07T17:43:04.349Z
Learning: In CLP project build scripts (specifically build.sh files in docker-images directories), maintain consistency with the established pattern of using separate `set -eu` and `set -o pipefail` commands rather than combining them into `set -euo pipefail`, to ensure uniform script structure across all platform build scripts.
Learnt from: anlowee
PR: y-scope/clp#925
File: .github/workflows/clp-s-antlr-generation.yaml:24-27
Timestamp: 2025-05-27T20:04:51.498Z
Learning: The clp codebase uses commit SHAs instead of version tags for GitHub Actions (like actions/checkout) as an established pattern across workflow files.
Learnt from: jackluo923
PR: y-scope/clp#1054
File: components/core/tools/scripts/lib_install/musllinux_1_2/install-prebuilt-packages.sh:6-15
Timestamp: 2025-07-01T14:52:15.217Z
Learning: For installation scripts in the CLP project, maintain consistency in command patterns across platforms rather than applying platform-specific optimizations. When a platform follows a pattern of separate update and install commands (like `apt-get update && apt-get install` or `apk update && apk add`), preserve this pattern for uniform script structure.
Learnt from: jackluo923
PR: y-scope/clp#1054
File: components/core/tools/scripts/lib_install/musllinux_1_2/install-prebuilt-packages.sh:6-15
Timestamp: 2025-07-01T14:52:15.217Z
Learning: For installation scripts in the CLP project, maintain consistency in command patterns across platforms rather than applying platform-specific optimizations. When a platform follows a pattern of separate update and install commands (like `apt-get update && apt-get install` or `apk update && apk add`), preserve this pattern for uniform script structure.
Learnt from: jackluo923
PR: y-scope/clp#1054
File: components/core/tools/scripts/lib_install/musllinux_1_2/install-prebuilt-packages.sh:6-15
Timestamp: 2025-07-01T14:52:15.217Z
Learning: For installation scripts in the CLP project, maintain consistency in command patterns across platforms rather than applying platform-specific optimizations. When a platform follows a pattern of separate update and install commands (like `apt-get update && apt-get install` or `apk update && apk add`), preserve this pattern for uniform script structure.
Learnt from: jackluo923
PR: y-scope/clp#1054
File: components/core/tools/scripts/lib_install/musllinux_1_2/install-prebuilt-packages.sh:6-15
Timestamp: 2025-07-01T14:52:15.217Z
Learning: For installation scripts in the CLP project, maintain consistency in command patterns across different platforms (e.g., using separate update and install commands like `apk update && apk add`, `apt update && apt install`, `yum update && yum install`) rather than platform-specific optimizations, to ensure uniform script structure and readability.
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.408Z
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.
📚 Learning: 2025-06-18T20:39:05.899Z
Learnt from: quinntaylormitchell
PR: y-scope/clp#968
File: docs/src/user-guide/quick-start/overview.md:73-109
Timestamp: 2025-06-18T20:39:05.899Z
Learning: The CLP project team prefers to use video content to demonstrate detailed procedural steps (like tarball extraction) rather than including every step in the written documentation, keeping the docs focused on conceptual guidance.
Applied to files:
docs/src/dev-guide/building-package.mddocs/src/dev-guide/tooling-containers.mddocs/src/dev-guide/tooling-gh-workflows.md
📚 Learning: 2025-07-01T14:52:02.418Z
Learnt from: jackluo923
PR: y-scope/clp#1054
File: components/core/tools/docker-images/clp-env-base-musllinux_1_2-x86/build.sh:18-24
Timestamp: 2025-07-01T14:52:02.418Z
Learning: In the CLP project, consistency across platform build scripts is prioritized over defensive programming when it comes to git remote handling. All build.sh files in docker-images directories should follow the same pattern for git metadata injection.
Applied to files:
docs/src/dev-guide/building-package.mddocs/src/dev-guide/tooling-containers.mddocs/src/dev-guide/tooling-gh-workflows.md
📚 Learning: 2025-05-26T16:03:05.519Z
Learnt from: quinntaylormitchell
PR: y-scope/clp#918
File: .github/workflows/clp-execution-image-build.yaml:77-97
Timestamp: 2025-05-26T16:03:05.519Z
Learning: In .github/workflows/clp-execution-image-build.yaml, the ubuntu-jammy-execution-image and ubuntu-noble-execution-image jobs are intentionally kept separate (rather than using a matrix strategy) to make it easier to remove individual platform versions when they reach end of life, such as when jammy eventually becomes obsolete.
Applied to files:
docs/src/dev-guide/building-package.mddocs/src/dev-guide/tooling-containers.mddocs/src/dev-guide/tooling-gh-workflows.md
📚 Learning: 2025-07-05T03:38:16.779Z
Learnt from: quinntaylormitchell
PR: y-scope/clp#1069
File: docs/src/user-guide/quick-start/clp-json.md:135-152
Timestamp: 2025-07-05T03:38:16.779Z
Learning: The CLP project team has decided to refrain from using include directives in their documentation at present, preferring to maintain duplicated content rather than using shared includes or partials for de-duplication.
Applied to files:
docs/src/dev-guide/building-package.mddocs/src/dev-guide/tooling-containers.mddocs/src/dev-guide/tooling-gh-workflows.md
📚 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:
docs/src/dev-guide/building-package.mddocs/src/dev-guide/tooling-containers.mddocs/src/dev-guide/tooling-gh-workflows.md
📚 Learning: 2025-07-07T17:43:04.349Z
Learnt from: jackluo923
PR: y-scope/clp#1054
File: components/core/tools/docker-images/clp-env-base-musllinux_1_2-aarch64/build.sh:3-5
Timestamp: 2025-07-07T17:43:04.349Z
Learning: In CLP project build scripts (specifically build.sh files in docker-images directories), maintain consistency with the established pattern of using separate `set -eu` and `set -o pipefail` commands rather than combining them into `set -euo pipefail`, to ensure uniform script structure across all platform build scripts.
Applied to files:
docs/src/dev-guide/building-package.md
📚 Learning: 2025-07-07T17:41:15.655Z
Learnt from: jackluo923
PR: y-scope/clp#1054
File: components/core/tools/scripts/lib_install/musllinux_1_2/install-prebuilt-packages.sh:27-32
Timestamp: 2025-07-07T17:41:15.655Z
Learning: In CLP installation scripts, consistency across platform scripts is prioritized over defensive programming improvements. For example, when extracting Task binaries with tar in `install-prebuilt-packages.sh`, the extraction pattern should remain consistent with other platform scripts rather than adding defensive flags like `--strip-components=1` to handle potential tarball layout changes.
Applied to files:
docs/src/dev-guide/building-package.md
📚 Learning: 2025-07-01T14:52:15.217Z
Learnt from: jackluo923
PR: y-scope/clp#1054
File: components/core/tools/scripts/lib_install/musllinux_1_2/install-prebuilt-packages.sh:6-15
Timestamp: 2025-07-01T14:52:15.217Z
Learning: For installation scripts in the CLP project, maintain consistency in command patterns across platforms rather than applying platform-specific optimizations. When a platform follows a pattern of separate update and install commands (like `apt-get update && apt-get install` or `apk update && apk add`), preserve this pattern for uniform script structure.
Applied to files:
docs/src/dev-guide/building-package.md
📚 Learning: 2025-02-12T22:24:17.723Z
Learnt from: jackluo923
PR: y-scope/clp#718
File: components/core/tools/scripts/utils/create-debian-package.py:41-41
Timestamp: 2025-02-12T22:24:17.723Z
Learning: For the clp-core Debian package creation script, strict version format validation is considered unnecessary complexity and should be avoided.
Applied to files:
docs/src/dev-guide/tooling-containers.md
📚 Learning: 2025-05-27T20:04:51.498Z
Learnt from: anlowee
PR: y-scope/clp#925
File: .github/workflows/clp-s-antlr-generation.yaml:24-27
Timestamp: 2025-05-27T20:04:51.498Z
Learning: The clp codebase uses commit SHAs instead of version tags for GitHub Actions (like actions/checkout) as an established pattern across workflow files.
Applied to files:
docs/src/dev-guide/tooling-gh-workflows.md
📚 Learning: 2025-05-06T09:48:55.408Z
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.408Z
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.
Applied to files:
docs/src/dev-guide/tooling-gh-workflows.md
⏰ 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: ubuntu-jammy-deps-image
- GitHub Check: centos-stream-9-deps-image
- GitHub Check: lint-check (ubuntu-24.04)
- GitHub Check: lint-check (macos-15)
- GitHub Check: build-macos (macos-15, false)
- GitHub Check: build-macos (macos-15, true)
| ## Building a Docker image | ||
|
|
||
| To build a Docker image containing the CLP package, run: | ||
|
|
||
| ```shell | ||
| task docker-image-package | ||
| ``` | ||
|
|
||
| This will create a Docker image named `clp-package-<os>-ubuntu-jammy:dev` where `<os>` is `x86` or | ||
| `arm64`. | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Clarify local tag vs published tag and note arm64 build constraints.
- Make it explicit that
:devis a local tag; published images use:main. - Mention that building
arm64requires an arm64 host or Docker Buildx with QEMU.
Proposed tweak:
## Building a Docker image
To build a Docker image containing the CLP package, run:
```shell
task docker-image-package-This will create a Docker image named clp-package-<os>-ubuntu-jammy:dev where <os> is x86 or
-arm64.
+This will create a local Docker image named clp-package-<os>-ubuntu-jammy:dev where <os> is x86 or
+arm64. Published CI images use the :main tag (e.g., ghcr.io/y-scope/clp/clp-package-x86-ubuntu-jammy:main).
+
+Note: Building an arm64 image requires either running on an arm64 host or using Docker Buildx with QEMU emulation.
<details>
<summary>🤖 Prompt for AI Agents</summary>
In docs/src/dev-guide/building-package.md around lines 72 to 82, clarify that
the Docker image tag :dev is a local tag and that published images use the
:main tag with an example. Also, add a note that building the arm64 image
requires either an arm64 host or Docker Buildx with QEMU emulation. Update the
text to explicitly state these points for better user understanding.
</details>
<!-- fingerprinting:phantom:triton:capybara -->
<!-- This is an auto-generated comment by CodeRabbit -->
| ## clp-package-x86-ubuntu-jammy | ||
|
|
||
| An image containing the CLP package built in an Ubuntu Jammy x86_64 environment. | ||
|
|
||
| * Path: | ||
|
|
||
| ```text | ||
| tools/docker-images/clp-package-ubuntu-jammy | ||
| ``` | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Align architecture wording and add GH Packages details to match other entries.
- Use “x86” (not “x86_64”) to be consistent with headings above.
- Add a GitHub Packages link and a pull command, like other images in this page.
Apply:
## clp-package-x86-ubuntu-jammy
-An image containing the CLP package built in an Ubuntu Jammy x86_64 environment.
+An image containing the CLP package built in an Ubuntu Jammy x86 environment.
-* Path:
+* [GitHub Packages page][pkg-ubuntu-jammy]
+* Pull command:
+
+ ```bash
+ docker pull ghcr.io/y-scope/clp/clp-package-x86-ubuntu-jammy:main
+ ```
+
+* Path:
```text
tools/docker-images/clp-package-ubuntu-jammy
Add this link reference at the bottom alongside the others:
```text
[pkg-ubuntu-jammy]: https://github.com/y-scope/clp/pkgs/container/clp%2Fclp-package-x86-ubuntu-jammy
If the package page isn’t public yet, we can temporarily omit the link and add it once the workflow publishes the image.
Let’s verify the existing entries and link references in docs/src/dev-guide/tooling-containers.md to match the pattern used elsewhere.
🏁 Script executed:
#!/bin/bash
set -euxo pipefail
# Show all clp-package sections for architecture naming and GH Packages link usage
rg "^## clp-package" -n docs/src/dev-guide/tooling-containers.md -A5
# Show all existing [pkg-…] link references
rg "^\[pkg-" -n docs/src/dev-guide/tooling-containers.mdLength of output: 298
Let’s inspect how neighbouring image entries are structured—particularly their headings, pull commands, and GitHub Packages links.
🏁 Script executed:
#!/bin/bash
set -euxo pipefail
# List all second-level sections to locate other image entries
rg "^## " -n docs/src/dev-guide/tooling-containers.md
# Show existing pull commands for comparison
rg -n -C3 "Pull command" docs/src/dev-guide/tooling-containers.md
# Show existing GitHub Packages link references
rg -n -C3 "GitHub Packages" docs/src/dev-guide/tooling-containers.mdLength of output: 2429
Standardize “x86” phrasing and add GitHub Packages details for the CLP package image
File: docs/src/dev-guide/tooling-containers.md, lines 136–145
- Change the description to use “x86” (not “x86_64”).
- Add a GitHub Packages page link and
docker pullcommand, consistent with other entries. - Add the
[pkg-ubuntu-jammy]link reference at the bottom.
Apply:
## clp-package-x86-ubuntu-jammy
-An image containing the CLP package built in an Ubuntu Jammy x86_64 environment.
+An image containing the CLP package built in an Ubuntu Jammy x86 environment.
-* Path:
+* [GitHub Packages page][pkg-ubuntu-jammy]
+* Pull command:
+
+ ```bash
+ docker pull ghcr.io/y-scope/clp/clp-package-x86-ubuntu-jammy:main
+ ```
+
+* Path:
```text
tools/docker-images/clp-package-ubuntu-jammy
Add at the end of the file:
```text
[pkg-ubuntu-jammy]: https://github.com/y-scope/clp/pkgs/container/clp%2Fclp-package-x86-ubuntu-jammy
🤖 Prompt for AI Agents
In docs/src/dev-guide/tooling-containers.md around lines 136 to 145, update the
description to use "x86" instead of "x86_64" for consistency. Add a GitHub
Packages link and a docker pull command for the CLP package image, matching the
format of other entries. Also, add the link reference [pkg-ubuntu-jammy] at the
bottom of the file pointing to the appropriate GitHub Packages URL.
| ## clp-package-image-build | ||
|
|
||
| This workflow builds a container image that contains the CLP package. | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Add a direct link to the workflow file for discoverability.
Linking to .github/workflows/clp-package-image-build.yaml here mirrors how other sections point readers to concrete artefacts.
You could append: “See .github/workflows/clp-package-image-build.yaml for details.”
🤖 Prompt for AI Agents
In docs/src/dev-guide/tooling-gh-workflows.md around lines 79 to 82, add a
direct link to the workflow file .github/workflows/clp-package-image-build.yaml
at the end of the section describing the clp-package-image-build workflow.
Append a sentence like "See .github/workflows/clp-package-image-build.yaml for
details." to improve discoverability and maintain consistency with other
sections.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
taskfile.yaml(1 hunks)
⏰ 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: centos-stream-9-deps-image
- GitHub Check: ubuntu-jammy-deps-image
- GitHub Check: lint-check (ubuntu-24.04)
- GitHub Check: lint-check (macos-15)
- GitHub Check: build-macos (macos-15, false)
- GitHub Check: build-macos (macos-15, true)
taskfile.yaml
Outdated
| docker-image-package: | ||
| vars: | ||
| SRC_DIR: "{{.ROOT_DIR}}/tools/docker-images/clp-package-ubuntu-jammy" | ||
| dir: "{{.SRC_DIR}}" | ||
| deps: | ||
| - "package" | ||
| sources: | ||
| - "{{.SRC_DIR}}/**/*" | ||
| cmds: | ||
| - "./build.sh" | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Make docker-image-package sensitive to package output changes and check Docker availability
Current sources only watch the Docker image directory. Since the build script uses the repo root as the Docker build context and the image embeds the freshly built package (deps: package), the task can be incorrectly considered up-to-date when only package outputs change. Also, adding a precondition to ensure Docker is installed will fail fast with a clear message.
Proposed fixes:
- Include the built package directory in sources so image rebuilds when package contents change.
- Add a Docker availability precondition.
Apply this diff:
docker-image-package:
vars:
SRC_DIR: "{{.ROOT_DIR}}/tools/docker-images/clp-package-ubuntu-jammy"
dir: "{{.SRC_DIR}}"
deps:
- "package"
sources:
- "{{.SRC_DIR}}/**/*"
+ - "{{.G_PACKAGE_BUILD_DIR}}/**/*"
+ preconditions:
+ - sh: "command -v docker"
+ msg: "Docker is required to build the package image. Please install Docker and try again."
cmds:
- "./build.sh"Follow-up:
- If the Dockerfile COPYs additional paths from the repo root (e.g., other tools/setup directories), consider adding those globs to sources as well to avoid stale images (or emit a checksum file in the build script and wire it via generates for stronger up-to-date checks). Would you like me to draft that too?
🏁 Script executed:
#!/bin/bash
set -e
echo "1. Searching for G_PACKAGE_BUILD_DIR in YAML files..."
rg -n 'G_PACKAGE_BUILD_DIR' --glob '*.yaml' --glob '*.yml' || true
echo "2. Extracting 'package' task from Taskfile.yaml..."
rg -n '^[ \\t]*package:' -n -C10 taskfile.yaml || true
echo "3. Checking for existing 'preconditions' usage..."
rg -n 'preconditions:' -n -C3 taskfile.yaml || true
echo "4. Showing COPY instructions in Dockerfile..."
grep -n -C2 'COPY' tools/docker-images/clp-package-ubuntu-jammy/Dockerfile || trueLength of output: 2018
Ensure docker-image-package rebuilds on package changes and verifies Docker availability
To prevent stale images when only the package output changes and to fail fast if Docker isn’t installed, update the docker-image-package task in taskfile.yaml as follows:
• Add the built package directory to sources so that any changes under {{.G_PACKAGE_BUILD_DIR}} trigger a rebuild.
• Introduce a preconditions check for Docker on the PATH with a clear error message.
Apply this diff:
docker-image-package:
vars:
SRC_DIR: "{{.ROOT_DIR}}/tools/docker-images/clp-package-ubuntu-jammy"
dir: "{{.SRC_DIR}}"
deps:
- "package"
sources:
- "{{.SRC_DIR}}/**/*"
+ - "{{.G_PACKAGE_BUILD_DIR}}/**/*"
+ preconditions:
+ - sh: "command -v docker"
+ msg: "Docker is required to build the package image. Please install Docker and try again."
cmds:
- "./build.sh"📝 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.
| docker-image-package: | |
| vars: | |
| SRC_DIR: "{{.ROOT_DIR}}/tools/docker-images/clp-package-ubuntu-jammy" | |
| dir: "{{.SRC_DIR}}" | |
| deps: | |
| - "package" | |
| sources: | |
| - "{{.SRC_DIR}}/**/*" | |
| cmds: | |
| - "./build.sh" | |
| docker-image-package: | |
| vars: | |
| SRC_DIR: "{{.ROOT_DIR}}/tools/docker-images/clp-package-ubuntu-jammy" | |
| dir: "{{.SRC_DIR}}" | |
| deps: | |
| - "package" | |
| sources: | |
| - "{{.SRC_DIR}}/**/*" | |
| - "{{.G_PACKAGE_BUILD_DIR}}/**/*" | |
| preconditions: | |
| - sh: "command -v docker" | |
| msg: "Docker is required to build the package image. Please install Docker and try again." | |
| cmds: | |
| - "./build.sh" |
🤖 Prompt for AI Agents
In taskfile.yaml around lines 165 to 175, the docker-image-package task must be
updated so image rebuilds when package output changes and to fail early if
Docker is missing: add the built package dir (e.g. include
"{{.G_PACKAGE_BUILD_DIR}}/**" or the appropriate path under that variable) to
the task's sources list so changes there trigger rebuilds, and add a
preconditions entry that verifies Docker is on PATH (run a shell check for the
docker binary and emit a clear error message and non-zero exit if not found)
before running cmds.
1fa03a5 to
9db7ca9
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (5)
.github/actions/run-on-image/action.yaml (1)
45-54: Add --rm to docker run to prevent exited container buildupThe cache comment is good. Also add --rm so CI doesn’t accumulate exited containers.
- run: >- docker run --user $(id -u):$(id -g) + --rm --env npm_config_cache=/tmp/.npm --volume "$GITHUB_WORKSPACE":/mnt/repo --workdir /mnt/repo ${{steps.get_image_props.outputs.qualified_image_name}} bash -c "${{inputs.run_command}}".github/actions/clp-build-runtime-image/action.yaml (3)
5-7: Document allowed values for image_typeState the enum in-place for callers.
image_type: - description: "Type of image to build" + description: "Type of image to build. One of: execution, package." required: true
50-66: Validate image_type, require platform for execution, and fail if Dockerfile missingAs written, any unexpected image_type silently selects the package Dockerfile. Add validation, normalise case, and check file existence.
- - name: "Compute metadata" - id: "compute-meta" - shell: "bash" - run: | - base_path="./tools/docker-images" - - if [[ "${{inputs.image_type}}" == "execution" ]]; then - platform="${{inputs.platform_id}}-${{inputs.platform_version_id}}" - dockerfile_path="$base_path/clp-execution-base-$platform/Dockerfile" - image_name="clp-${{inputs.image_type}}-$platform" - else - dockerfile_path="$base_path/clp-package/Dockerfile" - image_name="clp-${{inputs.image_type}}" - fi - - echo "DOCKERFILE=$dockerfile_path" >> "$GITHUB_OUTPUT" - echo "IMAGE_NAME=$image_name" >> "$GITHUB_OUTPUT" + - name: "Compute metadata" + id: "compute-meta" + shell: "bash" + run: | + set -euo pipefail + base_path="./tools/docker-images" + type="${{ inputs.image_type }}" + type="${type,,}" # normalise case + + case "$type" in + execution) + if [[ -z "${{ inputs.platform_id }}" || -z "${{ inputs.platform_version_id }}" ]]; then + echo "platform_id and platform_version_id are required when image_type=execution." >&2 + exit 1 + fi + platform="${{ inputs.platform_id }}-${{ inputs.platform_version_id }}" + dockerfile_path="$base_path/clp-execution-base-$platform/Dockerfile" + image_name="clp-${type}-$platform" + ;; + package) + dockerfile_path="$base_path/clp-package/Dockerfile" + image_name="clp-${type}" + ;; + *) + echo "Invalid image_type: '$type'. Expected one of: execution, package." >&2 + exit 1 + ;; + esac + + if [[ ! -f "$dockerfile_path" ]]; then + echo "Resolved Dockerfile does not exist: $dockerfile_path" >&2 + exit 1 + fi + + echo "DOCKERFILE=$dockerfile_path" >> "$GITHUB_OUTPUT" + echo "IMAGE_NAME=$image_name" >> "$GITHUB_OUTPUT"
1-2: Clarify action description to reflect both image typesMake it explicit this builds execution and package runtime images.
-name: "clp-build-runtime-image" -description: "Builds a container image to be used for running CLP." +name: "clp-build-runtime-image" +description: "Builds CLP runtime container images (execution or package).".github/workflows/clp-core-build.yaml (1)
526-529: Normalise step name: “Work around …” (consistency with other jobs)Align wording for grepability.
- - name: "Workaround actions/runner-images/issues/6775" + - name: "Work around actions/runner-images/issues/6775"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
.github/actions/clp-build-runtime-image/action.yaml(3 hunks).github/actions/run-on-image/action.yaml(1 hunks).github/workflows/clp-core-build.yaml(2 hunks)docs/src/dev-docs/building-package.md(1 hunks)docs/src/dev-docs/tooling-containers.md(1 hunks)docs/src/dev-docs/tooling-gh-workflows.md(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-05-26T16:03:05.519Z
Learnt from: quinntaylormitchell
PR: y-scope/clp#918
File: .github/workflows/clp-execution-image-build.yaml:77-97
Timestamp: 2025-05-26T16:03:05.519Z
Learning: In .github/workflows/clp-execution-image-build.yaml, the ubuntu-jammy-execution-image and ubuntu-noble-execution-image jobs are intentionally kept separate (rather than using a matrix strategy) to make it easier to remove individual platform versions when they reach end of life, such as when jammy eventually becomes obsolete.
Applied to files:
docs/src/dev-docs/tooling-containers.md.github/workflows/clp-core-build.yaml.github/actions/clp-build-runtime-image/action.yaml
📚 Learning: 2025-07-01T14:52:02.418Z
Learnt from: jackluo923
PR: y-scope/clp#1054
File: components/core/tools/docker-images/clp-env-base-musllinux_1_2-x86/build.sh:18-24
Timestamp: 2025-07-01T14:52:02.418Z
Learning: In the CLP project, consistency across platform build scripts is prioritized over defensive programming when it comes to git remote handling. All build.sh files in docker-images directories should follow the same pattern for git metadata injection.
Applied to files:
.github/actions/clp-build-runtime-image/action.yaml
📚 Learning: 2025-02-12T22:24:17.723Z
Learnt from: jackluo923
PR: y-scope/clp#718
File: components/core/tools/scripts/utils/create-debian-package.py:41-41
Timestamp: 2025-02-12T22:24:17.723Z
Learning: For the clp-core Debian package creation script, strict version format validation is considered unnecessary complexity and should be avoided.
Applied to files:
.github/actions/clp-build-runtime-image/action.yaml
📚 Learning: 2025-07-07T17:43:04.349Z
Learnt from: jackluo923
PR: y-scope/clp#1054
File: components/core/tools/docker-images/clp-env-base-musllinux_1_2-aarch64/build.sh:3-5
Timestamp: 2025-07-07T17:43:04.349Z
Learning: In CLP project build scripts (specifically build.sh files in docker-images directories), maintain consistency with the established pattern of using separate `set -eu` and `set -o pipefail` commands rather than combining them into `set -euo pipefail`, to ensure uniform script structure across all platform build scripts.
Applied to files:
.github/actions/clp-build-runtime-image/action.yaml
🪛 actionlint (1.7.7)
.github/workflows/clp-core-build.yaml
11-11: "paths-ignore" section must be sequence node but got alias node with "" tag
(syntax-check)
⏰ 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). (11)
- GitHub Check: manylinux_2_28-x86_64-static-linked-bins
- GitHub Check: manylinux_2_28-x86_64-dynamic-linked-bins
- GitHub Check: ubuntu-jammy-static-linked-bins
- GitHub Check: package-image
- GitHub Check: ubuntu-jammy-dynamic-linked-bins
- GitHub Check: ubuntu-jammy-lint
- GitHub Check: centos-stream-9-static-linked-bins
- GitHub Check: centos-stream-9-dynamic-linked-bins
- GitHub Check: musllinux_1_2-x86_64-static-linked-bins
- GitHub Check: musllinux_1_2-x86_64-dynamic-linked-bins
- GitHub Check: build-macos (macos-15, false)
🔇 Additional comments (4)
.github/actions/clp-build-runtime-image/action.yaml (3)
20-26: Platform inputs are optional here—ensure callers provide them for execution buildsSince platform_id/version_id are not required, verify all execution builds pass them (until runtime validation is added).
Would you like me to generate a repo-wide check to list all uses of this action missing platform_id/platform_version_id when image_type=execution?
68-74: LGTM: metadata usage is correctUsing IMAGE_NAME from compute-meta in metadata-action is correct.
75-83: LGTM: build uses resolved Dockerfile and tags/labels outputsPush gating is correct for main-only publishing.
docs/src/dev-docs/building-package.md (1)
72-81: Verify Taskfile target and Docker image tag
Docs reference a Taskfile target and image tag that aren’t present in the code:
- Taskfile target
docker-images:packageisn’t defined.- No Docker build step tags an image as
clp-package:dev.
Ensure the Taskfile includes this target and the build script produces an image taggedclp-package:dev.
| push: | ||
| paths: | ||
| - ".github/actions/clp-core-build-containers/action.yaml" | ||
| - ".github/actions/run-on-image/action.yaml" | ||
| - ".github/workflows/clp-core-build.yaml" | ||
| - ".gitmodules" | ||
| - "components/core/**" | ||
| - "taskfile.yaml" | ||
| - "taskfiles/**" | ||
| - "tools/scripts/deps-download/**" | ||
| - "!components/core/tools/scripts/lib_install/macos/**" | ||
| paths-ignore: *ignored_paths | ||
| schedule: |
There was a problem hiding this comment.
Fix actionlint error: alias in paths-ignore is invalid
actionlint flags the alias on Line 11. Inline the list for push.
push:
- paths-ignore: *ignored_paths
+ paths-ignore:
+ - ".github/*"
+ - ".github/ISSUE_TEMPLATE/**"
+ - "components/core/tools/scripts/lib_install/macos/**"
+ - "docs/**"📝 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.
| push: | |
| paths: | |
| - ".github/actions/clp-core-build-containers/action.yaml" | |
| - ".github/actions/run-on-image/action.yaml" | |
| - ".github/workflows/clp-core-build.yaml" | |
| - ".gitmodules" | |
| - "components/core/**" | |
| - "taskfile.yaml" | |
| - "taskfiles/**" | |
| - "tools/scripts/deps-download/**" | |
| - "!components/core/tools/scripts/lib_install/macos/**" | |
| paths-ignore: *ignored_paths | |
| schedule: | |
| push: | |
| paths-ignore: | |
| - ".github/*" | |
| - ".github/ISSUE_TEMPLATE/**" | |
| - "components/core/tools/scripts/lib_install/macos/**" | |
| - "docs/**" | |
| schedule: |
🧰 Tools
🪛 actionlint (1.7.7)
11-11: "paths-ignore" section must be sequence node but got alias node with "" tag
(syntax-check)
🤖 Prompt for AI Agents
.github/workflows/clp-core-build.yaml around lines 10 to 12: actionlint flags
the YAML anchor/alias used for paths-ignore on line 11 as invalid; replace the
alias with the actual inline list of ignored paths (expand the referenced
*ignored_paths sequence directly into the push.paths-ignore block) so the push
job contains a concrete array of path globs instead of an alias.
| ## Building a Docker image | ||
|
|
||
| To build a Docker image containing the CLP package, run: | ||
|
|
||
| ```shell | ||
| task docker-images:package | ||
| ``` | ||
|
|
||
| This will create a Docker image named `clp-package:dev`. | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
🛠️ Refactor suggestion
Clarify prerequisites, add a quick verification step, and document retag/push for Compose.
Users need to know which container engine is required, how to sanity-check the image, and how to retag/push it for Compose scenarios. Add the following edits:
## Building a Docker image
-To build a Docker image containing the CLP package, run:
+Prerequisites: Docker installed and running (or Podman configured with Docker compatibility).
+
+To build a Docker image containing the CLP package, run:
```shell
task docker-images:package-This will create a Docker image named clp-package:dev.
+This will create a locally tagged image named clp-package:dev.
+
+Quick sanity check:
+
+shell +docker run --rm clp-package:dev env | egrep '^(CLP_HOME|PYTHONPATH|PATH)=' +
+
+Optional: retag and push to GHCR (for Docker Compose use):
+
+shell +docker tag clp-package:dev ghcr.io/y-scope/clp/clp-package:dev +docker push ghcr.io/y-scope/clp/clp-package:dev +
+
+See also: the tooling container and GitHub workflow docs for image build/publish details.
<details>
<summary>🤖 Prompt for AI Agents</summary>
docs/src/dev-docs/building-package.md around lines 72 to 81: update the
"Building a Docker image" section to clarify prerequisites and verification
steps: change the sentence to say this will create a locally tagged image named
clp-package:dev, add a quick sanity-check command that runs the image and greps
for CLP_HOME, PYTHONPATH and PATH environment variables, and add optional
instructions to retag the image for GHCR and push it (for Docker Compose usage);
also add a brief pointer to the tooling container and GitHub workflow docs for
build/publish details.
</details>
<!-- fingerprinting:phantom:triton:chinchilla -->
<!-- This is an auto-generated comment by CodeRabbit -->
| ## clp-package | ||
|
|
||
| An image containing the CLP package. | ||
|
|
||
| * Path: | ||
|
|
||
| ```text | ||
| tools/docker-images/clp-package | ||
| ``` |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Match other sections: add GH Packages link and pull command
Keep the clp-package section consistent with the rest.
## clp-package
An image containing the CLP package.
+* [GitHub Packages page][clp-package]
+* Pull command:
+
+ ```bash
+ docker pull ghcr.io/y-scope/clp/clp-package:main
+ ```
+
* Path:
```text
tools/docker-images/clp-package+[clp-package]: https://github.com/y-scope/clp/pkgs/container/clp%2Fclp-package
<details>
<summary>🤖 Prompt for AI Agents</summary>
In docs/src/dev-docs/tooling-containers.md around lines 150 to 158, the
clp-package section is missing the GitHub Packages pull command and the
reference link used by other image sections; update the section to include a
code block with the docker pull command for ghcr.io/y-scope/clp/clp-package:main
and add the markdown reference link [clp-package]:
https://github.com/y-scope/clp/pkgs/container/clp%2Fclp-package at the end of
the section so it matches the format of the other image entries.
</details>
<!-- fingerprinting:phantom:triton:chinchilla -->
<!-- This is an auto-generated comment by CodeRabbit -->
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 (4)
components/core/tools/scripts/lib_install/ubuntu-jammy/install-prebuilt-packages.sh (4)
9-11: Slim the layer: clean apt cache and lists.Add cleanup after package installation to reduce image size.
apt-get update DEBIAN_FRONTEND=noninteractive apt-get install --no-install-recommends -y \ @@ software-properties-common \ unzip +apt-get clean +rm -rf /var/lib/apt/lists/*Also applies to: 31-31, 45-45
35-44: Robust temp-file cleanup on failure.If dpkg fails, the temp .deb remains. Add a trap to ensure cleanup.
task_pkg_arch=$(dpkg --print-architecture) -task_pkg_path="$(mktemp -t --suffix ".deb" task-pkg.XXXXXXXXXX)" +task_pkg_path="$(mktemp -t --suffix ".deb" task-pkg.XXXXXXXXXX)" +cleanup() { rm -f "$task_pkg_path"; } +trap cleanup EXIT curl \ @@ dpkg --install "$task_pkg_path" -rm "$task_pkg_path" +rm -f "$task_pkg_path"
37-44: Verify download integrity for Task .deb.Recommend validating the downloaded artefact against the release checksums.
curl \ --fail \ --location \ --output "$task_pkg_path" \ --show-error \ "https://github.com/go-task/task/releases/download/v3.44.0/task_linux_${task_pkg_arch}.deb" +# Verify checksum +curl --fail --location --show-error \ + "https://github.com/go-task/task/releases/download/v3.44.0/checksums.txt" \ + | grep " task_linux_${task_pkg_arch}\.deb$" \ + | sha256sum --check --status - dpkg --install "$task_pkg_path"
10-31: Removepython3-devandrsyncfrom install-prebuilt-packages.sh
Neither dependency is referenced by any consumer of this script—dropping them will slim the resulting image.
♻️ Duplicate comments (2)
docs/src/dev-docs/building-package.md (1)
71-79: Clarify prerequisites, add a quick verification step, and document retag/push for Compose.Enhance the section so users know what they need, how to sanity-check the image, and how to retag/push for Docker Compose.
## Building a Docker image -To build a Docker image containing the CLP package, run: +Prerequisites: Docker installed and running (or Podman configured with Docker compatibility). + +To build a Docker image containing the CLP package, run: ```shell task docker-images:package-This will create a Docker image named
clp-package:dev.
+This will create a locally tagged image namedclp-package:dev.
+
+Quick sanity check:
+
+shell +docker run --rm clp-package:dev env | egrep '^(CLP_HOME|PYTHONPATH|PATH)=' +
+
+Optional: retag and push to GHCR (for Docker Compose use):
+
+shell +docker tag clp-package:dev ghcr.io/y-scope/clp/clp-package:dev +docker push ghcr.io/y-scope/clp/clp-package:dev +
+
+See also: the tooling container and GitHub workflow docs for image build/publish details.</blockquote></details> <details> <summary>components/core/tools/scripts/lib_install/ubuntu-jammy/install-prebuilt-packages.sh (1)</summary><blockquote> `10-31`: **Avoid drift and bloat: keep Jammy core prebuilt deps minimal (python3-dev, rsync).** These two additions increase image size and diverge from the execution-base Jammy image that recently removed rsync and dev headers. If they’re package-image–specific, install them in that image or in a build-only stage, not in the shared core script. Proposed diff to keep core lean: ```diff DEBIAN_FRONTEND=noninteractive apt-get install --no-install-recommends -y \ @@ python3 \ - python3-dev \ python3-pip \ python3-venv \ - rsync \ software-properties-common \ unzipIf they are required here, please document the rationale in a brief comment above the apt list.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
components/core/tools/scripts/lib_install/ubuntu-jammy/install-prebuilt-packages.sh(1 hunks)docs/src/dev-docs/building-package.md(1 hunks)taskfile.yaml(3 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: quinntaylormitchell
PR: y-scope/clp#918
File: .github/workflows/clp-execution-image-build.yaml:77-97
Timestamp: 2025-05-26T16:03:05.519Z
Learning: In .github/workflows/clp-execution-image-build.yaml, the ubuntu-jammy-execution-image and ubuntu-noble-execution-image jobs are intentionally kept separate (rather than using a matrix strategy) to make it easier to remove individual platform versions when they reach end of life, such as when jammy eventually becomes obsolete.
📚 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:
components/core/tools/scripts/lib_install/ubuntu-jammy/install-prebuilt-packages.sh
📚 Learning: 2025-07-01T14:52:02.418Z
Learnt from: jackluo923
PR: y-scope/clp#1054
File: components/core/tools/docker-images/clp-env-base-musllinux_1_2-x86/build.sh:18-24
Timestamp: 2025-07-01T14:52:02.418Z
Learning: In the CLP project, consistency across platform build scripts is prioritized over defensive programming when it comes to git remote handling. All build.sh files in docker-images directories should follow the same pattern for git metadata injection.
Applied to files:
docs/src/dev-docs/building-package.md
📚 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:
taskfile.yaml
🔇 Additional comments (2)
taskfile.yaml (2)
8-8: Good move: delegate image tasks to a dedicated include.Including taskfiles/docker-images.yaml keeps the root Taskfile lean and matches the “docker-images:package” UX in docs.
97-97: LGTM: package task now emits a stable checksum path for downstream watchers.This aligns the docs (docker-images:package) and the included docker-images task to rebuild when the package output changes.
kirkrodrigues
left a comment
kirkrodrigues
left a comment
There was a problem hiding this comment.
Last comments (hopefully).
| ## clp-package-image-build | ||
|
|
||
| This workflow builds a container image that contains the CLP package. |
There was a problem hiding this comment.
We need to update this file to reflect the latest changes, right?
There was a problem hiding this comment.
Since this is no longer just to build core, how about renaming it to something like clp-artifact-build.yaml?
There was a problem hiding this comment.
i intentionally left the clp-core-macos workflow unchanged since it still only builds the core binaries. let me know if that should be updated as well
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)
95-116: Make package rebuild when Node.js toolchain changes.package copies node-22 into the image but doesn’t watch its checksum, risking stale binaries.
package: @@ sources: - "{{.G_BUILD_DIR}}/package-venv.md5" - "{{.G_BUILD_DIR}}/webui.md5" + - "{{.G_BUILD_DIR}}/nodejs-22.md5" - "{{.G_CORE_COMPONENT_BUILD_DIR}}/clg" - "{{.G_CORE_COMPONENT_BUILD_DIR}}/clo" - "{{.G_CORE_COMPONENT_BUILD_DIR}}/clp"
♻️ Duplicate comments (1)
taskfile.yaml (1)
8-8: Include LGTM; also add a Docker availability precondition in docker-images:package.The include is correct. Mirror earlier guidance by adding a
command -v dockerprecondition in taskfiles/docker-images.yaml so failures are fast and clear.Diff to apply in taskfiles/docker-images.yaml:
package: vars: SRC_DIR: "{{.ROOT_DIR}}/tools/docker-images/clp-package" dir: "{{.SRC_DIR}}" sources: - "{{.G_PACKAGE_CHECKSUM_FILE}}" - "{{.SRC_DIR}}/**/*" + preconditions: + - sh: "command -v docker" + msg: "Docker is required to build the package image. Please install Docker and try again." deps: - ":package" cmds: - "./build.sh"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
taskfile.yaml(3 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: quinntaylormitchell
PR: y-scope/clp#918
File: .github/workflows/clp-execution-image-build.yaml:77-97
Timestamp: 2025-05-26T16:03:05.519Z
Learning: In .github/workflows/clp-execution-image-build.yaml, the ubuntu-jammy-execution-image and ubuntu-noble-execution-image jobs are intentionally kept separate (rather than using a matrix strategy) to make it easier to remove individual platform versions when they reach end of life, such as when jammy eventually becomes obsolete.
📚 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:
taskfile.yaml
| {{default "" (env "CLP_CORE_MAX_PARALLELISM_PER_BUILD_TASK")}} | ||
| # Checksum files | ||
| G_PACKAGE_CHECKSUM_FILE: "{{.G_BUILD_DIR}}/package.md5" |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Use G_PACKAGE_CHECKSUM_FILE consistently (avoid hard-coded path).
package-tar still references the literal path; switch it to the new var to prevent drift.
@@
package-tar:
@@
sources:
- - "{{.G_BUILD_DIR}}/package.md5"
+ - "{{.G_PACKAGE_CHECKSUM_FILE}}"
- "{{.TASKFILE}}"🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Expect no hard-coded references after the change
rg -n '{{\.G_BUILD_DIR}}/package\.md5' -g '!**/node_modules/**' || trueLength of output: 198
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
# Verify no hard-coded references to the old path remain
rg -F "{{.G_BUILD_DIR}}/package.md5" -g '!**/node_modules/**' || echo "No hard-coded references found"Length of output: 208
Replace literal package.md5 path with G_PACKAGE_CHECKSUM_FILE in package-tar sources
The package-tar targets still reference "{{.G_BUILD_DIR}}/package.md5" directly; switch it to use the G_PACKAGE_CHECKSUM_FILE variable for consistency:
@@ package-tar:
sources:
- - "{{.G_BUILD_DIR}}/package.md5"
+ - "{{.G_PACKAGE_CHECKSUM_FILE}}"
- "{{.TASKFILE}}"🤖 Prompt for AI Agents
In taskfile.yaml around line 41, G_PACKAGE_CHECKSUM_FILE is set to
"{{.G_BUILD_DIR}}/package.md5" but the package-tar targets elsewhere still
reference the literal "{{.G_BUILD_DIR}}/package.md5"; update those package-tar
sources to use the G_PACKAGE_CHECKSUM_FILE variable instead (replace the literal
path with "{{ .G_PACKAGE_CHECKSUM_FILE }}" or the taskfile's variable
interpolation syntax) so all references are consistent and rely on the single
variable defined at line 41.
…t-build` for clarity; remove section `clp-package-image-build`
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
docs/src/dev-docs/contributing-linting.md (2)
15-19: macOS prerequisite is incorrect as writtenmacOS provides md5, not md5sum. Clarify to prevent setup failures.
-* `md5sum` +* `md5sum` (or `md5` on macOS)
18-19: Align version spec style with uv (use a lower bound)Be explicit about minimum Task version to avoid pin-drift confusion.
-* [Task] 3.44.0 +* [Task] >= 3.44.0docs/src/dev-docs/tooling-gh-workflows.md (1)
91-96: Update section to reflect the new composite action name and scopeThis still references the old “clp-execution-image-build” workflow. The PR renames/centralises to a composite action that builds both runtime types.
-## clp-execution-image-build - -This workflow builds a container image that contains the dependencies necessary to run the CLP -package. +## clp-build-runtime-image (composite action) + +This composite action builds and publishes CLP runtime images. It supports `image_type: execution|package` +and is invoked from workflows (e.g., `clp-artifact-build`) rather than being a standalone workflow.
♻️ Duplicate comments (3)
docs/src/dev-docs/tooling-gh-workflows.md (1)
66-66: Add a direct link and triggers for the package-image jobReaders shouldn’t have to hunt for where this runs; link the job and note its triggers.
-* `package-image`: Builds a container image containing CLP's package components. +* `package-image`: Builds a container image containing CLP's package components. See + `.github/workflows/clp-core-build.yaml` (job: `package-image`). It runs on the same triggers as + `clp-artifact-build` (pull_request, push, schedule, workflow_dispatch)..github/workflows/clp-core-build.yaml (2)
11-11: actionlint error: alias used in paths-ignore (breaks CI validation)Inline the sequence; actionlint rejects aliases here.
push: - paths-ignore: *ignored_paths + paths-ignore: + - ".github/*" + - ".github/ISSUE_TEMPLATE/**" + - "components/core/tools/scripts/lib_install/macos/**" + - "docs/**"#!/bin/bash set -euo pipefail curl -sSL https://raw.githubusercontent.com/rhysd/actionlint/main/scripts/download-actionlint.bash | bash -s -- -b /tmp /tmp/actionlint -color -ignore 'shellcheck.*' .github/workflows/clp-core-build.yaml
526-529: Normalise step name: “Work around …”Match existing convention used elsewhere for grepability.
- - name: "Workaround actions/runner-images/issues/6775" + - name: "Work around actions/runner-images/issues/6775"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
.github/workflows/clp-core-build.yaml(3 hunks)docs/src/dev-docs/contributing-linting.md(2 hunks)docs/src/dev-docs/tooling-gh-workflows.md(3 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: quinntaylormitchell
PR: y-scope/clp#918
File: .github/workflows/clp-execution-image-build.yaml:77-97
Timestamp: 2025-05-26T16:03:05.519Z
Learning: In .github/workflows/clp-execution-image-build.yaml, the ubuntu-jammy-execution-image and ubuntu-noble-execution-image jobs are intentionally kept separate (rather than using a matrix strategy) to make it easier to remove individual platform versions when they reach end of life, such as when jammy eventually becomes obsolete.
📚 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:
docs/src/dev-docs/contributing-linting.md
📚 Learning: 2025-05-26T16:03:05.519Z
Learnt from: quinntaylormitchell
PR: y-scope/clp#918
File: .github/workflows/clp-execution-image-build.yaml:77-97
Timestamp: 2025-05-26T16:03:05.519Z
Learning: In .github/workflows/clp-execution-image-build.yaml, the ubuntu-jammy-execution-image and ubuntu-noble-execution-image jobs are intentionally kept separate (rather than using a matrix strategy) to make it easier to remove individual platform versions when they reach end of life, such as when jammy eventually becomes obsolete.
Applied to files:
docs/src/dev-docs/tooling-gh-workflows.md.github/workflows/clp-core-build.yaml
📚 Learning: 2025-07-01T14:52:02.418Z
Learnt from: jackluo923
PR: y-scope/clp#1054
File: components/core/tools/docker-images/clp-env-base-musllinux_1_2-x86/build.sh:18-24
Timestamp: 2025-07-01T14:52:02.418Z
Learning: In the CLP project, consistency across platform build scripts is prioritized over defensive programming when it comes to git remote handling. All build.sh files in docker-images directories should follow the same pattern for git metadata injection.
Applied to files:
docs/src/dev-docs/tooling-gh-workflows.md
🪛 LanguageTool
docs/src/dev-docs/tooling-gh-workflows.md
[uncategorized] ~66-~66: Loose punctuation mark.
Context: ...uns core's unit tests. * package-image: Builds a container image containing CLP...
(UNLIKELY_OPENING_PUNCTUATION)
🪛 actionlint (1.7.7)
.github/workflows/clp-core-build.yaml
11-11: "paths-ignore" section must be sequence node but got alias node with "" tag
(syntax-check)
⏰ 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). (5)
- GitHub Check: ubuntu-jammy-deps-image
- GitHub Check: musllinux_1_2-x86_64-deps-image
- GitHub Check: centos-stream-9-deps-image
- GitHub Check: manylinux_2_28-x86_64-deps-image
- GitHub Check: build-macos (macos-15, false)
🔇 Additional comments (1)
.github/workflows/clp-core-build.yaml (1)
510-548: Confirm publish guard for package imagesEnsure clp-build-runtime-image only pushes on pushes to main; otherwise PRs may attempt to publish.
If not handled inside the composite action, gate the publish step here:
- - uses: "./.github/actions/clp-build-runtime-image" + - if: "github.event_name == 'push' && github.ref == 'refs/heads/main'" + uses: "./.github/actions/clp-build-runtime-image" with: image_type: "package" image_registry: "ghcr.io" image_registry_username: "${{github.actor}}" image_registry_password: "${{secrets.GITHUB_TOKEN}}"
| - ".github/workflows/clp-artifact-build.yaml" | ||
| - "components/core/tools/scripts/lib_install/*.sh" |
There was a problem hiding this comment.
Filters watch a non-existent workflow path
Filters reference .github/workflows/clp-artifact-build.yaml, but this file is clp-core-build.yaml. As-is, edits here won’t trigger filtered jobs.
- - ".github/workflows/clp-artifact-build.yaml"
+ - ".github/workflows/clp-core-build.yaml"(Apply to all five occurrences in centos_stream_9_image, manylinux_2_28_x86_64_image, musllinux_1_2_x86_64_image, ubuntu_jammy_image, and clp filters.)
Also applies to: 69-70, 75-76, 81-82, 87-88
🤖 Prompt for AI Agents
.github/workflows/clp-core-build.yaml lines 63-64 (and similarly at lines 69-70,
75-76, 81-82, 87-88): the workflow filter entries incorrectly reference
".github/workflows/clp-artifact-build.yaml" which does not exist; update each
occurrence to reference the correct workflow path
".github/workflows/clp-core-build.yaml" (replace the erroneous string in all
five filter blocks) so the filter matching triggers as intended.
| ``` | ||
|
|
||
| [clp-core-build]: https://github.com/y-scope/clp/blob/main/.github/workflows/clp-core-build.yaml | ||
| [clp-artifact-build]: https://github.com/y-scope/clp/blob/main/.github/workflows/clp-artifact-build.yaml |
There was a problem hiding this comment.
Fix broken link: file path doesn’t exist as written
Repo still contains .github/workflows/clp-core-build.yaml; the doc links to clp-artifact-build.yaml. Update link or rename the workflow file in this PR—don’t ship a dead link.
-[clp-artifact-build]: https://github.com/y-scope/clp/blob/main/.github/workflows/clp-artifact-build.yaml
+[clp-artifact-build]: https://github.com/y-scope/clp/blob/main/.github/workflows/clp-core-build.yaml📝 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.
| [clp-artifact-build]: https://github.com/y-scope/clp/blob/main/.github/workflows/clp-artifact-build.yaml | |
| [clp-artifact-build]: https://github.com/y-scope/clp/blob/main/.github/workflows/clp-core-build.yaml |
🤖 Prompt for AI Agents
In docs/src/dev-docs/contributing-linting.md around line 35, the markdown link
target points to .github/workflows/clp-artifact-build.yaml which doesn't exist
in the repo; update the link to reference the existing
.github/workflows/clp-core-build.yaml (or rename the workflow to match if you
intend the new name) so the reference is valid, and run a quick link-check to
verify no other dead links remain.
There was a problem hiding this comment.
the file should also has been renamed in the PR? could you double check?
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if both workflow files exist in the repository
echo "Checking for workflow files..."
ls -la .github/workflows/ | grep -E "(clp-core-build|clp-artifact-build)"
# Check git history for any file renames in this PR
echo -e "\n\nChecking for file renames in recent commits..."
git log --name-status --oneline -10 | grep -E "(clp-core-build|clp-artifact-build)"
# Specifically check if clp-artifact-build.yaml exists
echo -e "\n\nChecking if clp-artifact-build.yaml exists..."
if [ -f ".github/workflows/clp-artifact-build.yaml" ]; then
echo "✓ clp-artifact-build.yaml exists"
else
echo "✗ clp-artifact-build.yaml does not exist"
fi
# Check if clp-core-build.yaml exists
if [ -f ".github/workflows/clp-core-build.yaml" ]; then
echo "✓ clp-core-build.yaml exists"
else
echo "✗ clp-core-build.yaml does not exist"
fi
Length of output: 1083
@junhaoliao You're absolutely right! I apologize for the confusion.
The workflow file was indeed renamed in this PR from clp-core-build.yaml to clp-artifact-build.yaml. The git history shows a commit titled "ci: rename workflow clp-core-build -> clp-artifact-build" and the file system confirms:
✓ clp-artifact-build.yaml exists
✗ clp-core-build.yaml does not exist
The documentation link in contributing-linting.md is correct as written and points to the properly renamed workflow file. My initial assessment was wrong - there's no broken link here. The workflow renaming was part of this PR's changes, and all the documentation references have been properly updated to match the new filename.
Thank you for asking me to double-check!
junhaoliao
requested review from
kirkrodrigues
and removed request for
kirkrodrigues
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
.github/workflows/clp-artifact-build.yaml(3 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: quinntaylormitchell
PR: y-scope/clp#918
File: .github/workflows/clp-execution-image-build.yaml:77-97
Timestamp: 2025-05-26T16:03:05.519Z
Learning: In .github/workflows/clp-execution-image-build.yaml, the ubuntu-jammy-execution-image and ubuntu-noble-execution-image jobs are intentionally kept separate (rather than using a matrix strategy) to make it easier to remove individual platform versions when they reach end of life, such as when jammy eventually becomes obsolete.
📚 Learning: 2025-05-26T16:03:05.519Z
Learnt from: quinntaylormitchell
PR: y-scope/clp#918
File: .github/workflows/clp-execution-image-build.yaml:77-97
Timestamp: 2025-05-26T16:03:05.519Z
Learning: In .github/workflows/clp-execution-image-build.yaml, the ubuntu-jammy-execution-image and ubuntu-noble-execution-image jobs are intentionally kept separate (rather than using a matrix strategy) to make it easier to remove individual platform versions when they reach end of life, such as when jammy eventually becomes obsolete.
Applied to files:
.github/workflows/clp-artifact-build.yaml
🪛 actionlint (1.7.7)
.github/workflows/clp-artifact-build.yaml
11-11: "paths-ignore" section must be sequence node but got alias node with "" tag
(syntax-check)
⏰ 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). (9)
- GitHub Check: build-macos (macos-15, true)
- GitHub Check: build-macos (macos-15, false)
- GitHub Check: musllinux_1_2-x86_64-deps-image
- GitHub Check: ubuntu-jammy-deps-image
- GitHub Check: manylinux_2_28-x86_64-deps-image
- GitHub Check: centos-stream-9-deps-image
- GitHub Check: lint-check (macos-15)
- GitHub Check: lint-check (ubuntu-24.04)
- GitHub Check: build (macos-15)
🔇 Additional comments (2)
.github/workflows/clp-artifact-build.yaml (2)
63-87: Good: filters updated to reference this workflowAll image filters now correctly include .github/workflows/clp-artifact-build.yaml.
512-517: LGTM: sensible gating for package-imageJob runs if the jammy deps image is unchanged or successfully rebuilt; aligns with the pattern elsewhere.
| paths-ignore: &ignored_paths | ||
| - ".github/*" | ||
| - ".github/ISSUE_TEMPLATE/**" | ||
| - "components/core/tools/scripts/lib_install/macos/**" | ||
| - "docs/**" | ||
| push: | ||
| paths: | ||
| - ".github/actions/clp-core-build-containers/action.yaml" | ||
| - ".github/actions/run-on-image/action.yaml" | ||
| - ".github/workflows/clp-core-build.yaml" | ||
| - ".gitmodules" | ||
| - "components/core/**" | ||
| - "taskfile.yaml" | ||
| - "taskfiles/**" | ||
| - "tools/scripts/deps-download/**" | ||
| - "!components/core/tools/scripts/lib_install/macos/**" | ||
| paths-ignore: *ignored_paths | ||
| schedule: |
There was a problem hiding this comment.
Fix actionlint failure: avoid YAML alias for paths-ignore
actionlint 1.7.7 flags the alias as invalid for a sequence; duplicate the list to satisfy the linter.
Apply this diff:
pull_request:
- paths-ignore: &ignored_paths
+ paths-ignore:
- ".github/*"
- ".github/ISSUE_TEMPLATE/**"
- "components/core/tools/scripts/lib_install/macos/**"
- "docs/**"
push:
- paths-ignore: *ignored_paths
+ paths-ignore:
+ - ".github/*"
+ - ".github/ISSUE_TEMPLATE/**"
+ - "components/core/tools/scripts/lib_install/macos/**"
+ - "docs/**"📝 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.
| paths-ignore: &ignored_paths | |
| - ".github/*" | |
| - ".github/ISSUE_TEMPLATE/**" | |
| - "components/core/tools/scripts/lib_install/macos/**" | |
| - "docs/**" | |
| push: | |
| paths: | |
| - ".github/actions/clp-core-build-containers/action.yaml" | |
| - ".github/actions/run-on-image/action.yaml" | |
| - ".github/workflows/clp-core-build.yaml" | |
| - ".gitmodules" | |
| - "components/core/**" | |
| - "taskfile.yaml" | |
| - "taskfiles/**" | |
| - "tools/scripts/deps-download/**" | |
| - "!components/core/tools/scripts/lib_install/macos/**" | |
| paths-ignore: *ignored_paths | |
| schedule: | |
| pull_request: | |
| paths-ignore: | |
| - ".github/*" | |
| - ".github/ISSUE_TEMPLATE/**" | |
| - "components/core/tools/scripts/lib_install/macos/**" | |
| - "docs/**" | |
| push: | |
| paths-ignore: | |
| - ".github/*" | |
| - ".github/ISSUE_TEMPLATE/**" | |
| - "components/core/tools/scripts/lib_install/macos/**" | |
| - "docs/**" |
🧰 Tools
🪛 actionlint (1.7.7)
11-11: "paths-ignore" section must be sequence node but got alias node with "" tag
(syntax-check)
🤖 Prompt for AI Agents
.github/workflows/clp-artifact-build.yaml around lines 5 to 12 uses a YAML alias
for paths-ignore which actionlint flags as invalid for sequences; replace the
alias usage by duplicating the ignored paths list inline wherever paths-ignore
is used (remove the &ignored_paths anchor and *ignored_paths alias and paste the
same sequence under both the top-level and push.paths-ignore keys) so each
paths-ignore contains its own explicit sequence.
| shell: "bash" | ||
| run: "chown $(id -u):$(id -g) -R ." | ||
|
|
||
| - name: "Build the package" |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consistency nit: step name matches others
Use “Work around actions/runner-images/issues/6775” (with a space) to match other jobs.
Apply this diff:
- - name: "Workaround actions/runner-images/issues/6775"
+ - name: "Work around actions/runner-images/issues/6775"📝 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.
| - name: "Workaround actions/runner-images/issues/6775" | |
| shell: "bash" | |
| run: "chown $(id -u):$(id -g) -R ." | |
| - name: "Work around actions/runner-images/issues/6775" | |
| shell: "bash" | |
| run: "chown $(id -u):$(id -g) -R ." |
🤖 Prompt for AI Agents
.github/workflows/clp-artifact-build.yaml around lines 526 to 529: the step name
currently reads "Workaround actions/runner-images/issues/6775" and should be
changed to "Work around actions/runner-images/issues/6775" to match the naming
used in other jobs; update only the name value to include the space, leaving the
shell and run lines unchanged.
| env: | ||
| OS_NAME: "ubuntu-jammy" | ||
| with: | ||
| image_name: "${{env.DEPS_IMAGE_NAME_PREFIX}}${{env.OS_NAME}}" | ||
| use_published_image: >- | ||
| ${{needs.filter-relevant-changes.outputs.ubuntu_jammy_image_changed == 'false' | ||
| || (github.event_name != 'pull_request' && github.ref == 'refs/heads/main')}} | ||
| run_command: >- | ||
| CLP_CORE_MAX_PARALLELISM_PER_BUILD_TASK=$(getconf _NPROCESSORS_ONLN) task package | ||
| - uses: "./.github/actions/clp-build-runtime-image" |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify task alias exists and works in container
Ensure Taskfile defines a “package” task and it functions in the run-on-image environment.
Run:
🏁 Script executed:
#!/bin/bash
# Confirm a 'package' task exists and show its definition
rg -nP -C3 '^\s*package:\s*$' -g 'taskfile.yaml' -g 'Taskfile.yaml' -g 'taskfiles/**/Taskfile*.y*ml'
# Sanity-check run-on-image command string occurrences
rg -n 'task package' .github/actions/run-on-imageLength of output: 460
🏁 Script executed:
#!/bin/bash
# Show full 'package' task definition
sed -n '90,120p' taskfile.yaml
# Inspect action metadata to confirm 'run_command' input
sed -n '1,200p' .github/actions/run-on-image/action.yml
# Inspect entrypoint script to see how 'run_command' is executed
if [ -f .github/actions/run-on-image/entrypoint.sh ]; then
sed -n '1,200p' .github/actions/run-on-image/entrypoint.sh
fiLength of output: 1369
🏁 Script executed:
#!/bin/bash
# List all files in the run-on-image action directory
ls -R .github/actions/run-on-image
# Display first 200 lines of any YAML metadata files
sed -n '1,200p' .github/actions/run-on-image/*.yaml .github/actions/run-on-image/*.ymlLength of output: 2120
🏁 Script executed:
#!/bin/bash
# Locate all Dockerfiles to identify where ubuntu-jammy images are built
find . -type f -name 'Dockerfile*'Length of output: 706
🏁 Script executed:
#!/bin/bash
# Inspect clp-execution-base-ubuntu-jammy Dockerfile for 'task' CLI installation
sed -n '1,200p' tools/docker-images/clp-execution-base-ubuntu-jammy/Dockerfile || echo "File not found"
# Search for download or install of the Task binary (e.g., go install, curl, wget)
grep -nE 'go install|curl .*task|wget .*task' tools/docker-images/clp-execution-base-ubuntu-jammy/Dockerfile || echo "No install commands found"
# Inspect clp-env-base-ubuntu-jammy for any Taskfile or Task install steps
sed -n '1,200p' components/core/tools/docker-images/clp-env-base-ubuntu-jammy/Dockerfile || echo "File not found"
grep -nE 'go install|curl .*task|wget .*task' components/core/tools/docker-images/clp-env-base-ubuntu-jammy/Dockerfile || echo "No install commands found"Length of output: 1414
🏁 Script executed:
#!/bin/bash
# Inspect prebuilt packages installation script for Task CLI installation
sed -n '1,200p' tools/docker-images/clp-execution-base-ubuntu-jammy/setup-scripts/install-prebuilt-packages.sh || echo "Script not found"
grep -R "task" -n tools/docker-images/clp-execution-base-ubuntu-jammy/setup-scripts/install-prebuilt-packages.sh || echo "No 'task' found"
# Inspect environment base install-all script for Task CLI installation
sed -n '1,200p' components/core/tools/scripts/lib_install/ubuntu-jammy/install-all.sh || echo "Script not found"
grep -R "task" -n components/core/tools/scripts/lib_install/ubuntu-jammy/install-all.sh || echo "No 'task' found"Length of output: 1078
🏁 Script executed:
#!/bin/bash
# Inspect clp-core-ubuntu-jammy Dockerfile for Task CLI installation
sed -n '1,200p' components/core/tools/docker-images/clp-core-ubuntu-jammy/Dockerfile || echo "File not found"
grep -nE 'go install|curl .*task|wget .*task|task ' components/core/tools/docker-images/clp-core-ubuntu-jammy/Dockerfile || echo "No install commands found"
# Inspect clp-package Dockerfile for Task CLI or vendored binary
sed -n '1,200p' tools/docker-images/clp-package/Dockerfile || echo "File not found"
grep -nE 'go install|curl .*task|wget .*task|task ' tools/docker-images/clp-package/Dockerfile || echo "No install commands found"
# Inspect source installation script for Task CLI in env-base
sed -n '1,200p' components/core/tools/scripts/lib_install/ubuntu-jammy/install-packages-from-source.sh || echo "Script not found"
grep -R "task" -n components/core/tools/scripts/lib_install/ubuntu-jammy/install-packages-from-source.sh || echo "No 'task' found"Length of output: 2172
Install the Task CLI in the Ubuntu-Jammy container images
The Taskfile.yaml correctly defines a package task, but none of the Ubuntu-Jammy images used by .github/actions/run-on-image install the task binary. As a result, task package will fail inside the container. Add steps to install the Task CLI (for example, in tools/docker-images/clp-execution-base-ubuntu-jammy/Dockerfile or components/core/tools/docker-images/clp-env-base-ubuntu-jammy/Dockerfile) so that task is available at runtime.
🤖 Prompt for AI Agents
.github/workflows/clp-artifact-build.yaml around lines 531 to 541: the workflow
runs `task package` inside the ubuntu-jammy image but the container images used
by .github/actions/run-on-image do not have the Task CLI installed, causing the
command to fail; update the relevant ubuntu-jammy Dockerfile(s) (e.g.
tools/docker-images/clp-execution-base-ubuntu-jammy/Dockerfile or
components/core/tools/docker-images/clp-env-base-ubuntu-jammy/Dockerfile) to
install the Task binary during image build (install the official Task release
for the target architecture, place the executable in a directory on PATH like
/usr/local/bin, and ensure proper permissions), rebuild/push the updated images
and verify the workflow uses the new images so `task package` runs successfully.
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Publishing safeguards and permissions
Confirm the clp-build-runtime-image action only pushes on main; otherwise gate this step or pass a publish flag. Also, prefer explicit permissions for GHCR pushes.
-
Verify action behaviour:
-
Consider adding (outside this hunk):
permissions:
contents: read
packages: write🏁 Script executed:
#!/bin/bash
# Inspect local action for publish conditions
rg -n 'image_type|push|publish|github\.ref|github\.event_name' .github/actions/clp-build-runtime-imageLength of output: 857
Workflow step already gated; add explicit GHCR permissions
The clp-build-runtime-image action uses
if: github.event_name != 'pull_request' && github.ref == 'refs/heads/main'
in its action.yaml to only push on main. Add at the top of .github/workflows/clp-artifact-build.yaml:
permissions:
contents: read
packages: write🤖 Prompt for AI Agents
In .github/workflows/clp-artifact-build.yaml (context around lines 542-547), the
workflow lacks explicit GHCR permissions required for publishing the runtime
image even though the action itself gates execution; add a top-level permissions
block with contents: read and packages: write at the very top of the workflow
file so the runner has permission to read repo contents and push packages to
GHCR during the clp-build-runtime-image step.
kirkrodrigues
left a comment
kirkrodrigues
left a comment
There was a problem hiding this comment.
For the PR title, how about:
feat(docker): Add container image containing the CLP package, for Docker Compose integration (resolves #1164).
| ENV CLP_HOME="/opt/clp" | ||
| ENV PATH="${CLP_HOME}/bin:${PATH}" | ||
| ENV PATH="${CLP_HOME}/sbin:${PATH}" | ||
| ENV PYTHONPATH="${CLP_HOME}/lib/python3/site-packages" |
There was a problem hiding this comment.
Below syntax reduces building layers while keeping VCS friendliness:
ENV KEY1=VAL1
KEY2=VAL2
@coderabbitai create an issue for implementing the improvement
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Create a GitHub issue for the Dockerfile ENV optimization
gh issue create \
--repo y-scope/clp \
--title "Optimize ENV declarations in clp-package Dockerfile to reduce layers" \
--body "## Context
As discussed in PR #1166 (https://github.com/y-scope/clp/pull/1166#discussion_r2403578550), the ENV declarations in \`tools/docker-images/clp-package/Dockerfile\` can be optimized to reduce the number of image layers while maintaining VCS friendliness.
## Current Implementation
Currently, the Dockerfile uses separate ENV statements:
\`\`\`dockerfile
ENV CLP_HOME=\"/opt/clp\"
ENV PATH=\"\${CLP_HOME}/bin:\${PATH}\"
ENV PATH=\"\${CLP_HOME}/sbin:\${PATH}\"
ENV PYTHONPATH=\"\${CLP_HOME}/lib/python3/site-packages\"
\`\`\`
## Proposed Improvement
Use multi-line ENV syntax to reduce layers:
\`\`\`dockerfile
ENV KEY1=VAL1 \\
KEY2=VAL2
\`\`\`
## References
- PR: https://github.com/y-scope/clp/pull/1166
- Comment: https://github.com/y-scope/clp/pull/1166#discussion_r2403578550
- Requested by: @junhaoliao" \
--assignee junhaoliao
Length of output: 42
@junhaoliao I've created issue #1378 to track this optimization. The issue includes the context from this discussion, the current implementation, and the proposed improvement with appropriate backlinks to this PR and comment.
| # Flatten the image | ||
| FROM scratch | ||
| COPY --from=base / / |
There was a problem hiding this comment.
While currently the USER and ENV commands are unused, their effects would be lost after flattening the image. i.e., Those settings should be done after the image is flattened.
@coderabbitai create an issue to track this
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Create a GitHub issue to track the USER and ENV directive problem in the flattened Docker image
gh issue create \
--title "Docker image flattening loses USER and ENV directives in clp-package Dockerfile" \
--body "## Problem
In \`tools/docker-images/clp-package/Dockerfile\`, the \`USER\` and \`ENV\` commands are declared in the base stage (lines 13-18) before the image is flattened using \`FROM scratch\` and \`COPY --from=base\`. This causes their effects to be lost in the final image.
## Details
The following directives are currently in the base stage:
- \`ENV CLP_HOME=\"/opt/clp\"\`
- \`ENV PATH=\"\${CLP_HOME}/bin:\${PATH}\"\`
- \`ENV PATH=\"\${CLP_HOME}/sbin:\${PATH}\"\`
- \`ENV PYTHONPATH=\"\${CLP_HOME}/lib/python3/site-packages\"\`
- \`USER 1000:1000\`
When the image is flattened with \`FROM scratch\` followed by \`COPY --from=base / /\`, these directives are not preserved in the final image metadata.
## Solution
These directives should be moved to after the \`COPY --from=base / /\` command in the final stage to ensure they are present in the resulting image.
## References
- PR: https://github.com/y-scope/clp/pull/1166
- Comment: https://github.com/y-scope/clp/pull/1166#discussion_r2403579973
- Reported by: @junhaoliao" \
--assignee junhaoliao
Length of output: 42
@junhaoliao, I've created issue #1379 to track this problem. The issue documents that the USER and ENV directives in the base stage are lost after flattening and should be moved to the final stage after the COPY --from=base / / command.
|
|
||
| WORKDIR /root | ||
|
|
||
| COPY ./tools/docker-images/clp-package/setup-scripts ./setup-scripts |
There was a problem hiding this comment.
@coderabbitai it appears that we should always use COPY --link to speed up builds. if you agree, file an issue
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Docker COPY --link flag benefits and best practices BuildKit performance
💡 Result:
Summary — what COPY --link does
- COPY --link (Dockerfile frontend v1.4+) makes COPY/ADD create an independent layer containing only the copied files and then "link" that layer onto prior layers instead of copying files on top of the previous snapshot. This decouples the COPY from earlier commands and improves caching and parallelism. [1][2]
Key benefits
- Stronger cache stability: COPY steps don’t get invalidated when earlier commands in the same stage change, so cache hits are more reliable. Useful in multi-stage builds where intermediate stages often change. [1][2]
- Faster/parallel builds: BuildKit can finish COPY without waiting for earlier commands to complete, allowing more parallelism and shorter build times. [1][2]
- Avoids pulling full base-image blobs during build/export: BuildKit can link layers by metadata (manifest) so you don’t always need the base image blobs locally to complete a build; this speeds CI and rebasing. [1][2][3]
- Smaller/less redundant on-disk work: BuildKit avoids duplicating file blobs when possible (hard-link-like behavior), lowering disk I/O and temporary storage during builds. [3][6]
Practical best practices
- Enable the correct frontend: add the Dockerfile header: # syntax=docker/dockerfile:1.4 (or later) so the COPY --link frontend is used. [1][2]
- Use it for files/directories with stable destination semantics — avoid ambiguous destination semantics (e.g., relying on COPY to append into an existing dir vs create a file). When --link is used, the destination layer starts empty, so ambiguous targets like COPY my-file /data (expecting /data/my-file) can change behavior. Test paths explicitly. [1][3]
- Avoid copying into or relying on symlink resolution in the destination layer; symlink resolution that existed in earlier layers may not be present for a linked layer. [3]
- Prefer explicit destination directories (use trailing slash when you mean a directory) and avoid COPY --link . . with unclear WORKDIR/dot semantics unless you’ve validated it. [1][3]
- Use in CI with --cache-from / remote cache and Buildx to benefit from remote cache/higher parallelism; it enables easier image rebases and better remote-cache reuse. [2][3]
- Test interactions with --chown and platform/drivers: some environments or driver combos (e.g., docker-container driver, Docker Desktop on Windows) have had issues reported; validate your CI/worker combination. [4]
When not to use it / caveats
- Behavioral differences vs classic COPY (destination/symlink semantics) — if your Dockerfile relies on previous-layer filesystem state, do not enable --link for that COPY. [1][3]
- Some combinations historically had platform/driver bugs (e.g., chown resolution in certain buildx/docker-container setups); test across your build targets. [4]
- Because it's a semantics change, don’t flip a production pipeline to --link without validating image layout and runtime behavior. [1][4]
Quick example
- Top of Dockerfile: # syntax=docker/dockerfile:1.4
- Typical pattern:
FROM node:18-alpine AS builder
WORKDIR /app
COPY --link package*.json ./
RUN npm ci
COPY --link . .
RUN npm run build
FROM nginx:alpine
COPY --link --from=builder /app/dist /usr/share/nginx/html
(works best with BuildKit/Buildx and remote cache configured). [2][3]
Sources
[1] Docker blog — Image rebase and improved remote cache support in new BuildKit (March 17, 2022).
[2] Dockerfile reference (COPY --link docs).
[3] How-To‑Geek explanation and examples of COPY --link behavior and benefits.
[4] moby/buildkit GitHub issues (observed chown/driver interaction notes).
🏁 Script executed:
#!/bin/bash
# Find all Dockerfiles in the repository to include in the issue
fd -t f -e Dockerfile --full-path | head -20
Length of output: 0
🏁 Script executed:
#!/bin/bash
# Find all Dockerfiles in the repository (files named "Dockerfile")
fd -t f '^Dockerfile$' --full-path
# Also check for files with Dockerfile prefix
fd -t f '^Dockerfile' --full-path
Length of output: 0
🏁 Script executed:
#!/bin/bash
# Use find to locate Dockerfiles
find . -type f -name "Dockerfile" 2>/dev/null | head -20
# Also search for Dockerfile mentions in the repo to understand structure
rg -l "^FROM " --type-add 'docker:Dockerfile*' -t docker 2>/dev/null | head -10 || echo "rg with type filter failed, trying simple search"
# Alternative: just search for FROM statements which are typical in Dockerfiles
rg -l "^FROM " 2>/dev/null | grep -i dockerfile | head -10
Length of output: 1973
🏁 Script executed:
#!/bin/bash
# Create a GitHub issue for adopting COPY --link in Dockerfiles
gh issue create \
--title "Adopt COPY --link in Dockerfiles for improved build performance" \
--body "## Summary
This issue tracks the adoption of \`COPY --link\` in all Dockerfiles across the repository to improve build performance and cache stability.
## Background
The \`COPY --link\` flag (available in BuildKit with Dockerfile frontend v1.4+) creates independent layers for copied files, decoupling them from earlier commands in the build stage. This approach offers several advantages over standard \`COPY\` instructions.
**Reference**: Suggested in PR #1166, comment https://github.com/y-scope/clp/pull/1166#discussion_r2425008953 by @junhaoliao
## Benefits
1. **Stronger cache stability**: COPY steps won't be invalidated when earlier commands in the same stage change, leading to more reliable cache hits
2. **Faster parallel builds**: BuildKit can execute COPY operations without waiting for earlier commands to complete
3. **Reduced I/O and disk usage**: BuildKit avoids duplicating file blobs when possible through hard-link-like behavior
4. **Improved CI/CD performance**: Works well with remote cache and enables easier image rebasing
## Implementation Requirements
### 1. Add syntax header to all Dockerfiles
Each Dockerfile must start with:
\`\`\`dockerfile
# syntax=docker/dockerfile:1.4
\`\`\`
### 2. Update COPY instructions
Replace \`COPY\` with \`COPY --link\` where appropriate:
\`\`\`dockerfile
COPY --link ./source ./destination
\`\`\`
### 3. Dockerfiles to update
The following Dockerfiles should be evaluated and updated:
- \`tools/docker-images/clp-package/Dockerfile\`
- \`tools/docker-images/clp-execution-base-ubuntu-jammy/Dockerfile\`
- \`components/core/tools/docker-images/clp-core-ubuntu-jammy/Dockerfile\`
- \`components/core/tools/docker-images/clp-env-base-centos-stream-9/Dockerfile\`
- \`components/core/tools/docker-images/clp-env-base-manylinux_2_28-aarch64/Dockerfile\`
- \`components/core/tools/docker-images/clp-env-base-manylinux_2_28-x86_64/Dockerfile\`
- \`components/core/tools/docker-images/clp-env-base-musllinux_1_2-aarch64/Dockerfile\`
- \`components/core/tools/docker-images/clp-env-base-musllinux_1_2-x86_64/Dockerfile\`
- \`components/core/tools/docker-images/clp-env-base-ubuntu-jammy/Dockerfile\`
## Important Caveats
1. **Destination semantics**: \`COPY --link\` creates layers starting from an empty filesystem, which can change behavior when copying to ambiguous destinations. Use explicit trailing slashes for directories.
2. **Symlink resolution**: Avoid copying into or relying on symlink resolution from earlier layers, as linked layers don't inherit the previous layer's filesystem state.
3. **Testing required**: Before production deployment:
- Validate image behavior and file layouts
- Test across different platforms (Linux, macOS, Windows)
- Verify CI/CD pipelines work correctly
- Check interactions with \`--chown\` flags
4. **BuildKit requirement**: Ensure CI/CD environments use BuildKit (enabled by default in Docker 23.0+, or set \`DOCKER_BUILDKIT=1\`).
## Testing Plan
1. Update one Dockerfile as a proof-of-concept
2. Verify build times and cache behavior improvements
3. Test resulting images for correct runtime behavior
4. Roll out to remaining Dockerfiles incrementally
5. Update CI/CD workflows if needed to ensure BuildKit is enabled
## References
- [Docker BuildKit COPY --link documentation](https://docs.docker.com/engine/reference/builder/#copy---link)
- [Docker blog: Image rebase and improved remote cache support](https://www.docker.com/blog/image-rebase-and-improved-remote-cache-support-in-new-buildkit/)
- [How-To Geek: Docker COPY --link explanation](https://www.howtogeek.com/devops/how-docker-copy-link-improves-dockerfile-caching/)
## Success Criteria
- [ ] All Dockerfiles updated with syntax header and \`COPY --link\`
- [ ] Build times improved or maintained
- [ ] Cache hit rates improved
- [ ] All images validated for correct runtime behavior
- [ ] CI/CD pipelines functioning correctly" \
--assignee junhaoliao \
--label enhancement
Length of output: 42
@junhaoliao I completely agree! Using COPY --link offers significant benefits for build performance and cache stability, especially with the multi-stage builds in this repository.
I've created issue #1408 to track the adoption of COPY --link across all Dockerfiles in the repository. The issue includes:
- Benefits: Improved cache stability, faster parallel builds, reduced I/O, and better CI/CD performance
- Implementation details: Adding the
# syntax=docker/dockerfile:1.4header and updating COPY instructions - All 9 Dockerfiles that should be updated (including the new
clp-package/Dockerfileand existing execution/environment images) - Important caveats: Destination semantics, symlink resolution, and testing requirements
- Testing plan: Incremental rollout with validation at each step
The key benefits for your use case include better cache hits when earlier build stages change and improved build parallelism in CI/CD pipelines.
2 participants
Description
This PR introduces a new container image for the CLP package and refactors the image building infrastructure. Key changes include:
package-imageinside GH workflowclp-core-buildfor building the "package" imagetools/docker-images/clp-package/The new package image will enable Docker Compose integration as described in issue #1164. Once the package's container orchestration is replaced by Docker Compose, the clp-execution-ubuntu-jammy image can be removed.
Checklist
breaking change.
Explanation: I did not mark this as breaking because the execution image has not been removed.
Explanation: We shall add docs after the Docker Compose scripts are checked in because the new container image cannot be used by general users standalone anyways.
Validation performed
junliaoliao/clp:mainand observed newly added jobpackage-imagecompleted successfully:https://github.com/junhaoliao/clp/actions/runs/17224822650/job/48867575560(latest: https://github.com/junhaoliao/clp/actions/runs/17445954258/job/49540713145 )docker run -it ghcr.io/junhaoliao/clp/clp-package:main /bin/bashlocally. Verified:du -sh /opt/:root@4d45478222a6:/# env | grep -E "PATH|CLP_HOME"Summary by CodeRabbit
New Features
Bug Fixes
Chores
Documentation