Skip to content

Build release binaries with cargo auditable to embed dependency data - #937

Open
shashial wants to merge 6 commits into
developfrom
cargo-auditable-builds
Open

Build release binaries with cargo auditable to embed dependency data#937
shashial wants to merge 6 commits into
developfrom
cargo-auditable-builds

Conversation

@shashial

@shashial shashial commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

📝 Summary

Release builds now go through cargo auditable,
which embeds the resolved dependency list into a .dep-v0 section of each binary.

  • make build* targets, docker/Dockerfile.rbuilder, docker/Dockerfile.reproducible, and the
    Mac path of the release workflow build with cargo auditable build
  • cargo-auditable is pinned to 0.7.5 in all build paths so every artifact embeds identically
    formatted data
  • Dev builds (build-dev, CI test builds) are unchanged

💡 Motivation and Context

Makes binaries — and container images built from them — scannable for Rust dependencies and known
vulnerabilities with cargo audit bin, trivy, grype, syft, etc.


✅ I have completed the following steps:

  • Run make lint
  • Run make test
  • [n/a] Added tests (if applicable)

@claude

claude Bot commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

Claude finished @shashial's task in 3m 37s —— View job


Review: cargo auditable release builds

Reviewed the diff against origin/develop, plus Makefile, both Dockerfiles, release.yaml, and the surrounding build paths. The approach is sound; findings below are about the guarantees the PR claims versus what the code enforces. Five inline comments posted.

Blocking-ish

1. make audit-bin passes vacuouslyMakefile:163-172. The target doesn't depend on build, and an unmatched glob leaves the literal target/.../*, so -f fails, the loop never runs, and it exits 0. Running it on a fresh checkout, before make build, or with a BUILD_PROFILE/BUILD_TARGET different from the one built, reports success having scanned nothing. Suggested a count guard inline. (Fix this →)

2. The 0.7.5 pin isn't actually enforcedMakefile:74-77. command -v cargo-auditable >/dev/null || cargo install ...@0.7.5 skips the install if any version is already on PATH. The PR's stated goal ("pinned to 0.7.5 in all build paths so every artifact embeds identically formatted data") doesn't hold for the make path on a machine or runner that already has 0.6.x. Gate on version instead. Same applies to install-cargo-audit. (Fix this →)

Worth addressing

3. No verification that .dep-v0 is actually present in any artifact. This is the structural gap behind 1 and 2: nothing in CI fails if a binary ships without embedded data. The Dockerfile.rbuilder path is the most exposed (docker/Dockerfile.rbuilder:76) — it inherits RUSTC_WRAPPER=sccache from the base stage while cargo-auditable injects via RUSTC_WORKSPACE_WRAPPER, and cargo chef cook (line 66) builds deps unwrapped against a persistent sccache mount. That chain is supposed to work, but if it doesn't, the result is a released image with no audit data and a green build. A cargo audit bin / readelf -S | grep dep-v0 assertion after each build turns a silent regression into a hard failure.

4. Reproducible builds not re-verifieddocker/Dockerfile.reproducible:13. This image backs the Linux release artifacts and the repo works hard for bit-identical output (-C metadata='', --build-id=none, --remap-path-prefix, SOURCE_DATE_EPOCH). cargo-auditable links a generated object produced in a temp dir; worth an actual two-run hash comparison rather than trusting the "sorted and timestamp-free" claim in the new Makefile comment. Secondary: everything above that line is deliberately pinned to a Debian snapshot repo, and cargo install from crates.io is now the one un-snapshotted network fetch in that builder.

5. Version pinned in four independent placesMakefile:77, docker/Dockerfile.rbuilder:39, docker/Dockerfile.reproducible:13, .github/workflows/release.yaml:151. Given the goal is identical embedded data across paths, silent drift between these is the exact failure the PR is trying to prevent. At minimum a Makefile variable plus Docker ARGs.

6. README sections now conflict — the "Reproducible builds" section (lines 157–176) still instructs plain cargo build --release with a fixed sha256sum, which produces a binary without audit data and a hash matching nothing make build emits.

Minor

  • docker/Dockerfile.rbuilder-operator:16 builds via make build-reproducible, which isn't a target in the Makefile — so that path is already broken and isn't covered by this PR. Pre-existing, flagging only because it's another release-ish build path that won't get audit data.
  • cargo install cargo-auditable --locked compiles from source with no registry cache mount in both Dockerfiles and on every Mac release run; cargo-binstall or taiki-e/install-action would cut that.
  • Dangling comment at Makefile:74 reads as a claim about the installer rather than about the build.

