feat(docker): Generate unique tag for each clp-package dev image build to avoid conflicts on shared hosts. #1335
Conversation
WalkthroughAdds image lifecycle and IID handling to the clp-package Docker build script (temporary IID file, persistent IID, EXIT trap cleanup, previous-image removal, conditional Git OCI labels), and updates package build documentation with Docker prerequisites and a Docker reference link. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User
participant S as build.sh
participant D as Docker
participant G as Git (optional)
U->>S: invoke build.sh
activate S
S->>S: init iid_file, temp_iid_file, prev_image_id
S->>S: set EXIT trap -> remove_prev_image_and_temp_file
alt Git work tree
S->>G: read HEAD & origin URL
G-->>S: revision, source
S->>S: append OCI labels to build command
else no Git
S->>S: skip labels
end
S->>D: docker build --iidfile tmp_iid [with labels]
D-->>S: write new image ID to tmp_iid
S->>S: read tmp_iid -> new_image_id
S->>S: write new_image_id to iid_file
S->>D: docker tag new_image_id clp-package:dev-${USER}-${short_id}
note over S,D: On script EXIT
S->>D: remove previous image if exists and differs
S->>S: delete temp_iid_file
deactivate S
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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. 🧪 Early access (Sonnet 4.5): enabledWe are currently testing the Sonnet 4.5 model, which is expected to improve code review quality. However, this model may lead to increased noise levels in the review comments. Please disable the early access features if the noise level causes any inconvenience. Note:
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
taskfiles/docker-images.yaml(1 hunks)tools/docker-images/clp-package/build.sh(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 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:
tools/docker-images/clp-package/build.sh
📚 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:
tools/docker-images/clp-package/build.sh
⏰ 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). (10)
- GitHub Check: ubuntu-jammy-lint
- GitHub Check: ubuntu-jammy-dynamic-linked-bins
- GitHub Check: package-image
- GitHub Check: ubuntu-jammy-static-linked-bins
- GitHub Check: musllinux_1_2-x86_64-static-linked-bins
- GitHub Check: manylinux_2_28-x86_64-static-linked-bins
- GitHub Check: manylinux_2_28-x86_64-dynamic-linked-bins
- GitHub Check: musllinux_1_2-x86_64-dynamic-linked-bins
- GitHub Check: centos-stream-9-static-linked-bins
- GitHub Check: centos-stream-9-dynamic-linked-bins
🔇 Additional comments (2)
tools/docker-images/clp-package/build.sh (2)
31-37: Align git label injection with existing docker-image build scripts.Per project learnings, these scripts should follow the same pattern across platforms for git metadata injection. Double-check that using
git -C "$script_dir"and assuming a remote namedoriginis consistent with the other build.sh files in docker-images.If other scripts use a different working directory (e.g., repo root) or handle missing
origindifferently, mirror that here to maintain consistency. Based on learnings
25-27: Enhance Docker tag uniqueness and manually verify--iidfilesupport
- Use a more unique, POSIX-portable tag suffix:
- --tag "clp-package:dev-${USER}-$(date +%s)" + --tag "clp-package:dev-${USER:-unknown}-$(date -u +%Y%m%dT%H%M%S)-$$"
- Manually confirm your Docker build supports
--iidfile(e.g. rundocker build --help | grep -- '--iidfile').
taskfiles/docker-images.yaml
Outdated
| - ":package" | ||
| cmds: | ||
| - "./build.sh" | ||
| - "rsync -a 'image.id' '{{.G_PACKAGE_BUILD_DIR}}'" |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Ensure destination directory exists before rsync.
If {{.G_PACKAGE_BUILD_DIR}} doesn’t exist yet, rsync will fail. Create it first.
- "./build.sh"
- - "rsync -a 'image.id' '{{.G_PACKAGE_BUILD_DIR}}'"
+ - "mkdir -p '{{.G_PACKAGE_BUILD_DIR}}'"
+ - "rsync -a 'image.id' '{{.G_PACKAGE_BUILD_DIR}}'"🤖 Prompt for AI Agents
In taskfiles/docker-images.yaml around line 15, the rsync command assumes
{{.G_PACKAGE_BUILD_DIR}} exists; ensure the destination directory is created
first by invoking a mkdir -p on {{.G_PACKAGE_BUILD_DIR}} (or otherwise ensure
its parent exists) before running the rsync so the copy never fails when the
directory is missing.
There was a problem hiding this comment.
The docker-images:package task depends on the :package task, which already creates the G_PACKAGE_BUILD_DIR
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for the :package task definition in taskfiles
rg -n "package:" taskfiles/ -A 10 -B 2
# Also search for G_PACKAGE_BUILD_DIR usage to understand how it's created
rg -n "G_PACKAGE_BUILD_DIR" taskfiles/ -A 2 -B 2
Length of output: 1186
🏁 Script executed:
#!/bin/bash
# Find the main Taskfile
fd -t f "Taskfile" --max-depth 2
# Search for package task in all Taskfile files
rg -n "^[[:space:]]*package:" . -A 15 -B 2
# Also search for any mkdir commands that might create G_PACKAGE_BUILD_DIR
rg -n "mkdir.*G_PACKAGE_BUILD_DIR" .
Length of output: 7777
🏁 Script executed:
#!/bin/bash
# Get the complete package task definition from taskfile.yaml
rg -n "^[[:space:]]*package:" ./taskfile.yaml -A 25
# Also search for any scripts or commands that might create the directory
rg -n "G_PACKAGE_BUILD_DIR" ./taskfile.yaml -A 3 -B 3
# Look for mkdir commands that create package build directories
rg -n "mkdir.*package" . -i
Length of output: 2141
@junhaoliao You're absolutely correct! I can see that the :package task does indeed create the G_PACKAGE_BUILD_DIR directory with the command mkdir -p '{{.OUTPUT_DIR}}/lib/python3/site-packages' where OUTPUT_DIR is set to {{.G_PACKAGE_BUILD_DIR}}. Since the docker-images:package task depends on :package, the directory is guaranteed to exist before the rsync command runs.
Thank you for the clarification - my suggestion to add the redundant mkdir -p was unnecessary.
✏️ Learnings added
Learnt from: junhaoliao
PR: y-scope/clp#1335
File: taskfiles/docker-images.yaml:15-15
Timestamp: 2025-09-25T19:26:32.415Z
Learning: In the CLP project's Taskfile, the `:package` task creates the G_PACKAGE_BUILD_DIR directory structure, so any task that depends on `:package` (like `docker-images:package`) can safely assume this directory exists without needing additional mkdir commands.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| set -o pipefail | ||
|
|
||
| script_dir="$( cd "$( dirname "${BASH_SOURCE[0]}" )" &> /dev/null && pwd )" | ||
| iid_file="${script_dir}/image.id" |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider avoiding a shared image.id during concurrent builds.
Concurrent invocations writing the same image.id path can race. If parallel builds are possible, write to a per-invocation temp (then atomically move/rsync into place post-build).
Example approach:
- Write to "$(mktemp "${script_dir}/image.id.XXXXXX")"
- After a successful build,
mvto "${script_dir}/image.id"
Please confirm whether the task runner can trigger parallel builds for this target. If yes, I can provide a concrete patch.
🤖 Prompt for AI Agents
In tools/docker-images/clp-package/build.sh around line 7, the script writes a
shared image id file at ${script_dir}/image.id which can race during concurrent
builds; change it to write the id to a per-invocation temporary file (e.g.,
create a temp under ${script_dir} with mktemp), write the build output there,
and after a successful build atomically move the temp into
${script_dir}/image.id (mv on the same filesystem) so only completed builds
replace the shared file; also add a trap to clean the temp file on exit/failure
and ensure the temp is created on the same filesystem as script_dir; please
confirm whether the task runner can run this target in parallel so I can provide
an exact patch.
There was a problem hiding this comment.
it's not expected the user would invoke multiple image builds at the same time. @kirkrodrigues what do you think? shall we implement the rabbit's proposal and somehow guard against multiple concurrent builds?
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
I don't think it's necessary right now.
| # Remove the previous image after the build to allow layer reuse. | ||
| prev_image_id="" | ||
| if [[ -f "$iid_file" ]]; then | ||
| prev_image_id=$(cat "$iid_file") | ||
| fi | ||
| cleanup() { | ||
| if [[ -n "$prev_image_id" ]] && docker image inspect "$prev_image_id" >/dev/null 2>&1; then | ||
| echo "Removing previous image $prev_image_id" | ||
| docker image remove "$prev_image_id" | ||
| fi | ||
| } | ||
| trap cleanup EXIT | ||
|
|
There was a problem hiding this comment.
Do not delete the newly built image when its ID equals the previous one; avoid trap-based deletion.
If the build is reproducible, the new image ID can equal the previous image.id. With the current trap, you’ll remove the just-built image on exit, leaving image.id pointing to a non-existent image. Also, removing on any script exit (including failed builds) is risky.
Refactor to only remove the previous image after a successful build and only if it differs from the new ID. Remove the trap and perform the check post-build.
Apply this diff to remove the trap block:
-# Remove the previous image after the build to allow layer reuse.
-prev_image_id=""
-if [[ -f "$iid_file" ]]; then
- prev_image_id=$(cat "$iid_file")
-fi
-cleanup() {
- if [[ -n "$prev_image_id" ]] && docker image inspect "$prev_image_id" >/dev/null 2>&1; then
- echo "Removing previous image $prev_image_id"
- docker image remove "$prev_image_id"
- fi
-}
-trap cleanup EXIT
+# Track previously built image for optional cleanup after a successful build only.
+prev_image_id=""
+if [[ -f "$iid_file" ]]; then
+ prev_image_id=$(cat "$iid_file")
+fiThen, add this snippet immediately after the build command invocation to handle cleanup safely:
# Post‑build: if a different image was produced, remove the previous one to reduce clutter.
if [[ -n "$prev_image_id" ]]; then
if [[ -f "$iid_file" ]]; then
new_image_id=$(cat "$iid_file" || true)
if [[ -n "$new_image_id" && "$new_image_id" != "$prev_image_id" ]]; then
if docker image inspect "$prev_image_id" >/dev/null 2>&1; then
echo "Removing previous image $prev_image_id"
docker image remove "$prev_image_id"
fi
fi
fi
fiThere was a problem hiding this comment.
i'm not sure how your proposal is cleaner. it also has so many nested-ifs, can you revisit?
There was a problem hiding this comment.
@junhaoliao You're absolutely right - let me provide a much cleaner solution. Here's a simplified approach that eliminates the trap and reduces nesting:
# Track previous image for cleanup after successful build
prev_image_id=""
if [[ -f "$iid_file" ]]; then
prev_image_id=$(cat "$iid_file")
fiThen after the build command, add this simple cleanup:
# Clean up previous image if build produced a different one
if [[ -n "$prev_image_id" && -f "$iid_file" ]]; then
new_image_id=$(cat "$iid_file")
if [[ "$new_image_id" != "$prev_image_id" ]] && docker image inspect "$prev_image_id" &>/dev/null; then
echo "Removing previous image $prev_image_id"
docker image remove "$prev_image_id"
fi
fiThis approach:
- Removes the problematic trap entirely
- Uses a single conditional with logical AND operators instead of nested ifs
- Only runs cleanup after successful build
- Safely handles the case where image IDs are identical
- Is much more readable and maintainable
The key insight is combining conditions with && operators and using early evaluation to avoid deep nesting.
🧠 Learnings used
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: junhaoliao
PR: y-scope/clp#1335
File: taskfiles/docker-images.yaml:15-15
Timestamp: 2025-09-25T19:26:32.415Z
Learning: In the CLP project's Taskfile, the `:package` task creates the G_PACKAGE_BUILD_DIR directory structure, so any task that depends on `:package` (like `docker-images:package`) can safely assume this directory exists without needing additional mkdir commands.
There was a problem hiding this comment.
i can't be totally sure if the benefits are clear to me, do you just hate the trap in the original code?
There was a problem hiding this comment.
@junhaoliao No, it's not about hating trap - traps are useful tools! The issue is specifically about the timing and conditions when the cleanup runs in this context.
Here are the concrete technical problems with the current trap approach:
-
Failed builds shouldn't delete the previous working image: If the Docker build fails (network issues, syntax errors, etc.), the trap still runs and removes the previous image, potentially leaving you with no working image at all.
-
Reproducible builds create a self-deletion scenario: If your build is reproducible (same inputs → same image hash), the new
image.idwill contain the same ID as the previous one. The trap will then delete the image you just "built" (which is actually the same image). -
Signal interruption edge case: If someone Ctrl+C's during the build, the trap runs and deletes the previous image, even though the build never completed.
The post-build approach I suggested only runs cleanup after a successful build and only if the image actually changed. This makes the behavior more predictable and safer.
Both approaches work fine in the happy path, but the trap version has these edge cases that could surprise users. It's about robustness rather than style preference.
If you prefer keeping the trap for simplicity, that's totally reasonable too - these edge cases might be rare enough in your workflow that the current approach is perfectly adequate.
🧠 Learnings used
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: junhaoliao
PR: y-scope/clp#1335
File: taskfiles/docker-images.yaml:15-15
Timestamp: 2025-09-25T19:26:32.415Z
Learning: In the CLP project's Taskfile, the `:package` task creates the G_PACKAGE_BUILD_DIR directory structure, so any task that depends on `:package` (like `docker-images:package`) can safely assume this directory exists without needing additional mkdir commands.
There was a problem hiding this comment.
i can't be sure if you understand the code correctly. will let @kirkrodrigues decide
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
I think the rabbit is correct.
- If the user kills the script before it completes, the existing image will be deleted. Is that intentional?
- If the current and previous image are identical, their IDs will be identical (since the ID is a sha256sum of the image), so on exit, the image will be deleted.
- This doesn't currently happen since our builds are not exactly reproducible yet, but that's something we should work towards.
| build_cmd=( | ||
| docker build | ||
| --tag "clp-package:dev" | ||
| --iidfile "$iid_file" | ||
| --tag "clp-package:dev-${USER}-$(date +%s)" | ||
| "$repo_root" | ||
| --file "${script_dir}/Dockerfile" | ||
| ) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Confirm intended builder: buildx vs classic build.
PR text mentions buildx, but this uses docker build. If buildx is intended (for multi-arch or consistent --iidfile support), switch to docker buildx build accordingly.
🤖 Prompt for AI Agents
In tools/docker-images/clp-package/build.sh around lines 23 to 29, the script
uses docker build but the PR mentions buildx; confirm and, if buildx is
intended, replace the command with docker buildx build (and add appropriate
flags such as --platform or --load/push as needed) so --iidfile behavior and
multi-arch builds are supported; ensure the script verifies buildx is available
and a builder exists (or falls back to docker build) and update any related
flags/options to be compatible with buildx.
There was a problem hiding this comment.
docker build is an alias of docker buildx build which uses the BuildKit as the builder. The switch happened in https://docs.docker.com/engine/release-notes/23.0/#2300
There was a problem hiding this comment.
@junhaoliao Thank you for the clarification and the reference! You're absolutely right - since Docker Engine 23.0, docker build automatically uses BuildKit and is indeed an alias for docker buildx build. This means the current implementation is correct and the --iidfile flag will work as expected.
I appreciate you providing the documentation link to confirm this behavior change. The script is appropriately using docker build and no modifications are needed.
🐰💡
✏️ Learnings added
Learnt from: junhaoliao
PR: y-scope/clp#1335
File: tools/docker-images/clp-package/build.sh:23-29
Timestamp: 2025-09-25T19:36:35.345Z
Learning: Since Docker Engine 23.0, `docker build` is an alias for `docker buildx build` and automatically uses BuildKit as the builder. The `--iidfile` flag and other BuildKit features work correctly with the standard `docker build` command, so there's no need to explicitly use `docker buildx build` in build scripts unless specific buildx-only features are required.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
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: junhaoliao
PR: y-scope/clp#1335
File: taskfiles/docker-images.yaml:15-15
Timestamp: 2025-09-25T19:26:32.415Z
Learning: In the CLP project's Taskfile, the `:package` task creates the G_PACKAGE_BUILD_DIR directory structure, so any task that depends on `:package` (like `docker-images:package`) can safely assume this directory exists without needing additional mkdir commands.
There was a problem hiding this comment.
@kirkrodrigues i realise we don't really mention any docker version in our dev docs (we do mention docker in the userquick start guide, though we don't say the version still). if that's not intentionally left out, maybe we should specify the version in our dev docs as well?
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
Yeah, we should specify the version we require. I just don't know what version that is right now.
There was a problem hiding this comment.
it might be time consuming to figure out the exact version for the feature / syntax we use. maybe we can just take the latest version and put that into the docs? at least we can be sure that's a version we have validated
There was a problem hiding this comment.
Generally, we support up to 2 years of backwards compatibility with OSes. Perhaps we can do the same for Docker?
There was a problem hiding this comment.
https://containerd.io/releases/ 's LTS version is now 1.7 and it appears that the last retrievable version is 1.7.18 which was released on July 3, 2024 - https://download.docker.com/linux/ubuntu/dists/jammy/pool/stable/amd64/
i installed these versions on my machine and tested things fine:
sudo apt-get install -y \
docker-ce=5:27.0.3-1~ubuntu.22.04~jammy \
docker-ce-cli=5:27.0.3-1~ubuntu.22.04~jammy \
containerd.io=1.7.18-1 \
docker-buildx-plugin=0.15.1-1~ubuntu.22.04~jammy \
docker-compose-plugin=2.28.1-1~ubuntu.22.04~jammy
|
@kirkrodrigues if you think it makes sense to, in the same PR i can also remove clp/components/clp-py-utils/clp_py_utils/clp_config.py Lines 932 to 947 in 173755f we also discussed about cleaning up the image build process (that we don't have to copy everything into clp-package first; instead, we should specify the |
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)
tools/docker-images/clp-package/build.sh (1)
31-37: Guard origin lookup; current form can exit under set -e when no ‘origin’ exists.The command substitution for remote get-url origin will fail (and exit the script) if ‘origin’ is missing. Make it non-fatal.
Based on learnings
- --label "org.opencontainers.image.source=$(git -C "$script_dir" remote get-url origin)" + --label "org.opencontainers.image.source=$(git -C "$script_dir" remote get-url origin 2>/dev/null || true)"Optional follow-up: if you want to avoid an empty label entirely, compute variables first and append labels only when non-empty (while keeping the pattern consistent with other build.sh scripts).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
taskfiles/docker-images.yaml(1 hunks)tools/docker-images/clp-package/build.sh(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 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:
tools/docker-images/clp-package/build.sh
📚 Learning: 2025-09-25T19:36:35.345Z
Learnt from: junhaoliao
PR: y-scope/clp#1335
File: tools/docker-images/clp-package/build.sh:23-29
Timestamp: 2025-09-25T19:36:35.345Z
Learning: Since Docker Engine 23.0, `docker build` is an alias for `docker buildx build` and automatically uses BuildKit as the builder. The `--iidfile` flag and other BuildKit features work correctly with the standard `docker build` command, so there's no need to explicitly use `docker buildx build` in build scripts unless specific buildx-only features are required.
Applied to files:
tools/docker-images/clp-package/build.sh
📚 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:
tools/docker-images/clp-package/build.sh
📚 Learning: 2025-09-25T19:26:32.415Z
Learnt from: junhaoliao
PR: y-scope/clp#1335
File: taskfiles/docker-images.yaml:15-15
Timestamp: 2025-09-25T19:26:32.415Z
Learning: In the CLP project's Taskfile, the `:package` task creates the G_PACKAGE_BUILD_DIR directory structure, so any task that depends on `:package` (like `docker-images:package`) can safely assume this directory exists without needing additional mkdir commands.
Applied to files:
taskfiles/docker-images.yaml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (15)
- GitHub Check: musllinux_1_2-x86_64-static-linked-bins
- GitHub Check: musllinux_1_2-x86_64-dynamic-linked-bins
- GitHub Check: package-image
- GitHub Check: ubuntu-jammy-lint
- GitHub Check: ubuntu-jammy-dynamic-linked-bins
- GitHub Check: ubuntu-jammy-static-linked-bins
- GitHub Check: centos-stream-9-dynamic-linked-bins
- GitHub Check: manylinux_2_28-x86_64-dynamic-linked-bins
- GitHub Check: centos-stream-9-static-linked-bins
- GitHub Check: manylinux_2_28-x86_64-static-linked-bins
- GitHub Check: lint-check (macos-15)
- GitHub Check: build-macos (macos-15, true)
- GitHub Check: build-macos (macos-14, true)
- GitHub Check: build-macos (macos-14, false)
- GitHub Check: build-macos (macos-15, false)
🔇 Additional comments (3)
tools/docker-images/clp-package/build.sh (2)
8-9: Confirm filename aligns with consumers and docs.PR text mentions build/clp-package/image.id, while this writes build/clp-package-image.id. If downstream expects image.id, copy/rename accordingly in the packaging step.
As a minimal change, keep this path but copy to package as image.id (see suggested change in taskfiles/docker-images.yaml).
10-21: Avoid trap-based deletion; can remove the just-built or last good image on failure.The cleanup trap runs on any exit. If the build fails, you delete the last good image. If the new ID equals the previous, you delete the just-built image and leave iid_file pointing to a non-existent image. Prefer post-build conditional cleanup.
Apply this diff to drop the trap:
-# Remove the previous image to save space, but only after the build so its contents can be reused. -prev_image_id="" -if [[ -f "$iid_file" ]]; then - prev_image_id=$(cat "$iid_file") -fi -cleanup() { - if [[ -n "$prev_image_id" ]] && docker image inspect "$prev_image_id" >/dev/null 2>&1; then - echo "Removing previous image $prev_image_id" - docker image remove "$prev_image_id" - fi -} -trap cleanup EXITThen add this post-build cleanup immediately after the build command:
# Post-build cleanup: remove the previous image only if a different image was produced. if [[ -f "$iid_file" ]]; then new_image_id="$(cat "$iid_file" || true)" if [[ -n "${prev_image_id:-}" && -n "$new_image_id" && "$new_image_id" != "$prev_image_id" ]]; then if docker image inspect "$prev_image_id" >/dev/null 2>&1; then echo "Removing previous image $prev_image_id" docker image rm "$prev_image_id" fi fi fitaskfiles/docker-images.yaml (1)
15-15: Copy the packaged ID as image.id
In taskfiles/docker-images.yaml (line 15), update the rsync target to…/image.idto match docs/PR text, and verify any downstream consumers ofclp-package-image.idare updated.
| set -o pipefail | ||
|
|
||
| script_dir="$( cd "$( dirname "${BASH_SOURCE[0]}" )" &> /dev/null && pwd )" | ||
| iid_file="${script_dir}/image.id" |
There was a problem hiding this comment.
I don't think it's necessary right now.
| docker build | ||
| --tag "clp-package:dev" | ||
| --iidfile "$iid_file" | ||
| --tag "clp-package:dev-${USER}-$(date +%s)" |
There was a problem hiding this comment.
If we tag with the current timestamp, then identical image builds will result in two different tags pointing at the same image. Besides the clutter, that also means that the image can't be removed without a force flag. Instead, can we replace the timestamp with the image ID? I know that will make it harder to tell what's a newer image based on the tag, but that information is available from Docker since it keeps track of the date an image was built.
There was a problem hiding this comment.
If we tag with the current timestamp, then identical image builds will result in two different tags pointing at the same image. Besides the clutter, that also means that the image can't be removed without a force flag.
not sure if this behaviour is possible. if i understand correctly, every time we run docker build a new image is built (we can tell by the different sha256 image id)
There was a problem hiding this comment.
maybe i understood it wrong. do you mind giving more steps on how this can be reproduced?
There was a problem hiding this comment.
my initial implementation of the unique id part was done with uuidgen | head -c 4 though i think the chance of collision can be high.
later i replaced that with the timestamp because it's less likely a user can build multiple build in a second. (can probably change it to nanoseconds
There was a problem hiding this comment.
if i understand correctly, every time we run docker build a new image is built (we can tell by the different sha256 image id)
That's because task package's output is not 100% reproducible---i.e., some files will change slightly. If you run the build.sh script alone twice, it should generate an image with the same ID.
There was a problem hiding this comment.
actually i do mean running the build.sh gives a different sha256 id every time:
junhao@ASUS-X870E:~/workspace/2-clp/tools/docker-images/clp-package$ ./build.sh && echo '---' && cat /home/junhao/workspace/2-clp/build/clp-package-image.id && echo
[+] Building 0.9s (12/12) FINISHED docker:default
=> [internal] load build definition from Dockerfile 0.0s
=> => transferring dockerfile: 585B 0.0s
=> [internal] load metadata for docker.io/library/ubuntu:jammy 0.1s
=> [internal] load .dockerignore 0.0s
=> => transferring context: 2B 0.0s
=> [base 1/6] FROM docker.io/library/ubuntu:jammy@sha256:4e0171b9275e12d375863f2b3ae9ce00a4c53ddda176bd55868df97ac6f21a6e 0.0s
=> => resolve docker.io/library/ubuntu:jammy@sha256:4e0171b9275e12d375863f2b3ae9ce00a4c53ddda176bd55868df97ac6f21a6e 0.0s
=> [internal] load build context 0.6s
=> => transferring context: 2.24MB 0.6s
=> CACHED [base 2/6] WORKDIR /root 0.0s
=> CACHED [base 3/6] COPY ./tools/docker-images/clp-package/setup-scripts ./setup-scripts 0.0s
=> CACHED [base 4/6] RUN ./setup-scripts/install-prebuilt-packages.sh && rm -rf ./setup-scripts/ 0.0s
=> CACHED [base 5/6] RUN apt-get clean && rm -rf /var/lib/apt/lists/* /tmp/* /var/tmp/* 0.0s
=> CACHED [base 6/6] COPY ./build/clp-package /opt/clp 0.0s
=> CACHED [stage-1 1/1] COPY --from=base / / 0.0s
=> exporting to image 0.1s
=> => exporting layers 0.0s
=> => exporting manifest sha256:eb17ac6fd6ea60ce4eb1839a54f0bbc939b0d3dad201c2d9547d0b0ffd7fe2bf 0.0s
=> => exporting config sha256:41a6f9aae1dc947a3deeafcec0e82ab2891f2cb55ff0f9a3a0492cd5defd0739 0.0s
=> => exporting attestation manifest sha256:5cbe3ae1c541bb2237dbbb721e8635c31449bf9ef334bcbb691e98d4b882a168 0.0s
=> => exporting manifest list sha256:31e3696ede8077959e0dfe249f7be8caa42c12d2d351e0a93ba88f080b883e36 0.0s
=> => naming to docker.io/library/clp-package:dev-junhao-1759114001 0.0s
=> => unpacking to docker.io/library/clp-package:dev-junhao-1759114001 0.0s
Removing previous image sha256:754d92f3198297d74cae9ef9698f2d5f046392d6408158c4860ce820c7b08b17.
Untagged: clp-package:dev-junhao-1759113990
Deleted: sha256:754d92f3198297d74cae9ef9698f2d5f046392d6408158c4860ce820c7b08b17
---
sha256:31e3696ede8077959e0dfe249f7be8caa42c12d2d351e0a93ba88f080b883e36
junhao@ASUS-X870E:~/workspace/2-clp/tools/docker-images/clp-package$ ./build.sh && echo '---' && cat /home/junhao/workspace/2-clp/build/clp-package-image.id && echo
[+] Building 0.9s (12/12) FINISHED docker:default
=> [internal] load build definition from Dockerfile 0.0s
=> => transferring dockerfile: 585B 0.0s
=> [internal] load metadata for docker.io/library/ubuntu:jammy 0.1s
=> [internal] load .dockerignore 0.0s
=> => transferring context: 2B 0.0s
=> [base 1/6] FROM docker.io/library/ubuntu:jammy@sha256:4e0171b9275e12d375863f2b3ae9ce00a4c53ddda176bd55868df97ac6f21a6e 0.0s
=> => resolve docker.io/library/ubuntu:jammy@sha256:4e0171b9275e12d375863f2b3ae9ce00a4c53ddda176bd55868df97ac6f21a6e 0.0s
=> [internal] load build context 0.6s
=> => transferring context: 2.24MB 0.6s
=> CACHED [base 2/6] WORKDIR /root 0.0s
=> CACHED [base 3/6] COPY ./tools/docker-images/clp-package/setup-scripts ./setup-scripts 0.0s
=> CACHED [base 4/6] RUN ./setup-scripts/install-prebuilt-packages.sh && rm -rf ./setup-scripts/ 0.0s
=> CACHED [base 5/6] RUN apt-get clean && rm -rf /var/lib/apt/lists/* /tmp/* /var/tmp/* 0.0s
=> CACHED [base 6/6] COPY ./build/clp-package /opt/clp 0.0s
=> CACHED [stage-1 1/1] COPY --from=base / / 0.0s
=> exporting to image 0.1s
=> => exporting layers 0.0s
=> => exporting manifest sha256:eb17ac6fd6ea60ce4eb1839a54f0bbc939b0d3dad201c2d9547d0b0ffd7fe2bf 0.0s
=> => exporting config sha256:41a6f9aae1dc947a3deeafcec0e82ab2891f2cb55ff0f9a3a0492cd5defd0739 0.0s
=> => exporting attestation manifest sha256:262a3a97f6ac6ebe1264878a49027dd9382532eda8aa5b8386a8496acb684c5c 0.0s
=> => exporting manifest list sha256:fda46ace138ddb9700a1dbdaf26bbde1577dd1e15f7159d05858e9c6a8d7b245 0.0s
=> => naming to docker.io/library/clp-package:dev-junhao-1759114003 0.0s
=> => unpacking to docker.io/library/clp-package:dev-junhao-1759114003 0.0s
Removing previous image sha256:31e3696ede8077959e0dfe249f7be8caa42c12d2d351e0a93ba88f080b883e36.
Untagged: clp-package:dev-junhao-1759114001
Deleted: sha256:31e3696ede8077959e0dfe249f7be8caa42c12d2d351e0a93ba88f080b883e36
---
sha256:fda46ace138ddb9700a1dbdaf26bbde1577dd1e15f7159d05858e9c6a8d7b245
junhao@ASUS-X870E:~/workspace/2-clp/tools/docker-images/clp-package$ ./build.sh && echo '---' && cat /home/junhao/workspace/2-clp/build/clp-package-image.id && echo
[+] Building 0.9s (12/12) FINISHED docker:default
=> [internal] load build definition from Dockerfile 0.0s
=> => transferring dockerfile: 585B 0.0s
=> [internal] load metadata for docker.io/library/ubuntu:jammy 0.1s
=> [internal] load .dockerignore 0.0s
=> => transferring context: 2B 0.0s
=> [base 1/6] FROM docker.io/library/ubuntu:jammy@sha256:4e0171b9275e12d375863f2b3ae9ce00a4c53ddda176bd55868df97ac6f21a6e 0.0s
=> => resolve docker.io/library/ubuntu:jammy@sha256:4e0171b9275e12d375863f2b3ae9ce00a4c53ddda176bd55868df97ac6f21a6e 0.0s
=> [internal] load build context 0.6s
=> => transferring context: 2.24MB 0.6s
=> CACHED [base 2/6] WORKDIR /root 0.0s
=> CACHED [base 3/6] COPY ./tools/docker-images/clp-package/setup-scripts ./setup-scripts 0.0s
=> CACHED [base 4/6] RUN ./setup-scripts/install-prebuilt-packages.sh && rm -rf ./setup-scripts/ 0.0s
=> CACHED [base 5/6] RUN apt-get clean && rm -rf /var/lib/apt/lists/* /tmp/* /var/tmp/* 0.0s
=> CACHED [base 6/6] COPY ./build/clp-package /opt/clp 0.0s
=> CACHED [stage-1 1/1] COPY --from=base / / 0.0s
=> exporting to image 0.1s
=> => exporting layers 0.0s
=> => exporting manifest sha256:eb17ac6fd6ea60ce4eb1839a54f0bbc939b0d3dad201c2d9547d0b0ffd7fe2bf 0.0s
=> => exporting config sha256:41a6f9aae1dc947a3deeafcec0e82ab2891f2cb55ff0f9a3a0492cd5defd0739 0.0s
=> => exporting attestation manifest sha256:e1d31873b43c7ec6bcdeb4112671c6daf21b02b4019c1d40b6de746717c6d816 0.0s
=> => exporting manifest list sha256:b3a0bfc056f542a868cd1845256543dd585864fe0ca8d934f6a398bc07b52151 0.0s
=> => naming to docker.io/library/clp-package:dev-junhao-1759114005 0.0s
=> => unpacking to docker.io/library/clp-package:dev-junhao-1759114005 0.0s
Removing previous image sha256:fda46ace138ddb9700a1dbdaf26bbde1577dd1e15f7159d05858e9c6a8d7b245.
Untagged: clp-package:dev-junhao-1759114003
Deleted: sha256:fda46ace138ddb9700a1dbdaf26bbde1577dd1e15f7159d05858e9c6a8d7b245
---
sha256:b3a0bfc056f542a868cd1845256543dd585864fe0ca8d934f6a398bc07b52151There was a problem hiding this comment.
discussed offline - it appears that the above script generates a different hash every time only when Docker Desktop Windows + WSL is used. with a native Docker Engine installed inside WSL, the hash is consistent across docker build if the content inside build/clp-package is unchanged
a few facts:
- if we run
docker buildwithout a--tagparameter, in the build logs it would appear that we are building an image with tagmoby-dangling. - provided an id of an image, it's possible to add tags to the image afterwards
- there's no atomic way to build an image, get the id of the built image, and add tag to the image with the id included in the tag name
how about this:
- we read the image id file from
build/clp-package-image.idinto$prev_image_id, as we currently do. - we call
mktempto get a temporary file path, assigned to$temp_iid_file docker build --iidfile "$temp_iid_file" "$repo_root" --file "${script_dir}/Dockerfile"(note we don't supply tags here. the build logs will show we're building for projectmoby-danglingbut i guess we can live with that, so long the image is eventually tagged)- we
catthe$temp_iid_fileand store the value at$new_iid. fallback to an empty string ifcatfails (the failure can possibly be caused by a failed docker build?) - if the
$new_iidis non empty, truncate the$new_iidto 4 characters and tag the new image withclp-package:dev-${USER}-${short_id} - before the script exits, the
trap xxx EXITfunction is called. inside:- if
$prev_image_iddiffers from$new_iid(note: this is always true if$new_iidis empty), wedocker image remove $prev_image_id.
- if
There was a problem hiding this comment.
At step (3), it's possible two concurrent builds on the system create an image with tag moby-dangling, right? I think that will still succeed, but then after step (5) wouldn't we end up with two tags for the same image? I guess we need a step to remove the moby-dangling tag for the particular image we created.
There was a problem hiding this comment.
let me implement the proposal and monkey test a bit to see if there are issues. i could be wrong about how docker image remove works (or confused by the different behaviours in Docker Desktop)
There was a problem hiding this comment.
- it appears that the
moby-danglingisn't a real project / tag --- Runningdocker image lsdoesn't show themoby-danglingon the listed image, whether we manfully tag the image with our own tag or not. - the
moby-danglingproject only shows up in the build logs with Docker Desktop.
i have tested the script that's been pushed and didn't find any awkward issue, either with Docker Desktop or native Docker.
| # Remove the previous image after the build to allow layer reuse. | ||
| prev_image_id="" | ||
| if [[ -f "$iid_file" ]]; then | ||
| prev_image_id=$(cat "$iid_file") | ||
| fi | ||
| cleanup() { | ||
| if [[ -n "$prev_image_id" ]] && docker image inspect "$prev_image_id" >/dev/null 2>&1; then | ||
| echo "Removing previous image $prev_image_id" | ||
| docker image remove "$prev_image_id" | ||
| fi | ||
| } | ||
| trap cleanup EXIT | ||
|
|
There was a problem hiding this comment.
I think the rabbit is correct.
- If the user kills the script before it completes, the existing image will be deleted. Is that intentional?
- If the current and previous image are identical, their IDs will be identical (since the ID is a sha256sum of the image), so on exit, the image will be deleted.
- This doesn't currently happen since our builds are not exactly reproducible yet, but that's something we should work towards.
| build_cmd=( | ||
| docker build | ||
| --tag "clp-package:dev" | ||
| --iidfile "$iid_file" | ||
| --tag "clp-package:dev-${USER}-$(date +%s)" | ||
| "$repo_root" | ||
| --file "${script_dir}/Dockerfile" | ||
| ) |
There was a problem hiding this comment.
Yeah, we should specify the version we require. I just don't know what version that is right now.
| if [[ -f "$iid_file" ]]; then | ||
| prev_image_id=$(cat "$iid_file") | ||
| fi | ||
| cleanup() { |
There was a problem hiding this comment.
How about remove_prev_image?
| if [[ -f "$iid_file" ]]; then | ||
| prev_image_id=$(cat "$iid_file") | ||
| fi | ||
| cleanup() { |
There was a problem hiding this comment.
Can we move this function to the top of the file, just after the set commands? That makes it easier to read the control flow imo.
Let's do it in another PR.
Yeah, let's do it in another PR. |
yes, i think we want to invalidate the older image as soon as one requests to build a new one. otherwise, if the build fails and people are unaware of the failure status, they might still use the older image, thinking that's the latest |
…s from code review Co-authored-by: kirkrodrigues <[email protected]>
Co-authored-by: kirkrodrigues <[email protected]>
| docker build | ||
| --tag "clp-package:dev" | ||
| --iidfile "$iid_file" | ||
| --tag "clp-package:dev-${USER}-$(date +%s)" |
There was a problem hiding this comment.
if i understand correctly, every time we run docker build a new image is built (we can tell by the different sha256 image id)
That's because task package's output is not 100% reproducible---i.e., some files will change slightly. If you run the build.sh script alone twice, it should generate an image with the same ID.
| build_cmd=( | ||
| docker build | ||
| --tag "clp-package:dev" | ||
| --iidfile "$iid_file" | ||
| --tag "clp-package:dev-${USER}-$(date +%s)" | ||
| "$repo_root" | ||
| --file "${script_dir}/Dockerfile" | ||
| ) |
There was a problem hiding this comment.
Generally, we support up to 2 years of backwards compatibility with OSes. Perhaps we can do the same for Docker?
Fair enough. |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
tools/docker-images/clp-package/build.sh(2 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
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.
📚 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:
tools/docker-images/clp-package/build.sh
📚 Learning: 2025-09-25T19:36:35.382Z
Learnt from: junhaoliao
PR: y-scope/clp#1335
File: tools/docker-images/clp-package/build.sh:23-29
Timestamp: 2025-09-25T19:36:35.382Z
Learning: Since Docker Engine 23.0, `docker build` is an alias for `docker buildx build` and automatically uses BuildKit as the builder. The `--iidfile` flag and other BuildKit features work correctly with the standard `docker build` command, so there's no need to explicitly use `docker buildx build` in build scripts unless specific buildx-only features are required.
Applied to files:
tools/docker-images/clp-package/build.sh
📚 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:
tools/docker-images/clp-package/build.sh
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: package-image
- GitHub Check: lint-check (ubuntu-24.04)
- GitHub Check: build (macos-15)
| remove_prev_image() { | ||
| if [[ -n "$prev_image_id" && "$prev_image_id" != "$new_image_id" ]]; then | ||
| if docker image inspect "$prev_image_id" >/dev/null 2>&1; then | ||
| echo "Removing previous image $prev_image_id." | ||
| docker image remove "$prev_image_id" | ||
| fi | ||
| fi | ||
| rm -f "$temp_iid_file" | ||
| } |
There was a problem hiding this comment.
Prevent deleting the last good image on failed builds.
Line 19 runs during every EXIT. When the build fails, set -e aborts before new_image_id is populated, leaving it empty. The [[ -n "$prev_image_id" && "$prev_image_id" != "$new_image_id" ]] check still passes ("" ≠ previous), so we delete the only working image—exactly when the user needs it for rollback. Please gate the cleanup on new_image_id being set.
remove_prev_image() {
- if [[ -n "$prev_image_id" && "$prev_image_id" != "$new_image_id" ]]; then
- if docker image inspect "$prev_image_id" >/dev/null 2>&1; then
- echo "Removing previous image $prev_image_id."
- docker image remove "$prev_image_id"
- fi
- fi
- rm -f "$temp_iid_file"
+ rm -f "$temp_iid_file"
+
+ if [[ -z "$new_image_id" ]]; then
+ return
+ fi
+
+ if [[ -n "$prev_image_id" && "$prev_image_id" != "$new_image_id" ]]; then
+ if docker image inspect "$prev_image_id" >/dev/null 2>&1; then
+ echo "Removing previous image $prev_image_id."
+ docker image remove "$prev_image_id"
+ fi
+ fi
}📝 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.
| remove_prev_image() { | |
| if [[ -n "$prev_image_id" && "$prev_image_id" != "$new_image_id" ]]; then | |
| if docker image inspect "$prev_image_id" >/dev/null 2>&1; then | |
| echo "Removing previous image $prev_image_id." | |
| docker image remove "$prev_image_id" | |
| fi | |
| fi | |
| rm -f "$temp_iid_file" | |
| } | |
| remove_prev_image() { | |
| # Always clean up the temporary IID file first | |
| rm -f "$temp_iid_file" | |
| # If the new image ID was never set (build failed), do not remove the previous image | |
| if [[ -z "$new_image_id" ]]; then | |
| return | |
| fi | |
| # Only remove the previous image when a different one was successfully built | |
| if [[ -n "$prev_image_id" && "$prev_image_id" != "$new_image_id" ]]; then | |
| if docker image inspect "$prev_image_id" >/dev/null 2>&1; then | |
| echo "Removing previous image $prev_image_id." | |
| docker image remove "$prev_image_id" | |
| fi | |
| fi | |
| } |
🤖 Prompt for AI Agents
In tools/docker-images/clp-package/build.sh around lines 18 to 26, the cleanup
function can delete the last good image when a build fails because new_image_id
may be empty; update the conditional to require new_image_id is non-empty (e.g.
[[ -n "$prev_image_id" && -n "$new_image_id" && "$prev_image_id" !=
"$new_image_id" ]]) so removal only happens when a new image ID was successfully
set, and keep the rm -f "$temp_iid_file" at the end.
…ove_prev_image_and_temp_file to the top of the script.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
docs/src/dev-docs/building-package.md(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-25T19:26:32.436Z
Learnt from: junhaoliao
PR: y-scope/clp#1335
File: taskfiles/docker-images.yaml:15-15
Timestamp: 2025-09-25T19:26:32.436Z
Learning: In the CLP project's Taskfile, the `:package` task creates the G_PACKAGE_BUILD_DIR directory structure, so any task that depends on `:package` (like `docker-images:package`) can safely assume this directory exists without needing additional mkdir commands.
Applied to files:
docs/src/dev-docs/building-package.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). (2)
- GitHub Check: package-image
- GitHub Check: lint-check (macos-15)
🔇 Additional comments (2)
docs/src/dev-docs/building-package.md (2)
88-88: Link Validated – No Changes Required The Docker installation link returns a successful response (HTTP 200) and directs users to the official Docker Engine installation instructions.
13-17: Verify Docker version requirements and add rationale. The--iidfileflag is used in the build script; confirm the minimum Docker CLI and Buildx plugin versions needed to support this option, update version constraints if necessary, and add a brief note explaining why these specific versions are required.
| task docker-images:package | ||
| ``` | ||
|
|
||
| This will create a Docker image named `clp-package:dev`. |
There was a problem hiding this comment.
Update documentation to reflect the new image tagging scheme.
According to the PR summary, the build process now generates unique identifiers to avoid overwriting older builds on shared machines. The AI summary mentions tagging as clp-package:dev-${USER}-${short_id}, but this line still states the image will be named clp-package:dev.
Please update this documentation to:
- Explain that each build generates a unique image identifier
- Clarify how users can reference the built image (e.g., via the
image.idfile mentioned in the PR summary) - Optionally, mention the tagging convention if it's relevant for users
🤖 Prompt for AI Agents
In docs/src/dev-docs/building-package.md around line 86, the text still claims
the Docker image will be named `clp-package:dev` but the build now generates
unique image identifiers; update this line to state that each build produces a
uniquely tagged image to avoid overwrites, note where to find the exact image
reference (point to the generated image.id file created by the build), and
optionally show the tagging convention used (e.g.,
clp-package:dev-${USER}-${short_id}) so users know how to reference the image or
read image.id for the precise tag.
kirkrodrigues
left a comment
kirkrodrigues
left a comment
There was a problem hiding this comment.
For the PR title, how about:
feat(docker): Generate unique tag for each clp-package dev image build to avoid conflicts on shared hosts.
junhaoliao
changed the title
This is added as part of #1357 , which add the mechanism to select the image build by the unique id generated in this current PR #1335 .
This is strictly optimization and isn't blocking the Docker Compose migration, so this is of lower priority at the moment. @coderabbitai create an issue to track the request so we won't forget |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 69 @junhaoliao I've created issue #1358 to track the Docker build process optimization. The issue has been assigned to you and includes the context about optimizing the clp-package build to avoid intermediate copying by using |
|
|
||
| if docker image inspect "$prev_image_id" >/dev/null 2>&1; then | ||
| echo "Removing previous image $prev_image_id." | ||
| docker image remove "$prev_image_id" |
There was a problem hiding this comment.
The image might fail to be removed because some running container might still be referencing it, which causes a non-zero exit code in the script.
When the image fails to be removed, we should either exit gracefully (with 0) with a warning message printed, or the caller should gracefully handle the non-exit code. Currently, the caller (the docker-images.yaml taskfile) doesn't handle this --- when the script fails, any commands following the script call (e.g., to copy the unique image id file into build/clp-package) would fail to execute.
@coderabbitai create an issue to track this
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Create GitHub issue to track the image removal failure problem
gh issue create \
--title "Handle docker image removal failures gracefully in clp-package build script" \
--body "## Problem
In \`tools/docker-images/clp-package/build.sh\`, the \`remove_temp_file_and_prev_image\` function (called via EXIT trap) attempts to remove the previous image using \`docker image remove\`. This can fail with a non-zero exit code if a running container is still referencing the image.
Since the script uses \`set -eu\`, this failure causes the script to exit with an error code, which in turn causes subsequent commands in the caller (\`docker-images.yaml\` taskfile) to fail execution (e.g., copying the unique image ID file into \`build/clp-package\`).
## Proposed Solutions
Either:
1. Exit gracefully (with exit code 0) and print a warning message when image removal fails, or
2. Have the caller (taskfile) gracefully handle the non-zero exit code
## Context
Reported by @junhaoliao in PR #1335:
https://github.com/y-scope/clp/pull/1335#discussion_r2400399192
## Affected Files
- \`tools/docker-images/clp-package/build.sh\`
- \`docker-images.yaml\` (taskfile caller)" \
--assignee junhaoliao
Length of output: 42
@junhaoliao I've created issue #1370 to track this: #1370
The issue documents the problem where docker image remove can fail if a container is still referencing the image, causing the script to exit with an error due to set -eu. It includes both proposed solutions (graceful exit with warning or caller handling the error) and references this PR discussion.
| new_image_id="$(<"$temp_iid_file")" | ||
| echo "$new_image_id" > "$iid_file" | ||
|
|
||
| user="${USER:-$(id -un 2>/dev/null || whoami 2>/dev/null || echo unknown)}" |
There was a problem hiding this comment.
There's one edge case we missed to handle -
When the username cannot be found on the system (e.g., the username is not specified in /etc/passwd), the user id would still be printed by id -un command. However, since the id -un command exits with non-zero code, the other command would continue to run, which may cause line breaks to show up in the middle of the user string. e.g.,
id -un: Prints `123\n`; Exits with non-0
whoami: Prints nothing; Exits with non-9
echo unknown: Prints "unknown"
then `user` becomes: `123\nunknown`
@coderabbitai create an issue to track this
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Create GitHub issue to track the username fallback edge case
gh issue create \
--repo y-scope/clp \
--title "Edge case in username fallback logic in clp-package build script" \
--body "## Description
An edge case exists in the username fallback logic in \`tools/docker-images/clp-package/build.sh\` at line 51:
\`\`\`bash
user=\"\${USER:-\$(id -un 2>/dev/null || whoami 2>/dev/null || echo unknown)}\"
\`\`\`
When the username cannot be found on the system (e.g., the username is not specified in \`/etc/passwd\`), \`id -un\` may print the numeric user ID followed by a newline but exit with a non-zero code. Due to the \`||\` chaining, subsequent commands (\`whoami\` and \`echo unknown\`) continue to execute, causing their output to be appended, resulting in a multi-line string with line breaks embedded in the \`user\` variable.
## Example scenario
\`\`\`
id -un: Prints 123\\n; Exits with non-0
whoami: Prints nothing; Exits with non-0
echo unknown: Prints \"unknown\"
Result: user becomes \"123\\nunknown\"
\`\`\`
This could lead to invalid Docker image tag names or unexpected behaviour in the build process.
## References
- PR: https://github.com/y-scope/clp/pull/1335
- Comment: https://github.com/y-scope/clp/pull/1335#discussion_r2400404851
- Reported by: @junhaoliao" \
--assignee junhaoliao
Length of output: 42
@junhaoliao I've created issue #1371 to track this edge case: #1371
The issue documents the scenario where the || chain in the username fallback logic can result in a multi-line string when id -un prints a numeric ID but exits with a non-zero code, causing subsequent commands to append their output.
2 participants
Description
Developer of the CLP project may be sharing the same machine or run multiple builds on the machine. Previously, in the clp-package image build script, we always tag the image builds with tag "clp-package:dev", which causes any older build, whether owned by the current user or not, to be deleted.
This PR adds the
--iidfileto thedocker build(buildx) command so that a unique image SHA256 ID is written to the specified location. Then thedocker-image:packagecopies the generatedimage.idfile into the package. In a future PR, the package start script will read thisimage.idfile and use it for CLP service executions.Checklist
breaking change.
Validation performed
This ran successfully:
Summary by CodeRabbit
Chores
Documentation