Skip to content

build: add debian package build to the repo - #3233

Open
mhumeSF wants to merge 9 commits into
masterfrom
mh/local-deb-packaging
Open

mhumeSF wants to merge 9 commits into
masterfrom
mh/local-deb-packaging

Conversation

@mhumeSF

@mhumeSF mhumeSF commented Aug 29, 2026

Copy link
Copy Markdown
Contributor

Adds a Makefile and scripts to build the monad .deb from this repo: make deb builds in the docker/builder image, bundles in-tree libraries via ldd, and derives Depends with dpkg-shlibdeps instead of hand-maintained lists in the Jenkins shared library. Inert on merge — nothing calls it yet.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Adds first-class, in-repo Debian packaging support for monad by introducing build scripts, a containerized packaging workflow, and Debian control metadata, replacing external/hand-maintained dependency lists with dpkg-shlibdeps-derived Depends.

Changes:

  • Add shell scripts to compute a package version, build release binaries (Rust + CMake), and assemble a .deb with bundled in-tree shared libraries and dpkg-shlibdeps dependency inference.
  • Add a Makefile target set (deb, deb-container, etc.) and a Dockerfile to build the .deb in a reproducible builder image.
  • Add Debian control template used to generate the package control file.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
scripts/package-version Generates a Debian-compatible version string from git timestamp + short SHA.
scripts/build-deb Builds binaries, stages files into a Debian package layout, computes Depends, and produces a .deb.
scripts/build-binaries Builds Rust binaries/examples and monad-execution CMake artifacts with a pinned toolchain setup.
Makefile Adds local and containerized build targets for building binaries and producing the Debian package.
docker/debian-package/Dockerfile Defines a packaging container build that runs the deb build script and exports artifacts.
debian/DEBIAN/control.in Control-file template filled with computed version and dependency list.

💡 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 scripts/build-binaries Outdated
Comment thread docker/debian-package/Dockerfile
@mhumeSF

mhumeSF commented Aug 31, 2026

Copy link
Copy Markdown
Contributor Author

@claude review

@github-actions

This comment was marked as outdated.

Reproduces the flags the Jenkins buildRustBinaries step uses, so a local
build matches the published one.
Stages the binaries and their in-tree libraries, then derives Depends with
dpkg-shlibdeps instead of a hand-maintained list.
Missing libraries and basename collisions now fail the build, staging
moved out of the shared cache mount, the builder's libsecp256k1.so.6 is
bundled (unsatisfiable on the fleet as a Depends), GIT_COMMIT_HASH
reaches container builds, and the cmake toolchain file owns arch flags.
The container build is the only path that works without the builder
image's exact toolchain on the host; rename the host build to deb-host.
Valid Debian versions can contain characters that are invalid in image
tags; create the copy-out container from the image id instead.
@mhumeSF
mhumeSF force-pushed the mh/local-deb-packaging branch from 06da173 to fa25537 Compare September 8, 2026 16:13
- Fail the build if libsecp256k1.so.6 is missing from the builder image
  instead of silently skipping it (dpkg-shlibdeps and the in-builder ldd
  sweep cannot catch the omission).
- Declare cron and whiptail in Depends: they are invoked by maintainer
  scripts but invisible to dpkg-shlibdeps.
- Guard the crontab call in postrm so removal cannot fail on hosts
  installed under an older control file without cron.
'make deb' output under dist/ was being copied into the image build
context on subsequent builds, bloating the context with stale packages.
@mhumeSF
mhumeSF force-pushed the mh/local-deb-packaging branch from efa9b93 to 1312af5 Compare September 8, 2026 17:27
The package bundles shared libraries into /usr/local/lib but postrm
never refreshed the linker cache, leaving stale entries for removed
libraries (Debian Policy 8.1.1). postinst already calls ldconfig; this
mirrors it on removal.
@mhumeSF
mhumeSF force-pushed the mh/local-deb-packaging branch from 1312af5 to bb53d14 Compare September 8, 2026 17:29
@mhumeSF

mhumeSF commented Sep 8, 2026

Copy link
Copy Markdown
Contributor Author

@claude review

@github-actions

github-actions Bot commented Sep 8, 2026

Copy link
Copy Markdown

Claude finished @mhumeSF's task in 3m 11s —— View job


Code review

  • Read all changed files and diff against origin/master
  • Review packaging scripts, Makefile, Dockerfile, debian metadata
  • Post review feedback

Overall this is in good shape. The design is sound: version validation before use, staging outside the shared target/ cache mount, fail-closed checks (missing libtriedb_driver.so, basename collisions, unresolved sonames, test -n "$PACKAGE_VERSION"), --iidfile instead of a version-derived tag, and dpkg-shlibdeps run against a proper stub source tree so bundled libraries resolve in-tree and generate no Depends. The earlier Copilot findings all look genuinely addressed. One real bug and a few minor notes below.

Bug: exit 0 in postrm skips the new ldconfig

debian/DEBIAN/postrm:29-32 — the non-interactive guard in the remove|purge branch does exit 0 before the whiptail prompt, so the trailing ldconfig added in bb53d14 never runs:

if ! [ -t 0 ]; then
  echo "Non-interactive environment detected. Exiting triedb whiptail without running prompt."
  exit 0
fi

Non-interactive removal (apt in CI, Ansible, unattended-upgrades — dpkg maintainer scripts frequently run without a TTY even from an interactive apt) is exactly the common case, and it's also the removal path where a stale linker cache pointing at deleted /usr/local/lib libraries matters most. Run ldconfig before that exit 0, or restructure so the tail of the script is reached on all remove|purge paths. Fix this →

Minor notes (no change strictly required)

  • scripts/build-deb:14-16 — in a POSIX case bracket expression, \ is not an escape character, so [!0-9A-Za-z.+:~\-] includes a literal backslash in the allowed set; a version containing \ would pass. Writing the class as [!0-9A-Za-z.+:~-] (hyphen last, no backslash) is enough. The check is also intentionally loose (doesn't enforce "starts with a digit", epoch/revision placement) — fine for a sanity gate, just noting it isn't full Debian-version validation.
  • scripts/build-deb:109-114 — the "unresolved shared libraries" check runs inside the builder image, so any system library present in the 26.04 builder but absent on fleet hosts still resolves and passes ldd. The real protection is that dpkg-shlibdeps turns such a library into a versioned Depends, which then fails at install time on hosts that can't satisfy it (this is exactly the libsecp256k1 situation you special-case). Worth keeping in mind that "the deb built" doesn't imply "the deb installs on the fleet" — installability on the target distro is unverified here.
  • scripts/build-deb:66-79 — on a real libkeccak.so divergence between the two cmake trees, the loop silently keeps whichever path sorts last (sort -u order), rather than failing. Since the exception exists because they're built identically, a byte-compare (cmp) before accepting the collision would make the exemption self-verifying. Optional hardening.
  • debian/DEBIAN/postrm:17 — pre-existing, not from this PR: under set -e, if /opt/monad/scripts/clear-old-artifacts.cron is missing, grep -f emits nothing and crontab -u "$USER" - installs an empty crontab, wiping any unrelated cron entries for the monad user (pipeline status comes from crontab, so set -e doesn't catch it). Since you're already touching this line for the command -v crontab guard, guarding on the cron file's existence too would close it.
  • Makefile:29-42 — the double trap ... EXIT dance is correct (the second trap replaces the first and re-includes the iidfile cleanup), and failure at any && link propagates to make. One nit: the recipe assumes BuildKit (# syntax= directive + --mount=type=cache); docker < 23 without DOCKER_BUILDKIT=1 will fail with a confusing error. A one-line note or DOCKER_BUILDKIT=1 prefix would help, but modern Docker/Podman are fine as-is.
  • docker/debian-package/Dockerfile:47target=/src/target is a shared (default sharing=shared) cache mount; two concurrent make deb runs share one cargo target dir. Cargo's own locking makes this safe in practice, just be aware the execution build got sharing=locked and this one didn't — asymmetry is presumably intentional (cargo locks, cmake doesn't).

Things I checked that are fine

  • ldd for in-tree library discovery runs before patchelf --remove-rpath, so build-tree runpaths still resolve; the post-strip ldd "not found" gate then fails closed if anything relative ($ORIGIN) slipped through.
  • cp -a debian/. "$stage_dir" merges cleanly with the pre-created usr/local staging dirs, and control.in is removed after substitution. The sed replacement using % as delimiter is safe for dpkg-shlibdeps output (commas, parens, >= — no %/&).
  • postinst runs ldconfig on configure, so bundled /usr/local/lib libraries are picked up on install; Depends can't end up empty/malformed since libc6 always appears.
  • .dockerignore /dist addition prevents prior outputs from invalidating the COPY . . layer.

--- · Branch

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.

3 participants