diff --git a/internal/archinstall/archinstall.go b/internal/archinstall/archinstall.go index 0c21ece..d74e0fa 100644 --- a/internal/archinstall/archinstall.go +++ b/internal/archinstall/archinstall.go @@ -141,13 +141,16 @@ type LvmConfiguration struct { // DiskEncryption mirrors archinstall's DiskEncryption.json(). encryption_type is // "luks" (encrypt the single root partition) or "lvm_on_luks" (encrypt the PV // partitions under LVM). partitions holds the encrypted partition obj_ids (the -// root partition for luks, the PV partitions for lvm_on_luks); lvm_volumes holds -// LV obj_ids for the luks_on_lvm variant. The password is supplied separately as -// the top-level encryption_password field. +// root partition for luks, the PV partitions for lvm_on_luks). The password is +// supplied separately as the top-level encryption_password field. type DiskEncryption struct { EncryptionType string `json:"encryption_type"` // "luks"/"lvm_on_luks" Partitions []string `json:"partitions"` - LvmVolumes []string `json:"lvm_volumes"` + // LvmVolumes carries LV obj_ids for a per-LV (luks-on-lvm) encryption + // topology. That topology is reserved/unused for now — no encryption_type + // currently populates it — so it always renders as an empty list. Kept on + // the struct so the JSON shape matches archinstall's DiskEncryption.json(). + LvmVolumes []string `json:"lvm_volumes"` } // DiskConfig mirrors DiskLayoutConfiguration.json(). disk_encryption is nested @@ -333,14 +336,16 @@ func Build(cfg *config.Config, geom Geometry, password string) (*Config, *Creds, // and lvm config. For lvm_on_luks the encrypted partitions are exactly the VG's // LvmPvs; for luks it is the single partition mounted at "/". archinstall rejects // LVM encryption with more than two PV partitions (device_handler.py) — this is -// guarded here as well as in config validation. +// guarded here as well as in config validation. Only "luks" and "lvm_on_luks" +// are accepted (config validation restricts the set); a per-LV luks-on-lvm +// topology is not implemented. // -// VM-validation-pending: the encryption_type values, the partitions/lvm_volumes -// obj_id wiring, and the 2-PV limit are reverse-engineered from archinstall source -// and must be confirmed against a real archinstall run in a VM. +// VM-validation-pending: the encryption_type values, the partitions obj_id +// wiring, and the 2-PV limit are reverse-engineered from archinstall source and +// must be confirmed against a real archinstall run in a VM. func buildEncryption(encType string, devices []Device, lvm *LvmConfiguration) (*DiskEncryption, error) { switch encType { - case "lvm_on_luks", "luks_on_lvm": + case "lvm_on_luks": if lvm == nil || len(lvm.VolGroups) == 0 { return nil, fmt.Errorf("%s encryption requires an lvm layout", encType) } @@ -428,7 +433,7 @@ func (b *lvmBuilder) build(geom Geometry) ([]Device, *LvmConfiguration, error) { } } if disk1PV == "" { - return nil, nil, fmt.Errorf("no LVM PV found on disk 1 (%s); expected a partition like %s", disk1, partDev(disk1, 2)) + return nil, nil, fmt.Errorf("no LVM PV found on disk 1 (%s); expected a partition like %s", disk1, PartDev(disk1, 2)) } // Disk 1: ESP + PV partition (no swap partition). Sizes computed from geometry. @@ -575,22 +580,51 @@ type plainBuilder struct { } func (b *plainBuilder) build(geom Geometry) ([]Device, *LvmConfiguration, error) { - disk1 := b.esp.Device - espBytes := b.espBytes + rootFs := b.plain.Filesystem + devices, err := singleDiskRoot(b.esp, b.swap, b.espBytes, geom, rootSpec{ + fsType: rootFs, + mountOptions: []string{}, + btrfs: []any{}, + }) + if err != nil { + return nil, nil, err + } + return devices, nil, nil +} + +// rootSpec describes the per-layout root partition that follows the shared ESP + +// optional swap prefix on disk 1. Only these three fields differ between the +// plain and btrfs layouts; everything else (Status/Type/Start/Size/Mountpoint at +// "/"/Flags) is identical and supplied by singleDiskRoot. +type rootSpec struct { + fsType string + mountOptions []string + btrfs []any +} + +// singleDiskRoot builds the disk-1 layout shared by the plain and btrfs builders: +// the ESP partition, an optional linux-swap partition (when swap.type is +// "partition", sized from swap.size), and a root partition consuming the rest of +// disk 1. The root partition's fs_type, mount options and btrfs subvolume list +// come from spec; everything else is fixed. The newObjID() call order is ESP, +// swap (if present), then root — matching both original builders so golden renders +// stay byte-identical. +func singleDiskRoot(esp config.ESPConfig, swap config.SwapConfig, espBytes uint64, geom Geometry, spec rootSpec) ([]Device, error) { + disk1 := esp.Device disk1Total, ok := geom[disk1] if !ok || disk1Total == 0 { - return nil, nil, fmt.Errorf("no geometry for disk 1 (%s)", disk1) + return nil, fmt.Errorf("no geometry for disk 1 (%s)", disk1) } parts := []Partition{espPartition(espBytes)} offset := startOffset + espBytes // Optional swap partition (plain/btrfs only). Sized from swap.size. - if b.swap.EffectiveType() == "partition" { - swapBytes, err := parseSize(b.swap.Size) + if swap.EffectiveType() == "partition" { + swapBytes, err := parseSize(swap.Size) if err != nil { - return nil, nil, fmt.Errorf("swap size: %w", err) + return nil, fmt.Errorf("swap size: %w", err) } swapFs := "linux-swap" parts = append(parts, Partition{ @@ -603,20 +637,20 @@ func (b *plainBuilder) build(geom Geometry) ([]Device, *LvmConfiguration, error) used := offset + endReserve if disk1Total <= used { - return nil, nil, fmt.Errorf("disk 1 (%s, %d bytes) too small for ESP+swap (%d bytes)", disk1, disk1Total, used) + return nil, fmt.Errorf("disk 1 (%s, %d bytes) too small for ESP+swap (%d bytes)", disk1, disk1Total, used) } rootBytes := roundDownMiB(disk1Total - used) root := "/" - rootFs := b.plain.Filesystem + rootFs := spec.fsType parts = append(parts, Partition{ ObjID: newObjID(), Status: "create", Type: "primary", Start: bytes(offset), Size: bytes(rootBytes), FsType: &rootFs, Mountpoint: &root, - MountOptions: []string{}, Flags: []string{}, Btrfs: []any{}, + MountOptions: spec.mountOptions, Flags: []string{}, Btrfs: spec.btrfs, }) - return []Device{{Device: disk1, Wipe: true, Partitions: parts}}, nil, nil + return []Device{{Device: disk1, Wipe: true, Partitions: parts}}, nil } // --- btrfs layout ----------------------------------------------------------- @@ -642,39 +676,6 @@ type btrfsBuilder struct { } func (b *btrfsBuilder) build(geom Geometry) ([]Device, *LvmConfiguration, error) { - disk1 := b.esp.Device - espBytes := b.espBytes - - disk1Total, ok := geom[disk1] - if !ok || disk1Total == 0 { - return nil, nil, fmt.Errorf("no geometry for disk 1 (%s)", disk1) - } - - parts := []Partition{espPartition(espBytes)} - offset := startOffset + espBytes - - if b.swap.EffectiveType() == "partition" { - swapBytes, err := parseSize(b.swap.Size) - if err != nil { - return nil, nil, fmt.Errorf("swap size: %w", err) - } - swapFs := "linux-swap" - parts = append(parts, Partition{ - ObjID: newObjID(), Status: "create", Type: "primary", - Start: bytes(offset), Size: bytes(swapBytes), - FsType: &swapFs, MountOptions: []string{}, Flags: []string{"swap"}, Btrfs: []any{}, - }) - offset += swapBytes - } - - used := offset + endReserve - if disk1Total <= used { - return nil, nil, fmt.Errorf("disk 1 (%s, %d bytes) too small for ESP+swap (%d bytes)", disk1, disk1Total, used) - } - rootBytes := roundDownMiB(disk1Total - used) - - root := "/" - btrfsFs := "btrfs" mountOpts := []string{} if b.btrfs.Compress != "" { mountOpts = append(mountOpts, "compress="+b.btrfs.Compress) @@ -687,14 +688,15 @@ func (b *btrfsBuilder) build(geom Geometry) ([]Device, *LvmConfiguration, error) subvols = append(subvols, BtrfsSubvolume{Name: sv.Name, Mountpoint: &mp}) } - parts = append(parts, Partition{ - ObjID: newObjID(), Status: "create", Type: "primary", - Start: bytes(offset), Size: bytes(rootBytes), - FsType: &btrfsFs, Mountpoint: &root, - MountOptions: mountOpts, Flags: []string{}, Btrfs: subvols, + devices, err := singleDiskRoot(b.esp, b.swap, b.espBytes, geom, rootSpec{ + fsType: "btrfs", + mountOptions: mountOpts, + btrfs: subvols, }) - - return []Device{{Device: disk1, Wipe: true, Partitions: parts}}, nil, nil + if err != nil { + return nil, nil, err + } + return devices, nil, nil } // --- helpers ---------------------------------------------------------------- @@ -728,8 +730,10 @@ func splitLocale(locale string) (lang, enc string) { return locale, "UTF-8" } -// partDev mirrors stages.partDev for the disk-1 PV error hint. -func partDev(dev string, n int) string { +// PartDev returns the kernel partition device for a base device and number: +// /dev/sda -> /dev/sda1, but /dev/nvme0n1 -> /dev/nvme0n1p1. It is the single +// shared definition; the stages package calls it rather than duplicating it. +func PartDev(dev string, n int) string { if len(dev) > 0 { if last := dev[len(dev)-1]; last >= '0' && last <= '9' { return fmt.Sprintf("%sp%d", dev, n) diff --git a/internal/config/config.go b/internal/config/config.go index e43ddb2..20cccb1 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -229,10 +229,16 @@ type Subvol struct { Mountpoint string `yaml:"mountpoint" validate:"required"` } -// Encryption enables LUKS. Type: "luks" (encrypt the single root partition, -// for plain/btrfs) or "lvm_on_luks" (encrypt the PV partitions under LVM). +// Encryption enables LUKS. Type is one of: +// - "luks": encrypt the single root partition (for the plain/btrfs layouts). +// - "lvm_on_luks": encrypt the PV partitions under LVM (for the lvm layout). +// +// The "luks_on_lvm" topology (encrypt individual LVs on top of an unencrypted +// VG) is intentionally NOT accepted: it is unimplemented in the archinstall +// renderer, so allowing it would silently produce the wrong (lvm_on_luks) +// layout. Rejecting it here surfaces a clear validation error instead. type Encryption struct { - Type string `yaml:"type" validate:"required,oneof=luks lvm_on_luks luks_on_lvm"` + Type string `yaml:"type" validate:"required,oneof=luks lvm_on_luks"` } // SetupConfig drives the Phase B 85-setup stage, which runs after chezmoi has @@ -269,8 +275,12 @@ type Clone struct { // Script and Dir have a leading `~` expanded to the user's home at run time. // Script existence is NOT checked at validate time: a hook script may be // produced by an earlier hook or stage in the same run. +// +// Name is required: it identifies the hook for layered-config merge-by-name (a +// nameless element would silently flip the whole hooks slice from merge to +// wholesale replace — see internal/configsrc/merge.go) and for diagnostics. type Hook struct { - Name string `yaml:"name"` + Name string `yaml:"name" validate:"required"` At string `yaml:"at" validate:"required,hookpoint"` Run string `yaml:"run" validate:"required_without=Script"` Script string `yaml:"script" validate:"omitempty"` @@ -509,7 +519,7 @@ func (c *Config) encryptionErrors() []error { } layout := d.EffectiveLayout() switch d.Encryption.Type { - case "lvm_on_luks", "luks_on_lvm": + case "lvm_on_luks": if layout != "lvm" { errs = append(errs, fmt.Errorf("disks.encryption.type %s requires the lvm layout", d.Encryption.Type)) } diff --git a/internal/config/encryption_luksonlvm_test.go b/internal/config/encryption_luksonlvm_test.go new file mode 100644 index 0000000..13ed6a2 --- /dev/null +++ b/internal/config/encryption_luksonlvm_test.go @@ -0,0 +1,48 @@ +package config + +import ( + "strings" + "testing" +) + +// TestEncryptionLuksOnLvmRejected pins that disks.encryption.type: luks_on_lvm is +// no longer an accepted value. The archinstall renderer never implemented the +// per-LV luks-on-lvm topology — it collapsed it into lvm_on_luks, encrypting the +// PVs (the wrong layout) silently. Removing it from the oneof set makes Validate() +// surface a clear "must be one of" error instead. +func TestEncryptionLuksOnLvmRejected(t *testing.T) { + const y = ` +system: + hostname: arch + 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: + type: swapfile + size: 4GiB + layout: lvm + lvm: {vg: vg0, lv: root, filesystem: ext4, pvs: [/dev/nvme0n1p2]} + encryption: {type: luks_on_lvm} +` + err := validateYAML(t, y) + if err == nil { + t.Fatalf("want error for luks_on_lvm, got nil") + } + msg := err.Error() + if !strings.Contains(msg, "must be one of") { + t.Fatalf("error %q does not contain %q", msg, "must be one of") + } + // The accepted set must be named so the user knows the valid choices. + if !strings.Contains(msg, "luks") || !strings.Contains(msg, "lvm_on_luks") { + t.Fatalf("error %q should list the accepted types (luks, lvm_on_luks)", msg) + } +} diff --git a/internal/config/hook_name_required_test.go b/internal/config/hook_name_required_test.go new file mode 100644 index 0000000..46fe1fe --- /dev/null +++ b/internal/config/hook_name_required_test.go @@ -0,0 +1,82 @@ +package config + +import ( + "strings" + "testing" +) + +// TestHookNameRequired pins that every hook must carry a name. A nameless hook +// would silently flip the layered-config merge of the hooks list from +// merge-by-name to wholesale replace (see internal/configsrc/merge.go), dropping +// the base layer's hooks. Requiring a name fails validation instead, and the +// failure is reported against the indexed YAML path hooks[N].name. +func TestHookNameRequired(t *testing.T) { + const y = ` +system: + hostname: arch + 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: + type: swapfile + size: 4GiB + layout: lvm + lvm: {vg: vg0, lv: root, filesystem: ext4, pvs: [/dev/nvme0n1p2]} +hooks: + - at: post-install + run: echo hi +` + err := validateYAML(t, y) + if err == nil { + t.Fatalf("want error for nameless hook, got nil") + } + msg := err.Error() + if !strings.Contains(msg, "hooks[0].name") { + t.Fatalf("error %q should reference hooks[0].name", msg) + } + if !strings.Contains(msg, "is required") { + t.Fatalf("error %q should say the field is required", msg) + } +} + +// TestHookWithNameValid confirms a named hook still validates cleanly, so the new +// requirement doesn't break correctly-written hooks. +func TestHookWithNameValid(t *testing.T) { + const y = ` +system: + hostname: arch + 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: + type: swapfile + size: 4GiB + layout: lvm + lvm: {vg: vg0, lv: root, filesystem: ext4, pvs: [/dev/nvme0n1p2]} +hooks: + - name: greet + at: post-install + run: echo hi +` + if err := validateYAML(t, y); err != nil { + t.Fatalf("want valid, got error: %v", err) + } +} diff --git a/internal/config/hooks_config_test.go b/internal/config/hooks_config_test.go index b305642..233f0bd 100644 --- a/internal/config/hooks_config_test.go +++ b/internal/config/hooks_config_test.go @@ -12,19 +12,19 @@ func validateHook(h Hook) error { } func TestHook_ValidGlobalPoint(t *testing.T) { - if err := validateHook(Hook{At: "post-install", Run: "echo hi"}); err != nil { + if err := validateHook(Hook{Name: "h", At: "post-install", Run: "echo hi"}); err != nil { t.Errorf("valid hook should pass: %v", err) } } func TestHook_ValidPerStagePoint(t *testing.T) { - if err := validateHook(Hook{At: "before:packages", Run: "echo hi"}); err != nil { + if err := validateHook(Hook{Name: "h", At: "before:packages", Run: "echo hi"}); err != nil { t.Errorf("before:packages is well-formed and should pass hookpoint: %v", err) } } func TestHook_BadAtFailsHookpoint(t *testing.T) { - err := validateHook(Hook{At: "midway", Run: "echo hi"}) + err := validateHook(Hook{Name: "h", At: "midway", Run: "echo hi"}) if err == nil { t.Fatal("bad at should fail hookpoint") } @@ -34,7 +34,7 @@ func TestHook_BadAtFailsHookpoint(t *testing.T) { } func TestHook_EmptyStageTokenFailsHookpoint(t *testing.T) { - err := validateHook(Hook{At: "before:", Run: "echo hi"}) + err := validateHook(Hook{Name: "h", At: "before:", Run: "echo hi"}) if err == nil { t.Fatal("before: with empty stage should fail hookpoint") } @@ -44,7 +44,7 @@ func TestHook_EmptyStageTokenFailsHookpoint(t *testing.T) { } func TestHook_NeitherRunNorScriptFails(t *testing.T) { - err := validateHook(Hook{At: "post-install"}) + err := validateHook(Hook{Name: "h", At: "post-install"}) if err == nil { t.Fatal("hook with neither run nor script should fail required_without") } diff --git a/internal/configsrc/merge.go b/internal/configsrc/merge.go index ae8bc4b..0360062 100644 --- a/internal/configsrc/merge.go +++ b/internal/configsrc/merge.go @@ -108,6 +108,14 @@ func mergeMaps(base, over map[string]any) (map[string]any, error) { // mergeSlices applies the slice strategy: union+dedup for string slices, // key-merge-by-name for slices of maps that carry a "name", replace otherwise. +// +// Sharp edge: the merge-by-name path applies ONLY when every element of BOTH +// slices carries a non-empty "name" (see structuredByName). If even one element +// in either layer lacks a name, the whole slice is replaced wholesale instead of +// merged — so a single nameless element in an overriding layer silently drops the +// base's elements. This is why structured-list schema fields whose lists are +// meant to merge across layers (e.g. config Hooks) must make "name" required, so +// a nameless element fails validation rather than degrading the merge. func mergeSlices(base, over []any) (any, error) { if allStrings(base) && allStrings(over) { return unionStrings(base, over), nil @@ -146,7 +154,10 @@ func unionStrings(base, over []any) []any { } // structuredByName reports whether s is a non-empty slice whose every element is -// a map carrying a non-empty string "name" key. +// a map carrying a non-empty string "name" key. It is the gate for merge-by-name: +// merge-by-name applies only when this holds for BOTH the base and the over +// slice. A single nameless element makes this return false, which (in mergeSlices) +// downgrades the merge to a wholesale replace — see the mergeSlices doc comment. func structuredByName(s []any) bool { if len(s) == 0 { return false diff --git a/internal/run/run.go b/internal/run/run.go index 2d62ce1..fb63d4e 100644 --- a/internal/run/run.go +++ b/internal/run/run.go @@ -6,9 +6,7 @@ package run import ( - "bytes" "fmt" - "io" "os" "os/exec" "strings" @@ -20,10 +18,6 @@ type Runner struct { DryRun bool // print/record commands instead of executing them Sudo bool // prefix privileged commands with sudo (Phase B as user); false in Phase A (already root) - // Out, when non-nil, receives BOTH stdout and stderr of every executed - // command. It is the hook for a TUI viewport that captures streamed output; - // when nil, output streams to os.Stdout/os.Stderr exactly as before. - Out io.Writer // Env, when non-empty, is layered (key=value) on top of the inherited process // environment for every executed command. Dir, when set, is the working dir. Env map[string]string @@ -36,15 +30,6 @@ type Runner struct { func (r *Runner) record(line string) { r.Plan = append(r.Plan, line) } -// outWriter returns r.Out when set, else the given default stream, so a nil Out -// preserves the original os.Stdout/os.Stderr wiring exactly. -func (r *Runner) outWriter(def io.Writer) io.Writer { - if r.Out != nil { - return r.Out - } - return def -} - // prepare applies the Runner's Env and Dir (if any) to a command before it runs. func (r *Runner) prepare(cmd *exec.Cmd) { if len(r.Env) > 0 { @@ -70,7 +55,7 @@ func (r *Runner) Cmd(name string, args ...string) error { return nil } cmd := exec.Command(name, args...) - cmd.Stdout, cmd.Stderr, cmd.Stdin = r.outWriter(os.Stdout), r.outWriter(os.Stderr), os.Stdin + cmd.Stdout, cmd.Stderr, cmd.Stdin = os.Stdout, os.Stderr, os.Stdin r.prepare(cmd) if err := cmd.Run(); err != nil { return fmt.Errorf("%s: %w", name, err) @@ -78,28 +63,6 @@ func (r *Runner) Cmd(name string, args ...string) error { return nil } -// Capture runs name+args and returns the command's stdout as a string, recording -// it in .Plan exactly like Cmd. stderr still streams (to Out or os.Stderr). In -// dry-run it records and returns "" with a nil error. Use this for the rare -// state-querying command whose output a stage needs, rather than dropping to -// os/exec directly (which would bypass dry-run and the recorded plan). -func (r *Runner) Capture(name string, args ...string) (string, error) { - line := strings.TrimSpace(name + " " + strings.Join(args, " ")) - r.record(line) - ui.Step("%s", line) - if r.DryRun { - return "", nil - } - cmd := exec.Command(name, args...) - var out bytes.Buffer - cmd.Stdout, cmd.Stderr, cmd.Stdin = &out, r.outWriter(os.Stderr), os.Stdin - r.prepare(cmd) - if err := cmd.Run(); err != nil { - return out.String(), fmt.Errorf("%s: %w", name, err) - } - return out.String(), nil -} - // Root runs a command with root privileges: directly when already root (Phase A, // live ISO) or via sudo otherwise (Phase B, as the user). func (r *Runner) Root(name string, args ...string) error { @@ -109,12 +72,44 @@ func (r *Runner) Root(name string, args ...string) error { return r.Cmd(name, args...) } +// RootShell runs a shell script with root privileges through `bash -c`: via +// `sudo bash -c