Skip to content

Add s3d packaging - #249

Merged
ChrisSchinnerl merged 4 commits into
masterfrom
pj/s3d-package
Sep 23, 2026
Merged

ChrisSchinnerl merged 4 commits into
masterfrom
pj/s3d-package

Conversation

@peterjan

@peterjan peterjan commented Sep 17, 2026

Copy link
Copy Markdown
Member

Adds s3d to the apt repo. As I mentioned before we have an annoying naming conflict, I opted for sia-s3d. Takes into account the existing open issues as much as possible.

References:
#129
#179

Needs:
SiaFoundation/s3d#264

@peterjan peterjan self-assigned this Sep 17, 2026
Copilot AI lite review requested due to automatic review settings September 17, 2026 09:20
@peterjan
peterjan marked this pull request as ready for review September 17, 2026 09:27

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Changes recommended

Critical package-overwrite and secret-directory issues, plus configuration and logging fixes, remain unresolved.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Adds Debian packaging and CI integration for s3d, published as sia-s3d while installing the s3d service.

Changes:

  • Adds reusable Debian build tooling, templates, and maintainer scripts.
  • Defines package metadata and systemd integration.
  • Updates documentation, CI packaging, and ignored artifacts.
File summaries
File Change / review note
scripts/templates/service.tmpl Defines the systemd service; logging needs configuration or rotation.
scripts/templates/prerm.tmpl Stops services before removal.
scripts/templates/postrm.tmpl Handles removal cleanup.
scripts/templates/postinst.tmpl Handles post-install setup.
scripts/templates/copyright.tmpl Provides Debian copyright metadata; upstream years need correction.
scripts/templates/control.tmpl Defines control metadata; needs Replaces support.
scripts/build-deb.sh Builds packages; requires Replaces, secure data-directory permissions, and logging configuration.
README.md Documents installation and setup; setup commands need a consistent absolute data directory.
packages/s3d/package.env Defines package metadata; add Replaces: s3d alongside Conflicts.
.gitignore Ignores build artifacts.
.github/workflows/build.yml Builds package-defined projects in CI.
Review details

Suppressed comments (3)

README.md:73

  • The first-run s3d config wizard leaves Directory as . when no config exists, and login opens s3d.db relative to the caller's current directory. These commands can therefore create the app-key database outside /var/lib/s3d, while the unit runs in /var/lib/s3d and exits because it cannot find the app key. Set S3D_DATA_DIR=/var/lib/s3d for the setup commands (or otherwise make the CLI and unit use the same absolute data directory).
$ sudo s3d login

scripts/templates/copyright.tmpl:6

  • Files: * makes this the copyright declaration for all upstream s3d content, but the upstream LICENSE identifies the copyright as 2025 The Sia Foundation, not 2024–2026. The generated Debian metadata is therefore inaccurate; use the upstream year(s) or make the copyright years package-specific.
Copyright: 2024-2026 The Sia Foundation

scripts/templates/service.tmpl:10

  • For s3d, the upstream defaults enable file logging and choose <data directory>/s3d.log when no path is configured. Since the package deliberately ships no config, this unit writes an unrotated log to /var/lib/s3d/s3d.log rather than /var/log; over time it can consume the data volume. Package a config/logrotate rule or disable file logging and rely on journald.
ExecStart=/usr/bin/@PKG_NAME@
WorkingDirectory=/var/lib/@PKG_NAME@
  • Files reviewed: 10/11 changed files
  • Comments generated: 4
  • Review effort level: Lite

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

Comment thread packages/s3d/package.env
Comment thread scripts/build-deb.sh
Comment thread scripts/build-deb.sh Outdated
Comment thread scripts/build-deb.sh Outdated
Copilot AI review requested due to automatic review settings September 17, 2026 12:20
@peterjan peterjan changed the title Add s3d package definition and build script Add s3d packaging Sep 17, 2026

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Changes recommended

Critical service-startup and data-safety issues remain, along with packaging and documentation fixes.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (3)