Not run: make lint / make test — Bash is restricted in this environment, so I reviewed statically. The shell-semantics claim in finding 1 (unmatched glob → exit 0) is standard POSIX behavior and doesn't depend on execution.
· branch cargo-auditable-builds

Comment thread Makefile Outdated
Comment thread Makefile Outdated
Comment thread docker/Dockerfile.rbuilder
Comment thread docker/Dockerfile.reproducible Outdated
Comment thread README.md Outdated

Copilot AI left a comment

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.

Pull request overview

This PR updates the release build pipeline to run through cargo auditable so produced binaries embed a resolved dependency list (enabling downstream vulnerability/dep scanning), and adds a Makefile target to scan built binaries with cargo audit bin.

Changes:

  • Switch release-oriented make build* targets to cargo auditable build and add helper install targets for cargo-auditable/cargo-audit.
  • Update Docker build paths and the macOS release workflow path to build via cargo auditable.
  • Document auditable builds and add a make audit-bin target for RustSec scanning of built binaries.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
README.md Adds documentation for auditable release builds and make audit-bin.
Makefile Routes release builds through cargo auditable and introduces audit-bin scanning.
docker/Dockerfile.reproducible Installs cargo-auditable so reproducible Docker builds embed audit data.
docker/Dockerfile.rbuilder Installs cargo-auditable and builds packages via cargo auditable.
.github/workflows/release.yaml Installs cargo-auditable on macOS and builds macOS release binaries via cargo auditable.
Suppressed comments (1)

Makefile:161

  • install-cargo-audit pins 0.22.2 in the install command, but the command -v guard means any preinstalled cargo-audit version will be used (which may not behave consistently across environments). Consider checking the installed version and reinstalling when it differs.
	@command -v cargo-audit >/dev/null 2>&1 || cargo install cargo-audit@0.22.2 --locked

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread Makefile Outdated
Comment thread Makefile Outdated
Comment thread README.md Outdated
Comment thread .github/workflows/release.yaml Outdated
@shashial
shashial marked this pull request as draft August 21, 2026 12:36
@claude

claude Bot commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

Claude finished @shashial's task in 4m 23s —— View job


Re-review: cargo auditable release builds

  • Read full diff vs origin/develop
  • Verify prior findings
  • Review the clippy-lint commit
  • Post review (4 inline comments)

Prior round's findings are mostly addressed: audit-bin now fails on zero binaries (Makefile:169-183), both installers gate on version via cargo install --list instead of command -v, .dep-v0 is asserted after every Docker and Mac build, --locked was added to the Mac build, and the README reproducible/auditable sections were reconciled. Remaining items below.

Worth addressing

1. Linux release artifacts are only partially verifieddocker/Dockerfile.reproducible:17-20. The loop checks rbuilder-operator, rbuilder-rebalancer, bid-scraper, but this image produces all six binaries that release.yaml renames and uploads (release.yaml:181-231). rbuilder, reth-rbuilder, and tbv-bidding-service can ship without embedded data and the build stays green — and rbuilder is the artifact most consumers will scan. Inline suggestion posted.

2. Reproducibility still isn't verified for this change. reproducible-build-test (.github/workflows/reprotest.yml) runs only on schedule (cron, every 2 days) and workflow_dispatch — not on PRs. So nothing in this PR's CI confirms that linking cargo-auditable's generated object still yields bit-identical output under -C metadata='' / --build-id=none / --remap-path-prefix. It's cheap to settle: dispatch that workflow on this branch before merging. Note it also only compares rbuilder-operator and rbuilder-rebalancer, so it won't cover the other four either.

Related: cargo install cargo-auditable@0.7.5 --locked (Dockerfile.reproducible:13) is now the one crates.io fetch in a builder whose apt sources are deliberately pinned to a Debian snapshot. Pinned by version + lockfile, so it should resolve identically, but the reproducible image's inputs no longer come from a single snapshot. Worth a conscious call rather than an accident.

3. Version pinned in four placesMakefile:75, docker/Dockerfile.rbuilder:39, docker/Dockerfile.reproducible:13, release.yaml:151. The Makefile variable helps, but the three hardcoded copies are where drift will actually happen, and drift is the exact failure this PR is trying to prevent. ARG CARGO_AUDITABLE_VERSION=0.7.5 in the Dockerfiles at minimum.

4. Mac verify step can't be exercised before a releaserelease.yaml:172-177. Only runs on tag push / manual dispatch, and greps otool -l output for a section name. Should work (__DATA,.dep-v0), but a failure here means a broken release rather than a missed check. One workflow_dispatch run with build-mac: true / draft-release: false would confirm it.

Rust changes (clippy commit)

5. Over-broad allowcrates/rbuilder-operator/src/bidding_service_wrapper/mod.rs:1. The inner #![allow(clippy::result_large_err)] silences the lint for client, server, and any future submodule, not just the generated bidding_service. The bloxroute_grpc.rs:16 change in the same commit scopes it correctly to the generated module — do the same here. Inline suggestion posted.

as_chunks::<N>().0 (crates/eth-sparse-mpt/src/utils.rs:129) is behaviour-preserving — same min(len) zip, same N-multiple offset, and the only instantiation is N = 8, so no zero-N panic path. Fine. The #[allow] on bid-scraper's connect (best_bid_ws_connector.rs:88) papers over a large tungstenite::Error; boxing would be the real fix, but it's a hand-written signature and not worth blocking on.

Minor

  • audit-bin doesn't distinguish binaries with .dep-v0 from ones without: cargo audit bin falls back to guessing versions from panic strings and reports low-confidence results without failing, so a stale non-auditable artifact left in target/ mixes silently into the output. Given the target now guarantees it scanned something, guaranteeing it scanned something auditable is the natural follow-up.
  • docker/Dockerfile.rbuilder-operator:16 still builds via make build-reproducible, which is not a target in the Makefile. Pre-existing and out of scope, but it's another release-ish path that won't get audit data.
  • Makefile:75 — the comment # The cargo-auditable embedded data is sorted and timestamp-free now sits above CARGO_AUDITABLE_VERSION, where it reads as a claim about the version variable. It belongs next to build or in the reproducibility docs.
  • grep -q '^cargo-auditable v0.7.5:' — the .s are regex wildcards. Harmless in practice; grep -qF or escaping is tidier.

Not run: make lint / make test — Bash is restricted here to read-only inspection, so this is a static review. The Makefile shell-semantics and Docker/workflow claims above are from reading the files, not from executing them.
· branch cargo-auditable-builds

Comment thread docker/Dockerfile.reproducible Outdated
Comment thread crates/rbuilder-operator/src/bidding_service_wrapper/mod.rs Outdated
Comment thread docker/Dockerfile.rbuilder Outdated
Comment thread .github/workflows/release.yaml Outdated
@claude

claude Bot commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

Claude finished @shashial's task in 4m 45s —— View job


Re-review: cargo auditable release builds

  • Read full diff vs origin/develop
  • Verify prior findings
  • Review the single-source cargo-auditable versions commit
  • Post review (4 inline comments)

Round-2 findings are addressed: .dep-v0 is now asserted for all six binaries in Dockerfile.reproducible:18-21, the version is single-sourced through .tool-versions, and bidding_service_wrapper/mod.rs scopes result_large_err per-module instead of at module-tree level. Remaining items below.

Worth addressing

1. .tool-versions collides with asdf/mise — new in this commit. That filename is asdf's config file and mise reads it too, so it now applies to every contributor with either installed. There's no asdf plugin named cargo-auditable, so asdf install in the repo fails outright, and mise will try to resolve both entries through its own registry rather than cargo install. It also reads as a second, partial toolchain source next to rust-toolchain.toml (no rust entry). Nothing here is actually asdf-managed — all four consumers awk the file by hand — so a neutral name (build-tools.txt) sidesteps it. Inline comment has the five call sites. (Fix this →)

2. Empty version from awk fails confusinglyMakefile:75, 162, plus the same unguarded pattern in both Dockerfiles and release.yaml:152. If the file is missing or the key is renamed, $(shell awk ...) returns empty silently; the guard greps for ^cargo-auditable v: (no match) and you get cargo install cargo-auditable@ --locked, which errors pointing at cargo rather than at the missing file. Now that the version is load-bearing from exactly one place, asserting the read succeeded is worth four lines. Suggestion inline, along with the unescaped . in the grep pattern.

