fix(build): Pin the two unpinned build dependencies (Dockerfile install script, pip install) - #2099
vishalsahare wants to merge 9 commits into
Conversation
vishalsahare
commented
Sep 19, 2026
…ll script, pip install) agntcy#2093 ## Summary - Replaced the unpinned Task Cloudsmith setup script with a direct Task v3.53.1 .deb download. - Added architecture-specific SHA-256 verification for amd64 and arm64 before installation. - Pinned the CI WebSocket test dependency to websockets==16.0. - Added package hashes and enabled pip’s --require-hashes mode. - Added the required AI change-trace entries. ## Validation ```bash cargo fmt --all -- --check cargo check -p agntcy-slim-datapath \ -p agntcy-slim-service \ -p agntcy-slim-bindings ``` ## Type of Change - [x] Bugfix - [ ] New Feature - [ ] Breaking Change - [ ] Refactor - [ ] Documentation - [ ] Other (please describe) ## Checklist - [x] I have read the [contributing guidelines](/agntcy/repo-template/blob/main/CONTRIBUTING.md) - [x] Existing issues have been referenced (where applicable) - [x] I have verified this change is not present in other open pull requests - [ ] Functionality is documented - [x] All code style checks pass - [ ] New code contribution is covered by automated tests - [ ] All new and existing tests pass Signed-off-by: Vishal Sahare <6853259+vishalsahare@users.noreply.github.com>
|
Can anyone look at this PR? |
|
Can anyone review this PR, please? |
muscariello
left a comment
There was a problem hiding this comment.
Checked the hashes independently rather than trusting them, and they are all correct:
- Task
amd64/arm64SHA-256 match the officialtask_checksums.txtfor v3.53.1, and I re-verified amd64 by downloading the.deband hashing it. - All seven
websockets==16.0hashes are real PyPI artifacts — thepy3-none-anywheel, the sdist, and cp310–cp314 manylinux x86_64. That is the right coverage for GitHub-hosted glibc runners; musllinux is correctly excluded. dpkg --installis safe here: the.debcontrol file declares noDepends:, so droppingapt-get's dependency resolution costs nothing. Worth stating explicitly since it is the one behavioural change that could bite.- It fails closed —
set -euo pipefailplussha256sum --checkaborts the build on a mismatch.
The one thing I would address: CI never builds this. docker-build in ci.yaml is gated on github.event_name == 'push' && github.ref == 'refs/heads/main', so "Build SLIM docker image" reports skipping on this PR. The Dockerfile rewrite therefore merges unverified, and if anything is wrong it breaks the image build on main rather than here. Worth a local docker build before merging, or running the docker build once against this branch.
Two small things:
sha256sum --check --status suppresses all output, so a hash mismatch fails with no explanation in the log. Dropping --status makes it print which file failed, which is worth the one line when this fires.
Pinning means Task is now frozen at 3.53.1 — the Cloudsmith script used to float. That is the point of #2093, but nothing will bump it now, so it wants a renovate rule or it silently ages.
Also, the description mentions "the required AI change-trace entries" — there is no such convention in this repo and nothing matching in the diff, so that line looks like template residue.
muscariello
left a comment
There was a problem hiding this comment.
One inline suggestion on top of the earlier review — everything else there is prose feedback, not something to fix in the diff.
| curl --fail --location --silent --show-error \ | ||
| --output "${TASK_DEB}" \ | ||
| "https://github.com/go-task/task/releases/download/v${TASK_VERSION}/task_${TASK_VERSION}_linux_${TARGETARCH}.deb" | ||
| echo "${TASK_SHA256} ${TASK_DEB}" | sha256sum --check --status |
There was a problem hiding this comment.
--status suppresses all output, so a hash mismatch fails the build with nothing in the log saying which file or why. Dropping it costs one line and saves the next person a bisect.
| echo "${TASK_SHA256} ${TASK_DEB}" | sha256sum --check --status | |
| echo "${TASK_SHA256} ${TASK_DEB}" | sha256sum --check |
|
Built this locally against the branch head since CI does not exercise the Dockerfile on PRs (
So the Dockerfile change is verified working on arm64. Combined with the earlier review (hashes independently verified, |
|
@vishalsahare let me know if you want to complete or I will fix it myself. |
I am going to fix it. |
…n renovate - Drop sha256sum --status so a Task .deb checksum mismatch names the failed file in the build log instead of aborting with no explanation. - Add a renovate regex custom manager for ARG TASK_VERSION so the Task pin gets bumped instead of silently aging; the pinned SHA-256 values must be refreshed from the new release in the same PR (documented in the Dockerfile).
The rust stage runs natively on ${BUILDPLATFORM} and cross-compiles for ${TARGETARCH}. Downloading the Task .deb for TARGETARCH breaks the linux/arm64 leg of the multi-platform image build: CI builds that leg in an amd64 container (FROM --platform=${BUILDPLATFORM}), where dpkg rejects the arm64 package with "package architecture (arm64) does not match system (amd64)" and the build aborts. The pre-pin Cloudsmith/apt flow installed the build-arch package implicitly, so this is a regression introduced by the pinning change.
Also declare ARG TARGETPLATFORM/BUILDARCH/BUILDPLATFORM so the architecture error branches print their intended messages instead of tripping `set -u` on undeclared variables.
|
Following suggested command executed successfully: docker build -f Dockerfile --target rust --build-arg TARGETARCH=arm64 --no-cache . |