Skip to content

fix(build): Pin the two unpinned build dependencies (Dockerfile install script, pip install) - #2099

Open
vishalsahare wants to merge 9 commits into
agntcy:mainfrom
vishalsahare:two_unpinned_build_dep
Open

vishalsahare wants to merge 9 commits into
agntcy:mainfrom
vishalsahare:two_unpinned_build_dep

Conversation

@vishalsahare

Copy link
Copy Markdown
Contributor
## 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

…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>
@vishalsahare
vishalsahare requested a review from a team as a code owner September 19, 2026 20:50
@vishalsahare

Copy link
Copy Markdown
Contributor Author

Can anyone look at this PR?

@vishalsahare

Copy link
Copy Markdown
Contributor Author

Can anyone review this PR, please?

@muscariello muscariello left a comment

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.

Checked the hashes independently rather than trusting them, and they are all correct:

  • Task amd64/arm64 SHA-256 match the official task_checksums.txt for v3.53.1, and I re-verified amd64 by downloading the .deb and hashing it.
  • All seven websockets==16.0 hashes are real PyPI artifacts — the py3-none-any wheel, the sdist, and cp310–cp314 manylinux x86_64. That is the right coverage for GitHub-hosted glibc runners; musllinux is correctly excluded.
  • dpkg --install is safe here: the .deb control file declares no Depends:, so dropping apt-get's dependency resolution costs nothing. Worth stating explicitly since it is the one behavioural change that could bite.
  • It fails closed — set -euo pipefail plus sha256sum --check aborts 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 muscariello left a comment

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.

One inline suggestion on top of the earlier review — everything else there is prose feedback, not something to fix in the diff.

Comment thread Dockerfile Outdated
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

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.

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

Suggested change
echo "${TASK_SHA256} ${TASK_DEB}" | sha256sum --check --status
echo "${TASK_SHA256} ${TASK_DEB}" | sha256sum --check

@muscariello

Copy link
Copy Markdown
Member

Built this locally against the branch head since CI does not exercise the Dockerfile on PRs (docker-build only runs on push to main).

docker build -f Dockerfile --target rust --build-arg TARGETARCH=arm64 --no-cache . — full success, no cache reuse. The log confirms the new path actually ran: .deb downloaded, checksum verified (build would have aborted under set -euo pipefail otherwise), dpkg --install unpacked and set up task (3.53.1), and the workspace compiled through to a finished image.

So the Dockerfile change is verified working on arm64. Combined with the earlier review (hashes independently verified, dpkg has no unmet deps since Task ships no Depends:), I don't see a blocker left. Still worth someone confirming amd64 before merge, since I only had arm64 hardware to test on.

@muscariello

Copy link
Copy Markdown
Member

@vishalsahare let me know if you want to complete or I will fix it myself.

@vishalsahare

Copy link
Copy Markdown
Contributor Author

@vishalsahare let me know if you want to complete or I will fix it myself.

I am going to fix it.

muscariello and others added 3 commits October 2, 2026 09:08
…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.
@vishalsahare

Copy link
Copy Markdown
Contributor Author

Following suggested command executed successfully:

docker build -f Dockerfile --target rust --build-arg TARGETARCH=arm64 --no-cache .

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants