Add Arch packages and proper Windows installers to the packaging pipeline - #2118
Conversation
…line
Arch Linux (.pkg.tar.zst), mirroring the deb/rpm builders:
- packaging/arch/build.sh runs in a pinned archlinux:base container; makepkg
as a dedicated build user repackages the prebuilt release binaries
(PKGBUILD templates, install scriptlet applies the preset policy)
- dependencies resolved from the binaries via pacman -F on the arch-matched
x86_64 leg; the aarch64 leg repackages cross-target (Arch publishes no
arm64 images) with a static fallback map, unstripped binaries and stub man
pages
- namcap gate (keyed on its ' E: ' output: namcap always exits 0) and
fresh-container install smoke tests; arm64 skips the install smoke since
pacman refuses foreign-arch packages
- release_linux.yml builds + uploads both arches; test-local.sh replicates
Windows (NSIS), replacing the GUI-only setup.exe:
- renderable templates (build-tools/win/nsi/*.nsi.in + common.nsh macros)
and create-nsis-installers.ps1 produce two installers: a new node Setup
(all CLI tools, optional PATH entry with length-gated registry rewrite and
backup, optional mainnet service + TCP firewall rule) and an improved GUI
setup (all-users shortcuts, upgrade handling, fixed the broken
\${SMPROGRAMS} constant carried over from the old script)
- smoke-install.ps1 silent-installs, verifies files/registry/PATH/--help and
uninstalls on the CI runner; release_windows.yml gains a workflow_dispatch
dry run and version fallback like the Linux workflow
Shared helpers (packaging/common/lib.sh): the 7-binary list, version
validation (the deb/rpm charset checks rejected any suffixed version - e.g.
1.4.1-rc1 - due to a glob range quirk) and man-page generation, now
single-sourced across the three builders and smoke tests; container image
pins moved to packaging/images.env (also fixes the fedora:latest drift in
test-local.sh); test-local.sh run_step/summary block moved out of the
--skip-build branch where a default (building) run could never reach it.
Checkout hardening: persist-credentials: false and read-only permissions on
the packaging jobs.
|
🔍 OpenCodeReview found 22 issue(s) in this PR.
📄
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| - name: Smoke test Arch packages | ||
| if: matrix.arch == 'x86_64' |
There was a problem hiding this comment.
Unlike the deb and rpm legs (whose smoke tests run under qemu via --platform linux/arm64 for both matrix archs), the Arch arm64 package is built and uploaded without any install/runtime verification: this smoke step is gated to x86_64 and the smoke-arch.sh arm64 leg never runs. A broken aarch64 payload (wrong binary, missing files, bad .PKGINFO) would ship silently. Consider adding a container-free verification (e.g. extract the .pkg.tar.zst with tar and check binaries/paths/.PKGINFO arch, similar to verify-artifacts.sh) for the aarch64 leg instead of relying only on the builder's self-check.
| .\build-tools\win\smoke-install.ps1 -Installer "Mintlayer_Node_win_${{ steps.get_version.outputs.VERSION }}_Setup.exe" -AppName "Mintlayer Node" -Kind node -Version "${{ steps.get_version.outputs.VERSION }}" | ||
| .\build-tools\win\smoke-install.ps1 -Installer "Mintlayer_Node_GUI_win_${{ steps.get_version.outputs.VERSION }}_Setup.exe" -AppName "Mintlayer Node GUI" -Kind gui -Version "${{ steps.get_version.outputs.VERSION }}" |
There was a problem hiding this comment.
The extracted VERSION is interpolated directly into this pwsh run: block via ${{ steps.get_version.outputs.VERSION }}. On a non-tag trigger the fallback uses git describe output, and a crafted tag/branch name (or ref) could break out of the quoted string and execute arbitrary code before the regex validation inside the .ps1 scripts ever runs. Pass the value through an env: block and reference $env:VERSION instead.
Suggestion:
| .\build-tools\win\smoke-install.ps1 -Installer "Mintlayer_Node_win_${{ steps.get_version.outputs.VERSION }}_Setup.exe" -AppName "Mintlayer Node" -Kind node -Version "${{ steps.get_version.outputs.VERSION }}" | |
| .\build-tools\win\smoke-install.ps1 -Installer "Mintlayer_Node_GUI_win_${{ steps.get_version.outputs.VERSION }}_Setup.exe" -AppName "Mintlayer Node GUI" -Kind gui -Version "${{ steps.get_version.outputs.VERSION }}" | |
| env: | |
| VERSION: ${{ steps.get_version.outputs.VERSION }} | |
| run: | | |
| .\build-tools\win\smoke-install.ps1 -Installer "Mintlayer_Node_win_${VERSION}_Setup.exe" -AppName "Mintlayer Node" -Kind node -Version "${VERSION}" |
| if ($Version -notmatch '^[0-9][0-9A-Za-z.~+-]*$') { | ||
| throw "invalid version '$Version' (expected digits-first X.Y.Z[-suffix])" | ||
| } |
There was a problem hiding this comment.
The Windows version validation is looser than the Linux one in packaging/common/lib.sh (validate_version requires X.Y.Z with an optional -suffix). Here, strings like "1", "1.2" or "1.2.3-rc1-x-y" pass. Since both package families ship from the same tag, keeping a single consistent rule avoids producing a Windows installer filename that the Linux gates would reject (or vice versa). Consider requiring at least two dots, mirroring the X.Y.Z[-suffix] rule.
Suggestion:
| if ($Version -notmatch '^[0-9][0-9A-Za-z.~+-]*$') { | |
| throw "invalid version '$Version' (expected digits-first X.Y.Z[-suffix])" | |
| } | |
| if ($Version -notmatch '^[0-9]+\.[0-9]+\.[0-9]+([-+~][0-9A-Za-z.~+-]*)?$') { | |
| throw "invalid version '$Version' (expected X.Y.Z[-suffix])" | |
| } |
| $InstallDir = Join-Path $env:ProgramFiles (Join-Path "Mintlayer" $AppName) | ||
| $UninstKey = "HKLM:\SOFTWARE\Microsoft\Windows\CurrentVersion\Uninstall\Mintlayer $AppName" |
There was a problem hiding this comment.
The smoke test assumes a 64-bit PowerShell: it reads $env:ProgramFiles and HKLM:\SOFTWARE...\Uninstall. The templates install to $PROGRAMFILES64 and write the uninstall key in the native registry view, so under 32-bit PowerShell (e.g. Windows PowerShell x86, or a CI step launched from a 32-bit shell) $InstallDir would resolve to 'Program Files (x86)' and Get-ItemProperty would be redirected to WOW6432Node, causing spurious failures. Add an explicit 64-bit check up front (or use $env:ProgramW6432 and the Sysnative path).
Suggestion:
| $InstallDir = Join-Path $env:ProgramFiles (Join-Path "Mintlayer" $AppName) | |
| $UninstKey = "HKLM:\SOFTWARE\Microsoft\Windows\CurrentVersion\Uninstall\Mintlayer $AppName" | |
| if ([Environment]::Is64BitProcess -ne $true) { | |
| throw "run this script under 64-bit PowerShell (installers use $PROGRAMFILES64 and the native HKLM view)" | |
| } | |
| $InstallDir = Join-Path $env:ProgramFiles (Join-Path "Mintlayer" $AppName) | |
| $UninstKey = "HKLM:\SOFTWARE\Microsoft\Windows\CurrentVersion\Uninstall\Mintlayer $AppName" |
| echo "$VERSION_FORMAT_ERROR" >&2 | ||
| exit 2 | ||
| fi | ||
| PKGVER="$(printf '%s' "$VERSION" | tr -- '-~' '__')" |
There was a problem hiding this comment.
validate_version permits '' but not '_', so mapping both '-' and '' to '_' makes distinct versions collide on the same pkgver (e.g. 1.4.1-rc1 and 1.4.1rc1 both become 1.4.1_rc1, which pacman treats as the same package version). The rpm builder maps only '-' to '', which is injective — do the same here.
Suggestion:
| PKGVER="$(printf '%s' "$VERSION" | tr -- '-~' '__')" | |
| PKGVER="$(printf '%s' "$VERSION" | tr -- '-' '_')" |
| for binpath in "$BR"/usr/bin/*; do | ||
| file "$binpath" | grep -q "not stripped" && strip --strip-unneeded "$binpath" | ||
| done |
There was a problem hiding this comment.
Under set -e, file ... | grep -q "not stripped" && strip ... aborts the whole build if a binary is already stripped (grep returns 1 and the && list's failure propagates). The deb builder deliberately uses an if wrapper for exactly this reason (see deb/build.sh comment about the "already stripped" match failure tripping errexit). Use the same form here.
Suggestion:
| for binpath in "$BR"/usr/bin/*; do | |
| file "$binpath" | grep -q "not stripped" && strip --strip-unneeded "$binpath" | |
| done | |
| for binpath in "$BR"/usr/bin/*; do | |
| if file "$binpath" | grep -q "not stripped"; then | |
| strip --strip-unneeded "$binpath" | |
| fi | |
| done |
| @@ -71,6 +78,7 @@ fi | |||
| echo "pulling container images..." | |||
| docker pull -q debian:12 >/dev/null | |||
| docker pull -q fedora:latest >/dev/null | |||
There was a problem hiding this comment.
images.env is sourced above and its header claims it is the single source of truth shared with CI, but this script still pulls and runs fedora:latest throughout instead of the pinned $FEDORA_IMAGE. Local rpm builds therefore drift from CI (fedora:44) and the pin provides no reproducibility here. Use "$FEDORA_IMAGE" in the docker pull and all fedora docker run invocations (debian:12 could be similarly pinned or noted as intentionally unpinned).
Suggestion:
| docker pull -q fedora:latest >/dev/null | |
| docker pull -q "$FEDORA_IMAGE" >/dev/null |
- release_linux.yml: smoke-test the Arch packages on both matrix legs; the
aarch64 leg installs the foreign-arch package via IgnoreArch and runs the
binaries through the qemu binfmt handlers, matching the deb/rpm legs
- release_windows.yml: pass the version and the workflow input through
environment variables instead of interpolating them into the pwsh run
blocks (script-injection hardening)
- create-nsis-installers.ps1: enforce the same strict version grammar as
packaging/common/lib.sh (X.Y.Z with an optional -suffix)
- smoke-install.ps1: fail fast under 32-bit PowerShell, which would read
the redirected WOW6432Node registry view and x86 Program Files
- packaging/common/lib.sh: reject '~' and '+' in versions so that the rpm
('-' -> '~') and Arch ('-' -> '_') version mappings stay injective;
distinct versions can no longer collide on the same package version
- packaging/arch/build.sh, packaging/rpm/build.sh: guard the strip step
with an if so an already-stripped binary cannot trip errexit
- packaging/test-local.sh: use the pinned $FEDORA_IMAGE everywhere
instead of fedora:latest, so local runs cannot drift from CI
| packaging/arch/build.sh \ | ||
| --package node --arch "$ARCH" \ | ||
| --version "${{ steps.get_version.outputs.VERSION }}" \ |
There was a problem hiding this comment.
The version string, which is derived from the attacker-controllable tag/ref (see the 'Extract version from tag' step: GITHUB_REF tag or unvalidated git describe output), is interpolated directly into these run: blocks via ${{ steps.get_version.outputs.VERSION }}. A crafted tag value can therefore inject shell commands into the packaging/smoke-test steps. The Windows workflow in this same change explicitly fixed this exact pattern by passing the value through an env: variable (see release_windows.yml 'Package Mintlayer Node' comment); the Linux workflow should do the same for consistency and safety. Additionally, like the confirmed Windows finding, the git-describe fallback here is never validated against the version grammar before being passed to packaging/build.sh.
Suggestion:
| packaging/arch/build.sh \ | |
| --package node --arch "$ARCH" \ | |
| --version "${{ steps.get_version.outputs.VERSION }}" \ | |
| env: | |
| VERSION: ${{ steps.get_version.outputs.VERSION }} | |
| run: | | |
| source packaging/images.env | |
| ARCH=${{ matrix.arch }} | |
| docker run --rm -v "$PWD":/work -w /work $ARCH_IMAGE \ | |
| packaging/arch/build.sh \ | |
| --package node --arch "$ARCH" \ | |
| --version "$VERSION" \ |
| $VERSION = git describe --tags --abbrev=0 2>$null | ||
| $VERSION = $VERSION -replace '^v', '' |
There was a problem hiding this comment.
The fallback version from git describe --tags --abbrev=0 is not validated against any grammar before being exported. A non-semver tag (e.g. v1.2 or a date-based tag) yields VERSION like 1.2, which makes create-nsis-installers.ps1 throw mid-release ("invalid version"), failing the workflow at the packaging step instead of being caught here. Validate or normalize the version at extraction time so failures surface early with a clear message.
Suggestion:
| $VERSION = git describe --tags --abbrev=0 2>$null | |
| $VERSION = $VERSION -replace '^v', '' | |
| $VERSION = git describe --tags --abbrev=0 2>$null | |
| $VERSION = $VERSION -replace '^v', '' | |
| if ($VERSION -notmatch '^[0-9]+\.[0-9]+\.[0-9]+(-[0-9A-Za-z.-]+)?$') { | |
| throw "derived version '$VERSION' is not X.Y.Z[-suffix]; tag a semver release" | |
| } |
| & $exe --help *> $null | ||
| if ($LASTEXITCODE -ne 0) { | ||
| throw "$bin --help exited with $LASTEXITCODE" | ||
| } |
There was a problem hiding this comment.
Each installed binary is executed with & $exe --help and its exit code checked, but there is no timeout around the invocation. If a binary ignores --help and starts serving (as daemons may), or blocks waiting for input, this loop hangs forever. The release_windows.yml job has no timeout-minutes either, so a hung smoke test consumes the runner indefinitely. Wrap the invocation in a bounded wait (e.g. Start-Process -PassThru + WaitForExit(timeout), kill on timeout) or add a job-level timeout-minutes in the workflow.
Suggestion:
| & $exe --help *> $null | |
| if ($LASTEXITCODE -ne 0) { | |
| throw "$bin --help exited with $LASTEXITCODE" | |
| } | |
| $proc = Start-Process -FilePath $exe -ArgumentList "--help" -NoNewWindow -Wait $false -PassThru -RedirectStandardOutput NUL -RedirectStandardError NUL | |
| if (-not $proc.WaitForExit(60000)) { | |
| $proc.Kill() | |
| throw "$bin --help timed out after 60s (possible daemon start)" | |
| } | |
| if ($proc.ExitCode -ne 0) { | |
| throw "$bin --help exited with $($proc.ExitCode)" | |
| } |
| echo "$VERSION_FORMAT_ERROR" >&2 | ||
| exit 2 | ||
| fi | ||
| PKGVER="$(printf '%s' "$VERSION" | tr -- '-' '_')" |
There was a problem hiding this comment.
Mapping '-' to '_' makes pre-release versions sort incorrectly under Arch's vercmp: pacman considers '1.4.1_rc1' NEWER than '1.4.1' (for the segment '1_rc1' vs '1', vercmp prefers the longer segment after the equal numeric prefix), so systems that installed the RC package would refuse to upgrade to the final release. The rpm builder maps '-' to '' precisely to sort before the final release; Arch has no '', so this builder either needs a different scheme (e.g. keep the plain '1.4.1' pkgver with pkgrel distinguishing, or document/accept the ordering quirk explicitly) or a guard rejecting pre-release versions.
Suggestion:
| PKGVER="$(printf '%s' "$VERSION" | tr -- '-' '_')" | |
| # Note: Arch vercmp treats '1.4.1_rc1' as newer than '1.4.1', so pre-release | |
| # versions do not sort before the final release. Reject them or handle ordering | |
| # explicitly. | |
| case "$VERSION" in | |
| *-*) echo "pre-release versions are not supported for Arch packages" >&2; exit 2 ;; | |
| esac | |
| PKGVER="$(printf '%s' "$VERSION" | tr -- '-')" |
| # installed, so provision them here (package names are x86_64 repo names, | ||
| # which is what the amd64 container's repos provide regardless of the target | ||
| # arch being packaged). | ||
| pacman -S --noconfirm --needed --asdeps $DEPENDS >/dev/null |
There was a problem hiding this comment.
If the ldd/pacman -F resolution ever produces an empty DEPENDS (e.g. all binaries statically linked, or LIBS filtered to nothing), this runs pacman -S --noconfirm --needed --asdeps with no package arguments, which fails with pacman's usage error rather than a clear message. Also, $DEPENDS is intentionally word-split here, which breaks on any package name containing whitespace (unlikely for Arch, but worth a defensive check). Consider an explicit [ -n "$DEPENDS" ] || { echo ...; exit 1; } guard before invoking pacman.
Suggestion:
| pacman -S --noconfirm --needed --asdeps $DEPENDS >/dev/null | |
| [ -n "$DEPENDS" ] || { echo "empty depends list" >&2; exit 1; } | |
| pacman -S --noconfirm --needed --asdeps $DEPENDS >/dev/null |
| for binpath in "$bin_dir"/usr/bin/*; do | ||
| binname="$(basename "$binpath")" |
There was a problem hiding this comment.
Without nullglob, if $bin_dir/usr/bin is empty (or the directory is missing), the glob expands to the literal string and this loop iterates once with binpath='.../usr/bin/'. The stub branch then writes a file literally named '.1' and gzip renames it to '*.1.gz', producing a garbage artifact instead of failing (or cleanly no-oping). Since gen_man is now shared by deb, rpm and arch builders, a builder whose copy step is skipped/failed early would surface this confusingly.
Suggestion:
| for binpath in "$bin_dir"/usr/bin/*; do | |
| binname="$(basename "$binpath")" | |
| local binpath binname | |
| shopt -s nullglob | |
| for binpath in "$bin_dir"/usr/bin/*; do | |
| binname="$(basename "$binpath")" |
- release_linux.yml: the version and the workflow input are passed through
environment variables instead of being interpolated into the run blocks
(script-injection hardening, same as the Windows workflow), and the
derived version is validated against X.Y.Z[-suffix] at extraction time
- release_windows.yml: the git-describe fallback version is validated up
front with a clear error instead of failing mid-release in the NSIS
step; job-level timeout-minutes bounds the whole build
- smoke-install.ps1: every installed binary is executed with a 60s
timeout and killed on expiry, so a binary that ignores --help and starts
serving cannot hang the smoke test forever
- packaging/arch/build.sh: pre-release versions are rejected explicitly,
because Arch vercmp has no '~' equivalent ('1.4.1_rc1' would sort newer
than '1.4.1' and refuse the upgrade to the final release); the depends
provisioning guards against an empty dependency list
- packaging/common/lib.sh: gen_man sets nullglob, so an empty or missing
bin dir fails loudly instead of producing a garbage '*.1.gz' man page
| jobs: | ||
| build: |
There was a problem hiding this comment.
This job declares no permissions key, so it inherits the default (potentially broad) token permissions, while release_windows.yml was updated to set permissions: contents: read. Since this job only builds and uploads artifacts, restrict it the same way for least-privilege consistency.
Suggestion:
| jobs: | |
| build: | |
| jobs: | |
| build: | |
| runs-on: ubuntu-latest | |
| permissions: | |
| contents: read |
| name: Mintlayer_Node_linux_${VERSION}_${{ matrix.arch }}_deb | ||
| path: dist/Mintlayer_Node_linux_${{ steps.get_version.outputs.VERSION }}_${{ matrix.arch == 'x86_64' && 'amd64' || 'arm64' }}.deb |
There was a problem hiding this comment.
Shell-style ${VERSION} is not expanded in with: blocks — only ${{ }} expressions are. The artifact name will literally be Mintlayer_Node_linux_${VERSION}_${{ matrix.arch }}_deb. These upload steps don't have a VERSION env var, so they must keep ${{ steps.get_version.outputs.VERSION }}.
Suggestion:
| name: Mintlayer_Node_linux_${VERSION}_${{ matrix.arch }}_deb | |
| path: dist/Mintlayer_Node_linux_${{ steps.get_version.outputs.VERSION }}_${{ matrix.arch == 'x86_64' && 'amd64' || 'arm64' }}.deb | |
| name: Mintlayer_Node_linux_${{ steps.get_version.outputs.VERSION }}_${{ matrix.arch }}_deb | |
| path: dist/Mintlayer_Node_linux_${{ steps.get_version.outputs.VERSION }}_${{ matrix.arch == 'x86_64' && 'amd64' || 'arm64' }}.deb |
| name: Mintlayer_Node_linux_${VERSION}_${{ matrix.arch }}_pkg | ||
| path: dist/Mintlayer_Node_linux_${VERSION}_${{ matrix.arch }}.pkg.tar.zst |
There was a problem hiding this comment.
Same problem, and worse here: both name and path use ${VERSION}, so the path matches no file and the PKG artifact upload fails outright. Use the step output expression instead.
Suggestion:
| name: Mintlayer_Node_linux_${VERSION}_${{ matrix.arch }}_pkg | |
| path: dist/Mintlayer_Node_linux_${VERSION}_${{ matrix.arch }}.pkg.tar.zst | |
| name: Mintlayer_Node_linux_${{ steps.get_version.outputs.VERSION }}_${{ matrix.arch }}_pkg | |
| path: dist/Mintlayer_Node_linux_${{ steps.get_version.outputs.VERSION }}_${{ matrix.arch }}.pkg.tar.zst |
| function Assert-PathEntry { | ||
| param ([bool]$ShouldExist) | ||
| $machinePath = [Environment]::GetEnvironmentVariable("Path", "Machine") | ||
| $present = ($machinePath -split ";" -contains $InstallDir) |
There was a problem hiding this comment.
Cross-file contract risk: node.nsi.in's AddToPath deliberately skips the PATH write when the machine PATH exceeds PATH_MAX_SAFE_LEN (900 chars) and, in a silent install, the warning MessageBox/DetailPrint is effectively invisible. On a CI runner or machine with a long PATH this makes Assert-PathEntry fail as a false positive even though the installer behaved as designed. Consider reading the same limit (or having the installer record a marker, e.g. a 'PathSkipped' registry value under the uninstall key) so the smoke test can distinguish 'installer broken' from 'PATH too long, skipped by design'.
Suggestion:
| function Assert-PathEntry { | |
| param ([bool]$ShouldExist) | |
| $machinePath = [Environment]::GetEnvironmentVariable("Path", "Machine") | |
| $present = ($machinePath -split ";" -contains $InstallDir) | |
| function Assert-PathEntry { | |
| param ([bool]$ShouldExist) | |
| $machinePath = [Environment]::GetEnvironmentVariable("Path", "Machine") | |
| $present = ($machinePath -split ";" -contains $InstallDir) | |
| # Node: the installer skips the PATH write when the machine PATH exceeds | |
| # its NSIS safe-length limit; treat that case explicitly to avoid false failures. |
| pkg="$(pacman -F --machinereadable "usr/lib/$lib" 2>/dev/null \ | ||
| | tr '\0' '\t' | awk -F'\t' 'NR == 1 {print $2}')" |
There was a problem hiding this comment.
The comment says fallback names were "verified against pacman -F", but the native path here resolves each library with pacman -F and blindly takes the first result (NR == 1). A soname can be provided by several packages, and the first machinereadable row is not guaranteed to be the canonical runtime provider (ordering depends on repo/db order), so a wrong or overly-specific package can end up in depends= and wind up in the published PKGBUILD metadata. Consider collecting all provider rows and preferring a well-known provider (e.g. skip -impl/-devel style names, or cross-check that the chosen package actually owns the file in the synced repos).
| # pacman refuses foreign-architecture packages, so the arch pkg smoke | ||
| # test runs on the x86_64 leg only (like in release_linux.yml). | ||
| if [ "$arch" = x86_64 ]; then |
There was a problem hiding this comment.
This comment is inaccurate and the corresponding gap is real: release_linux.yml DOES smoke-test the Arch packages on both matrix legs (the aarch64 leg installs with IgnoreArch and runs the binaries through the qemu binfmt handlers registered by docker/setup-qemu-action), while test-local.sh only runs the Arch smoke tests on x86_64. Local testing is therefore weaker than CI for the aarch64 pkg artifacts (and the justification given in the comment does not hold). Since the host already registers binfmt handlers for the deb/rpm arm64 legs, consider passing the architecture as the fourth argument and running the aarch64 pkg smoke tests here too, or at least fixing the comment.
Suggestion:
| # pacman refuses foreign-architecture packages, so the arch pkg smoke | |
| # test runs on the x86_64 leg only (like in release_linux.yml). | |
| if [ "$arch" = x86_64 ]; then | |
| # aarch64 leg relies on qemu binfmt handlers (already required for the | |
| # deb/rpm arm64 smoke tests) plus smoke-arch.sh's IgnoreArch handling. | |
| run_step "smoke pkg node ($arch)" \ | |
| docker run --rm -v "$REPO_ROOT:/work" -w /work "$ARCH_IMAGE" \ | |
| packaging/checks/smoke-arch.sh packaging/dist/Mintlayer_Node_linux_${VERSION}_${arch}.pkg.tar.zst \ | |
| mintlayer-node node "$arch" |
- release_linux.yml: restore the ${{ }} expressions in the artifact
upload name/path fields, where shell-style ${VERSION} does not expand
(regression introduced by the env-var change); declare
'permissions: contents: read' like the Windows workflow
- smoke-install.ps1: treat the installer's PATH-too-long skip (PATH above
its 900-char NSIS safe limit) as the designed behavior instead of a
false failure
- packaging/arch/build.sh: collect all pacman -F providers per soname and
pick the shortest package name deterministically (first row is not
guaranteed to be the canonical runtime provider), logging when several
packages provide the same library
- packaging/test-local.sh: run the aarch64 pkg smoke tests too, mirroring
CI (IgnoreArch + the qemu binfmt handlers already required for the
deb/rpm arm64 legs)
| permissions: | ||
| contents: read | ||
| permissions: | ||
| contents: read |
There was a problem hiding this comment.
This job declares permissions: twice. Duplicate YAML keys are invalid — GitHub's YAML parser may reject the workflow outright (breaking the whole release pipeline) or silently keep only one value depending on the parser version. Keep a single permissions: contents: read block.
Suggestion:
| permissions: | |
| contents: read | |
| permissions: | |
| contents: read | |
| permissions: | |
| contents: read |
| source packaging/images.env | ||
| ARCH=${{ matrix.arch }} | ||
| docker run --rm -v "$PWD":/work -w /work $ARCH_IMAGE \ | ||
| packaging/arch/build.sh \ |
There was a problem hiding this comment.
On the arm64 matrix leg this step runs an amd64-only container ($ARCH_IMAGE) without --platform. That requires an amd64 binfmt handler on the arm64 runner, but the setup step above only documents/registers handlers so --platform linux/arm64 works on the x86_64 leg. If the setup action doesn't register the amd64 (qemu-x86_64) handler on arm64 runners, this step fails on the arm64 leg. Either register the needed handler explicitly or document that the setup action covers all architectures.
| if ($VERSION -eq $env:GITHUB_REF) { | ||
| $VERSION = git describe --tags --abbrev=0 2>$null | ||
| $VERSION = $VERSION -replace '^v', '' | ||
| } |
There was a problem hiding this comment.
On workflow_dispatch there is no tag: the regex replace leaves GITHUB_REF (refs/heads/) untouched, so the fallback runs git describe --tags --abbrev=0, which returns the newest tag anywhere in the fetched history — not the version of the code being dispatched. A manual run from an older branch produces installers stamped with an unrelated, newer version (and the semver check then passes). Consider deriving the version from git describe --tags --exact-match first and falling back to a commit-based dev version (e.g. 0.0.0-<short-sha>) instead of the nearest tag.
Suggestion:
| if ($VERSION -eq $env:GITHUB_REF) { | |
| $VERSION = git describe --tags --abbrev=0 2>$null | |
| $VERSION = $VERSION -replace '^v', '' | |
| } | |
| if ($VERSION -eq $env:GITHUB_REF) { | |
| $VERSION = git describe --tags --exact-match 2>$null | |
| $VERSION = $VERSION -replace '^v', '' | |
| if ([string]::IsNullOrEmpty($VERSION)) { | |
| $VERSION = "0.0.0-$(git rev-parse --short HEAD)" | |
| } | |
| } |
| if (-not $present -and $machinePath.Length -gt 900) { | ||
| Write-Warning "machine PATH is longer than the installer's safe limit (900 chars); the PATH entry was skipped by design" | ||
| return | ||
| } |
There was a problem hiding this comment.
When the machine PATH exceeds 900 chars, this skip also bypasses the negative assertion during uninstall: a stale PATH entry left behind by a failed cleanup would be silently accepted. On install this is fine (the installer refuses to write a long PATH), but on uninstall the entry can only exist via manual edits or an installer that did write before PATH grew — in both cases a leftover entry is exactly what the smoke test should report. Consider skipping only the ShouldExist $true branch and still asserting removal.
Suggestion:
| if (-not $present -and $machinePath.Length -gt 900) { | |
| Write-Warning "machine PATH is longer than the installer's safe limit (900 chars); the PATH entry was skipped by design" | |
| return | |
| } | |
| if (-not $ShouldExist) { | |
| if ($present) { throw "machine PATH still contains '$InstallDir'" } | |
| return | |
| } | |
| if (-not $present -and $machinePath.Length -gt 900) { | |
| Write-Warning "machine PATH is longer than the installer's safe limit (900 chars); the PATH entry was skipped by design" | |
| return | |
| } | |
| if (-not $present) { | |
| throw "machine PATH does not contain '$InstallDir'" | |
| } |
| # '_?=' pins the uninstaller to the install dir (unquoted, as documented) so | ||
| # it does not copy itself to a temp location, which would make -Wait return | ||
| # before deletion finishes. | ||
| $p = Start-Process -FilePath $uninstaller -ArgumentList "/S", "_?=$InstallDir" -Wait -PassThru |
There was a problem hiding this comment.
NSIS documents _?= as requiring an unquoted path; Start-Process -ArgumentList will quote this argument because $InstallDir ("C:\Program Files\Mintlayer\Mintlayer Node") contains spaces. A quoted _?= value makes the uninstaller fail to resolve the copy location, so it may copy itself to temp and return before deletion finishes (or leave residue), producing flaky smoke-test failures.
Suggestion:
| $p = Start-Process -FilePath $uninstaller -ArgumentList "/S", "_?=$InstallDir" -Wait -PassThru | |
| $p = Start-Process -FilePath $uninstaller -ArgumentList "/S _?=$InstallDir" -Wait -PassThru # or invoke via cmd /c to control quoting |
| pkg="$(printf '%s\n' "$providers" \ | ||
| | awk '{ print length($0), $0 }' | sort -n -k1,1 -k2,2 | head -n1 | cut -d' ' -f2-)" |
There was a problem hiding this comment.
The deterministic provider pick ('shortest package name, ties alphabetical') is only a heuristic: a soname can be provided by a meta/compat/-headers variant whose selection as a runtime dependency is wrong (and namcap errors are the only safety net, with warnings allowed). Consider cross-checking the chosen provider against a small allowlist/known-good set (e.g. asserting the result matches ^[a-z0-9@._+-]+$ and excluding obvious meta names), or at least extending the multi-provider note so CI output makes a suspicious choice easy to audit.
Suggestion:
| pkg="$(printf '%s\n' "$providers" \ | |
| | awk '{ print length($0), $0 }' | sort -n -k1,1 -k2,2 | head -n1 | cut -d' ' -f2-)" | |
| pkg="$(printf '%s\n' "$providers" \ | |
| | awk '{ print length($0), $0 }' | sort -n -k1,1 -k2,2 | head -n1 | cut -d' ' -f2-)" | |
| case "$pkg" in | |
| *-headers|*-docs|*-devel*) | |
| echo "warning: suspicious provider for usr/lib/$lib: $pkg" >&2 ;; | |
| esac |
| PKG_FILE="$(readlink -f "$1")" | ||
| PKG_NAME="$2" | ||
| KIND="$3" | ||
| PKG_ARCH="${4:-$(uname -m)}" |
There was a problem hiding this comment.
The /etc/pacman.conf is mutated (IgnoreArch injection) and the database synced before the package file is validated. If $1 does not point to an existing file, readlink -f silently yields a bogus path and the failure surfaces later from pacman -U, after the container state has already been changed. Add an explicit existence check right after resolving PKG_FILE so bad arguments fail fast and the diagnostic is unambiguous.
Suggestion:
| PKG_FILE="$(readlink -f "$1")" | |
| PKG_NAME="$2" | |
| KIND="$3" | |
| PKG_ARCH="${4:-$(uname -m)}" | |
| PKG_FILE="$(readlink -f "$1")" | |
| [ -f "$PKG_FILE" ] || { echo "package file not found: $1" >&2; exit 2; } | |
| PKG_NAME="$2" | |
| KIND="$3" | |
| PKG_ARCH="${4:-$(uname -m)}" |
| shopt -u nullglob | ||
| [ ${#units[@]} -gt 0 ] || { echo "ERROR: no mintlayer units installed" >&2; exit 1; } | ||
| for unit in "${units[@]}"; do | ||
| systemd-analyze verify "$unit" |
There was a problem hiding this comment.
The node smoke test runs systemd-analyze verify, but the Arch node package declares systemd-libs as its dependency — the systemd-analyze binary ships with the full systemd package, which is typically not installed in the minimal archlinux:base container (and unlike desktop-file-utils in the GUI branch, nothing here installs it). If it's absent, set -e aborts the smoke test with a confusing 'command not found' error unrelated to the package under test. Either self-provision it (pacman -S --noconfirm --needed systemd or systemd-libs+tool availability check) before this block, or skip the verify step with a clear message when the tool is unavailable.
Suggestion:
| systemd-analyze verify "$unit" | |
| if command -v systemd-analyze >/dev/null 2>&1; then | |
| systemd-analyze verify "$unit" | |
| else | |
| echo " warning: systemd-analyze not available, skipping unit verification" >&2 | |
| fi |
| export LC_ALL=C.UTF-8 | ||
| local man_dir="$bin_dir/usr/share/man/man1" |
There was a problem hiding this comment.
gen_man mutates the caller's shell state without restoring it: export LC_ALL=C.UTF-8 persists in the caller's environment after the function returns (it is not scoped), and shopt -u nullglob unconditionally disables nullglob even if the caller had it enabled before calling. All current callers run under set -euo pipefail and source this lib, so the leaks are live. Save/restore the previous state: local _oldshopt="$(shopt -p nullglob)" ... eval "$_oldshopt", and prefer prefixing the help2man invocation with LC_ALL=C.UTF-8 instead of exporting.
Suggestion:
| export LC_ALL=C.UTF-8 | |
| local man_dir="$bin_dir/usr/share/man/man1" | |
| local _lc_all_was_set="${LC_ALL+set}" | |
| local _old_lc_all="${LC_ALL-}" | |
| export LC_ALL=C.UTF-8 | |
| ... | |
| # restore at end of function: | |
| if [ -n "$_lc_all_was_set" ]; then export LC_ALL="$_old_lc_all"; else unset LC_ALL; fi |
2 participants
Summary
Extends the native packaging pipeline (deb/rpm from #2115) with:
Arch Linux (
.pkg.tar.zst) —packaging/arch/archlinux:basecontainer; makepkg (as a dedicated build user) repackages the prebuilt release binaries from PKGBUILD templates; install scriptlet applies the preset policy (Arch convention: enable, don't start)pacman -Fon the arch-matched x86_64 leg; the aarch64 leg repackages cross-target (Arch publishes no arm64 images) with a static fallback map, unstripped binaries and stub man pages — no qemu neededMintlayer_Node{,_GUI}_linux_<version>_<arch>.pkg.tar.zst—release.yml'sMintlayer*/*glob picks them up unchangedpacman -Udirectly (documented in packaging/README.md)Windows (NSIS) —
build-tools/win/create-nsis-script.ps1with renderable templates (nsi/*.nsi.in+ sharedcommon.nshmacros) and a generator producing two installers:Mintlayer_Node_win_<v>_Setup.exe(new): all CLI tools, optional PATH entry, optional mainnet service (sc.exe) + TCP firewall ruleSetShellVarContext all), upgrade handling, and a fix for the broken${SMPROGRAMS}constant carried over from the old script (Start Menu shortcuts never worked)smoke-install.ps1: silent install → verify files/registry/PATH/--help→ silent uninstall → verify clean, on the CI runnerrelease_windows.yml: gainsworkflow_dispatchdry-run + git-describe version fallback (parity with the Linux workflow)Shared helpers —
packaging/common/lib.sh+packaging/images.envfedora:latestvsfedora:44drift between test-local.sh and CI)Drive-by fixes (found by the review agents + local testing)
1.4.1-rc1) in deb/rpm/arch due to a bash glob range quirk (+-aparsed as a range)E:output (namcap always exits 0, even on errors)run_step/summary block was only defined in the--skip-buildbranch — default (building) runs could never reach it;;residue on removalpersist-credentials: false+ explicit read-only permissions on the packaging jobsTesting
Local end-to-end (docker, real 1.4.x binaries from the deb-container build):
CI plan:
workflow_dispatchdry-runs of both release workflows on this branch will exercise the real matrix (arm64 binaries, Windows silent install) before the 1.4.1 tag.