README.md:73

  • When the wizard leaves its default data directory, s3d writes directory: . to /etc/s3d/s3d.yml. This command can therefore create the login database in the caller's current directory, while the unit runs from /var/lib/s3d; the service and the later users/keys commands can then use different databases. Pass S3D_DATA_DIR=/var/lib/s3d (or require that directory during setup) for the documented initialization.
$ sudo s3d login

README.md:91

  • This alternative does not actually write logs instead of the journal: enabling file logging leaves stdout logging enabled, and s3d's default file path is /var/lib/s3d/s3d.log, with no rotation. If this option is retained, configure a /var/log/s3d path, disable stdout, and ship rotation support; otherwise describe it as an additional log destination.
ExecStart=/usr/bin/s3d -log.file.enabled=true

packages/s3d/package.env:4

  • The package installs /usr/bin/s3d, which is the path owned by the Debian package named in PKG_CONFLICTS. Conflicts prevents co-installation but does not authorize dpkg to take over that file during a package transition, so direct dpkg -i can fail with a file-overwrite error. Emit Replaces: s3d alongside Conflicts: s3d in the generated control file.
PKG_CONFLICTS=s3d
  • Files reviewed: 10/11 changed files
  • Comments generated: 3
  • Review effort level: Lite

Comment thread README.md Outdated
Comment thread packages/s3d/package.env
Comment thread scripts/templates/postrm.tmpl

@ChrisSchinnerl ChrisSchinnerl left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looking pretty good overall.

Comment thread .github/workflows/build.yml
Copilot AI review requested due to automatic review settings September 22, 2026 07:03

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

Unresolved service hardening, logging/data-path, and package reproducibility issues remain.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1 High severity

Open (1)
Resolved since last review (6)
Previously missed (1)

In code that hasn't changed since last review

Medium severity Document log rotation and avoid unbounded data-directory logs

README.md:91

Following this documented override re-enables s3d's default file path (<data directory>/s3d.log, i.e. /var/lib/s3d/s3d.log) and the package provides no rotation policy. That brings back the unbounded data-directory log issue for users who follow the instructions; either document/configure a /var/log/s3d/... path together with rotation, or remove this snippet until the package supplies both.

Comment thread scripts/templates/service.tmpl
Copilot AI review requested due to automatic review settings September 22, 2026 12:26

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

Unresolved lifecycle-script issues affect upgrades, ownership handling, and account removal.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 2 Medium severity

Open (2)
Resolved since last review (1)
Previously missed (1)

In code that hasn't changed since last review

Low severity Override leaves stdout logging enabled and lacks explicit rotated log path

README.md:94

This override does not actually switch logging from the journal to a file: it only re-enables file logging, while s3d's default/configured stdout logger remains enabled, so output is duplicated. With no log.file.path in the config, s3d also resolves the file to /var/lib/s3d/s3d.log, outside /var/log, with no rotation policy. Document a config-based override that disables stdout and uses an explicit /var/log path with rotation, or remove this optional snippet.

Comment thread scripts/templates/postinst.tmpl
Comment thread scripts/templates/postinst.tmpl
Copilot AI review requested due to automatic review settings September 22, 2026 12:32

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

Unresolved documentation, maintainer-script, and reproducibility issues remain.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 2 Medium severity · 1 Low severity

Open (3)

Comment thread README.md Outdated
Copilot AI review requested due to automatic review settings September 22, 2026 13:12

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot review overview

🔵 Needs a closer look

Five moderate issues remain around suite propagation, reproducibility, service lifecycle handling, and purge cleanup.

Review effort: Lite
Findings: None

Resolved since last review (3)

@chris124567 chris124567 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We need to tag a new version for the log flags, other than that this LGTM

@ChrisSchinnerl
ChrisSchinnerl merged commit 5d8896e into master Sep 23, 2026
3 checks passed
@ChrisSchinnerl
ChrisSchinnerl deleted the pj/s3d-package branch September 23, 2026 09:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants