Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions taskfiles/docker-images.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -12,3 +12,4 @@ tasks:
- ":package"
cmds:
- "./build.sh"
- "rsync -a '{{.G_BUILD_DIR}}/clp-package-image.id' '{{.G_PACKAGE_BUILD_DIR}}'"
Comment thread
junhaoliao marked this conversation as resolved.
Resolved
17 changes: 16 additions & 1 deletion tools/docker-images/clp-package/build.sh
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,25 @@ set -o pipefail

script_dir="$( cd "$( dirname "${BASH_SOURCE[0]}" )" &> /dev/null && pwd )"
repo_root=${script_dir}/../../../
iid_file="${repo_root}/build/clp-package-image.id"

# 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")
Comment thread
junhaoliao marked this conversation as resolved.
Resolved
fi
cleanup() {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about remove_prev_image?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if [[ -n "$prev_image_id" ]] && docker image inspect "$prev_image_id" >/dev/null 2>&1; then
echo "Removing previous image $prev_image_id"
Comment thread
junhaoliao marked this conversation as resolved.
Resolved
docker image remove "$prev_image_id"
fi
}
trap cleanup EXIT

build_cmd=(
docker build
--tag "clp-package:dev"
--iidfile "$iid_file"
--tag "clp-package:dev-${USER}-$(date +%s)"
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Resolved

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe i understood it wrong. do you mind giving more steps on how this can be reproduced?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:b3a0bfc056f542a868cd1845256543dd585864fe0ca8d934f6a398bc07b52151

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. if we run docker build without a --tag parameter, in the build logs it would appear that we are building an image with tag moby-dangling.
  2. provided an id of an image, it's possible to add tags to the image afterwards
  3. 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:

  1. we read the image id file from build/clp-package-image.id into $prev_image_id, as we currently do.
  2. we call mktemp to get a temporary file path, assigned to $temp_iid_file
  3. 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 project moby-dangling but i guess we can live with that, so long the image is eventually tagged)
  4. we cat the $temp_iid_file and store the value at $new_iid. fallback to an empty string if cat fails (the failure can possibly be caused by a failed docker build?)
  5. if the $new_iid is non empty, truncate the $new_iid to 4 characters and tag the new image with clp-package:dev-${USER}-${short_id}
  6. before the script exits, the trap xxx EXIT function is called. inside:
    1. if $prev_image_id differs from $new_iid (note: this is always true if $new_iid is empty), we docker image remove $prev_image_id.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. it appears that the moby-dangling isn't a real project / tag --- Running docker image ls doesn't show the moby-dangling on the listed image, whether we manfully tag the image with our own tag or not.
  2. the moby-dangling project 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.

"$repo_root"
--file "${script_dir}/Dockerfile"
)
Comment on lines 30 to 35

@coderabbitai coderabbitai Bot Sep 25, 2025 •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, we should specify the version we require. I just don't know what version that is right now.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally, we support up to 2 years of backwards compatibility with OSes. Perhaps we can do the same for Docker?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Expand Down
Loading