ci: Add Linux package builds (DEB + RPM) to CI#6639
ci: Add Linux package builds (DEB + RPM) to CI#6639
Conversation
legleux
added
the
DraftRunCI
legleux
removed
the
DraftRunCI
![semgrep-companion-app[bot]](https://avatars.githubusercontent.com/u/67929741?s=60&v=4){kind=small}
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #6639 +/- ##
=========================================
- Coverage 81.6% 81.6% -0.0%
=========================================
Files 1010 1010
Lines 75982 75982
Branches 7633 7633
=========================================
- Hits 61982 61981 -1
- Misses 14000 14001 +1 see 1 file with indirect coverage changes 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Adds Linux package (DEB + RPM) build support to the repository and CI, producing installable artifacts from pre-built xrpld binaries and introducing local CMake targets + smoke-test tooling to validate package installs.
Changes:
- Introduces packaging infrastructure (RPM spec template, Debian
debian/metadata, shared service/sysusers/tmpfiles/logrotate/update scripts, and install smoke tests). - Adds CMake targets (
package-deb,package-rpm) and Docker-based CTest fixtures intended to validate package installs. - Updates GitHub Actions workflows/matrix generation to build required Release binaries and run packaging on PRs, triggers, and tags (plus a manual dispatch workflow).
Reviewed changes
Copilot reviewed 28 out of 28 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| package/test/smoketest.sh | Installs locally built package and runs basic verification + unittests. |
| package/test/check_install_paths.sh | Validates installed paths and compatibility symlinks. |
| package/shared/xrpld.tmpfiles | Adds tmpfiles.d entries for runtime directories. |
| package/shared/xrpld.sysusers | Defines system user for the daemon. |
| package/shared/xrpld.service | Adds systemd unit for running xrpld from packaged layout. |
| package/shared/xrpld.logrotate | Adds logrotate configuration for packaged log paths. |
| package/shared/update-xrpld.sh | Adds an auto-update helper script for DEB/RPM systems. |
| package/shared/update-xrpld-cron | Adds a cron entry to invoke the auto-update script. |
| package/rpm/xrpld.spec.in | Defines RPM package layout, symlinks, and systemd/sysusers/tmpfiles integration. |
| package/deb/debian/xrpld.links | Defines Debian compatibility symlinks for legacy rippled paths. |
| package/deb/debian/xrpld.install | Defines Debian install set for runtime + docs. |
| package/deb/debian/xrpld.conffiles | Marks config files as conffiles for dpkg. |
| package/deb/debian/source/format | Sets Debian source format. |
| package/deb/debian/rules | Debian build rules that stage prebuilt artifacts. |
| package/deb/debian/copyright | Debian copyright metadata. |
| package/deb/debian/control | Debian package metadata incl. transitional rippled. |
| package/build_pkg.sh | Single entry script to build either RPM or DEB from a prebuilt binary. |
| package/README.md | Documents packaging layout, usage, and verification steps. |
| cspell.config.yaml | Adds packaging-related words to spellchecker config. |
| cmake/XrplPackaging.cmake | Adds packaging targets and Docker-based install verification tests. |
| cmake/XrplInstall.cmake | Adjusts config install destinations for packaging layout. |
| CMakeLists.txt | Includes the new packaging CMake module. |
| .github/workflows/reusable-package.yml | Reusable workflow to build DEB/RPM packages from prebuilt artifacts. |
| .github/workflows/on-trigger.yml | Triggers packaging after build on pushes/schedule/dispatch. |
| .github/workflows/on-tag.yml | Builds packages for version tags. |
| .github/workflows/on-pr.yml | Runs packaging on PRs when relevant paths change. |
| .github/workflows/manual-package.yml | Adds workflow_dispatch for on-demand package builds. |
| .github/scripts/strategy-matrix/generate.py | Expands minimal matrix to include required Release builds for packaging. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Several issues flagged inline: critical CI matrix reduction (on-trigger.yml), a dead artifact_run_id input, unpinned download-artifact action (supply-chain), unvalidated inputs interpolated into shell, missing set -e causing false-pass in smoketest, RPM missing compat symlinks and license file, unquoted variables in a root-running update script, and a stray debug comment in debian/rules.
Review by Claude Opus 4.6 · Prompt: V12
There was a problem hiding this comment.
Several issues flagged inline: wrong tmpfiles paths (correctness), shell injection via unquoted heredoc and sed delimiter, self-hosted runner executing PR code (security), --rm defeating diagnostic container retention, stale temp files masking test failures, and unpinned :latest Docker images.
Review by Claude Opus 4.6 · Prompt: V12
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 28 out of 28 changed files in this pull request and generated 12 comments.
Comments suppressed due to low confidence (3)
.github/workflows/on-trigger.yml:132
container_imageis set to a mutable tag (ghcr.io/xrplf/ci/rhel-9:gcc-12). Elsewhere in CI the repo pins container images to immutable*-sha-*tags for reproducibility (e.g..github/workflows/reusable-build-test.yml:58,.github/workflows/reusable-upload-recipe.yml:43). Consider pinning this packaging image similarly to avoid unexpected CI drift.
with:
pkg_type: rpm
artifact_name: xrpld-rhel-9-gcc-12-amd64-release
version: ${{ needs.generate-version.outputs.version }}
container_image: ghcr.io/xrplf/ci/rhel-9:gcc-12
.github/workflows/on-tag.yml:71
container_imageis set to a mutable tag (ghcr.io/xrplf/ci/rhel-9:gcc-12). Elsewhere in CI the repo pins container images to immutable*-sha-*tags for reproducibility (e.g..github/workflows/reusable-build-test.yml:58,.github/workflows/reusable-upload-recipe.yml:43). Consider pinning this packaging image similarly to avoid unexpected CI drift.
uses: ./.github/workflows/reusable-package.yml
with:
pkg_type: rpm
artifact_name: xrpld-rhel-9-gcc-12-amd64-release
version: ${{ needs.generate-version.outputs.version }}
container_image: ghcr.io/xrplf/ci/rhel-9:gcc-12
.github/workflows/manual-package.yml:63
container_imageis set to a mutable tag (ghcr.io/xrplf/ci/rhel-9:gcc-12). Elsewhere in CI the repo pins container images to immutable*-sha-*tags for reproducibility (e.g..github/workflows/reusable-build-test.yml:58,.github/workflows/reusable-upload-recipe.yml:43). Consider pinning this packaging image similarly to avoid unexpected CI drift.
pkg_type: rpm
artifact_name: xrpld-rhel-9-gcc-12-amd64-release
version: ${{ needs.generate-version.outputs.version }}
pkg_release: ${{ inputs.pkg_release }}
container_image: ghcr.io/xrplf/ci/rhel-9:gcc-12
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Several security issues flagged inline: unvalidated container image input, mutable image tags on privileged containers, missing artifact integrity check, sed delimiter injection via free-form version inputs, world-writable lock dir DoS, and an unsanitized env-var repo override. See inline comments.
Review by Claude Opus 4.6 · Prompt: V12
There was a problem hiding this comment.
Several high-severity issues flagged inline: untrusted PR code running on self-hosted runners, mismatched SHA/version comments on pinned actions, world-writable lock dir DoS, unvalidated REPO env var used in root package install, privileged test containers, and a few correctness/reliability issues in the update script.
Review by Claude Opus 4.6 · Prompt: V12
There was a problem hiding this comment.
Took a pass through this
Several issues flagged: critical supply-chain risk in the auto-update script (no package signature verification, unvalidated REPO env var injected into root-run yum), unsanitized inputs.version in the workflow, sed delimiter collision, mutable :latest Docker image tags, a 45 GB memory limit that will break on most machines, logrotate config installed to a non-standard path (inactive on all distros), BuildArch misuse in the RPM spec, missing xrpld_version guard in CMake, and a few minor issues. See inline comments.
Review by ReviewBot 🤖
Review by Claude Opus 4.6 · Prompt: V12
legleux
force-pushed
the
legleux/linux-packages
branch
from
9871d41 to
cf38fc5
Compare
There was a problem hiding this comment.
Several security and correctness issues flagged inline: bogus action version comments across both workflow files, missing permissions block, no artifact integrity check before packaging, sed delimiter injection, world-writable lock dir in the update script, incomplete yum repo restriction, unguarded xrpld_version in CMake, mutable :latest test images, and --privileged Docker containers. See inline comments for details.
Review by Claude Opus 4.6 · Prompt: V12
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 28 out of 28 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| add_custom_target( | ||
| package-deb | ||
| COMMAND | ||
| ${CMAKE_SOURCE_DIR}/package/build_pkg.sh deb ${CMAKE_SOURCE_DIR} | ||
| ${CMAKE_BINARY_DIR} ${xrpld_version} | ||
| WORKING_DIRECTORY ${CMAKE_BINARY_DIR} | ||
| COMMENT "Building Debian package" | ||
| VERBATIM | ||
| ) |
There was a problem hiding this comment.
The package-deb CMake target passes ${xrpld_version} but not ${pkg_release} into build_pkg.sh, so locally built DEBs will always use the default release 1 even when -Dpkg_release=... is set. Pass pkg_release through to keep DEB and RPM release numbers consistent.
There was a problem hiding this comment.
@copilot apply changes based on this feedback
There was a problem hiding this comment.
RPM path — pkg_release works:
- CMake configure_file (line 23-27) substitutes @pkg_release@ into xrpld.spec.in → xrpld.spec
- package-rpm (line 34-35) calls build_pkg.sh rpm — only 3 args, no version/release passed
- build_pkg.sh:build_rpm() copies the already-configured spec file (line 36), so %{pkg_release} in the spec is already baked in from step 1
- The spec uses it at line 11: Release: %{?ver_suffix:0.%{ver_suffix}.}%{pkg_release}%{?dist}
RPM is fine — it gets pkg_release via configure_file, not via the shell argument.
DEB path — pkg_release works:
- package-deb (line 49-50) calls build_pkg.sh deb ${xrpld_version} ${pkg_release} — 5 args
- build_pkg.sh:17 picks up PKG_RELEASE="${5:-1}"
- build_pkg.sh:72 writes the changelog: xrpld (${deb_version}-${PKG_RELEASE}) unstable; ...
DEB is fine — it gets pkg_release via the 5th argument.
Both paths correctly propagate -Dpkg_release=N. The RPM gets it at configure time through the spec template; the DEB gets it at build time through the shell argument. The PR comment's concern was already
addressed in the current code.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 27 out of 27 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Several issues flagged inline: two high-severity security concerns (untrusted PR code on self-hosted runners, suspicious action SHAs that don't match known versions), a container breakout risk from --privileged + writable cgroupfs, an RPM build breakage when run outside CMake, missing RPM compat symlinks that will fail check_install_paths.sh, and a handful of medium/low correctness and hardening issues across the shell scripts and CMake.
Review by Claude Opus 4.6 · Prompt: V12
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 27 out of 27 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| pkg_type: rpm | ||
| artifact_name: xrpld-rhel-9-gcc-12-amd64-release | ||
| version: ${{ needs.generate-version.outputs.version }} | ||
| container_image: ghcr.io/xrplf/ci/rhel-9:gcc-12 |
There was a problem hiding this comment.
This RPM packaging job uses a mutable container tag (ghcr.io/xrplf/ci/rhel-9:gcc-12). The repo generally pins Linux container images to immutable *-sha-* tags (see .github/workflows/reusable-build-test.yml:58). Please pin this image as well.
| container_image: ghcr.io/xrplf/ci/rhel-9:gcc-12 | |
| container_image: ghcr.io/xrplf/ci/rhel-9:gcc-12-sha-<pin-to-the-exact-immutable-tag-used-elsewhere-for-this-image> |
There was a problem hiding this comment.
@bthomee @mathbunnyru How do you want to specify the image used for packaging?
There was a problem hiding this comment.
I think specifying the exact SHA of the image is a reasonable thing to do, especially considering we already pin everything else that can be pinned.
| pkg_type: rpm | ||
| artifact_name: xrpld-rhel-9-gcc-12-amd64-release | ||
| version: ${{ needs.generate-version.outputs.version }} | ||
| container_image: ghcr.io/xrplf/ci/rhel-9:gcc-12 |
There was a problem hiding this comment.
This RPM packaging job uses a mutable container tag (ghcr.io/xrplf/ci/rhel-9:gcc-12). For reproducibility, please switch to the same *-sha-* pinning convention used by other workflows (e.g. .github/workflows/reusable-build-test.yml:58).
| container_image: ghcr.io/xrplf/ci/rhel-9:gcc-12 | |
| container_image: ghcr.io/xrplf/ci/rhel-9:gcc-12-sha-<replace-with-the-exact-pinned-tag-used-elsewhere> |
| # - 9 using GCC 12: Debug and Release on linux/amd64 | ||
| # (Release is required for RPM packaging). | ||
| # - 10 using Clang: Release on linux/amd64. |
There was a problem hiding this comment.
To avoid introducing an additional configuration, I propose to switch RHEL 9 to release only, and RHEL 10 to debug.
There was a problem hiding this comment.
Both Release and Debug builds include debug symbols, so either works for packaging purposes. I'd suggest we drop the overrides to CMake defaults here and figure out what configurations we actually need in a separate discussion — that's a broader question than this PR.
There was a problem hiding this comment.
@mathbunnyru any thoughts here? Maybe the point is moot given the effort to consolidate to a single recent GCC and Clang per distro.
|
|
||
| # Ubuntu: | ||
| # - Jammy using GCC 12: Debug on linux/arm64. | ||
| # - Jammy using GCC 12: Debug on linux/arm64, Release on |
There was a problem hiding this comment.
Same here, can switch Jammy to release only instead of introducing an additional config.
|
|
||
| generate-version: | ||
| needs: should-run | ||
| if: ${{ needs.should-run.outputs.go == 'true' }} |
There was a problem hiding this comment.
For PRs, should we add an additional condition to limit on a label, e.g. contains(github.event.pull_request.labels.*.name, 'Package')? Right now this will run on each commit, which is probably overkill.
If we want to control .deb and .rpm separately then the condition can be moved into the separate packaging jobs below, using e.g. PkgDeb and PkgRpm or so.
There was a problem hiding this comment.
The packaging jobs download a pre-built binary and run dpkg-buildpackage/rpmbuild. Since there's no compilation step, it's quick. Gating on a label adds process overhead that people will forget and we'll end up either building it or forgetting and allow packaging regressions for negligible time savings.
There was a problem hiding this comment.
How much storage space is this going to require per commit? I'm concerned this is going to consume a lot of disk = $$$.
Can the packages be overwritten at least, so that we only produce and upload one .deb and one .rpm per PR? (this includes avoiding creating packages with the same name but with different version suffixes like -1 and -2)
| description: "Package release number. Increment when repackaging the same executable." | ||
| required: false | ||
| type: string | ||
| default: "1" |
There was a problem hiding this comment.
Is it possible to have the very first package not use a release number, or at least to not include it in the package name? I think the rippled-1 package is just silly.
Since this is numeric, you should consider using the number type instead of string.
There was a problem hiding this comment.
Trust me, I wanted to kill the -1 too. But after wrestling with both RPM and DEB packaging systems and their very opinionated tooling, I came around: the release number is a load-bearing convention baked into decades of Linux packaging. It exists so you can increment to -2 when repackaging the same upstream version (dependency rebuild, config fix, etc.) without bumping the version itself. dpkg, rpm, apt, yum — they all expect it, sort by it, and make upgrade decisions based on it. Fighting that buys us nothing and costs us compatibility with every tool in the ecosystem.
There was a problem hiding this comment.
Boo, ok it is what it is.
| - name: Upload package artifact | ||
| uses: actions/upload-artifact@bbbca2ddaa5d8feaa63e36b76fdaad77386f024f # v7.0.0 | ||
| with: | ||
| name: xrpld-${{ inputs.pkg_type }}-${{ inputs.version }} |
There was a problem hiding this comment.
Would there be a benefit to also including the ${{ inputs.pkg_release }} here in the name? All three values are used to build the package in the step above.
There was a problem hiding this comment.
The release number only changes when repackaging the same upstream version — a manual, exceptional operation. In normal flow, every commit produces a different version string, so pkg_release is always 1. Adding it to the artifact name would imply automated tracking of "have we already released this version?" which doesn't exist and is out of scope here. If repackaging is ever needed, that's a manual workflow with an explicit pkg_release override.
| add_custom_target( | ||
| package-deb | ||
| COMMAND | ||
| ${CMAKE_SOURCE_DIR}/package/build_pkg.sh deb ${CMAKE_SOURCE_DIR} | ||
| ${CMAKE_BINARY_DIR} ${xrpld_version} | ||
| WORKING_DIRECTORY ${CMAKE_BINARY_DIR} | ||
| COMMENT "Building Debian package" | ||
| VERBATIM | ||
| ) |
| return() | ||
| endif() | ||
|
|
||
| set(DEB_TEST_IMAGE "geerlingguy/docker-ubuntu2204-ansible:latest") |
There was a problem hiding this comment.
@legleux you stated in another comment that testing on the latest release is the point, but why exactly?
If we're ready to push out a release and a new version of one of these Docker images just got published that breaks the tests, then what are we supposed to do? If it's an urgent fix release, I don't think we'll want to spend a few hours or longer figuring out how to get the test image to play nice again.
|
This PR has conflicts, please resolve them in order for the PR to be reviewed. |
legleux
force-pushed
the
legleux/linux-packages
branch
from
8bfc19d to
6366ea9
Compare
|
All conflicts have been resolved. Assigned reviewers can now start or resume their review. |
legleux
force-pushed
the
legleux/linux-packages
branch
from
7bc0e61 to
b23b325
Compare
legleux
added
CI
bthomee
left a comment
bthomee
left a comment
There was a problem hiding this comment.
I've not reviewed the contents of package/ as I'm not familiar with any of that. While I'd love to review it now, time off is calling. I can attempt to review (and learn) when I'm back or otherwise I defer to @mathbunnyru.
| # - 9 using GCC 12: Debug and Release on linux/amd64 | ||
| # (Release is required for RPM packaging). | ||
| # - 10 using Clang: Release on linux/amd64. |
There was a problem hiding this comment.
@mathbunnyru any thoughts here? Maybe the point is moot given the effort to consolidate to a single recent GCC and Clang per distro.
|
|
||
| generate-version: | ||
| needs: should-run | ||
| if: ${{ needs.should-run.outputs.go == 'true' }} |
There was a problem hiding this comment.
How much storage space is this going to require per commit? I'm concerned this is going to consume a lot of disk = $$$.
Can the packages be overwritten at least, so that we only produce and upload one .deb and one .rpm per PR? (this includes avoiding creating packages with the same name but with different version suffixes like -1 and -2)
| uses: ./.github/workflows/reusable-package.yml | ||
| with: | ||
| pkg_type: deb | ||
| artifact_name: xrpld-ubuntu-jammy-gcc-12-amd64-release |
There was a problem hiding this comment.
Following up then on my other comment above, if we can overwrite packages, then using the PR number would be preferred over the shortened commit hash, so we don't get a storage explosion.
Although PR numbers are increasing, there's no guarantee that they are merged in that order. I don't have a good suggestion offhand to make them sortable in that case.
If we don't use PR numbers, then prefixing a (shortened) timestamp would at least help...
| build-test: | ||
| if: ${{ github.repository == 'XRPLF/rippled' }} | ||
| uses: ./.github/workflows/reusable-build-test.yml | ||
| strategy: | ||
| fail-fast: true | ||
| matrix: | ||
| os: [linux] | ||
| with: | ||
| ccache_enabled: false | ||
| os: ${{ matrix.os }} | ||
| strategy_matrix: minimal | ||
| secrets: | ||
| CODECOV_TOKEN: ${{ secrets.CODECOV_TOKEN }} |
| description: "Package release number. Increment when repackaging the same executable." | ||
| required: false | ||
| type: string | ||
| default: "1" |
There was a problem hiding this comment.
Boo, ok it is what it is.
| sed -e "s/@xrpld_version@/${PKG_VERSION}/" \ | ||
| -e "s/@pkg_release@/${PKG_RELEASE}/" \ |
There was a problem hiding this comment.
@legleux both AI reviewers raise similar concerns. Do you want to add any escaping or other kind of protection/sanitization here?
legleux
force-pushed
the
legleux/linux-packages
branch
from
b23b325 to
ac7c4ef
Compare
High Level Overview of Change
.debplus a.ddebdebug symbols package; RPM produces a main.rpmplus-debuginfoPackaging infrastructure:
package/build_pkg.sh— single script driving both RPM and DEB builds, stages files from the repo and a pre-built binarypackage/rpm/xrpld.spec.in— RPM spec template with systemd macros, debug package support, and full compat symlinkspackage/deb/debian/— complete Debian packaging (control, rules, links, conffiles, copyright, transitional rippled package)package/shared/— service file, sysusers, tmpfiles, logrotate, update script, and cron entrycmake/XrplPackaging.cmake— CMake targets (package-deb,package-rpm) for local development buildspackage/test/— install smoke test and path validation scripts (for future CTest integration)CI workflows:
reusable-package.yml— reusable workflow called by all triggersmanual-package.yml— workflow_dispatch for on-demand builds with version/release overrideson-trigger.yml,on-pr.yml,on-tag.ymlto run packaging after buildgenerate.pyto includeubuntu-jammy-gcc-12-amd64-releaseandrhel-9-gcc-12-amd64-releasein the minimal matrixInstall layout:
/opt/xrpld/bin/xrpld/opt/xrpld/etc/xrpld.cfg,/opt/xrpld/etc/validators.txtrippled-->xrpldnaming transition (/opt/ripple,/usr/bin/rippled,/etc/opt/ripple, etc.)Test plan
.deb,.ddeb,.rpm,-debuginfo.rpm)Future Tasks
xrplddevelopment build naming convention so they sort lexicographicallyhttps://conan.ripplex.io/artifactory/generic-local/xrpld/