Conversation
|
🔍 OpenCodeReview found 22 issue(s) in this PR.
📄
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| [ "$VERSION" = "$GITHUB_REF" ] && VERSION="$(git describe --tags --abbrev=0 | sed -e 's/^v//')" | ||
| [ -n "$VERSION" ] || VERSION="0.0.0-ci" |
There was a problem hiding this comment.
This line breaks every tag-triggered build. The workflow's default shell is bash -e; when building from a tag (e.g. GITHUB_REF=refs/tags/v1.2.3), VERSION becomes '1.2.3' and the test [ "$VERSION" = "$GITHUB_REF" ] evaluates false, making this AND-list return exit code 1, which triggers errexit and aborts the step immediately. Use an if statement instead.
Suggestion:
| [ "$VERSION" = "$GITHUB_REF" ] && VERSION="$(git describe --tags --abbrev=0 | sed -e 's/^v//')" | |
| [ -n "$VERSION" ] || VERSION="0.0.0-ci" | |
| if [ "$VERSION" = "$GITHUB_REF" ]; then | |
| VERSION="$(git describe --tags --abbrev=0 | sed -e 's/^v//')" | |
| fi | |
| [ -n "$VERSION" ] || VERSION="0.0.0-ci" |
| docker run --rm -v "$PWD":/work -w /work fedora:latest \ | ||
| packaging/rpm/build.sh \ |
There was a problem hiding this comment.
Pinning to the mutable fedora:latest tag makes release packaging non-reproducible: a future Fedora release can change dnf/rpmbuild/rpmlint behavior and silently alter or break release artifacts. Prefer a pinned tag (e.g. fedora:40). Same applies to the RPM smoke-test steps below. Note also that the RPM build/smoke containers run without --platform, relying on native amd64 legs only being meaningful for x86_64 — fine today since rpmbuild only repackages prebuilt binaries, but worth a comment.
Suggestion:
| docker run --rm -v "$PWD":/work -w /work fedora:latest \ | |
| packaging/rpm/build.sh \ | |
| docker run --rm -v "$PWD":/work -w /work fedora:40 \ | |
| packaging/rpm/build.sh \ |
| docker run --rm -v "$PWD":/work -w /work fedora:latest \ | ||
| packaging/checks/smoke-rpm.sh dist/Mintlayer_Node_linux_${{ steps.get_version.outputs.VERSION }}_${ARCH}.rpm mintlayer-node node |
There was a problem hiding this comment.
Unlike the DEB smoke-test step, the RPM smoke tests omit --platform linux/$ARCH, so on the arm64 matrix leg a aarch64 RPM is installed inside an x86_64 fedora container. dnf install ./<file>.rpm will reject it with an architecture mismatch ("package ... aarch64 is intended for a different architecture"), failing the arm64 job. Either add --platform linux/arm64 (QEMU, slow) or skip RPM smoke tests for non-x86_64, or pass rpm -i --ignorearch-style handling explicitly.
Suggestion:
| docker run --rm -v "$PWD":/work -w /work fedora:latest \ | |
| packaging/checks/smoke-rpm.sh dist/Mintlayer_Node_linux_${{ steps.get_version.outputs.VERSION }}_${ARCH}.rpm mintlayer-node node | |
| docker run --rm --platform linux/$ARCH -v "$PWD":/work -w /work fedora:latest \ | |
| packaging/checks/smoke-rpm.sh dist/Mintlayer_Node_linux_${{ steps.get_version.outputs.VERSION }}_${ARCH}.rpm mintlayer-node node |
| for unit in /usr/lib/systemd/system/mintlayer-*.service; do | ||
| systemd-analyze verify "$unit" |
There was a problem hiding this comment.
If no units match the glob (e.g. the package failed to install the systemd units), the literal unexpanded string /usr/lib/systemd/system/mintlayer-*.service is passed to systemd-analyze verify, producing a confusing error instead of a clear "units missing" failure. Use nullglob or an explicit existence check.
Suggestion:
| for unit in /usr/lib/systemd/system/mintlayer-*.service; do | |
| systemd-analyze verify "$unit" | |
| shopt -s nullglob | |
| units=(/usr/lib/systemd/system/mintlayer-*.service) | |
| [ ${#units[@]} -gt 0 ] || { echo "error: no mintlayer systemd units installed" >&2; exit 1; } | |
| for unit in "${units[@]}"; do | |
| systemd-analyze verify "$unit" |
| { echo "warning: help2man failed for $binname, shipping stub" >&2 | ||
| printf '.TH %s 1\n.SH NAME\n%s \\- Mintlayer tool\n' "$binname" "$binname" \ | ||
| > "$MAN_DIR/$binname.1"; } |
There was a problem hiding this comment.
Man pages are generated by executing the packaged binaries with --help on the build host. For cross-arch builds (e.g. arm64 binaries on an x86 host) this cannot run and every man page silently becomes the stub; consider failing the build when no real man page could be generated, or detecting cross-arch upfront and documenting the stub path.
| BUILT_RPM="$TOPDIR/RPMS/$RPMARCH/mintlayer-*.${RPMARCH}.rpm" | ||
| [ -e $BUILT_RPM ] || { echo "expected rpm not found: $BUILT_RPM" >&2; exit 1; } | ||
| mv $BUILT_RPM "$OUT_DIR/$ARTIFACT" |
There was a problem hiding this comment.
$BUILT_RPM contains a glob and is expanded unquoted in both the [ -e ] test and the mv. If the glob matches nothing, [ -e $BUILT_RPM ] happens to work (literal string doesn't exist), but if more than one rpm matches (e.g. a stale rpm in RPMS/$RPMARCH, or subpackages), mv receives multiple arguments and fails confusingly. Quote and iterate explicitly.
Suggestion:
| BUILT_RPM="$TOPDIR/RPMS/$RPMARCH/mintlayer-*.${RPMARCH}.rpm" | |
| [ -e $BUILT_RPM ] || { echo "expected rpm not found: $BUILT_RPM" >&2; exit 1; } | |
| mv $BUILT_RPM "$OUT_DIR/$ARTIFACT" | |
| shopt -s nullglob | |
| rpms=("$TOPDIR/RPMS/$RPMARCH/"mintlayer-*."${RPMARCH}".rpm) | |
| [ ${#rpms[@]} -eq 1 ] || { echo "expected exactly one rpm, found ${#rpms[@]}" >&2; exit 1; } | |
| mv "${rpms[0]}" "$OUT_DIR/$ARTIFACT" |
| RPM_BASE="$(basename "$ARTIFACT" | sed 's/\.rpm$//')" | ||
| if grep -qE "^${RPM_BASE}\.[a-z0-9_]+: E:" "$OUT_DIR/rpmlint.log"; then |
There was a problem hiding this comment.
The rpmlint error check assumes output lines are keyed on the artifact filename (${RPM_BASE}.${arch}: E:). rpmlint keys lines on the package NEVRA it derives from the rpm header (e.g. mintlayer-node-1.4.0-1.x86_64: E: ...), which does not match Mintlayer_Node_linux_1.4.0_x86_64 after the rename, so real errors can pass silently. A safer check greps for any : E: line in the log.
Suggestion:
| RPM_BASE="$(basename "$ARTIFACT" | sed 's/\.rpm$//')" | |
| if grep -qE "^${RPM_BASE}\.[a-z0-9_]+: E:" "$OUT_DIR/rpmlint.log"; then | |
| if grep -qE ": E: " "$OUT_DIR/rpmlint.log"; then |
| fi | ||
| RESULTS=() | ||
| run_step() { # run_step <label> <cmd...> | ||
| local label="$1"; shift |
There was a problem hiding this comment.
Control flow is broken: RESULTS=() and run_step() are defined inside the if [ "$SKIP_BUILD" -eq 1 ] branch (the else at line 124 attaches to this same if), so when the default build path (SKIP_BUILD=0) runs, run_step is never defined and every later run_step ... call fails with "command not found" under set -e. Move the helper/RESULTS definitions above the if/else, and keep the prebuilt-binary existence check (lines 120-123) in the SKIP_BUILD=1 branch only.
Suggestion:
| fi | |
| RESULTS=() | |
| run_step() { # run_step <label> <cmd...> | |
| local label="$1"; shift | |
| for arch in "${ARCHES[@]}"; do | |
| test -f "${BIN_DIR[$arch]}/node-daemon" || | |
| { echo "no prebuilt binary for $arch at ${BIN_DIR[$arch]}" >&2; exit 1; } | |
| done | |
| fi | |
| RESULTS=() | |
| run_step() { # run_step <label> <cmd...> | |
| local label="$1"; shift |
| RESULTS+=("FAIL $label (rc=$rc)") | ||
| return $rc | ||
| fi | ||
| } |
There was a problem hiding this comment.
The script runs under set -euo pipefail, but run_step returns the failing command's rc. A failing step aborts the whole script immediately, so the intended "collect all results and print a SUMMARY" flow is unreachable — the summary loop never executes on failure. Call sites need to tolerate the failure, e.g. run_step "label" cmd... || true, with the final summary deciding the exit code.
| # --------------------------------------------------------------------------- | ||
| PAIRS=() | ||
| for arch in "${ARCHES[@]}"; do PAIRS+=("${DEBARCH[$arch]}:$arch"); done | ||
| run_step "artifact names" packaging/checks/verify-artifacts.sh "$DIST" "$VERSION" "${PAIRS[@]}" || true |
There was a problem hiding this comment.
The || true makes the artifact-name gate advisory: verify-artifacts.sh exits nonzero when artifacts are missing/misnamed, but the failure is swallowed here and, with the summary logic, can still report success. Drop the || true (once run_step failures are made non-fatal) or explicitly fail the script when this step reports FAIL.
| apt-get update -qq | ||
|
|
||
| apt-get install -y -qq systemd desktop-file-utils >/dev/null 2>&1 || true | ||
| apt-get install -y -qq "./$DEB_FILE" >/dev/null |
There was a problem hiding this comment.
The package is installed as "./$DEB_FILE", which only works when DEB_FILE is a relative path and the script's cwd equals the directory containing the .deb. If the caller passes an absolute path (a natural usage given the usage line says <path/to/pkg.deb>), apt-get will fail with an unrecognized argument. Strip a leading ./ or handle absolute paths explicitly.
Suggestion:
| apt-get install -y -qq "./$DEB_FILE" >/dev/null | |
| case "$DEB_FILE" in | |
| /*) PKG_PATH="$DEB_FILE" ;; | |
| *) PKG_PATH="./$DEB_FILE" ;; | |
| esac | |
| apt-get install -y -qq "$PKG_PATH" >/dev/null |
| PKG_NAME="$2" | ||
| KIND="$3" | ||
|
|
||
| dnf install -y -q systemd desktop-file-utils >/dev/null 2>&1 || true |
There was a problem hiding this comment.
The dnf install is wrapped in || true, so a failure to install systemd/desktop-file-utils is silently ignored and the smoke test only fails later with confusing errors (e.g. desktop-file-validate: command not found), or the file-presence checks can mask a broken test environment rather than a broken package. Drop || true so a tooling failure is clearly distinguished from a package failure — the deb smoke test should be checked for the same pattern for consistency.
Suggestion:
| dnf install -y -q systemd desktop-file-utils >/dev/null 2>&1 || true | |
| dnf install -y -q systemd desktop-file-utils >/dev/null |
| # prerm for mintlayer-node | ||
| set -e | ||
|
|
||
| if [ "$1" = remove ] && [ -d /run/systemd/system ]; then |
There was a problem hiding this comment.
prerm only stops the services on remove, but dpkg also runs prerm with upgrade before replacing binaries. During an upgrade the running daemons keep executing while their binaries are deleted/replaced under them; then postinst only calls deb-systemd-invoke start, which is a no-op for already-running units, so the new binaries never actually get (re)started. Handle the upgrade case (or use try-restart/reload-or-restart in postinst).
Suggestion:
| if [ "$1" = remove ] && [ -d /run/systemd/system ]; then | |
| if { [ "$1" = remove ] || [ "$1" = upgrade ] || [ "$1" = deconfigure ]; } && [ -d /run/systemd/system ]; then |
| rpmbuild -bb \ | ||
| --define "_topdir $TOPDIR" \ | ||
| --define "debug_package %{nil}" \ | ||
| --define "__strip /bin/true" \ | ||
| --define "__objdump /bin/true" \ | ||
| --define "_buildrootdir $TOPDIR/BUILDROOT.dir" \ | ||
| --buildroot "$BR" \ | ||
| --target "$TARGET" \ | ||
| "$TOPDIR/SPECS/$SPEC_NAME" 2>&1 | sed -n '/^Processing files/,$p' || { | ||
| echo "rpmbuild FAILED; rerun without output filter for details" >&2 |
There was a problem hiding this comment.
Failure detection for rpmbuild goes through 2>&1 | sed -n '/^Processing files/,$p': (a) all output before the first 'Processing files' line is discarded, so spec-parse and %prep/file-missing errors are invisible; (b) on failure the retry re-runs rpmbuild into the same partially-built BUILDROOT, which can produce stale/inconsistent payload. Prefer capturing full output to a log file and tailing it, or let rpmbuild fail loudly the first time:
Suggestion:
| rpmbuild -bb \ | |
| --define "_topdir $TOPDIR" \ | |
| --define "debug_package %{nil}" \ | |
| --define "__strip /bin/true" \ | |
| --define "__objdump /bin/true" \ | |
| --define "_buildrootdir $TOPDIR/BUILDROOT.dir" \ | |
| --buildroot "$BR" \ | |
| --target "$TARGET" \ | |
| "$TOPDIR/SPECS/$SPEC_NAME" 2>&1 | sed -n '/^Processing files/,$p' || { | |
| echo "rpmbuild FAILED; rerun without output filter for details" >&2 | |
| rpmbuild -bb ... 2>&1 | tee "$TOPDIR/rpmbuild.log" | tail -n 40 | |
| # remove the retry block; on failure point the user at "$TOPDIR/rpmbuild.log" |
| for arch in "${ARCHES[@]}"; do | ||
| BIN_DIR[$arch]="$REPO_ROOT/target/$arch-unknown-linux-gnu/release" | ||
| [ "$arch" = x86_64 ] && BIN_DIR[$arch]="$REPO_ROOT/target/release" | ||
| done |
There was a problem hiding this comment.
This [ ... ] && BIN_DIR[...] list fails (exit 1) for every arch that is not x86_64, and since it is the last command in the loop body under set -e, the script will abort when ARCHES contains aarch64. Use an explicit if, or append || true.
Suggestion:
| for arch in "${ARCHES[@]}"; do | |
| BIN_DIR[$arch]="$REPO_ROOT/target/$arch-unknown-linux-gnu/release" | |
| [ "$arch" = x86_64 ] && BIN_DIR[$arch]="$REPO_ROOT/target/release" | |
| done | |
| if [ "$arch" = x86_64 ]; then BIN_DIR[$arch]="$REPO_ROOT/target/release"; fi |
| RESULTS=() | ||
| run_step() { # run_step <label> <cmd...> |
There was a problem hiding this comment.
RESULTS=() and run_step() are defined inside the if [ "$SKIP_BUILD" -eq 1 ] branch. When the script is run without --skip-build (the default path), run_step is never defined, so every later run_step ... call fails with 'command not found', and under set -u the summary loop over "${RESULTS[@]}" aborts with an unbound variable. Move the function definition and array initialization above the build/reuse branch.
Suggestion:
| RESULTS=() | |
| run_step() { # run_step <label> <cmd...> | |
| # (outside the if/else, before the packaging matrix) | |
| RESULTS=() | |
| run_step() { # run_step <label> <cmd...> | |
| local label="$1"; shift | |
| ... | |
| } |
| for arch in "${ARCHES[@]}"; do | ||
| debarch="${DEBARCH[$arch]}" | ||
| platform="" | ||
| [ "$arch" != "$(uname -m)" ] && platform="--platform linux/$arch" |
There was a problem hiding this comment.
Same set -e pitfall as the loop above: [ "$arch" != "$(uname -m)" ] && platform=... returns non-zero for the native arch. Since this line ends the iteration's command list, set -e aborts the whole script when building the host arch.
Suggestion:
| [ "$arch" != "$(uname -m)" ] && platform="--platform linux/$arch" | |
| platform="" | |
| if [ "$arch" != "$(uname -m)" ]; then platform="--platform linux/$arch"; fi |
|
|
||
| run_step "smoke rpm node ($arch)" \ | ||
| docker run --rm -v "$REPO_ROOT:/work" -w /work fedora:latest \ | ||
| packaging/checks/smoke-rpm.sh packaging/dist/Mintlayer_Node_linux_${VERSION}_${arch}.rpm \ |
There was a problem hiding this comment.
For pre-release tags (e.g. VERSION=1.4.0-rc1), rpm/build.sh sanitizes the dash to ~ when naming the artifact (Mintlayer_Node_linux_1.4.0~rc1_x86_64.rpm), but this script and the smoke/verify steps look the file up with the raw VERSION (Mintlayer_Node_linux_${VERSION}_${arch}.rpm). The smoke rpm steps and the artifact-name gate will fail to find the file for any version containing a dash. Apply the same VERSION=${VERSION//-/~} sanitization (to a separate lookup variable for RPM names) before composing the rpm/smoke/verify paths.
Suggestion:
| packaging/checks/smoke-rpm.sh packaging/dist/Mintlayer_Node_linux_${VERSION}_${arch}.rpm \ | |
| RPM_VER="${VERSION//-/~}" | |
| packaging/checks/smoke-rpm.sh packaging/dist/Mintlayer_Node_linux_${RPM_VER}_${arch}.rpm \ |
| jobs: | ||
| build: | ||
| runs-on: ubuntu-22.04 |
There was a problem hiding this comment.
This workflow has no permissions: block, so jobs run with the repository's default (potentially broad) GITHUB_TOKEN permissions. The build job only needs to upload artifacts; declare least-privilege permissions explicitly.
Suggestion:
| jobs: | |
| build: | |
| runs-on: ubuntu-22.04 | |
| permissions: | |
| contents: read | |
| jobs: | |
| build: | |
| runs-on: ubuntu-22.04 |
| jobs: | ||
| build: | ||
| runs-on: ubuntu-22.04 | ||
| strategy: | ||
| fail-fast: false |
There was a problem hiding this comment.
The QEMU-emulated arm64 deb/rpm container builds are slow, and there is no timeout-minutes on the job nor a concurrency group — a hung emulated build can run indefinitely, and simultaneous tag pushes/dispatches spawn duplicate expensive runs. Add a job-level timeout and a concurrency group (e.g. keyed on ref).
Suggestion:
| jobs: | |
| build: | |
| runs-on: ubuntu-22.04 | |
| strategy: | |
| fail-fast: false | |
| jobs: | |
| build: | |
| runs-on: ubuntu-22.04 | |
| timeout-minutes: 120 | |
| concurrency: | |
| group: release-linux-${{ github.ref }} | |
| cancel-in-progress: false | |
| strategy: | |
| fail-fast: false |
| VERSION=${GITHUB_REF#refs/tags/} | ||
| VERSION=${VERSION#v} | ||
| [ "$VERSION" = "$GITHUB_REF" ] && VERSION="$(git describe --tags --abbrev=0 | sed -e 's/^v//')" | ||
| [ -n "$VERSION" ] || VERSION="0.0.0-ci" |
There was a problem hiding this comment.
The new fallback git describe --tags --abbrev=0 requires full git history and tags, but the checkout step above uses the default shallow clone (no fetch-depth: 0). On a manual workflow_dispatch run (non-tag ref), git describe will fail and VERSION silently becomes 0.0.0-ci, so the intended 'use latest tag' behavior never works and packages get mis-versioned. Add fetch-depth: 0 to the checkout step, or fetch tags explicitly (git fetch --tags --depth=1).
Suggestion:
| VERSION=${GITHUB_REF#refs/tags/} | |
| VERSION=${VERSION#v} | |
| [ "$VERSION" = "$GITHUB_REF" ] && VERSION="$(git describe --tags --abbrev=0 | sed -e 's/^v//')" | |
| [ -n "$VERSION" ] || VERSION="0.0.0-ci" | |
| - uses: actions/checkout@v5 | |
| with: | |
| submodules: recursive | |
| fetch-depth: 0 |
| DEB_VERSION="$VERSION" | ||
| case "$DEB_VERSION" in | ||
| *-*) ;; # already has a revision | ||
| *) DEB_VERSION="${DEB_VERSION}-1" ;; | ||
| esac |
There was a problem hiding this comment.
The check for an existing Debian revision only tests for any dash, so a pre-release version like "1.4.0-rc1" or "1.4.0-beta.2" is passed through unchanged. In those cases "rc1"/"beta.2" becomes the Debian revision, which is invalid (a Debian revision must start with a digit, optionally prefixed by a letter, per Debian policy). dpkg will warn and the resulting package has broken/incorrect upgrade ordering versus the intent described in the comment ("1.4.0-rc1-1"). Only a purely numeric (or ~-containing upstream) version should be considered revision-free; a rc/beta suffix is part of the upstream version and should get "-1" appended.
Suggestion:
| DEB_VERSION="$VERSION" | |
| case "$DEB_VERSION" in | |
| *-*) ;; # already has a revision | |
| *) DEB_VERSION="${DEB_VERSION}-1" ;; | |
| esac | |
| DEB_VERSION="$VERSION" | |
| case "$DEB_VERSION" in | |
| *-[0-9]*) ;; # already has a numeric Debian revision | |
| *) DEB_VERSION="${DEB_VERSION}-1" ;; | |
| esac |
| if command -v deb-systemd-helper >/dev/null 2>&1; then | ||
| deb-systemd-helper enable mintlayer-node@mainnet.service >/dev/null || true | ||
| fi |
There was a problem hiding this comment.
postinst auto-enables and starts mintlayer-node@mainnet.service, but every step — including the enable and the start — ends in || true with output suppressed. A failed enable/start (broken unit, missing binary) is invisible during package installation. At minimum, drop >/dev/null || true from the deb-systemd-helper enable call so failures surface in apt output; silencing helper failures is only conventional for cosmetic steps like udev reload.
Suggestion:
| if command -v deb-systemd-helper >/dev/null 2>&1; then | |
| deb-systemd-helper enable mintlayer-node@mainnet.service >/dev/null || true | |
| fi | |
| if command -v deb-systemd-helper >/dev/null 2>&1; then | |
| deb-systemd-helper enable mintlayer-node@mainnet.service | |
| fi |
| if "$@"; then | ||
| RESULTS+=("PASS $label") | ||
| else | ||
| local rc=$? | ||
| RESULTS+=("FAIL $label (rc=$rc)") | ||
| return $rc | ||
| fi |
There was a problem hiding this comment.
run_step records a FAIL entry and then return $rc, but most call sites (icons step, deb/rpm packaging, smoke tests) are top-level statements under set -e, so the script exits immediately on the first failure. The FAIL entry is never printed by the summary and later steps (including the artifact-name gate, which is the point of the report) never run. Either invoke steps in a failure-tolerant way (e.g. run_step ... || true) and rely on the summary exit code, or drop the RESULTS/summary mechanism for the fail-fast behavior you actually get.
Suggestion:
| if "$@"; then | |
| RESULTS+=("PASS $label") | |
| else | |
| local rc=$? | |
| RESULTS+=("FAIL $label (rc=$rc)") | |
| return $rc | |
| fi | |
| if "$@"; then | |
| RESULTS+=("PASS $label") | |
| else | |
| local rc=$? | |
| RESULTS+=("FAIL $label (rc=$rc)") | |
| return 0 # defer failure to the summary | |
| fi |
| for arch in "${ARCHES[@]}"; do | ||
| test -f "${BIN_DIR[$arch]}/node-daemon" || | ||
| { echo "no prebuilt binary for $arch at ${BIN_DIR[$arch]}" >&2; exit 1; } | ||
| done |
There was a problem hiding this comment.
The prebuilt-binary existence check lives only in the --skip-build branch. In the default build path, BIN_DIR[aarch64] points to target/aarch64-unknown-linux-gnu/release which is never populated locally (the arm64 container build is deliberately skipped), so the missing binaries surface only as an obscure docker/packaging failure deep in the loop. Hoist this existence check out of the if/else so it runs for every arch regardless of build mode.
Suggestion:
| for arch in "${ARCHES[@]}"; do | |
| test -f "${BIN_DIR[$arch]}/node-daemon" || | |
| { echo "no prebuilt binary for $arch at ${BIN_DIR[$arch]}" >&2; exit 1; } | |
| done | |
| for arch in "${ARCHES[@]}"; do | |
| test -f "${BIN_DIR[$arch]}/node-daemon" || | |
| { echo "no prebuilt binary for $arch at ${BIN_DIR[$arch]}" >&2; exit 1; } | |
| done | |
| # (place directly after BIN_DIR population, before the SKIP_BUILD if/else) |
| run: | | ||
| VERSION=${GITHUB_REF#refs/tags/} | ||
| VERSION=${VERSION#v} | ||
| [ "$VERSION" = "$GITHUB_REF" ] && VERSION="$(git describe --tags --abbrev=0 | sed -e 's/^v//')" |
There was a problem hiding this comment.
The new version fallback git describe --tags --abbrev=0 requires tags in the fetched history, but actions/checkout@v5 with default settings does a shallow clone (fetch-depth: 1) without tags. On a non-tag ref (e.g. workflow_dispatch on a branch), git describe will fail with 'fatal: No names found', causing the whole version-extraction step to fail. Add with: { fetch-depth: 0, fetch-tags: true } (or run git fetch --tags) before this step so the fallback works as intended.
Suggestion:
| [ "$VERSION" = "$GITHUB_REF" ] && VERSION="$(git describe --tags --abbrev=0 | sed -e 's/^v//')" | |
| - uses: actions/checkout@v5 | |
| with: | |
| submodules: recursive | |
| fetch-depth: 0 | |
| fetch-tags: true |
| docker run --rm -v "$PWD":/work -w /work fedora:latest \ | ||
| packaging/rpm/build.sh \ | ||
| --package gui --rpmarch $ARCH \ | ||
| --version "${{ steps.get_version.outputs.VERSION }}" \ | ||
| --gui-binary /work/target/$ARCH-unknown-linux-gnu/release/node-gui \ | ||
| --repo-root /work --out /work/dist |
There was a problem hiding this comment.
The GUI RPM build will fail here: each docker run uses a fresh fedora container, so /work/dist/assets/icons/usr (the pre-generated icon set checked by packaging/rpm/build.sh) does not exist at this point, and nothing in this workflow ever creates it. The builder therefore falls back to packaging/make-icons.sh, which requires ImageMagick's 'convert' — but rpm/build.sh's dnf install list does not include imagemagick, so make-icons.sh exits with 'convert not found' and this step fails. Fix by adding imagemagick to the packages installed by packaging/rpm/build.sh, or by running make-icons.sh once into dist/assets/icons in a prior step shared by both deb and rpm builds.
Suggestion:
| docker run --rm -v "$PWD":/work -w /work fedora:latest \ | |
| packaging/rpm/build.sh \ | |
| --package gui --rpmarch $ARCH \ | |
| --version "${{ steps.get_version.outputs.VERSION }}" \ | |
| --gui-binary /work/target/$ARCH-unknown-linux-gnu/release/node-gui \ | |
| --repo-root /work --out /work/dist | |
| docker run --rm -v "$PWD":/work -w /work fedora:latest \ | |
| packaging/rpm/build.sh \ | |
| --package gui --rpmarch $ARCH \ | |
| --version "${{ steps.get_version.outputs.VERSION }}" \ | |
| --gui-binary /work/target/$ARCH-unknown-linux-gnu/release/node-gui \ | |
| --repo-root /work --out /work/dist | |
| # NOTE: ensure packaging/rpm/build.sh installs imagemagick (or pre-generate | |
| # dist/assets/icons/usr) or this gui build fails in make-icons.sh. |
| api-blockchain-scanner-daemon dns-server wallet-cli \ | ||
| wallet-address-generator; do | ||
| test -x "/usr/bin/mintlayer-$bin" | ||
| "/usr/bin/mintlayer-$bin" --help >/dev/null 2>&1 |
There was a problem hiding this comment.
All binary output (stdout and stderr) is discarded, so when --help fails (segfault under emulation, non-zero exit for a benign reason) the smoke test aborts via set -e with zero diagnostic output, making CI failures hard to triage. Capture output to a temp file and print it on failure.
Suggestion:
| "/usr/bin/mintlayer-$bin" --help >/dev/null 2>&1 | |
| out=$(mktemp) | |
| if ! "/usr/bin/mintlayer-$bin" --help >"$out" 2>&1; then | |
| echo "FAILED: mintlayer-$bin --help" >&2 | |
| cat "$out" >&2 | |
| exit 1 | |
| fi | |
| rm -f "$out" |
| api-blockchain-scanner-daemon dns-server wallet-cli \ | ||
| wallet-address-generator; do | ||
| test -x "/usr/bin/mintlayer-$bin" | ||
| "/usr/bin/mintlayer-$bin" --help >/dev/null 2>&1 |
There was a problem hiding this comment.
Same triage concern as smoke-deb.sh: --help output is fully suppressed, so a failure under set -e aborts with no diagnostics. Consider capturing and printing output on failure. (Mirror the deb-side fix for consistency.)
| sed -e "s/@VERSION@/$DEB_VERSION/g" -e "s/@DATE@/$(date -R)/" \ | ||
| "$PKG_ROOT/deb/changelog.in" > "$CHANGELOG" |
There was a problem hiding this comment.
DEB_VERSION is interpolated directly into sed replacement text; a version containing sed metacharacters ('|', '&', '/') or an otherwise malformed value silently corrupts the control file or changelog, failing late with a confusing dpkg-deb/lintian error. Validate that VERSION is a plausible Debian upstream version (and escape it) before substitution.
Suggestion:
| sed -e "s/@VERSION@/$DEB_VERSION/g" -e "s/@DATE@/$(date -R)/" \ | |
| "$PKG_ROOT/deb/changelog.in" > "$CHANGELOG" | |
| case "$VERSION" in | |
| [0-9A-Za-z.+-]*) ;; | |
| *) echo "invalid --version: $VERSION (expected e.g. 1.4.0 or 1.4.0-rc1)" >&2; exit 2 ;; | |
| esac | |
| sed -e "s|@VERSION@|$DEB_VERSION|g" -e "s|@DATE@|$(date -R)|" \ | |
| "$PKG_ROOT/deb/changelog.in" > "$CHANGELOG" |
|
|
||
| while [ $# -gt 0 ]; do | ||
| case "$1" in | ||
| --package) PACKAGE="$2"; shift 2 ;; |
There was a problem hiding this comment.
Flag parsing does not verify that a value exists: e.g. build.sh --package alone makes "$2" unset, and under set -u the script dies with an obscure unbound-variable error instead of a usage message. Check $# before shifting.
Suggestion:
| --package) PACKAGE="$2"; shift 2 ;; | |
| --package) [ $# -ge 2 ] || { echo "missing value for $1" >&2; exit 2; }; PACKAGE="$2"; shift 2 ;; |
| HOST_ARCH="$(uname -m)" | ||
| if [ "$RPMARCH" = "$HOST_ARCH" ]; then | ||
| for binpath in "$BR"/usr/bin/*; do | ||
| file "$binpath" | grep -q "not stripped" && strip --strip-unneeded "$binpath" |
There was a problem hiding this comment.
Under set -e, grep -q ... && strip ... makes the loop's exit status non-zero whenever a binary is already stripped (grep fails), which aborts the whole build. Use an explicit if statement to avoid the spurious failure.
Suggestion:
| file "$binpath" | grep -q "not stripped" && strip --strip-unneeded "$binpath" | |
| if file "$binpath" | grep -q "not stripped"; then | |
| strip --strip-unneeded "$binpath" | |
| fi |
| VERSION="$(git -C "$REPO_ROOT" describe --tags --abbrev=0 2>/dev/null | sed -e 's/^v//' || true)" | ||
| VERSION="${VERSION:-0.0.0-local}" |
There was a problem hiding this comment.
The version fallback can yield values invalid for package versioning (e.g. "0.0.0-local" or 0.1.0-5-gabc from git describe with suffixes — note --abbrev=0 only works when HEAD is exactly on a tag). Invalid strings will only surface as a confusing failure deep inside the deb/rpm builders. Validate against a strict pattern (e.g. ^[0-9]+.[0-9]+.[0-9]+$) before use.
Suggestion:
| VERSION="$(git -C "$REPO_ROOT" describe --tags --abbrev=0 2>/dev/null | sed -e 's/^v//' || true)" | |
| VERSION="${VERSION:-0.0.0-local}" | |
| if ! [[ "$VERSION" =~ ^[0-9]+\.[0-9]+\.[0-9]+$ ]]; then | |
| echo "invalid version string: '$VERSION'" >&2; exit 1 | |
| fi |
| declare -A BIN_DIR | ||
| for arch in "${ARCHES[@]}"; do | ||
| BIN_DIR[$arch]="$REPO_ROOT/target/$arch-unknown-linux-gnu/release" | ||
| [ "$arch" = x86_64 ] && BIN_DIR[$arch]="$REPO_ROOT/target/release" |
There was a problem hiding this comment.
This bare [ ... ] && ... evaluates to exit status 1 when arch is aarch64, and since it is the last command of the loop body under set -euo pipefail, the whole script aborts before any packaging runs. Guard it so a false condition doesn't fail the loop.
Suggestion:
| [ "$arch" = x86_64 ] && BIN_DIR[$arch]="$REPO_ROOT/target/release" | |
| if [ "$arch" = x86_64 ]; then | |
| BIN_DIR[$arch]="$REPO_ROOT/target/release" | |
| fi |
| - uses: actions/checkout@v5 | ||
| with: | ||
| submodules: recursive |
There was a problem hiding this comment.
actions/checkout@v5 defaults to fetch-depth: 1, which does not fetch tags. On workflow_dispatch (non-tag ref), git describe --tags --abbrev=0 will fail and every package silently builds as version 0.0.0-ci; if tags are partially available it could also pick a stale previous-release tag and mislabel published artifacts. Add fetch-depth: 0 to the checkout step so the describe fallback resolves the real latest tag.
Suggestion:
| - uses: actions/checkout@v5 | |
| with: | |
| submodules: recursive | |
| - uses: actions/checkout@v5 | |
| with: | |
| submodules: recursive | |
| fetch-depth: 0 |
| docker run --rm --platform linux/$ARCH -v "$PWD":/work -w /work fedora:latest \ | ||
| packaging/rpm/build.sh \ | ||
| --package node --rpmarch $ARCH \ |
There was a problem hiding this comment.
All four RPM build/smoke steps run inside fedora:latest, a mutable tag: image contents (and the rpmbuild/toolchain behavior the packaging scripts rely on) can change between release runs, breaking reproducibility or introducing supply-chain risk for published artifacts. Pin a versioned tag (e.g. fedora:40/41) or a digest, as done with debian:12 for the deb side.
Suggestion:
| docker run --rm --platform linux/$ARCH -v "$PWD":/work -w /work fedora:latest \ | |
| packaging/rpm/build.sh \ | |
| --package node --rpmarch $ARCH \ | |
| docker run --rm --platform linux/$ARCH -v "$PWD":/work -w /work fedora:41 \ | |
| packaging/rpm/build.sh \ | |
| --package node --rpmarch $ARCH \ |
| test -x "/usr/bin/mintlayer-$bin" | ||
| "/usr/bin/mintlayer-$bin" --help >/dev/null 2>&1 |
There was a problem hiding this comment.
No timeout around the --help probes: a daemon binary that ignores --help and waits on stdin (or blocks on network/config init in the bare container) will hang this smoke test — and the CI job that runs it — indefinitely. Wrap the invocation in timeout 30 ... and fail explicitly.
Suggestion:
| test -x "/usr/bin/mintlayer-$bin" | |
| "/usr/bin/mintlayer-$bin" --help >/dev/null 2>&1 | |
| test -x "/usr/bin/mintlayer-$bin" | |
| timeout 30 "/usr/bin/mintlayer-$bin" --help >/dev/null 2>&1 |
| test -x "/usr/bin/mintlayer-$bin" | ||
| "/usr/bin/mintlayer-$bin" --help >/dev/null 2>&1 |
There was a problem hiding this comment.
Same concern as the deb smoke script: the --help probes have no timeout, so a binary that blocks instead of exiting can hang the container. Also note head -n -2 on rpm -q --requires output assumes a fixed trailing-line count; consider grep -v ... on the marker lines instead. Add timeout around binary invocations here too.
Suggestion:
| test -x "/usr/bin/mintlayer-$bin" | |
| "/usr/bin/mintlayer-$bin" --help >/dev/null 2>&1 | |
| test -x "/usr/bin/mintlayer-$bin" | |
| timeout 30 "/usr/bin/mintlayer-$bin" --help >/dev/null 2>&1 |
| MISSING_LIBS=0 | ||
| for binpath in "$PKGDIR"/usr/bin/*; do | ||
| if ldd "$binpath" 2>/dev/null | grep -q "not found"; then |
There was a problem hiding this comment.
Running under emulation is documented, but nothing verifies the binaries match the host arch. On an arch mismatch, ldd reports the ELF interpreter itself as 'not found' (misleading 'install missing libraries' error) and every --help fails, silently shipping stub man pages. Fail fast with an explicit arch check.
Suggestion:
| MISSING_LIBS=0 | |
| for binpath in "$PKGDIR"/usr/bin/*; do | |
| if ldd "$binpath" 2>/dev/null | grep -q "not found"; then | |
| # Refuse to build if the binaries' architecture doesn't match this host. | |
| HOST_ARCH="$(dpkg --print-architecture)" | |
| BIN_ARCH="$(dpkg --print-architecture <(file -L "$PKGDIR"/usr/bin/* | head -n1))" | |
| [ "$HOST_ARCH" = "$BIN_ARCH" ] || { echo "binary arch ($BIN_ARCH) != host arch ($HOST_ARCH); run in a matching container" >&2; exit 1; } | |
| MISSING_LIBS=0 | |
| for binpath in "$PKGDIR"/usr/bin/*; do | |
| if ldd "$binpath" 2>/dev/null | grep -q "not found"; then |
| # Lint (errors fail; warnings are reported but allowed) | ||
| # --------------------------------------------------------------------------- | ||
| dnf install -y -q rpmlint >/dev/null | ||
| rpmlint "$OUT_DIR/$ARTIFACT" | tee "$OUT_DIR/rpmlint.log" |
There was a problem hiding this comment.
With set -euo pipefail, rpmlint ... | tee ... will abort the script immediately whenever rpmlint exits non-zero (which it does when it reports errors). That means the intended graceful handling below — teeing the log and grepping for : E: to print "rpmlint found errors" — is unreachable in the error case: the script dies at this line with no explanatory message and before rpmlint.log has served its purpose of a kept artifact... actually tee does write the log first, but the custom error message and exit path never execute. Disable immediate exit for this pipeline (e.g. rpmlint ... | tee ... || true) and let the grep check decide pass/fail.
Suggestion:
| rpmlint "$OUT_DIR/$ARTIFACT" | tee "$OUT_DIR/rpmlint.log" | |
| rpmlint "$OUT_DIR/$ARTIFACT" | tee "$OUT_DIR/rpmlint.log" || true | |
| if grep -qE "^${RPM_BASE}\\.[a-z0-9_]+: E:" "$OUT_DIR/rpmlint.log"; then ... |
| if [ "$SKIP_BUILD" -eq 1 ]; then | ||
| if [ -n "$BINARIES_OVERRIDE" ]; then | ||
| for pair in ${BINARIES_OVERRIDE//,/ }; do | ||
| [ -n "${pair%%=*}" ] && BIN_DIR[${pair%%=*}]="${pair##*=}" | ||
| done | ||
| fi | ||
| RESULTS=() |
There was a problem hiding this comment.
Broken control flow: the if [ "$SKIP_BUILD" -eq 1 ] block is interleaved with unrelated code. RESULTS=() and the run_step() definition are nested inside the SKIP_BUILD branch, a second for arch loop is opened at line 120 and closed by the done at line 123, and the else/fi at lines 124/153 pair across these blocks. This is a paste/merge error: run_step, RESULTS, and the icons step only exist when --skip-build is set, and the script is not valid bash in either branch shape (unbalanced for/done inside the if-body). Restructure into sequential sections or functions so the build/skip-build choice only affects binary location.
Suggestion:
| if [ "$SKIP_BUILD" -eq 1 ]; then | |
| if [ -n "$BINARIES_OVERRIDE" ]; then | |
| for pair in ${BINARIES_OVERRIDE//,/ }; do | |
| [ -n "${pair%%=*}" ] && BIN_DIR[${pair%%=*}]="${pair##*=}" | |
| done | |
| fi | |
| RESULTS=() | |
| if [ "$SKIP_BUILD" -eq 1 ]; then | |
| if [ -n "$BINARIES_OVERRIDE" ]; then | |
| for pair in ${BINARIES_OVERRIDE//,/ }; do | |
| [ -n "${pair%%=*}" ] && BIN_DIR[${pair%%=*}]="${pair##*=}" | |
| done | |
| fi | |
| for arch in "${ARCHES[@]}"; do | |
| test -f "${BIN_DIR[$arch]}/node-daemon" || | |
| { echo "no prebuilt binary for $arch at ${BIN_DIR[$arch]}" >&2; exit 1; } | |
| done | |
| else | |
| ... | |
| fi | |
| RESULTS=() | |
| run_step() { ... } |
| # node-gui binary lives in the same target dir as the other binaries | ||
| GUI_BIN="${BIN_DIR[${ARCHES[0]}]}/node-gui" | ||
| test -f "$GUI_BIN" || { echo "node-gui binary not found at $GUI_BIN" >&2; exit 1; } |
There was a problem hiding this comment.
GUI_BIN is only validated for the first architecture; with aarch64 enabled and only x86_64 prebuilts present, the arm64 deb-gui/rpm-gui packaging steps will fail deep inside the builders with an unclear error instead of the explicit early check the node binaries get.
Suggestion:
| # node-gui binary lives in the same target dir as the other binaries | |
| GUI_BIN="${BIN_DIR[${ARCHES[0]}]}/node-gui" | |
| test -f "$GUI_BIN" || { echo "node-gui binary not found at $GUI_BIN" >&2; exit 1; } | |
| for arch in "${ARCHES[@]}"; do | |
| test -f "${BIN_DIR[$arch]}/node-gui" || | |
| { echo "node-gui binary not found for $arch at ${BIN_DIR[$arch]}/node-gui" >&2; exit 1; } | |
| done |
| run_step "smoke rpm node ($arch)" \ | ||
| docker run --rm -v "$REPO_ROOT:/work" -w /work fedora:latest \ | ||
| packaging/checks/smoke-rpm.sh packaging/dist/Mintlayer_Node_linux_${VERSION}_${arch}.rpm \ | ||
| mintlayer-node node |
There was a problem hiding this comment.
The deb smoke steps correctly pass $platform so arm64 debs are installed in an arm64 container, but the rpm smoke steps omit it. With aarch64 enabled (non-quick mode), this runs dnf install ./packaging/dist/..._aarch64.rpm inside the host-arch (x86_64) fedora:latest container, which cannot execute the aarch64 payload (postinst scriptlets, --help probes) and will fail or misreport. Pass the same $platform used for the rpm packaging steps.
Suggestion:
| run_step "smoke rpm node ($arch)" \ | |
| docker run --rm -v "$REPO_ROOT:/work" -w /work fedora:latest \ | |
| packaging/checks/smoke-rpm.sh packaging/dist/Mintlayer_Node_linux_${VERSION}_${arch}.rpm \ | |
| mintlayer-node node | |
| run_step "smoke rpm node ($arch)" \ | |
| docker run --rm $platform -v "$REPO_ROOT:/work" -w /work fedora:latest \ | |
| packaging/checks/smoke-rpm.sh packaging/dist/Mintlayer_Node_linux_${VERSION}_${arch}.rpm \ | |
| mintlayer-node node |
| run_step "smoke rpm gui ($arch)" \ | ||
| docker run --rm -v "$REPO_ROOT:/work" -w /work fedora:latest \ | ||
| packaging/checks/smoke-rpm.sh packaging/dist/Mintlayer_Node_GUI_linux_${VERSION}_${arch}.rpm \ | ||
| mintlayer-node-gui gui |
There was a problem hiding this comment.
The rpm gui smoke step has the same missing $platform issue: aarch64 rpms would be installed in a host-arch fedora container and the binary/desktop checks would fail or not exercise the real package.
Suggestion:
| run_step "smoke rpm gui ($arch)" \ | |
| docker run --rm -v "$REPO_ROOT:/work" -w /work fedora:latest \ | |
| packaging/checks/smoke-rpm.sh packaging/dist/Mintlayer_Node_GUI_linux_${VERSION}_${arch}.rpm \ | |
| mintlayer-node-gui gui | |
| run_step "smoke rpm gui ($arch)" \ | |
| docker run --rm $platform -v "$REPO_ROOT:/work" -w /work fedora:latest \ | |
| packaging/checks/smoke-rpm.sh packaging/dist/Mintlayer_Node_GUI_linux_${VERSION}_${arch}.rpm \ | |
| mintlayer-node-gui gui |
| concurrency: | ||
| group: release_linux-${{ github.ref }} | ||
| cancel-in-progress: true |
There was a problem hiding this comment.
cancel-in-progress: true on a release workflow means a new tag push or dispatch while a release build is running will cancel the in-flight build — potentially mid-way through uploads (triggering tag pushes, e.g. by release automation, make this realistic). For release/tag-triggered workflows, cancellation of a partially-published release is usually worse than a redundant run. Consider scoping cancellation to non-tag events, or setting cancel-in-progress: false.
| if [ -z "$VERSION" ]; then | ||
| VERSION="0.0.0-ci" | ||
| fi |
There was a problem hiding this comment.
The 0.0.0-ci fallback (and the git describe fallback above it) silently proceeds to package, smoke-test, and upload artifacts with a placeholder or possibly-wrong version. Because artifact and file names are keyed on this version, a placeholder build can publish misleadingly named release artifacts, and two different fallback runs collide on the same artifact names. Consider failing the job (or at least warning) when the version cannot be derived from a tag.
Suggestion:
| if [ -z "$VERSION" ]; then | |
| VERSION="0.0.0-ci" | |
| fi | |
| if [ -z "$VERSION" ]; then | |
| echo "::error::Could not derive version from tag or git describe" | |
| exit 1 | |
| fi |
| sed -e "s/@VERSION@/$DEB_VERSION/g" \ | ||
| -e "s/@DEBARCH@/$DEBARCH/g" \ | ||
| -e "s/@INSTALLED_SIZE@/$INSTALLED_SIZE/g" \ | ||
| -e "s/@SHLIBS@/$SHLIBS/g" \ |
There was a problem hiding this comment.
Interpolating $SHLIBS (raw dpkg-shlibdeps output) into a sed replacement is fragile: any &, \, or / appearing in a future dependency string would silently corrupt DEBIAN/control. A delimiter-safe substitution (e.g. awk -v or escaping the metacharacters) is more robust. Also note the dpkg-shlibdeps | grep | cut pipeline only detects a fully-empty result; a partial/garbled shlibs output would pass this guard, so consider checking the exit status of dpkg-shlibdeps via set -o pipefail semantics explicitly.
Suggestion:
| -e "s/@SHLIBS@/$SHLIBS/g" \ | |
| -e "s/@SHLIBS@/$(printf '%s' "$SHLIBS" | sed -e 's/[&\/]/\\&/g')/g" \ |
| if [ -d /var/lib/mintlayer ]; then | ||
| chown -R root:root /var/lib/mintlayer >/dev/null 2>&1 || true | ||
| fi |
There was a problem hiding this comment.
On purge, chown -R root:root /var/lib/mintlayer recursively rewrites ownership of the entire chain-state tree, which can be many gigabytes / millions of inodes and will stall dpkg for a long time during removal. Only the top-level directory actually needs reassignment once the mintlayer user is deleted; children can keep the dangling numeric UID harmlessly.
Suggestion:
| if [ -d /var/lib/mintlayer ]; then | |
| chown -R root:root /var/lib/mintlayer >/dev/null 2>&1 || true | |
| fi | |
| if [ -d /var/lib/mintlayer ]; then | |
| chown root:root /var/lib/mintlayer >/dev/null 2>&1 || true | |
| fi |
| width="$(identify -format '%w' "$SRC" 2>/dev/null || echo 0)" | ||
| [ "${width:-0}" -ge 512 ] || { | ||
| echo "error: $SRC is ${width}px wide, need at least 512" >&2 | ||
| exit 1 | ||
| } |
There was a problem hiding this comment.
The guard only checks width, not height, and never verifies the source is actually 512x512. A non-square or 512x800 source would be copied verbatim as the 512x512 hicolor entry and resized inconsistently for the other sizes, producing a malformed icon set. Consider validating both dimensions (and ideally enforcing exactly 512x512, or center-cropping to square before resizing).
Suggestion:
| width="$(identify -format '%w' "$SRC" 2>/dev/null || echo 0)" | |
| [ "${width:-0}" -ge 512 ] || { | |
| echo "error: $SRC is ${width}px wide, need at least 512" >&2 | |
| exit 1 | |
| } | |
| read -r width height < <(identify -format '%w %h' "$SRC" 2>/dev/null || echo "0 0") | |
| [ "${width:-0}" -ge 512 ] && [ "${height:-0}" -ge 512 ] || { | |
| echo "error: $SRC is ${width}x${height}, need at least 512x512" >&2 | |
| exit 1 | |
| } |
| case "$VERSION" in | ||
| ''|*[!0-9.+-a-zA-Z~]*) echo "invalid --version: $VERSION" >&2; exit 2 ;; | ||
| esac |
There was a problem hiding this comment.
The version character-class allows /, &, and %, all of which are problematic downstream: / and & are special in sed "s/@VERSION@/$RPM_VERSION/g" (breaking or injecting into the spec), and %/- can corrupt the RPM NEVR since @Version@ is substituted directly into Version: in the spec template. Restrict the charset to what a valid RPM version can actually contain (alnum, ., _, +, ~, and an interior - only after X.Y.Z), e.g. ''|*[!0-9A-Za-z._+~-]*. The comment about bash 5.3 tilde expansion only concerns the replacement character, not the input charset.
Suggestion:
| case "$VERSION" in | |
| ''|*[!0-9.+-a-zA-Z~]*) echo "invalid --version: $VERSION" >&2; exit 2 ;; | |
| esac | |
| case "$VERSION" in | |
| ''|*[!0-9A-Za-z._+~-]*) echo "invalid --version: $VERSION" >&2; exit 2 ;; | |
| esac |
| dnf install -y -q rpm-build systemd-rpm-macros help2man file binutils \ | ||
| dbus-libs libusb1 systemd-libs >/dev/null |
There was a problem hiding this comment.
The gui fallback path invokes make-icons.sh, which requires ImageMagick (magick/convert/identify), but the self-provisioning dnf install above never installs the imagemagick package (the deb builder does, via NEED_INSTALL+=(imagemagick)). In a fresh fedora:latest container without pre-generated $OUT_DIR/assets/icons/usr, a gui build aborts with "ImageMagick not found" instead of self-provisioning.
Suggestion:
| dnf install -y -q rpm-build systemd-rpm-macros help2man file binutils \ | |
| dbus-libs libusb1 systemd-libs >/dev/null | |
| dnf install -y -q rpm-build systemd-rpm-macros help2man file binutils \ | |
| dbus-libs libusb1 systemd-libs ImageMagick >/dev/null |
| RESULTS=() | ||
| FAILED=0 | ||
| run_step() { # run_step <label> <cmd...> — records the result, never aborts |
There was a problem hiding this comment.
Control-flow bug: RESULTS=() and the run_step() definition are nested inside the if [ "$SKIP_BUILD" -eq 1 ] branch (note the misplaced for arch ... done and the else right after). On the default path (SKIP_BUILD=0) run_step is never defined, so the first run_step "icons" ... call fails with "command not found" and set -e aborts the script — the harness is broken for its primary use case (full build + package + smoke). Move RESULTS=()/FAILED=0, the run_step() definition, and the binary-existence validation loop out of the if/else so they execute unconditionally.
Suggestion:
| RESULTS=() | |
| FAILED=0 | |
| run_step() { # run_step <label> <cmd...> — records the result, never aborts | |
| RESULTS=() | |
| FAILED=0 | |
| run_step() { # run_step <label> <cmd...> — records the result, never aborts | |
| local label="$1"; shift | |
| echo "" | |
| echo "=== $label ===" | |
| if "$@"; then | |
| RESULTS+=("PASS $label") | |
| else | |
| local rc=$? | |
| RESULTS+=("FAIL $label (rc=$rc)") | |
| FAILED=1 | |
| fi | |
| return 0 | |
| } | |
| if [ "$SKIP_BUILD" -eq 1 ]; then | |
| for arch in "${ARCHES[@]}"; do | |
| test -f "${BIN_DIR[$arch]}/node-daemon" || | |
| { echo "no prebuilt binary for $arch at ${BIN_DIR[$arch]}" >&2; exit 1; } | |
| done | |
| else |
e530e1b to
326d53a
Compare
| export DEBIAN_FRONTEND=noninteractive | ||
| apt-get update -qq | ||
|
|
||
| apt-get install -y -qq systemd desktop-file-utils >/dev/null 2>&1 || true |
There was a problem hiding this comment.
apt-get install -y -qq systemd desktop-file-utils >/dev/null 2>&1 || true silently swallows install failures. If systemd (or desktop-file-utils) is not installed, the later systemd-analyze verify / preset / desktop-file-validate checks fail with opaque errors unrelated to the package under test, undermining the smoke test's diagnostics. Drop || true so a prerequisite failure aborts with a clear message.
| case "$VERSION" in | ||
| ''|*[!0-9.+-a-zA-Z~]*) echo "invalid --version: $VERSION" >&2; exit 2 ;; | ||
| esac |
There was a problem hiding this comment.
The bracket expression 0-9.+-a-zA-Z~ does not enumerate the intended characters: in POSIX bracket expressions, +-a is parsed as a range from '+' (0x2B) to 'a' (0x61), which also accepts '/', ':', ';', '<', '=', '>', '?', '@', '[' and similar characters. E.g. a version like "1.4.0:x" passes this guard. '/' is invalid in Debian versions and other metacharacters may later corrupt the sed substitutions or the .deb metadata. Spell the set explicitly (e.g. [!0-9.+~A-Za-z-] with '-' placed first/last) or validate with a POSIX-extended regex.
Suggestion:
| case "$VERSION" in | |
| ''|*[!0-9.+-a-zA-Z~]*) echo "invalid --version: $VERSION" >&2; exit 2 ;; | |
| esac | |
| case "$VERSION" in | |
| ''|*[!0-9.+~A-Za-z-]*) echo "invalid --version: $VERSION" >&2; exit 2 ;; | |
| esac |
| if [ ! -d /var/lib/mintlayer ]; then | ||
| mkdir -p /var/lib/mintlayer | ||
| chown mintlayer:mintlayer /var/lib/mintlayer || true | ||
| fi |
There was a problem hiding this comment.
If /var/lib/mintlayer already exists (e.g. created by an admin, another package, or as a symlink planted by a local user) the postinst unconditionally takes ownership of it and hands it to the 'mintlayer' system user. On a pre-planted symlink this follows the link and transfers ownership of an arbitrary directory. Guard with an ownership check (or create it at build time as a dpkg directory entry) instead of chown-ing whatever is already there.
Suggestion:
| if [ ! -d /var/lib/mintlayer ]; then | |
| mkdir -p /var/lib/mintlayer | |
| chown mintlayer:mintlayer /var/lib/mintlayer || true | |
| fi | |
| if [ ! -d /var/lib/mintlayer ]; then | |
| mkdir -p /var/lib/mintlayer | |
| fi | |
| if [ "$(stat -c '%U:%G' /var/lib/mintlayer)" = 'root:root' ]; then | |
| chown mintlayer:mintlayer /var/lib/mintlayer || true | |
| fi |
| mkdir -p "$MAN_DIR" | ||
| for binpath in "$BR"/usr/bin/*; do | ||
| binname="$(basename "$binpath")" | ||
| if "$binpath" --help >/dev/null 2>&1; then |
There was a problem hiding this comment.
For cross-target builds (e.g. aarch64 RPMs built in an x86_64 container), this loop executes every payload binary with --help; foreign-arch ELFs fail with exec format error, so help2man is skipped and every binary in the package silently (only a stderr warning) gets the generic stub man page instead of real documentation. Consider detecting the target arch mismatch up front and either generating stubs deliberately or failing/warning once with a clear summary, rather than emitting N warnings that are easy to miss in CI logs.
Suggestion:
| if "$binpath" --help >/dev/null 2>&1; then | |
| if [ "$RPMARCH" != "$HOST_ARCH" ]; then | |
| echo "cross-target build: shipping stub man pages for $binname" >&2 | |
| elif "$binpath" --help >/dev/null 2>&1; then |
| --version) VERSION="$2"; shift 2 ;; | ||
| --binaries-dir) BINARIES_OVERRIDE="$2"; shift 2 ;; |
There was a problem hiding this comment.
--version and --binaries-dir read $2 and shift 2 without checking the value exists; if the flag is passed without a value, set -u aborts with a confusing 'unbound variable' error instead of a usage message.
Suggestion:
| --version) VERSION="$2"; shift 2 ;; | |
| --binaries-dir) BINARIES_OVERRIDE="$2"; shift 2 ;; | |
| --version) [ $# -ge 2 ] || { echo "--version requires a value" >&2; exit 2; }; VERSION="$2"; shift 2 ;; | |
| --binaries-dir) [ $# -ge 2 ] || { echo "--binaries-dir requires a value" >&2; exit 2; }; BINARIES_OVERRIDE="$2"; shift 2 ;; |
Replace the bare-binaries deb/rpm packaging with proper distribution packages: - deb: computed shared-library Depends (dpkg-shlibdeps), maintscripts using deb-systemd-helper, conffiles under /etc/mintlayer, changelog, copyright, lintian gate - rpm: systemd scriptlets, sysusers, %config(noreplace), %license, rpmlint gate; aarch64 rpms now ship alongside x86_64 - both: hardened template units per daemon (mainnet node enabled via preset, everything else opt-in), mintlayer system user, Ledger/Trezor udev rules, man pages generated from --help, hicolor icons and a validated desktop entry for the GUI release_linux.yml now runs the same packaging/ scripts inside debian:12 / fedora:latest containers, smoke-tests every artifact in a fresh container, and can be dispatched manually without a tag. packaging/test-local.sh replicates the whole pipeline locally.
A version without a hyphen (1.4.0) makes the package native, while a tagged pre-release like 1.4.0-rc1 makes it non-native; lintian expects different changelog names in each case and the CI dry-run failed on 0.0.0-ci. Always append revision -1 when the version has none and ship changelog.Debian.gz, which is correct for every version shape.
The runner image ships qemu-user binaries but no binfmt_misc registration, so 'docker run --platform linux/arm64' failed with 'exec format error'. Use docker/setup-qemu-action, and stop cancelling the sibling matrix leg when one arch fails.
- tilde-expands the replacement word on bash 5.3+ (fedora container), mangling pre-release versions into a $HOME path; use tr - fedora binutils cannot strip foreign-arch ELF, so skip strip when the container arch differs from the rpm target (aarch64 rpms ship unstripped instead of failing the build) - provision the runtime libraries (dbus-libs, libusb1, systemd-libs) so help2man generates real man pages instead of stubs
The GUI rpm builder needs the shared icon set (ImageMagick is not installed in the fedora container), and running the rpm builder and smoke tests in an arch-matched container gives native strip plus real help2man man pages on the arm64 leg (binaries run under qemu).
Hardening from the OCR review (real findings only; several others were already addressed by the arch-matched containers): workflow: - fix tag-build version extraction: the [ .. ] && .. idiom aborted under bash -e exactly when building from a real tag; use if-statements and fetch full history for the dispatch fallback - pin fedora:44 (reproducible packaging), add concurrency group, timeout-minutes and least-privilege permissions deb builder: - validate the version string; only treat a trailing dash component as a Debian revision when it starts with a digit (1.4.0-rc1 -> 1.4.0-rc1-1) - make a dpkg-shlibdeps failure fatal instead of shipping empty Depends - strip via an if-guard so an already-stripped binary cannot trip errexit rpm builder: - gate rpmlint on its exit code instead of parsing output lines keyed by NEVRA (never matched the artifact filename) - pick up the built rpm via an exact single-match find, no globs - drop the duplicated rpmbuild retry and the output filter that hid early spec errors maintscripts: - prerm stops services on upgrade too (binaries get replaced) - postinst keeps stderr on start, recreates /var/lib/mintlayer - postrm chowns the kept datadir to root before deleting the user checks and helpers: - smoke scripts resolve package paths, fail loudly on missing units or missing helper packages, and assert the full conffile set - verify-artifacts validates arch-pair arguments - make-icons validates arguments, prefers magick over deprecated convert, and rejects undersized sources - test-local: fix the set -e hazards in the arch loops and make run_step non-fatal so the summary always prints
326d53a to
403be7a
Compare
2 participants
Summary
Replaces the bare-binaries deb/rpm packaging with proper distribution packages and makes the whole release packaging pipeline locally reproducible.
What the packages now contain
mintlayer-node: node-daemon, wallet-rpc-daemon, api-web-server, api-blockchain-scanner-daemon, dns-server, wallet-cli, wallet-address-generatormintlayer-node@<chain>.serviceruns as system user with--datadir /var/lib/mintlayer/<chain>,ProtectSystem=strict, journald logging; other daemons opt-in via env-file-driven units)mintlayersystem user (sysusers.d), mainnet-only preset policy (all other units opt-in)uaccess), man pages generated from--help,/etc/mintlayerconffilesmintlayer-node-gui: binary + hicolor icons + validated desktop entry + man pageCorrectness fixes over the current packaging
Dependscomputed viadpkg-shlibdeps(previously none — broken on clean installs), maintscripts viadeb-systemd-helper, conffiles flagged, changelog/copyright, lintian gateRequires, systemd scriptlets (%systemd_post/preun/postun),%sysusers_create_package,%config(noreplace),%license, rpmlint gate; aarch64 rpms now ship (previously x86_64-only); misusedBuildArch: x86_64removedLocal reproducibility
packaging/test-local.shreplicates the full release pipeline locally in the same container images CI uses (debian:12 / fedora:latest): build (glibc-floor parity), package, lint, and install-smoke every artifact in fresh containers, then verify artifact names against the release globs. Validated end-to-end locally: 9/9 steps green.release_linux.ymlnow invokes the samepackaging/scripts per matrix arch and gainedworkflow_dispatchfor no-tag dry runs (recommended before tagging, to exercise the arm64 legs).release.ymlattaches them as before.🤖 Generated with OpenCode