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)" ] } } 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 diff --git a/internal/archinstall/archinstall.go b/internal/archinstall/archinstall.go index d74e0fa..c40afe3 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 } } @@ -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", @@ -643,16 +648,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/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/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" ], 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" +} diff --git a/internal/stages/archinstall.go b/internal/stages/archinstall.go index 7a4759b..adc055b 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,105 @@ 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, 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) { + 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 { + 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"}, + } + sortByDepth(others, func(s config.Subvol) string { return s.Mountpoint }) + for _, sv := range others { + mounts = append(mounts, mount{ + dev: rootDev, + target: "/mnt" + sv.Mountpoint, + opts: []string{"subvol=" + sv.Name}, + }) + } + 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/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..067cf64 100644 --- a/internal/stages/flatpak_test.go +++ b/internal/stages/flatpak_test.go @@ -21,21 +21,36 @@ 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 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 { + 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) + } + } } func TestFlatpak_NoRemotesNoImplicitFlathub(t *testing.T) { 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 +} 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", ) } diff --git a/internal/stages/target_mounts_test.go b/internal/stages/target_mounts_test.go new file mode 100644 index 0000000..72d9a61 --- /dev/null +++ b/internal/stages/target_mounts_test.go @@ -0,0 +1,129 @@ +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_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) { + 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]) + } + } + } +} 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": {