3. audit-bin is the one path that doesn't assert .dep-v0Makefile:169-183. Both Dockerfiles and the Mac job check the section; this doesn't. cargo audit bin on a binary without embedded data doesn't fail — it guesses versions from panic strings and reports low confidence, silently mixed in with real results. Stale artifacts from a pre-cargo-auditable build stay in target/x86_64-unknown-linux-gnu/reproducible/ since nothing cleans it. Given the target now guarantees it scanned something, guaranteeing it scanned something auditable closes the loop. Suggestion inline (note readelf is Linux-only; cargo audit bin --deny warnings is the portable version).

4. Reproducibility still isn't verified for this change — carried from round 2, still the highest-stakes open item. reproducible-build-test (.github/workflows/reprotest.yml:3-6) runs on schedule and workflow_dispatch only, not on PRs, so nothing here confirms that linking cargo-auditable's generated object still yields bit-identical output under -C metadata='' / --build-id=none / --remap-path-prefix. The risk is lower than it looked — profile.reproducible inherits release, so there's no debuginfo to carry the generated object's temp path — but the README now asserts bit-identical output, and one workflow_dispatch run on this branch settles it. It also only diffs rbuilder-operator and rbuilder-rebalancer, so it won't cover the other four.

5. README overstates the release buildREADME.md:158-161. "sets SOURCE_DATE_EPOCH from the last commit timestamp" holds for a local make build (Makefile:52), but docker/Dockerfile.reproducible:19 overrides it with SOURCE_DATE=1730000000. Following the README on a release commit produces a self-consistent hash that won't match the published artifact — the same mismatch the old section had, relocated rather than removed.

Minor

  • cargo install cargo-auditable --locked compiles from source with no registry cache mount in either Dockerfile and on every Mac release run. cargo-binstall / taiki-e/install-action would cut a couple of minutes per cold build.
  • docker/Dockerfile.reproducible:14 is still the only crates.io fetch in a builder whose apt sources are deliberately pinned to a Debian snapshot. Pinned by version + --locked, so it should resolve identically — flagging so it's a conscious call.
  • Mac verify (release.yaml:174-179) only executes during an actual release, so a failure there breaks a release rather than a PR. cargo audit bin would be name-agnostic and double as the scan.
  • docker/Dockerfile.rbuilder-operator:16 still builds via make build-reproducible, which is not a target in the Makefile. Pre-existing, out of scope, but it's another release-ish path with no audit data.
  • Makefile:74 — the comment # The cargo-auditable embedded data is sorted and timestamp-free still sits above CARGO_AUDITABLE_VERSION, where it reads as a claim about the variable rather than about the build.

Checked and fine: tbv-bidding-service does exist (crates/rbuilder-operator/src/bin/tbv-bidding-service.rs, auto-discovered), so the unguarded six-binary loop in Dockerfile.reproducible won't spuriously fail. install-cargo-audit's ^cargo-audit v pattern doesn't false-match the cargo-auditable line. readelf is present in both rust:*-bookworm and rust:*-trixie.

Not run: make lint / make test — Bash here is restricted to read-only inspection, so this is a static review. The Makefile shell semantics, Docker, and workflow claims come from reading the files, not executing them.
· branch cargo-auditable-builds

Comment thread .tool-versions Outdated
Comment thread Makefile Outdated
Comment thread Makefile Outdated
Comment thread README.md Outdated
@claude

claude Bot commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

Claude finished @shashial's task in 4m 54s —— View job


Re-review: cargo auditable release builds

  • Read full diff vs origin/develop
  • Verify prior round-3 findings
  • Review the fix review commit
  • Post review (4 inline comments)

Round-3 findings are addressed: .tool-versions is gone (replaced by the neutral .cargo-auditable-version), the empty-version case is now guarded in all four consumers, the grep uses -qxF, and the README no longer contradicts itself on which command produces the released artifact.

Two things changed in fix review that weren't asked for and go the wrong way:

Worth addressing

1. Mac .dep-v0 verification was removedrelease.yaml:167. The Verify audit data embedded (Mac) step added in the previous commit is gone, so the darwin release artifacts are now the only build path in this PR without the assertion (Dockerfile.rbuilder:80 and Dockerfile.reproducible:20 both have it). That's the wrong path to drop it from: the two Docker checks cover ELF, and Mach-O is the one object format nothing here exercises. Inline comment has the re-add plus the cargo audit bin alternative if the concern was that it can only fail during a real release. (Fix this →)

