From 6a0bbaf939dd772c1d0a9036e1f01415b0313485 Mon Sep 17 00:00:00 2001 From: Adam Hall Date: Tue, 23 Jun 2026 18:42:56 +0930 Subject: [PATCH 1/9] fix(flatpak): use per-user scope to avoid polkit hang during bootstrap System-scope `flatpak remote-add`/`install` run unprivileged (Cmd) as the user triggers the polkit action org.freedesktop.Flatpak.modify-repo, which prompts for a password. In a headless/TTY session (first boot, SSH, the e2e harness) there is no polkit agent, so bootstrap hangs forever on `Password:`. Pass `--user` to both flatpak invocations so the operations target the per-user installation, which needs no polkit/root. Stays unprivileged (Cmd) and keeps the existing `-y --noninteractive` flags. Updates the flatpak plan assertions accordingly. Co-Authored-By: Claude Opus 4.8 --- internal/stages/flatpak.go | 4 ++-- internal/stages/flatpak_test.go | 27 +++++++++++++++++++-------- internal/stages/selectors_test.go | 6 +++--- internal/stages/stages_test.go | 4 ++-- 4 files changed, 26 insertions(+), 15 deletions(-) diff --git a/internal/stages/flatpak.go b/internal/stages/flatpak.go index 1f2d505..8d85e9c 100644 --- a/internal/stages/flatpak.go +++ b/internal/stages/flatpak.go @@ -31,7 +31,7 @@ func (flatpak) Run(ctx *Context) error { // used, must be listed in flatpak_remotes (validation guarantees every app's // remote is declared). for _, rem := range ctx.Cfg.FlatpakRemotes { - if err := ctx.R.Cmd("flatpak", "remote-add", "--if-not-exists", rem.Name, rem.URL); err != nil { + if err := ctx.R.Cmd("flatpak", "--user", "remote-add", "--if-not-exists", rem.Name, rem.URL); err != nil { return err } } @@ -40,7 +40,7 @@ func (flatpak) Run(ctx *Context) error { // unambiguous (an accepted cost over one batched install). for _, app := range apps { remote, appid, _ := strings.Cut(app, ":") - if err := ctx.R.Cmd("flatpak", "install", "-y", "--noninteractive", remote, appid); err != nil { + if err := ctx.R.Cmd("flatpak", "--user", "install", "-y", "--noninteractive", remote, appid); err != nil { return err } } diff --git a/internal/stages/flatpak_test.go b/internal/stages/flatpak_test.go index 2912554..92566d4 100644 --- a/internal/stages/flatpak_test.go +++ b/internal/stages/flatpak_test.go @@ -21,21 +21,32 @@ flatpaks: - flathub-beta:org.mozilla.firefox `) mustContain(t, plan, - // exactly the declared remotes, added verbatim - "flatpak remote-add --if-not-exists flathub https://flathub.org/repo/flathub.flatpakrepo", - "flatpak remote-add --if-not-exists flathub-beta https://flathub.org/beta-repo/flathub-beta.flatpakrepo", - // each app installed from its own named remote (per-app) - "flatpak install -y --noninteractive flathub com.spotify.Client", - "flatpak install -y --noninteractive flathub-beta org.mozilla.firefox", + // exactly the declared remotes, added verbatim — per-user scope (--user) + // so no polkit/root is needed (org.freedesktop.Flatpak.modify-repo hang). + "flatpak --user remote-add --if-not-exists flathub https://flathub.org/repo/flathub.flatpakrepo", + "flatpak --user remote-add --if-not-exists flathub-beta https://flathub.org/beta-repo/flathub-beta.flatpakrepo", + // each app installed from its own named remote (per-app), per-user + noninteractive + "flatpak --user install -y --noninteractive flathub com.spotify.Client", + "flatpak --user install -y --noninteractive flathub-beta org.mozilla.firefox", ) + joined := strings.Join(plan, "\n") + // No unconditional/built-in flathub remote-add: the only flathub remote-add // is the one the config declared (asserted above). There must be no install // that pins every app to flathub regardless of its declared remote. - joined := strings.Join(plan, "\n") - if strings.Contains(joined, "flatpak install -y --noninteractive flathub org.mozilla.firefox") { + if strings.Contains(joined, "flatpak --user install -y --noninteractive flathub org.mozilla.firefox") { t.Errorf("firefox should install from flathub-beta, not flathub; plan:\n%s", joined) } + + // Every flatpak invocation must stay unprivileged (Cmd, never Root/sudo): + // a system-scope flatpak op as the user drops into a polkit Password: prompt + // and bootstrap hangs forever in a headless/TTY session. + for _, line := range plan { + if strings.HasPrefix(line, "sudo ") && strings.Contains(line, "flatpak") { + t.Errorf("flatpak must run unprivileged (no sudo) to avoid a polkit hang; got: %q", line) + } + } } func TestFlatpak_NoRemotesNoImplicitFlathub(t *testing.T) { diff --git a/internal/stages/selectors_test.go b/internal/stages/selectors_test.go index 59acc49..378a17f 100644 --- a/internal/stages/selectors_test.go +++ b/internal/stages/selectors_test.go @@ -97,9 +97,9 @@ flatpaks: [flathub-beta:com.spotify.Client] `) mustContain(t, plan, // exactly the declared remotes — no remote is implicit - "flatpak remote-add --if-not-exists flathub https://flathub.org/repo/flathub.flatpakrepo", - "flatpak remote-add --if-not-exists flathub-beta https://flathub.org/beta-repo/flathub-beta.flatpakrepo", + "flatpak --user remote-add --if-not-exists flathub https://flathub.org/repo/flathub.flatpakrepo", + "flatpak --user remote-add --if-not-exists flathub-beta https://flathub.org/beta-repo/flathub-beta.flatpakrepo", // the app installs from its named remote - "flatpak install -y --noninteractive flathub-beta com.spotify.Client", + "flatpak --user install -y --noninteractive flathub-beta com.spotify.Client", ) } diff --git a/internal/stages/stages_test.go b/internal/stages/stages_test.go index a1d16be..6eda719 100644 --- a/internal/stages/stages_test.go +++ b/internal/stages/stages_test.go @@ -208,8 +208,8 @@ func TestPlan_AUR(t *testing.T) { func TestPlan_Flatpak(t *testing.T) { mustContain(t, planFor(t, Bootstrap, "flatpak"), - "flatpak remote-add --if-not-exists flathub https://flathub.org/repo/flathub.flatpakrepo", - "flatpak install -y --noninteractive flathub com.spotify.Client", + "flatpak --user remote-add --if-not-exists flathub https://flathub.org/repo/flathub.flatpakrepo", + "flatpak --user install -y --noninteractive flathub com.spotify.Client", ) } From c089b0feb11c4148558461f3b62609085e351d3c Mon Sep 17 00:00:00 2001 From: Adam Hall Date: Tue, 23 Jun 2026 18:44:13 +0930 Subject: [PATCH 2/9] fix(plymouth): gate stage when unconfigured and make bootctl update graceful On systemd-boot installs Phase B bootstrap aborted at the always-on plymouth stage: regenerateBootConfig ran `bootctl update`, which exits non-zero when the loader archinstall just installed is already current, and archwright treats that as a stage failure. Because the plymouth stage ran unconditionally (defaulting the theme to bgrt), every systemd-boot bootstrap hit this out of the box. - regenerateBootConfig: systemd-boot path now runs `bootctl update --graceful` (exits 0 when there is nothing to do) through TryRoot (best-effort, still adds sudo in Phase B) so a nonzero exit can never abort the stage. The grub path is byte-identical: grub-mkconfig -o /boot/grub/grub.cfg through Root. - plymouth: early-return no-op when Plymouth.Theme == "", so a default config never installs plymouth or touches the bootloader. Adds plymouth_test.go covering the unconfigured no-op, the graceful systemd-boot invocation, and the unchanged grub path. Co-Authored-By: Claude Opus 4.8 --- internal/stages/helpers.go | 19 ++++++--- internal/stages/plymouth.go | 6 ++- internal/stages/plymouth_test.go | 73 ++++++++++++++++++++++++++++++++ 3 files changed, 90 insertions(+), 8 deletions(-) create mode 100644 internal/stages/plymouth_test.go diff --git a/internal/stages/helpers.go b/internal/stages/helpers.go index e06c001..db1f423 100644 --- a/internal/stages/helpers.go +++ b/internal/stages/helpers.go @@ -50,15 +50,20 @@ func ensureKernelParam(ctx *Context, tok string) error { // "apply the config" step, keyed off the configured bootloader: // - grub: runs grub-mkconfig -o /boot/grub/grub.cfg through Root — UNCHANGED from // before, byte-identical command. -// - systemd-boot: there is no grub.cfg to regenerate. We run `bootctl update` to -// refresh the installed boot loader binary on the ESP; loader entries are -// regenerated separately by kernel-install when the cmdline changes. This is -// VM-validation-pending: on some setups `bootctl update` is a no-op and the -// entry refresh happens via the kernel-install hooks instead — verify against a -// real systemd-boot system in a QEMU VM before trusting on hardware. +// - systemd-boot: there is no grub.cfg to regenerate. We refresh the installed +// boot loader binary on the ESP with `bootctl update --graceful`; loader entries +// are regenerated separately by kernel-install when the cmdline changes. The +// `--graceful` flag makes bootctl exit 0 when there is nothing to do (the loader +// archinstall just installed is already current) instead of failing, and the +// whole step is best-effort via TryRoot so a nonzero exit can never abort the +// stage — the loader was already installed by archinstall and our cmdline edits +// live in /etc/kernel/cmdline, which systemd-boot reads directly. (See +// docs/bugs/plymouth-bootctl-update-fails-systemd-boot.md.) func regenerateBootConfig(ctx *Context) error { if ctx.Cfg.Bootloader.EffectiveKind() == "systemd-boot" { - return ctx.R.Root("bootctl", "update") + // Best-effort + graceful: never fail the stage on "already current". + ctx.R.TryRoot("bootctl", "update", "--graceful") + return nil } return ctx.R.Root("grub-mkconfig", "-o", "/boot/grub/grub.cfg") } diff --git a/internal/stages/plymouth.go b/internal/stages/plymouth.go index 4f9e2ba..58887e1 100644 --- a/internal/stages/plymouth.go +++ b/internal/stages/plymouth.go @@ -18,9 +18,13 @@ func (plymouth) Name() string { return "plymouth" } func (plymouth) Phase() Phase { return Bootstrap } func (plymouth) Run(ctx *Context) error { + // Gate off when unconfigured: with no plymouth.theme set the stage is a clean + // no-op so a default config never installs plymouth or touches the bootloader + // (see docs/bugs/plymouth-bootctl-update-fails-systemd-boot.md). theme := ctx.Cfg.Plymouth.Theme if theme == "" { - theme = "bgrt" + ui.Warn("no plymouth theme in config — skipping") + return nil } cmdline := ctx.Cfg.GRUB.CmdlineExtra if cmdline == "" { diff --git a/internal/stages/plymouth_test.go b/internal/stages/plymouth_test.go new file mode 100644 index 0000000..f122411 --- /dev/null +++ b/internal/stages/plymouth_test.go @@ -0,0 +1,73 @@ +package stages + +import "testing" + +// Regression coverage for docs/bugs/plymouth-bootctl-update-fails-systemd-boot.md: +// the plymouth stage must (a) be a clean no-op when no plymouth theme is configured +// and (b) refresh systemd-boot via the graceful, best-effort `bootctl update +// --graceful` rather than a plain `bootctl update` that aborts bootstrap when the +// loader is already current. The grub path stays byte-identical. Self-contained +// inline configs — no shared fixtures touched. + +// With no plymouth: block the stage installs nothing and never touches the +// bootloader, even on a systemd-boot config (the default-config case that broke +// every systemd-boot bootstrap). +func TestPlan_PlymouthUnconfiguredIsNoop(t *testing.T) { + plan := planForCfg(t, Bootstrap, "plymouth", ` +bootloader: + kind: systemd-boot +`) + if len(plan) != 0 { + t.Errorf("plymouth stage should be a no-op when unconfigured, got plan:\n%v", plan) + } + mustNotContain(t, plan, + "plymouth", + "bootctl", + "grub-mkconfig", + "/etc/kernel/cmdline", + "plymouth-set-default-theme", + ) +} + +// A configured theme on systemd-boot regenerates boot config via the graceful, +// best-effort invocation and never the plain failing form. +func TestPlan_PlymouthSystemdBootGraceful(t *testing.T) { + plan := planForCfg(t, Bootstrap, "plymouth", ` +bootloader: + kind: systemd-boot +plymouth: + theme: bgrt +`) + // best-effort step still goes through Root semantics (sudo in Phase B). + mustContain(t, plan, + "sudo bootctl update --graceful", + ) + // the plain, non-graceful form (which exits non-zero when already current and + // aborted bootstrap) must be gone, as must any grub regeneration. + if hasExact(plan, "sudo bootctl update") || hasExact(plan, "bootctl update") { + t.Errorf("plan must not contain a plain `bootctl update`; plan:\n%v", plan) + } + mustNotContain(t, plan, "grub-mkconfig -o /boot/grub/grub.cfg") +} + +// A configured theme on a grub config regenerates grub.cfg unchanged. +func TestPlan_PlymouthGrubUnchanged(t *testing.T) { + plan := planForCfg(t, Bootstrap, "plymouth", ` +plymouth: + theme: bgrt +`) + mustContain(t, plan, "grub-mkconfig -o /boot/grub/grub.cfg") + mustNotContain(t, plan, "bootctl") +} + +// hasExact reports whether any recorded plan line equals line exactly (as opposed +// to mustContain's substring match, which `bootctl update --graceful` would +// satisfy for the substring `bootctl update`). +func hasExact(plan []string, line string) bool { + for _, p := range plan { + if p == line { + return true + } + } + return false +} From abf766bfb062daf01c8edf2e72edca2740d03587 Mon Sep 17 00:00:00 2001 From: Adam Hall Date: Tue, 23 Jun 2026 18:44:49 +0930 Subject: [PATCH 3/9] fix(archinstall): set PV fs_type for multi-volume LVM so archinstall doesn't abort MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In multi-volume LVM mode (disks.lvm.volumes), lvm.filesystem is empty by schema, so the PV partition was rendered with fs_type: "" — archinstall 4.3 rejects this with `ValueError: File system type is not set` and the install aborts before partitioning completes. The PV partition's fs_type is cosmetic (the filesystem is never written; the partition is pvcreated) but must be non-empty for parted. Fall back to the first volume's filesystem when lvm.filesystem is empty, so both the disk-1 PV and any whole-disk PVs (which reuse pvFs) get a valid non-empty fs_type. Single-LV mode is unchanged. Adds an lvm-volumes render golden + a self-contained regression test asserting every PV partition has a non-empty fs_type. Co-Authored-By: Claude Opus 4.8 --- internal/archinstall/archinstall.go | 11 +- internal/archinstall/golden_test.go | 4 + internal/archinstall/lvm_volumes_test.go | 59 +++++++ .../testdata/configs/lvm-volumes.yaml | 29 ++++ .../testdata/golden/lvm-volumes.config.json | 150 ++++++++++++++++++ 5 files changed, 250 insertions(+), 3 deletions(-) create mode 100644 internal/archinstall/lvm_volumes_test.go create mode 100644 internal/archinstall/testdata/configs/lvm-volumes.yaml create mode 100644 internal/archinstall/testdata/golden/lvm-volumes.config.json diff --git a/internal/archinstall/archinstall.go b/internal/archinstall/archinstall.go index d74e0fa..2aca927 100644 --- a/internal/archinstall/archinstall.go +++ b/internal/archinstall/archinstall.go @@ -447,10 +447,15 @@ func (b *lvmBuilder) build(geom Geometry) ([]Device, *LvmConfiguration, error) { } pvOnDisk1 := roundDownMiB(disk1Total - used) - // A PV partition carries the LV filesystem as its fs_type purely so parted can - // create it (archinstall 4.x requires a non-null fs_type per partition); the - // filesystem is never written, the partition is pvcreated. + // A PV partition carries an LV filesystem as its fs_type purely so parted can + // create it (archinstall 4.x rejects an empty fs_type per partition); the + // filesystem is never written, the partition is pvcreated. In single-LV mode + // this is the root LV's filesystem; in multi-volume mode Filesystem is empty by + // schema, so fall back to the first volume's filesystem (any valid fs works). pvFs := b.lvm.Filesystem + if pvFs == "" && len(b.lvm.Volumes) > 0 { + pvFs = b.lvm.Volumes[0].Filesystem + } espPart := espPartition(espBytes) disk1PVPart := Partition{ ObjID: newObjID(), Status: "create", Type: "primary", diff --git a/internal/archinstall/golden_test.go b/internal/archinstall/golden_test.go index ef7f066..606dcc6 100644 --- a/internal/archinstall/golden_test.go +++ b/internal/archinstall/golden_test.go @@ -47,6 +47,10 @@ var renderCases = []struct { name: "lvm-zram", geom: Geometry{"/dev/nvme0n1": 256 << 30}, // 256 GiB }, + { + name: "lvm-volumes", + geom: Geometry{"/dev/nvme0n1": 256 << 30}, // 256 GiB + }, } // TestRenderGolden renders every fixture config against fixed geometry and diff --git a/internal/archinstall/lvm_volumes_test.go b/internal/archinstall/lvm_volumes_test.go new file mode 100644 index 0000000..8cafb2c --- /dev/null +++ b/internal/archinstall/lvm_volumes_test.go @@ -0,0 +1,59 @@ +package archinstall + +import ( + "testing" + + "github.com/AdamJHall/archwright/internal/config" +) + +// TestMultiVolumeLVMPVFsTypeNonEmpty guards the multi-volume LVM PV-partition +// regression: in multi-volume mode lvm.filesystem is empty by schema, yet every +// PV partition must still carry a non-empty fs_type or archinstall 4.x aborts +// with "File system type is not set". The PV partitions inherit the first +// volume's filesystem. +func TestMultiVolumeLVMPVFsTypeNonEmpty(t *testing.T) { + defer setDeterministicObjIDs()() + + b := &lvmBuilder{ + esp: config.ESPConfig{Device: "/dev/nvme0n1", Size: "1GiB"}, + swap: config.SwapConfig{Type: "swapfile", Size: "8GiB"}, + espBytes: 1 << 30, + lvm: config.LVMLayout{ + VG: "vg0", + PVs: []string{"/dev/nvme0n1p2", "/dev/sda"}, + Volumes: []config.LVMVolume{ + {Name: "root", Mountpoint: "/", Filesystem: "xfs", Size: "50GiB"}, + {Name: "home", Mountpoint: "/home", Filesystem: "ext4"}, + }, + }, + } + geom := Geometry{ + "/dev/nvme0n1": 256 << 30, + "/dev/sda": 512 << 30, + } + + devices, lvm, err := b.build(geom) + if err != nil { + t.Fatalf("build: %v", err) + } + if lvm == nil { + t.Fatal("expected an LvmConfiguration") + } + + var pvParts int + for _, dev := range devices { + for _, p := range dev.Partitions { + // PV partitions are the non-ESP partitions (no mountpoint). + if p.Mountpoint != nil { + continue + } + pvParts++ + if p.FsType == nil || *p.FsType == "" { + t.Errorf("PV partition on %s has empty fs_type; archinstall would abort", dev.Device) + } + } + } + if pvParts == 0 { + t.Fatal("expected at least one PV partition") + } +} diff --git a/internal/archinstall/testdata/configs/lvm-volumes.yaml b/internal/archinstall/testdata/configs/lvm-volumes.yaml new file mode 100644 index 0000000..4414847 --- /dev/null +++ b/internal/archinstall/testdata/configs/lvm-volumes.yaml @@ -0,0 +1,29 @@ +# Single NVMe disk, multi-volume LVM: a fixed root LV (xfs) plus a /home LV +# (ext4) taking the rest of the VG. Exercises the multi-volume PV fs_type path. +system: + hostname: arch-volumes + timezone: Europe/London + locale: en_GB.UTF-8 + keymap: uk +user: + name: adam +pacstrap: [base-devel, git, zsh, sudo, networkmanager, efibootmgr, intel-ucode] +kernel: + base: [linux] +disks: + esp: + device: /dev/nvme0n1 + size: 1GiB + swap: + size: 8GiB + lvm: + vg: vg0 + pvs: [/dev/nvme0n1p2] + volumes: + - name: root + mountpoint: / + filesystem: xfs + size: 50GiB + - name: home + mountpoint: /home + filesystem: ext4 diff --git a/internal/archinstall/testdata/golden/lvm-volumes.config.json b/internal/archinstall/testdata/golden/lvm-volumes.config.json new file mode 100644 index 0000000..1eed940 --- /dev/null +++ b/internal/archinstall/testdata/golden/lvm-volumes.config.json @@ -0,0 +1,150 @@ +{ + "archinstall-language": "English", + "bootloader_config": { + "bootloader": "Grub", + "uki": false, + "removable": false + }, + "kernels": [ + "linux" + ], + "hostname": "arch-volumes", + "packages": [ + "base-devel", + "git", + "zsh", + "sudo", + "networkmanager", + "efibootmgr", + "intel-ucode" + ], + "timezone": "Europe/London", + "ntp": true, + "swap": false, + "locale_config": { + "kb_layout": "uk", + "sys_enc": "UTF-8", + "sys_lang": "en_GB" + }, + "network_config": { + "type": "nm" + }, + "disk_config": { + "config_type": "manual_partitioning", + "device_modifications": [ + { + "device": "/dev/nvme0n1", + "wipe": true, + "partitions": [ + { + "obj_id": "objid-0001", + "status": "create", + "type": "primary", + "start": { + "value": 1048576, + "unit": "B", + "sector_size": { + "value": 512, + "unit": "B" + } + }, + "size": { + "value": 1073741824, + "unit": "B", + "sector_size": { + "value": 512, + "unit": "B" + } + }, + "fs_type": "fat32", + "mountpoint": "/boot", + "mount_options": [], + "flags": [ + "boot", + "esp" + ], + "dev_path": null, + "btrfs": [] + }, + { + "obj_id": "objid-0002", + "status": "create", + "type": "primary", + "start": { + "value": 1074790400, + "unit": "B", + "sector_size": { + "value": 512, + "unit": "B" + } + }, + "size": { + "value": 273802067968, + "unit": "B", + "sector_size": { + "value": 512, + "unit": "B" + } + }, + "fs_type": "xfs", + "mountpoint": null, + "mount_options": [], + "flags": [], + "dev_path": null, + "btrfs": [] + } + ] + } + ], + "lvm_config": { + "config_type": "default", + "vol_groups": [ + { + "name": "vg0", + "lvm_pvs": [ + "objid-0002" + ], + "volumes": [ + { + "obj_id": "objid-0003", + "status": "create", + "name": "root", + "fs_type": "xfs", + "length": { + "value": 53687091200, + "unit": "B", + "sector_size": { + "value": 512, + "unit": "B" + } + }, + "mountpoint": "/", + "mount_options": [], + "btrfs": [] + }, + { + "obj_id": "objid-0004", + "status": "create", + "name": "home", + "fs_type": "ext4", + "length": { + "value": 220106588160, + "unit": "B", + "sector_size": { + "value": 512, + "unit": "B" + } + }, + "mountpoint": "/home", + "mount_options": [], + "btrfs": [] + } + ] + } + ] + }, + "disk_encryption": null + }, + "version": "4.3", + "config_version": "4.3" +} From 538ad102b00cb1e89b34c4da7575b29b8542fa63 Mon Sep 17 00:00:00 2001 From: Adam Hall Date: Tue, 23 Jun 2026 20:40:51 +0930 Subject: [PATCH 4/9] fix(archinstall): null btrfs root partition mountpoint so @ is used as root MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit For the btrfs layout the root partition was rendered with both a partition-level mountpoint "/" and a subvolume @ also mapped to "/". archinstall 4.3 resolves that by installing into the top-level subvolume (subvolid 5), leaving the configured @ created but empty and unused as root — the standard Arch/snapper layout (system inside @, mounted subvol=@) was never produced. singleDiskRoot now emits the root partition with a null mountpoint when it carries btrfs subvolumes, so the subvolume entry ({"name":"@","mountpoint": "/"}) drives the root mount. Plain/ext4/lvm layouts (no subvolumes) keep their direct partition mountpoint "/" unchanged, so only the btrfs-subvols golden changes (mountpoint: "/" -> null). Knock-on: buildEncryption located the root partition by Mountpoint == "/", which a null-mountpoint btrfs root no longer matches. Introduce partitionIsRoot(), which treats a partition as root if its own mountpoint is "/" OR it carries a subvolume mapped to "/", and use it for the luks-on-btrfs root lookup. Adds a self-contained regression test asserting the btrfs root partition mountpoint is null while @ maps to "/". Co-Authored-By: Claude Opus 4.8 --- internal/archinstall/archinstall.go | 32 +++++- internal/archinstall/btrfs_root_test.go | 100 ++++++++++++++++++ internal/archinstall/btrfs_test.go | 5 +- internal/archinstall/encryption_test.go | 6 +- .../testdata/golden/btrfs-subvols.config.json | 2 +- 5 files changed, 139 insertions(+), 6 deletions(-) create mode 100644 internal/archinstall/btrfs_root_test.go diff --git a/internal/archinstall/archinstall.go b/internal/archinstall/archinstall.go index d74e0fa..21c0ba4 100644 --- a/internal/archinstall/archinstall.go +++ b/internal/archinstall/archinstall.go @@ -358,7 +358,7 @@ func buildEncryption(encType string, devices []Device, lvm *LvmConfiguration) (* var rootObjID string for _, dev := range devices { for _, p := range dev.Partitions { - if p.Mountpoint != nil && *p.Mountpoint == "/" { + if partitionIsRoot(p) { rootObjID = p.ObjID } } @@ -643,16 +643,44 @@ func singleDiskRoot(esp config.ESPConfig, swap config.SwapConfig, espBytes uint6 root := "/" rootFs := spec.fsType + + // When the root partition carries btrfs subvolumes, the subvolume entries + // (e.g. {"name":"@","mountpoint":"/"}) provide the mountpoints, so the + // partition's own mountpoint must be null — otherwise archinstall mounts the + // bare partition (top-level subvol, subvolid 5) at "/" and the configured @ + // subvolume is created but never used as root. With no subvolumes + // (plain/ext4 layouts) the partition itself is mounted at "/". + rootMount := &root + if len(spec.btrfs) > 0 { + rootMount = nil + } parts = append(parts, Partition{ ObjID: newObjID(), Status: "create", Type: "primary", Start: bytes(offset), Size: bytes(rootBytes), - FsType: &rootFs, Mountpoint: &root, + FsType: &rootFs, Mountpoint: rootMount, MountOptions: spec.mountOptions, Flags: []string{}, Btrfs: spec.btrfs, }) return []Device{{Device: disk1, Wipe: true, Partitions: parts}}, nil } +// partitionIsRoot reports whether p provides the "/" mount, accounting for both +// shapes: a plain/ext4/lvm partition mounted directly at "/" (Mountpoint == "/"), +// and a btrfs partition whose own Mountpoint is null but which carries a +// subvolume (e.g. "@") mapped to "/". The btrfs case matters because the root +// partition's Mountpoint is deliberately null when subvolumes drive the mounts. +func partitionIsRoot(p Partition) bool { + if p.Mountpoint != nil && *p.Mountpoint == "/" { + return true + } + for _, sv := range p.Btrfs { + if bs, ok := sv.(BtrfsSubvolume); ok && bs.Mountpoint != nil && *bs.Mountpoint == "/" { + return true + } + } + return false +} + // --- btrfs layout ----------------------------------------------------------- // btrfsBuilder is a single btrfs root partition carrying subvolumes (the common diff --git a/internal/archinstall/btrfs_root_test.go b/internal/archinstall/btrfs_root_test.go new file mode 100644 index 0000000..4140511 --- /dev/null +++ b/internal/archinstall/btrfs_root_test.go @@ -0,0 +1,100 @@ +package archinstall + +import ( + "os" + "path/filepath" + "testing" + + "github.com/AdamJHall/archwright/internal/config" +) + +// btrfsRootYAML is a self-contained btrfs layout: a single NVMe disk with an ESP +// and a btrfs root carrying @ (/) and @home (/home) subvolumes. It deliberately +// does not edit any shared fixture so this regression test stands alone. +const btrfsRootYAML = ` +system: + hostname: arch-btrfs-root + timezone: Europe/London + locale: en_GB.UTF-8 + keymap: uk +user: + name: adam +pacstrap: [base-devel, networkmanager] +kernel: + base: [linux] +disks: + layout: btrfs + esp: + device: /dev/nvme0n1 + size: 1GiB + swap: + type: zram + btrfs: + device: /dev/nvme0n1 + compress: zstd + subvolumes: + - {name: "@", mountpoint: /} + - {name: "@home", mountpoint: /home} +` + +// TestBtrfsRootPartitionMountpointNull asserts the bug fix: for a btrfs layout +// the root partition itself must have a nil (null) mountpoint, while the @ +// subvolume entry is what maps to "/". Otherwise archinstall installs into the +// top-level subvolume (subvolid 5) and leaves @ empty. +func TestBtrfsRootPartitionMountpointNull(t *testing.T) { + dir := t.TempDir() + cfgPath := filepath.Join(dir, "btrfs-root.yaml") + if err := os.WriteFile(cfgPath, []byte(btrfsRootYAML), 0o644); err != nil { + t.Fatalf("write config: %v", err) + } + + cfg, err := config.Load(cfgPath) + if err != nil { + t.Fatalf("load config: %v", err) + } + if err := cfg.Validate(); err != nil { + t.Fatalf("config invalid: %v", err) + } + + geom := Geometry{"/dev/nvme0n1": 256 << 30} + c, _, err := Build(cfg, geom, "TESTPASS") + if err != nil { + t.Fatalf("Build: %v", err) + } + + // Find the btrfs root partition (the one carrying subvolumes). + var root *Partition + for i := range c.DiskConfig.DeviceModifications { + for j := range c.DiskConfig.DeviceModifications[i].Partitions { + p := &c.DiskConfig.DeviceModifications[i].Partitions[j] + if len(p.Btrfs) > 0 { + root = p + } + } + } + if root == nil { + t.Fatal("no btrfs root partition found in rendered config") + } + + if root.Mountpoint != nil { + t.Errorf("btrfs root partition mountpoint = %q, want nil (null) so @ drives the root mount", *root.Mountpoint) + } + + // The @ subvolume must be the one mounted at "/". + var atRoot bool + for _, sv := range root.Btrfs { + bs, ok := sv.(BtrfsSubvolume) + if !ok { + t.Fatalf("subvolume entry has unexpected type %T", sv) + } + if bs.Name == "@" { + if bs.Mountpoint == nil || *bs.Mountpoint != "/" { + t.Errorf("@ subvolume mountpoint = %v, want %q", bs.Mountpoint, "/") + } + atRoot = true + } + } + if !atRoot { + t.Error("no @ subvolume mapping to / found on the btrfs root partition") + } +} diff --git a/internal/archinstall/btrfs_test.go b/internal/archinstall/btrfs_test.go index 259e5d1..08c0e27 100644 --- a/internal/archinstall/btrfs_test.go +++ b/internal/archinstall/btrfs_test.go @@ -67,7 +67,10 @@ func TestBuild_BtrfsLayout(t *testing.T) { t.Fatalf("want ESP + btrfs root, got %d", len(parts)) } root := parts[1] - if root.FsType == nil || *root.FsType != "btrfs" || root.Mountpoint == nil || *root.Mountpoint != "/" { + // The btrfs root partition's own mountpoint is null: the @ subvolume below + // (mountpoint "/") is what archinstall mounts at /, so the system lands + // inside @ rather than the top-level subvolume. + if root.FsType == nil || *root.FsType != "btrfs" || root.Mountpoint != nil { t.Errorf("btrfs root wrong: %+v", root) } if len(root.MountOptions) != 1 || root.MountOptions[0] != "compress=zstd" { diff --git a/internal/archinstall/encryption_test.go b/internal/archinstall/encryption_test.go index d4bc022..08c416c 100644 --- a/internal/archinstall/encryption_test.go +++ b/internal/archinstall/encryption_test.go @@ -121,10 +121,12 @@ func TestBuild_LuksOnBtrfsRoot(t *testing.T) { if len(enc.Partitions) != 1 { t.Fatalf("want exactly 1 encrypted partition, got %d", len(enc.Partitions)) } - // The encrypted partition must be the one mounted at "/". + // The encrypted partition must be the one providing "/". For btrfs that is + // the partition carrying the @ subvolume mapped to "/" (its own mountpoint + // is null), not a partition mounted directly at "/". var rootObjID string for _, p := range c.DiskConfig.DeviceModifications[0].Partitions { - if p.Mountpoint != nil && *p.Mountpoint == "/" { + if partitionIsRoot(p) { rootObjID = p.ObjID } } diff --git a/internal/archinstall/testdata/golden/btrfs-subvols.config.json b/internal/archinstall/testdata/golden/btrfs-subvols.config.json index 6f62641..fed8f37 100644 --- a/internal/archinstall/testdata/golden/btrfs-subvols.config.json +++ b/internal/archinstall/testdata/golden/btrfs-subvols.config.json @@ -87,7 +87,7 @@ } }, "fs_type": "btrfs", - "mountpoint": "/", + "mountpoint": null, "mount_options": [ "compress=zstd" ], From 9a15840acfba1aa4b5cfd562a30792b5acd5563a Mon Sep 17 00:00:00 2001 From: Adam Hall Date: Tue, 23 Jun 2026 20:45:17 +0930 Subject: [PATCH 5/9] chore: allow pkill qemu in local permission settings Adds the pkill qemu-system-x86_64 permission used when cleaning up stray headless VM e2e processes. Co-Authored-By: Claude Opus 4.8 --- .claude/settings.local.json | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/.claude/settings.local.json b/.claude/settings.local.json index 075eabd..bb289dc 100644 --- a/.claude/settings.local.json +++ b/.claude/settings.local.json @@ -31,7 +31,8 @@ "Bash(bash -n test/vm.sh)", "Bash(git worktree *)", "Bash(git --no-pager diff --stat)", - "Bash(python3 -c \"import yaml,sys; yaml.safe_load\\(open\\('config.example.yaml'\\)\\); print\\('config.example.yaml: YAML OK'\\)\")" + "Bash(python3 -c \"import yaml,sys; yaml.safe_load\\(open\\('config.example.yaml'\\)\\); print\\('config.example.yaml: YAML OK'\\)\")", + "Bash(pkill -9 -f qemu-system-x86_64)" ] } } From 56a57e16c880b19aa9e5a86d8d6f4df342253efc Mon Sep 17 00:00:00 2001 From: Adam Hall Date: Tue, 23 Jun 2026 20:51:37 +0930 Subject: [PATCH 6/9] fix(install): mount btrfs root subvolume for post-install staging MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The btrfs render fix (root partition mountpoint:null, @ drives /) means archinstall now installs the system INSIDE the @ subvolume. postInstall still mounted the bare partition at /mnt, which exposes only the empty top-level subvolume — so `mount /mnt/boot` failed with "mount point does not exist" and Phase A aborted before staging. targetMounts now rebuilds the installed mount tree: for btrfs it mounts the root subvolume (subvol=@) at /mnt, the ESP at /mnt/boot, and any non-root subvolume (e.g. @home at /home, parents before children) so the staged binary/config land in the right subvolume instead of being shadowed at boot. Plain/lvm layouts are unchanged. Adds a targetMounts unit test. Co-Authored-By: Claude Opus 4.8 --- internal/stages/archinstall.go | 87 ++++++++++++++++++++++++--- internal/stages/target_mounts_test.go | 85 ++++++++++++++++++++++++++ 2 files changed, 165 insertions(+), 7 deletions(-) create mode 100644 internal/stages/target_mounts_test.go diff --git a/internal/stages/archinstall.go b/internal/stages/archinstall.go index 7a4759b..654213f 100644 --- a/internal/stages/archinstall.go +++ b/internal/stages/archinstall.go @@ -5,6 +5,7 @@ import ( "fmt" "os" "os/exec" + "sort" "strconv" "strings" @@ -128,13 +129,30 @@ func postInstall(ctx *Context) error { } esp := archinstall.PartDev(ctx.Cfg.Disks.ESP.Device, 1) - // Checked mounts (Issue #2): a failed remount must abort, not silently run the - // chroot steps against an empty /mnt. (The umounts below stay best-effort.) - if err := ctx.R.Root("mount", rootDev, "/mnt"); err != nil { + // Remount the target for the chroot work. archinstall unmounts on finish, so we + // rebuild the mount tree exactly as the installed system sees it. + // + // For btrfs the system lives INSIDE the root subvolume (e.g. @), not the + // top-level subvolume: archinstall installs into @ and mounts it at /. Mounting + // the bare partition would expose only the (empty) top-level subvol — /boot and + // /home/ would be missing and staging would fail — so we must mount the + // root subvolume with subvol=, and mount any non-root subvolumes + // (e.g. @home at /home) so staged files land in the right subvolume. + mounts, err := targetMounts(ctx.Cfg, rootDev, esp) + if err != nil { return err } - if err := ctx.R.Root("mount", esp, "/mnt/boot"); err != nil { - return err + // Checked mounts (Issue #2): a failed remount must abort, not silently run the + // chroot steps against an empty /mnt. (The umounts below stay best-effort.) + for _, m := range mounts { + var args []string + if len(m.opts) > 0 { + args = append(args, "-o", strings.Join(m.opts, ",")) + } + args = append(args, m.dev, m.target) + if err := ctx.R.Root("mount", args...); err != nil { + return err + } } if err := setupSwapfile(ctx); err != nil { @@ -164,11 +182,66 @@ func postInstall(ctx *Context) error { return err } - ctx.R.Try("umount", "/mnt/boot") - ctx.R.Try("umount", "/mnt") + // Unmount in reverse so nested subvolume/ESP mounts come off before /mnt. + for i := len(mounts) - 1; i >= 0; i-- { + ctx.R.Try("umount", mounts[i].target) + } return nil } +// mount is one entry in the target mount tree rebuilt for post-install chroot +// work: a device mounted at an absolute /mnt path with optional mount options. +type mount struct { + dev string + target string + opts []string +} + +// targetMounts builds the ordered mount tree for the installed system. For +// plain/lvm layouts it is just the root device at /mnt and the ESP at /mnt/boot. +// For btrfs the system lives inside the root subvolume, so the root device is +// mounted with subvol= at /mnt, the ESP at /mnt/boot, and every non-root +// subvolume at its mountpoint under /mnt (parents before children) so staging +// into /mnt/home/ lands in the correct subvolume rather than being +// shadowed once the subvolume is mounted at boot. +func targetMounts(cfg *config.Config, rootDev, esp string) ([]mount, error) { + if cfg.Disks.EffectiveLayout() == "btrfs" && cfg.Disks.Btrfs != nil { + var rootSub string + var others []config.Subvol + for _, sv := range cfg.Disks.Btrfs.Subvolumes { + if sv.Mountpoint == "/" { + rootSub = sv.Name + } else { + others = append(others, sv) + } + } + if rootSub == "" { + return nil, fmt.Errorf("btrfs layout has no subvolume mounted at /") + } + mounts := []mount{ + {dev: rootDev, target: "/mnt", opts: []string{"subvol=" + rootSub}}, + {dev: esp, target: "/mnt/boot"}, + } + // Shallower mountpoints first so a parent subvolume is mounted before any + // nested child (e.g. /home before /home/foo). + sort.SliceStable(others, func(i, j int) bool { + return strings.Count(others[i].Mountpoint, "/") < strings.Count(others[j].Mountpoint, "/") + }) + for _, sv := range others { + mounts = append(mounts, mount{ + dev: rootDev, + target: "/mnt" + sv.Mountpoint, + opts: []string{"subvol=" + sv.Name}, + }) + } + return mounts, nil + } + return []mount{ + {dev: rootDev, target: "/mnt"}, + {dev: esp, target: "/mnt/boot"}, + }, nil +} + // setupSwapfile creates /swapfile on the freshly installed root and enables it // via fstab. archinstall 4.x can't format a raw swap partition in an LVM layout // (its LVM path formats only the boot partition), so swap lives as a file sized diff --git a/internal/stages/target_mounts_test.go b/internal/stages/target_mounts_test.go new file mode 100644 index 0000000..376f1bf --- /dev/null +++ b/internal/stages/target_mounts_test.go @@ -0,0 +1,85 @@ +package stages + +import ( + "testing" + + "github.com/AdamJHall/archwright/internal/config" +) + +// TestTargetMounts_Btrfs asserts the post-install mount tree for btrfs uses the +// root subvolume (subvol=@), not the bare partition, and mounts non-root +// subvolumes at their mountpoints (parents before children). Mounting the bare +// partition would expose only the empty top-level subvolume, so /boot and +// /home/ staging would fail — this is the regression guarded here. +func TestTargetMounts_Btrfs(t *testing.T) { + cfg := &config.Config{} + cfg.Disks.Layout = "btrfs" + cfg.Disks.Btrfs = &config.BtrfsLayout{ + Subvolumes: []config.Subvol{ + {Name: "@home", Mountpoint: "/home"}, + {Name: "@", Mountpoint: "/"}, + }, + } + + got, err := targetMounts(cfg, "/dev/vda2", "/dev/vda1") + if err != nil { + t.Fatalf("targetMounts: %v", err) + } + + want := []mount{ + {dev: "/dev/vda2", target: "/mnt", opts: []string{"subvol=@"}}, + {dev: "/dev/vda1", target: "/mnt/boot"}, + {dev: "/dev/vda2", target: "/mnt/home", opts: []string{"subvol=@home"}}, + } + assertMounts(t, want, got) +} + +// TestTargetMounts_BtrfsNoRoot errors when no subvolume maps to "/". +func TestTargetMounts_BtrfsNoRoot(t *testing.T) { + cfg := &config.Config{} + cfg.Disks.Layout = "btrfs" + cfg.Disks.Btrfs = &config.BtrfsLayout{ + Subvolumes: []config.Subvol{{Name: "@home", Mountpoint: "/home"}}, + } + if _, err := targetMounts(cfg, "/dev/vda2", "/dev/vda1"); err == nil { + t.Fatal("want error when no subvolume is mounted at /, got nil") + } +} + +// TestTargetMounts_Plain leaves the non-btrfs path unchanged: bare root at /mnt, +// ESP at /mnt/boot, no subvol options. +func TestTargetMounts_Plain(t *testing.T) { + cfg := &config.Config{} + cfg.Disks.Layout = "plain" + + got, err := targetMounts(cfg, "/dev/vda2", "/dev/vda1") + if err != nil { + t.Fatalf("targetMounts: %v", err) + } + want := []mount{ + {dev: "/dev/vda2", target: "/mnt"}, + {dev: "/dev/vda1", target: "/mnt/boot"}, + } + assertMounts(t, want, got) +} + +func assertMounts(t *testing.T, want, got []mount) { + t.Helper() + if len(got) != len(want) { + t.Fatalf("mount count = %d, want %d: %+v", len(got), len(want), got) + } + for i := range want { + if got[i].dev != want[i].dev || got[i].target != want[i].target { + t.Errorf("mount[%d] = {%s %s}, want {%s %s}", i, got[i].dev, got[i].target, want[i].dev, want[i].target) + } + if len(got[i].opts) != len(want[i].opts) { + t.Errorf("mount[%d] opts = %v, want %v", i, got[i].opts, want[i].opts) + continue + } + for j := range want[i].opts { + if got[i].opts[j] != want[i].opts[j] { + t.Errorf("mount[%d] opts[%d] = %q, want %q", i, j, got[i].opts[j], want[i].opts[j]) + } + } + } +} From ddfd80d77d2c7335fe92b32f52a478212c4a7719 Mon Sep 17 00:00:00 2001 From: Adam Hall Date: Tue, 23 Jun 2026 21:01:02 +0930 Subject: [PATCH 7/9] fix(install): mount separate /home for btrfs @home and multi-volume LVM MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A layout with a separate /home (btrfs @home, or an LVM /home volume) staged the Phase B binary/config into /mnt/home/ on the ROOT filesystem, which is shadowed once the real /home mounts over it at boot — stranding the binary/config (and, in the e2e harness, the autorun trigger, so the VM hangs at the login prompt). targetMounts now also remounts non-root LVM volumes at their mountpoints (parents before children), matching the btrfs subvolume handling, so staging lands in the filesystem each path resolves to at boot. Factored the shallowest-first ordering into a sortByDepth helper. Updates the e2e descriptors (btrfs mounts subvol=@; lvm-volumes mounts the home LV) and the now-stale btrfs-basic config comment, and adds LVM targetMounts tests. Co-Authored-By: Claude Opus 4.8 --- internal/stages/archinstall.go | 65 +++++++++++++++++++++------ internal/stages/target_mounts_test.go | 44 ++++++++++++++++++ test/e2e/vm/configs/btrfs-basic.yaml | 8 ++-- test/e2e/vm/matrix/btrfs.py | 14 +++--- test/e2e/vm/matrix/lvm_variants.py | 4 ++ 5 files changed, 110 insertions(+), 25 deletions(-) diff --git a/internal/stages/archinstall.go b/internal/stages/archinstall.go index 654213f..adc055b 100644 --- a/internal/stages/archinstall.go +++ b/internal/stages/archinstall.go @@ -197,15 +197,27 @@ type mount struct { opts []string } -// targetMounts builds the ordered mount tree for the installed system. For -// plain/lvm layouts it is just the root device at /mnt and the ESP at /mnt/boot. -// For btrfs the system lives inside the root subvolume, so the root device is -// mounted with subvol= at /mnt, the ESP at /mnt/boot, and every non-root -// subvolume at its mountpoint under /mnt (parents before children) so staging -// into /mnt/home/ lands in the correct subvolume rather than being -// shadowed once the subvolume is mounted at boot. +// targetMounts builds the ordered mount tree for the installed system, rebuilt +// so post-install staging writes into the same filesystem each path resolves to +// at boot. The root device is at /mnt and the ESP at /mnt/boot; crucially, any +// layout with a SEPARATE /home (or other non-root mount) must also remount it, +// otherwise staging into /mnt/home/ lands on the root fs and is shadowed +// once the real /home mounts over it at boot — which strands the Phase B binary +// and config (and, in the e2e harness, the autorun trigger). +// +// - btrfs: the system lives inside the root subvolume, so root is mounted with +// subvol= and every non-root subvolume at its mountpoint under /mnt. +// - lvm multi-volume: each non-root volume (e.g. a /home LV) is mounted at its +// mountpoint under /mnt via /dev//. +// +// Non-root mounts are ordered shallowest-first so a parent is mounted before any +// nested child (e.g. /home before /home/foo). func targetMounts(cfg *config.Config, rootDev, esp string) ([]mount, error) { - if cfg.Disks.EffectiveLayout() == "btrfs" && cfg.Disks.Btrfs != nil { + switch cfg.Disks.EffectiveLayout() { + case "btrfs": + if cfg.Disks.Btrfs == nil { + break + } var rootSub string var others []config.Subvol for _, sv := range cfg.Disks.Btrfs.Subvolumes { @@ -222,11 +234,7 @@ func targetMounts(cfg *config.Config, rootDev, esp string) ([]mount, error) { {dev: rootDev, target: "/mnt", opts: []string{"subvol=" + rootSub}}, {dev: esp, target: "/mnt/boot"}, } - // Shallower mountpoints first so a parent subvolume is mounted before any - // nested child (e.g. /home before /home/foo). - sort.SliceStable(others, func(i, j int) bool { - return strings.Count(others[i].Mountpoint, "/") < strings.Count(others[j].Mountpoint, "/") - }) + sortByDepth(others, func(s config.Subvol) string { return s.Mountpoint }) for _, sv := range others { mounts = append(mounts, mount{ dev: rootDev, @@ -235,13 +243,44 @@ func targetMounts(cfg *config.Config, rootDev, esp string) ([]mount, error) { }) } return mounts, nil + + case "lvm": + mounts := []mount{ + {dev: rootDev, target: "/mnt"}, + {dev: esp, target: "/mnt/boot"}, + } + if cfg.Disks.LVM != nil { + var others []config.LVMVolume + for _, v := range cfg.Disks.LVM.Volumes { + if v.Mountpoint != "/" { + others = append(others, v) + } + } + sortByDepth(others, func(v config.LVMVolume) string { return v.Mountpoint }) + for _, v := range others { + mounts = append(mounts, mount{ + dev: fmt.Sprintf("/dev/%s/%s", cfg.Disks.LVM.VG, v.Name), + target: "/mnt" + v.Mountpoint, + }) + } + } + return mounts, nil } + return []mount{ {dev: rootDev, target: "/mnt"}, {dev: esp, target: "/mnt/boot"}, }, nil } +// sortByDepth orders entries shallowest-mountpoint first (by path separator +// count) so a parent mount is always applied before a nested child. +func sortByDepth[T any](s []T, mp func(T) string) { + sort.SliceStable(s, func(i, j int) bool { + return strings.Count(mp(s[i]), "/") < strings.Count(mp(s[j]), "/") + }) +} + // setupSwapfile creates /swapfile on the freshly installed root and enables it // via fstab. archinstall 4.x can't format a raw swap partition in an LVM layout // (its LVM path formats only the boot partition), so swap lives as a file sized diff --git a/internal/stages/target_mounts_test.go b/internal/stages/target_mounts_test.go index 376f1bf..72d9a61 100644 --- a/internal/stages/target_mounts_test.go +++ b/internal/stages/target_mounts_test.go @@ -46,6 +46,50 @@ func TestTargetMounts_BtrfsNoRoot(t *testing.T) { } } +// TestTargetMounts_LvmVolumes mounts non-root LVM volumes (e.g. a /home LV) at +// their mountpoints so staging into /mnt/home/ isn't shadowed once the +// home LV mounts at boot. Single-LV layouts (no extra volumes) add no extra +// mounts. +func TestTargetMounts_LvmVolumes(t *testing.T) { + cfg := &config.Config{} + cfg.Disks.Layout = "lvm" + cfg.Disks.LVM = &config.LVMLayout{ + VG: "vg0", + Volumes: []config.LVMVolume{ + {Name: "root", Mountpoint: "/", Filesystem: "xfs"}, + {Name: "home", Mountpoint: "/home", Filesystem: "ext4"}, + }, + } + + got, err := targetMounts(cfg, "/dev/vg0/root", "/dev/vda1") + if err != nil { + t.Fatalf("targetMounts: %v", err) + } + want := []mount{ + {dev: "/dev/vg0/root", target: "/mnt"}, + {dev: "/dev/vda1", target: "/mnt/boot"}, + {dev: "/dev/vg0/home", target: "/mnt/home"}, + } + assertMounts(t, want, got) +} + +// TestTargetMounts_LvmSingle adds no extra mounts when there is one root LV. +func TestTargetMounts_LvmSingle(t *testing.T) { + cfg := &config.Config{} + cfg.Disks.Layout = "lvm" + cfg.Disks.LVM = &config.LVMLayout{VG: "vg0", LV: "root", Filesystem: "xfs"} + + got, err := targetMounts(cfg, "/dev/vg0/root", "/dev/vda1") + if err != nil { + t.Fatalf("targetMounts: %v", err) + } + want := []mount{ + {dev: "/dev/vg0/root", target: "/mnt"}, + {dev: "/dev/vda1", target: "/mnt/boot"}, + } + assertMounts(t, want, got) +} + // TestTargetMounts_Plain leaves the non-btrfs path unchanged: bare root at /mnt, // ESP at /mnt/boot, no subvol options. func TestTargetMounts_Plain(t *testing.T) { diff --git a/test/e2e/vm/configs/btrfs-basic.yaml b/test/e2e/vm/configs/btrfs-basic.yaml index 93947df..f9c79f1 100644 --- a/test/e2e/vm/configs/btrfs-basic.yaml +++ b/test/e2e/vm/configs/btrfs-basic.yaml @@ -26,11 +26,9 @@ disks: btrfs: device: /dev/vda compress: zstd - # Single root subvolume: /home lives inside @. A SEPARATE @home subvolume - # currently breaks Phase B — archwright stages the binary/config into the - # wrong subvolume (postInstall remounts the bare partition, not @ + @home), - # so they're shadowed once /home mounts @home at boot. Flagged for an - # archwright-side fix; see test/e2e/vm/README.md. + # Single root subvolume: /home lives inside @. archwright installs the system + # inside @ and postInstall mounts subvol=@ (plus any non-root subvolume such + # as @home at its mountpoint), so a separate @home would also stage correctly. subvolumes: - { name: "@", mountpoint: / } diff --git a/test/e2e/vm/matrix/btrfs.py b/test/e2e/vm/matrix/btrfs.py index ada49b4..7035d34 100644 --- a/test/e2e/vm/matrix/btrfs.py +++ b/test/e2e/vm/matrix/btrfs.py @@ -9,13 +9,12 @@ "user": "e2e", "phase_b": True, "esp_part": "/dev/vda1", - # archinstall installs the system into the btrfs TOP-LEVEL subvolume (the - # default, subvolid 5) — the configured @ is created but not used as root — - # so we mount the bare partition (its default subvol), matching archwright's - # own postInstall/rootDevice. (That the named subvolumes aren't the root - # mount is a separate btrfs finding noted in README.md.) + # The system is installed inside the @ subvolume (archwright renders the + # btrfs root partition with a null mountpoint so @ drives /), so the + # scaffold injection must mount @, matching archwright's own postInstall. + # Mounting the bare partition would expose only the empty top-level subvol. "root_mount": [ - "mount /dev/vda2 /mnt", + "mount -o subvol=@ /dev/vda2 /mnt", ], "grub_serial": True, "expect": { @@ -31,8 +30,9 @@ "user": "e2e", "phase_b": True, "esp_part": "/dev/vda1", + # System installed inside @ (see btrfs-basic); mount @ for scaffold inject. "root_mount": [ - "mount /dev/vda2 /mnt", + "mount -o subvol=@ /dev/vda2 /mnt", ], "grub_serial": True, "expect": { diff --git a/test/e2e/vm/matrix/lvm_variants.py b/test/e2e/vm/matrix/lvm_variants.py index eb24334..5d65895 100644 --- a/test/e2e/vm/matrix/lvm_variants.py +++ b/test/e2e/vm/matrix/lvm_variants.py @@ -28,9 +28,13 @@ "user": "e2e", "phase_b": True, "esp_part": "/dev/vda1", + # /home is a separate LV: mount it too so the scaffold injected into + # /mnt/home/e2e isn't shadowed when the home LV mounts at boot (matches + # archwright's postInstall, which now remounts non-root LVM volumes). "root_mount": [ "vgchange -ay vg0", "mount /dev/vg0/root /mnt", + "mount /dev/vg0/home /mnt/home", ], "grub_serial": True, "expect": { From 4546435a4f3fc4dce8c9bf14346c1a04fc9a8677 Mon Sep 17 00:00:00 2001 From: Adam Hall Date: Tue, 23 Jun 2026 21:11:12 +0930 Subject: [PATCH 8/9] docs(readme): replace duplicated VM testing section with CONTRIBUTING ref The "Testing in a VM" section duplicated CONTRIBUTING.md's richer testing pyramid. Trim the body to a short pointer; keep the heading so existing #testing-in-a-vm anchors stay valid. Co-Authored-By: Claude Opus 4.8 --- README.md | 33 ++++++--------------------------- 1 file changed, 6 insertions(+), 27 deletions(-) diff --git a/README.md b/README.md index caa7e19..86fca4e 100644 --- a/README.md +++ b/README.md @@ -661,34 +661,13 @@ disks.lvm.pvs must have at least 1 item(s) ## Testing in a VM **Recommended before real hardware.** Phase A repartitions disks, so smoke-test the whole flow -in QEMU with three virtual disks. This is also where you **validate the generated archinstall -JSON** against the archinstall version on the ISO. +in QEMU before trusting it on hardware — this is also where you **validate the generated +archinstall JSON** against the archinstall version on the ISO. -```sh -# Three disks: 100G (disk 1: ESP+swap+PV) + 2× 50G (whole-disk PVs) -qemu-img create -f qcow2 disk1.qcow2 100G -qemu-img create -f qcow2 disk2.qcow2 50G -qemu-img create -f qcow2 disk3.qcow2 50G - -qemu-system-x86_64 \ - -enable-kvm -m 8G -smp 4 \ - -cpu host \ # required: CachyOS repo setup probes CPU features - -bios /usr/share/edk2/x64/OVMF.4m.fd \ # UEFI firmware (edk2-ovmf) - -drive file=disk1.qcow2,if=virtio \ - -drive file=disk2.qcow2,if=virtio \ - -drive file=disk3.qcow2,if=virtio \ - -cdrom archlinux-x86_64.iso \ - -boot d -``` - -Inside the VM the disks appear as `/dev/vda`, `/dev/vdb`, `/dev/vdc` — set `config.yaml` -accordingly (`esp.device: /dev/vda`, PVs `/dev/vda3`, `/dev/vdb`, `/dev/vdc`). Use -`./archwright install --yes` to skip the interactive prompts during automated runs. - -`install --dry-run` prints the rendered config without running anything; a real `install` writes -`/tmp/archinstall-config.json` + `/tmp/archinstall-creds.json` and invokes `archinstall ---silent`. If archinstall rejects the config after a version bump, diff its schema and update -`internal/archinstall` + the pinned `Version`. +The harnesses and the full test pyramid (fast checks → loopback integration → interactive VM → +automated VM e2e) live in [CONTRIBUTING.md](CONTRIBUTING.md#testing). In short, `task vm-fresh` +boots the live ISO with clean disks and shares the repo over 9p, and `task vm-e2e` runs the +headless install → bootstrap → validate flow unattended. ## For contributors From 3e84adbc0e4001c86abbf7099c56bffc010fb8a7 Mon Sep 17 00:00:00 2001 From: Adam Hall Date: Tue, 23 Jun 2026 21:14:02 +0930 Subject: [PATCH 9/9] test(flatpak): match flatpak as command, not argument, in sudo guard MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The "no privileged flatpak" assertion flagged any sudo line containing the word "flatpak", which false-positived on `sudo pacman -S … flatpak` — the ensureTool package install that is only recorded when flatpak is absent (true on CI runners, not on a dev box with flatpak installed). Match the flatpak binary as the command (after stripping a sudo prefix) so the legitimate privileged package install is ignored while a real `sudo flatpak` op is still caught. Co-Authored-By: Claude Opus 4.8 --- internal/stages/flatpak_test.go | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/internal/stages/flatpak_test.go b/internal/stages/flatpak_test.go index 92566d4..067cf64 100644 --- a/internal/stages/flatpak_test.go +++ b/internal/stages/flatpak_test.go @@ -39,11 +39,15 @@ flatpaks: t.Errorf("firefox should install from flathub-beta, not flathub; plan:\n%s", joined) } - // Every flatpak invocation must stay unprivileged (Cmd, never Root/sudo): - // a system-scope flatpak op as the user drops into a polkit Password: prompt - // and bootstrap hangs forever in a headless/TTY session. + // Every invocation of the flatpak *binary* must stay unprivileged (Cmd, never + // Root/sudo): a system-scope flatpak op as the user drops into a polkit + // Password: prompt and bootstrap hangs forever in a headless/TTY session. We + // match flatpak as the command (after any sudo prefix), not as an argument — + // `sudo pacman -S … flatpak` (the ensureTool package install, only recorded + // when flatpak is absent) is legitimately privileged and must not trip this. for _, line := range plan { - if strings.HasPrefix(line, "sudo ") && strings.Contains(line, "flatpak") { + cmd := strings.TrimPrefix(line, "sudo ") + if cmd != line && strings.HasPrefix(cmd, "flatpak ") { t.Errorf("flatpak must run unprivileged (no sudo) to avoid a polkit hang; got: %q", line) } }