Re: [PATCH v4 00/14] System updates based on systemd-sysupdate

[Alyssa Ross <hi@alyssa.is>]

[-- Attachment #1: Type: text/plain, Size: 8959 bytes --]

Demi Marie Obenour <demiobenour@gmail.com> writes:

> This implements updates via systemd-sysupdate.  See individual commit
> messages for details.
>
> There are major changes to the image build process.
>
> Signed-off-by: Demi Marie Obenour <demiobenour@gmail.com>
> ---
> Changes in v4:
> - Fix build errors in intermediate patches.
> - Apply suggestions from code review.
> - Link to v3: https://spectrum-os.org/lists/archives/spectrum-devel/20251119-updates-v3-0-b88a99915509@gmail.com
>
> Changes in v3:
> - See individual commits for details.  There are too many to mention
>   here.
> - Link to v2: https://spectrum-os.org/lists/archives/spectrum-devel/20251112-updates-v2-0-88d96bf81b79@gmail.com
>
> Changes in v2:
> - updates-dir-check:
>   - Do not check that there is a SHA256SUMS or SHA256SUMS.gpg file in the
>     update directory.  systemd-sysupdate will fail if it cannot find a
>     manifest or its signature.
>   - Follow symlinks in opening the directory.  The path is from a
>     trusted source and will always point to a BTRFS snapshot, never a
>     symlink.  The only exception is the last component, which is still
>     checked to not be a symlink.
> - VM:
>   - Link SHA256SUMS.sha256.asc to SHA256SUMS.gpg.  Recent
>     systemd-sysupdate seems to use the former name.
>   - Get update URL from host.
>   - Use an execline script instead of a shell script.
> - Update script:
>   - Unmount shared directory if already mounted.  This avoids errors
>     when mounting it again.
>   - Delete old snapshot if present.
>   - Provide the VM information with a different directory layout.
>   - Do not bind-mount the information passed into the VM into the shared
>     VM folder.  Instead rely on this folder being read-only to the
>     guest.  This is enforced by a read-only bind mount in virtiofs's
>     mount namespace.
> - Testing:
>   - Lots of manual update testing.
>   - Disable the test for the live image as it doesn't work anymore.
> - Nix:
>   - Move validation to a separate low-priority patch.
> - Documentation:
>   - Document that updating the system is now possible.
> - Installer:
>   - Remove the "Try Spectrum" button.
>
> - Link to v1: https://spectrum-os.org/lists/archives/spectrum-devel/20251029-updates-v1-0-401c1be2a11b@gmail.com
>
> ---
> Demi Marie Obenour (14):
>       host/rootfs: Install all programs from util-linuxMinimal
>       host/rootfs: Install systemd-pull
>       tools: Add directory checker for updates
>       scripts: port make-gpt.sh to bash
>       scripts/make-gpt.sh: Allow specifying partition size
>       Support generating multiple partition UUIDs
>       scripts: Use shell expansion to get partition path
>       release: Compress installation images and remove live image
>       Use OS version to set partition labels and UKI name
>       Add B partitions to installation images
>       release: Create directory with system update
>       Support updates via systemd-sysupdate
>       Documentation: Update support
>       Validate configuration parameters
>
>  Documentation/development/build-configuration.adoc |  13 ++
>  Documentation/installation/getting-spectrum.adoc   |  56 +++++++--
>  Documentation/installation/index.adoc              |   4 +-
>  Documentation/using-spectrum/index.adoc            |   2 +
>  Documentation/using-spectrum/updates.adoc          |  30 +++++
>  host/efi.nix                                       |   2 +-
>  host/initramfs/Makefile                            |  18 +--
>  host/initramfs/etc/probe                           |  20 ---
>  host/initramfs/shell.nix                           |   2 +
>  host/rootfs/Makefile                               |  27 ++++-
>  host/rootfs/busybox-config                         | 134 +++++++++++++++++++++
>  host/rootfs/busybox-config.license                 |   4 +
>  host/rootfs/default.nix                            |  92 +++++++++-----
>  host/rootfs/file-list.mk                           |   7 ++
>  host/rootfs/image/etc/fstab                        |   1 +
>  .../image/etc/sysupdate.d/50-verity.transfer       |  20 +++
>  host/rootfs/image/etc/sysupdate.d/60-root.transfer |  20 +++
>  .../image/etc/sysupdate.d/70-kernel.transfer       |  20 +++
>  .../image/etc/vm-sysupdate.d/50-verity.transfer    |  18 +++
>  .../image/etc/vm-sysupdate.d/60-root.transfer      |  18 +++
>  .../image/etc/vm-sysupdate.d/70-kernel.transfer    |  18 +++
>  host/rootfs/image/usr/bin/spectrum-update          |  83 +++++++++++++
>  host/rootfs/os-release.in                          |  15 +++
>  host/rootfs/shell.nix                              |   2 +
>  img/app/Makefile                                   |   2 +-
>  lib/config.default.nix                             |   4 +
>  lib/config.nix                                     |  15 ++-
>  lib/fake-update-signing-key.gpg                    |   3 +
>  release.nix                                        |   2 +
>  release/checks/integration/meson.build             |   2 +-
>  release/checks/integration/try.c                   |  29 -----
>  release/combined/eosimages.nix                     |  19 ++-
>  release/combined/grub.cfg.in                       |   5 -
>  release/live/Makefile                              |  15 ++-
>  release/live/default.nix                           |   5 +-
>  release/live/shell.nix                             |   3 +-
>  release/update.nix                                 |  33 +++++
>  scripts/format-uuid.awk                            |  35 ++++++
>  scripts/format-uuid.sh                             |  19 ---
>  scripts/make-gpt.sh                                |  30 ++---
>  tools/default.nix                                  |   1 +
>  tools/meson.build                                  |   4 +
>  tools/updates-dir-check.c                          | 134 +++++++++++++++++++++
>  vm/app/systemd-sysupdate/default.nix               |  57 +++++++++
>  vm/app/systemd-sysupdate/escape-url.awk            |  31 +++++
>  .../systemd-sysupdate/populate-transfer-directory  |  26 ++++
>  vm/sys/net/Makefile                                |   2 +-
>  47 files changed, 928 insertions(+), 174 deletions(-)
> ---
> base-commit: e89924f5613539e4dcd9d485a82f976c817b34c1
> change-id: 20250928-updates-92e99849e231
> prerequisite-patch-id: c518b0e42e0c87755ef725ace8e961cdfb862285
> prerequisite-patch-id: 0ed2b2073c0ab6d422aa642fd238b15428c6f7d1

I'm finding this series quite difficult to review, because information I
need to review isn't presented when I need it; I have to go and find it.

"scripts: port make-gpt.sh to bash" doesn't include its motivation.  I
assume that's coming later, but now I have to go through potentially 9
more patches to find out what that is, and keep this patch in my head to
make sure I remember to go back to it once I find out what that is.

In "scripts/make-gpt.sh: Allow specifying partition size" I see bash
features being used to strip a "MiB" suffix off the end of a size, so
maybe that's it?  But I already asked why we need to do that in my
review of v2[1], and didn't get a response, so I'm still in the dark…

As I'm going through these patches, I'm encountering a lot of merge
conflicts.  Usually when I'm reviewing a series I apply it on top of its
base commit so I don't have to worry about conflicts — they're a lot
easier to fix all at once in a rebase once I've reviewed all the patches
and am immediately familiar with what they do — but the base commit in
this series does not exist in the upstream repository, so I can't do
that.  After solving enough conflicts I realise that it must be because
this series depends on your other one to separate out verity data, which
I now recall you saying would be needed for updates, so I'll go and
review that one now, and then come back here when I'm done.

All this is just to say that as a patch submitter, there's a lot that
can be done relatively cheaply to save a lot of inference on the part of
the reviewer.  It might be helpful to go through your cover letter and
patches one by one in order before submission, and imagine being a
reviewer who has not read the whole series in its current version, and
has probably forgotten the intricacies of previous versions — what
information will help them understand what's happening?  If I could just
review patches one by one without having to jump back and forth, I think
it could go so much faster.  I'd be able to stop between any two patches
as well, so I wouldn't need to wait until I have enough uninterrupted
time to go through the whole thing at once.  Like this it's a bit of a
slog, because I'm spending a lot of time trying to remember or even
guess what's going on, rather than being told.

[1]: https://spectrum-os.org/lists/archives/spectrum-devel/87ikfdapmx.fsf@alyssa.is

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 227 bytes --]