2. make audit-bin and install-cargo-audit were deleted. The README was updated consistently, so this looks deliberate — but the PR description still lists "New make audit-bin target scans built binaries against the RustSec advisory database" as a bullet, and the repo now has no way to actually consume the data it embeds. If the removal was intentional, update the PR body; if it was collateral from the .tool-versions rename, it needs to come back (the hardened version from the last round). Leftover: BUILD_OUTPUT_DIR (Makefile:120) now exists only to dedupe three deb paths — harmless, just noting it came from that target.

3. Version guard checks the wrong thingMakefile:82. cargo install --list reports $CARGO_HOME/.crates2.json; cargo auditable dispatches to the first cargo-auditable on PATH. An externally-installed copy earlier in PATH isn't listed, so the guard installs 0.7.5 into ~/.cargo/bin and the build uses the other one anyway. cargo-auditable --version reflects what will actually run. Also Makefile:75 — the cat path is cwd-relative and the $(error) is parse-time, so make -f /abs/path/Makefile help from another directory hard-fails every target. Both inline.

4. Reproducibility still isn't verified for this change — carried from rounds 2 and 3. reprotest.yml:3-6 runs on schedule/workflow_dispatch only, never on PRs, so nothing confirms that linking cargo-auditable's generated object stays bit-identical under -C metadata='' / --build-id=none / --remap-path-prefix. The risk is genuinely low — profile.reproducible inherits release (Cargo.toml:65-70) with no debug override, so there's no debuginfo to carry the generated object's temp path — but the README now asserts bit-identical output and one workflow_dispatch run on this branch settles it. It also only diffs rbuilder-operator and rbuilder-rebalancer.

5. README reproducible section still contradicts the release imageREADME.md:159-160. "sets SOURCE_DATE_EPOCH (last commit timestamp)" is true for a local make build, but Dockerfile.reproducible:19 pins SOURCE_DATE=1730000000 for the published Linux artifacts. Same trap as the old section, relocated. Carried from round 3, inline suggestion posted.

Minor

  • Dockerfile.rbuilder:79 — the .dep-v0 check resolves last round's correctness concern, but RUSTC_WRAPPER=sccache + cargo-auditable's RUSTC_WORKSPACE_WRAPPER now means workspace crates compile through chained wrappers. Deps from cargo chef cook are unaffected; worth confirming the workspace hit rate off one build log. Flagged as unverified.
  • Dockerfile.rbuilder:39 / Dockerfile.reproducible:13 leave /tmp/cargo-auditable-version in the image; a bind mount avoids the layer.
  • cargo install cargo-auditable --locked still compiles from source with no registry cache mount in both Dockerfiles and on every Mac release run, and is still the only crates.io fetch in a builder whose apt sources are pinned to a Debian snapshot.
  • Makefile:74 — the comment # The cargo-auditable embedded data is sorted and timestamp-free still sits above CARGO_AUDITABLE_VERSION, reading as a claim about the variable.
  • docker/Dockerfile.rbuilder-operator:16 still builds via make build-reproducible, not a target in the Makefile. Pre-existing, out of scope.

Rust changes

bidding_service_wrapper/mod.rs is fixed — per-module #[allow] instead of the module-tree #![allow]. The client one still covers a whole subdirectory rather than the two Result-returning fns in bidding_service_client_adapter.rs, but that's a much smaller blast radius and not worth another round.

Checked and fine: .dockerignore doesn't exclude .cargo-auditable-version, so both COPYs resolve. grep -qxF 'cargo-auditable v0.7.5:' matches cargo's registry-install line format exactly and won't false-match. The six-binary loop in Dockerfile.reproducible is unguarded, so a missing binary fails loudly — intended.

Not run: make lint / make test — Bash is restricted to read-only inspection here, so this is a static review. The Makefile, Docker, and workflow claims come from reading the files, not executing them.
· branch cargo-auditable-builds

Comment thread README.md
Comment on lines +159 to +160
`make build` produces reproducible binaries on x86_64: it sets the `SOURCE_DATE_EPOCH`
environment variable (last commit timestamp) and the required compiler flags automatically:

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.

Still unaddressed from the last round: "sets SOURCE_DATE_EPOCH (last commit timestamp)" is true for a local make build (Makefile:54, SOURCE_DATE ?= $(shell git log -1 --pretty=%ct)), but docker/Dockerfile.reproducible:19 builds the published Linux artifacts with SOURCE_DATE=1730000000. Following these instructions on a release commit gives you two matching hashes that don't match the released binary — which is the same trap the old section had, just relocated.

Suggested change
`make build` produces reproducible binaries on x86_64: it sets the `SOURCE_DATE_EPOCH`
environment variable (last commit timestamp) and the required compiler flags automatically:
`make build` produces reproducible binaries on x86_64: it sets the `SOURCE_DATE_EPOCH`
environment variable (last commit timestamp) and the required compiler flags automatically.
Note that the released Linux binaries are built by `docker/Dockerfile.reproducible`, which pins
`SOURCE_DATE_EPOCH` to a fixed value, so their hashes will not match a local `make build`:

Also, SOURCE_DATE comes from git log -1, so this only works from a checkout with history — in a shallow/tarball checkout it expands to empty and SOURCE_DATE_EPOCH= is silently unset, producing a non-reproducible build with no warning.

FEATURE_FLAG="--features $FEATURES"
fi
cargo build --profile ${{ matrix.profile }} $FEATURE_FLAG \
cargo auditable build --locked --profile ${{ matrix.profile }} $FEATURE_FLAG \

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.

The Verify audit data embedded (Mac) step that followed this build in the previous commit was dropped in fix review, so the Mac release artifacts are now the only build path in this PR with no .dep-v0 assertion — docker/Dockerfile.rbuilder:80 and docker/Dockerfile.reproducible:20 both check, this doesn't.

That's the wrong path to drop it from. The two Docker checks exercise ELF section emission; Mach-O is the one object format nothing here covers. If cargo auditable silently no-ops on darwin — linker dropping the __DATA,.dep-v0 section, a toolchain change, a bad cargo install — six darwin release binaries ship without embedded data and the release is green.

If it was removed because it can only ever fail during a real release, cargo audit bin is the better version of the check: it errors on a binary with no embedded data rather than grepping a tool-specific section rendering, and it doubles as the vulnerability scan. Otherwise, re-add the otool -l step and confirm it once with a workflow_dispatch run (build-mac: true, draft-release: false) before merging.

Comment thread Makefile Outdated
--mount=type=cache,target=/usr/local/cargo/git \
--mount=type=cache,target=$SCCACHE_DIR,sharing=locked \
cargo build --release --features="$FEATURES" --package=${RBUILDER_BIN}
cargo auditable build --release --features="$FEATURES" --package=${RBUILDER_BIN}

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.

The .dep-v0 check below settles the correctness question from the last round — good. Remaining concern here is build time, not correctness.

This stage has RUSTC_WRAPPER=sccache (line 47) and cargo auditable sets RUSTC_WORKSPACE_WRAPPER, so workspace crates are now compiled through a chained wrapper rather than sccache rustc directly. Dependencies from cargo chef cook (line 70) still go through plain sccache and cache normally, but the workspace crates are the slow part of an incremental image rebuild. Worth reading the sccache --show-stats / compile-request counts off one build log to confirm hit rate didn't collapse — I haven't verified the chaining semantics, only that the two wrappers are now both set.

Minor: line 39 leaves /tmp/cargo-auditable-version in the base stage, inherited by every stage built from it. --mount=type=bind,source=.cargo-auditable-version,target=/tmp/v avoids the layer.

Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com>
@claude

claude Bot commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

Claude finished @shashial's task in 2m 41s —— View job


Re-review: cargo auditable release builds

  • Read full diff vs origin/develop
  • Verify prior round-4 findings
  • Review the new Update Makefile commit
  • Post review (3 inline comments)

Update Makefile (05afbfd) fixes the cwd-relative version read — $(dir $(firstword $(MAKEFILE_LIST))) resolves against the Makefile, so make -f /abs/path/Makefile <target> from another directory no longer hard-fails every target, and 2>/dev/null suppresses the raw cat: error. That's the whole commit; everything else is carried forward.

Correction to my last round

The cargo install --list guard is fineMakefile:82. I said an externally-installed cargo-auditable earlier in PATH would shadow the pinned one. That's backwards: cargo's subcommand lookup searches $CARGO_HOME/bin before PATH, so what --list reports and what cargo auditable dispatches to agree. Withdrawing that finding — no change needed.

Still open

1. Mac .dep-v0 verification is still absentrelease.yaml:167. Darwin remains the only build path in this PR without the assertion (Dockerfile.rbuilder:80 and Dockerfile.reproducible:20-23 both check). It's the path where the check is worth the most: the Docker checks cover ELF, and Mach-O is the one object format nothing here exercises. Inline comment has the re-add and the cargo audit bin alternative. (Fix this →)

2. PR description is now stale. make audit-bin and install-cargo-audit are gone from the Makefile and the README was updated to match, so the removal reads as deliberate — but the PR body still lists "New make audit-bin target scans built binaries against the RustSec advisory database" as a bullet. Either drop that bullet or restore the target; as it stands the repo embeds the data with no first-party way to consume it. (BUILD_OUTPUT_DIR, Makefile:120, now exists only to dedupe three deb paths — harmless leftover.)

3. Reproducibility still isn't verified for this change — rounds 2, 3, 4, now 5. reprotest.yml:3-6 runs on schedule/workflow_dispatch only, never on PRs, so nothing confirms that linking cargo-auditable's generated object stays bit-identical under -C metadata='' / --build-id=none / --remap-path-prefix. The risk is low — profile.reproducible inherits release with no debug override, so there's no debuginfo to carry the generated object's temp path — but README.md:172 now asserts "the two hashes are identical", and one workflow_dispatch run on this branch settles it. It also only diffs rbuilder-operator and rbuilder-rebalancer, so it won't cover the other four.

4. README reproducible section still contradicts the release imageREADME.md:158-159. "sets SOURCE_DATE_EPOCH (last commit timestamp)" holds for a local make build, but Dockerfile.reproducible:19 pins SOURCE_DATE=1730000000 for the published Linux artifacts, so the documented procedure gives two self-consistent hashes that match each other and not the release. Suggestion inline. Also: SOURCE_DATE comes from git log -1, so in a shallow/tarball checkout it expands to empty and SOURCE_DATE_EPOCH= is silently unset.

Minor

  • Makefile:75$(dir ...) already ends in /, so the extra literal slash yields .//.cargo-auditable-version. Harmless.
  • Makefile:74 — the comment # The cargo-auditable embedded data is sorted and timestamp-free still sits above CARGO_AUDITABLE_VERSION, reading as a claim about the variable.
  • Dockerfile.rbuilder:81 — the .dep-v0 gate now applies to the test-relay stage too, so a local make docker-image-test-relay fails hard for a release-only reason.
  • Dockerfile.rbuilder:39 / Dockerfile.reproducible:13 leave /tmp/cargo-auditable-version in the image; --mount=type=bind avoids the layer.
  • cargo install cargo-auditable --locked still compiles from source with no registry cache mount in both Dockerfiles and on every Mac release run, and is still the only crates.io fetch in a builder whose apt sources are pinned to a Debian snapshot.
  • Dockerfile.rbuilder:79RUSTC_WRAPPER=sccache plus cargo-auditable's RUSTC_WORKSPACE_WRAPPER means workspace crates compile through chained wrappers. Deps from cargo chef cook are unaffected; worth reading sccache --show-stats off one build log. Unverified.
  • docker/Dockerfile.rbuilder-operator:16 still builds via make build-reproducible, not a target in the Makefile. Pre-existing, out of scope.

Checked and fine

$(dir $(firstword $(MAKEFILE_LIST))) is correct — no include precedes line 75, so it's the top-level Makefile. grep -qxF 'cargo-auditable v0.7.5:' matches cargo's registry-install line exactly. The six-binary loop in Dockerfile.reproducible is unguarded, so a missing binary fails loudly. RBUILDER_BIN defaults to rbuilder, where package name and binary name coincide, so the readelf path resolves. as_chunks::<8>().0 and the per-module #[allow(clippy::result_large_err)] scoping are unchanged and fine.

Not run: make lint / make test — Bash here is restricted to read-only inspection, so this is a static review. The Makefile, Docker, and workflow claims come from reading the files, not executing them.
· branch cargo-auditable-builds

Comment thread README.md
Comment thread .github/workflows/release.yaml
Comment thread Makefile
@shashial
shashial marked this pull request as ready for review August 27, 2026 22:00
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