From fdfbc8bf82790ce90d84c2c6ef436404c98c1277 Mon Sep 17 00:00:00 2001 From: indaco Date: Mon, 10 Aug 2026 18:16:34 +0200 Subject: [PATCH] fix(post-install): finish the install for formulas that clean up and glob --- ...-install-steps-keg-templates-and-remove.sh | 18 +- src/core/post_install_steps.zig | 251 +++++++++++++++++- 2 files changed, 261 insertions(+), 8 deletions(-) diff --git a/scripts/regressions/post-install-steps-keg-templates-and-remove.sh b/scripts/regressions/post-install-steps-keg-templates-and-remove.sh index 607f66d5..575cbfa6 100755 --- a/scripts/regressions/post-install-steps-keg-templates-and-remove.sh +++ b/scripts/regressions/post-install-steps-keg-templates-and-remove.sh @@ -56,6 +56,14 @@ if ! grep -q 'readLinkAbsolute' "$STEPS"; then exit 1 fi +# 3b. The recursive form retires a subtree the formula owns. Its boundary is +# `sharedPrefixDir`: without it, confinement alone would let formula data +# delete `/lib` and every other package's files inside it. +if ! grep -q 'fn sharedPrefixDir' "$STEPS"; then + echo "FAIL: recursive remove no longer refuses shared top-level prefix directories" >&2 + exit 1 +fi + # 4. Behavioural guards. If a test block is deleted the run below goes green # vacuously, so assert each is present first. TEMPLATE_TEST="execute links keg-relative template paths into the prefix" @@ -65,8 +73,13 @@ KEEP_LINK_TEST="execute keeps a symlink whose target does not match the guard" # The pre-existing base-form path shares stepSymlink; judged too so a fix to # the template form cannot quietly break the form every other formula uses. BASE_FORM_TEST="execute runs the filesystem tier natively and leaves the log clean" +# The recursive form and its refusal. A first install must stay silent, and a +# shared prefix directory must survive even when the guard finds it. +UNMET_GUARD_TEST="execute skips a remove whose if_exists guard is unmet" +SHARED_DIR_TEST="execute refuses a recursive remove that would take a whole prefix dir" -for t in "$TEMPLATE_TEST" "$REMOVE_TEST" "$KEEP_FILE_TEST" "$KEEP_LINK_TEST" "$BASE_FORM_TEST"; do +for t in "$TEMPLATE_TEST" "$REMOVE_TEST" "$KEEP_FILE_TEST" "$KEEP_LINK_TEST" "$BASE_FORM_TEST" \ + "$UNMET_GUARD_TEST" "$SHARED_DIR_TEST"; do if ! grep -Rqs -- "$t" "$STEPS"; then echo "FAIL: guard test missing from post_install_steps.zig: $t" >&2 exit 1 @@ -80,7 +93,8 @@ fi # One run of the suite, judged per guard line: a pass ends in "OK". out=$("$ROOT/zig-out/test-bin/lib_tests" 2>&1 || true) -for t in "$TEMPLATE_TEST" "$REMOVE_TEST" "$KEEP_FILE_TEST" "$KEEP_LINK_TEST" "$BASE_FORM_TEST"; do +for t in "$TEMPLATE_TEST" "$REMOVE_TEST" "$KEEP_FILE_TEST" "$KEEP_LINK_TEST" "$BASE_FORM_TEST" \ + "$UNMET_GUARD_TEST" "$SHARED_DIR_TEST"; do line=$(printf '%s\n' "$out" | grep -F -- "$t" || true) if [[ -z "$line" ]]; then echo "FAIL: guard test did not run: $t" >&2 diff --git a/src/core/post_install_steps.zig b/src/core/post_install_steps.zig index 0648a189..c95365f1 100644 --- a/src/core/post_install_steps.zig +++ b/src/core/post_install_steps.zig @@ -517,12 +517,67 @@ fn stepSymlink(ctx: StepsCtx, obj: std.json.ObjectMap) bool { logUnsupported(ctx, "symlink with a relative source"); return false; } + if (getFlag(obj, "source_glob")) return symlinkGlob(ctx, obj, source, target); mkParent(ctx, target); if (getFlag(obj, "force")) std.Io.Dir.cwd().deleteFile(ctx.io, target) catch {}; std.Io.Dir.symLinkAbsolute(ctx.io, source, target, .{}) catch {}; return true; } +/// `source_glob`: the source's last component is a pattern and `target` is the +/// directory every match lands in, keeping its own name. Without this the +/// pattern would be symlinked verbatim and the files silently go unlinked. +fn symlinkGlob(ctx: StepsCtx, obj: std.json.ObjectMap, source: []const u8, target: []const u8) bool { + const dir_path = std.fs.path.dirname(source) orelse return false; + const pattern = std.fs.path.basename(source); + + std.Io.Dir.cwd().createDirPath(ctx.io, target) catch {}; + var dir = std.Io.Dir.openDirAbsolute(ctx.io, dir_path, .{ .iterate = true }) catch return true; + defer dir.close(ctx.io); + + const force = getFlag(obj, "force"); + var iter = dir.iterate(); + while (iter.next(ctx.io) catch null) |entry| { + if (!globMatch(ctx.allocator, pattern, entry.name)) continue; + const child = std.fs.path.join(ctx.allocator, &.{ dir_path, entry.name }) catch continue; + const link_path = std.fs.path.join(ctx.allocator, &.{ target, entry.name }) catch continue; + // Each match is its own write: a planted symlink must not redirect + // one of them out of the prefix. + if (!confined(ctx, link_path)) return false; + if (force) std.Io.Dir.cwd().deleteFile(ctx.io, link_path) catch {}; + std.Io.Dir.symLinkAbsolute(ctx.io, child, link_path, .{}) catch {}; + } + return true; +} + +/// Shell-style match limited to what formula globs use: `*` and `{a,b}`. +/// ponytail: no `?` or `[...]` — no formula in homebrew-core ships one. +fn globMatch(a: std.mem.Allocator, pattern: []const u8, name: []const u8) bool { + const open = std.mem.indexOfScalar(u8, pattern, '{') orelse return starMatch(pattern, name); + const close = std.mem.indexOfScalarPos(u8, pattern, open + 1, '}') orelse return starMatch(pattern, name); + var alts = std.mem.splitScalar(u8, pattern[open + 1 .. close], ','); + while (alts.next()) |alt| { + const expanded = std.fmt.allocPrint(a, "{s}{s}{s}", .{ + pattern[0..open], alt, pattern[close + 1 ..], + }) catch return false; + if (globMatch(a, expanded, name)) return true; + } + return false; +} + +fn starMatch(pattern: []const u8, name: []const u8) bool { + if (pattern.len == 0) return name.len == 0; + if (pattern[0] == '*') { + var i: usize = 0; + while (true) : (i += 1) { + if (starMatch(pattern[1..], name[i..])) return true; + if (i == name.len) return false; + } + } + if (name.len == 0 or pattern[0] != name[0]) return false; + return starMatch(pattern[1..], name[1..]); +} + /// `guards` gate a step on the target's state. `if_exists` is the only /// condition this executor evaluates; `unsupported` keeps a condition it /// cannot check distinguishable from one that genuinely did not hold, so a @@ -638,12 +693,17 @@ fn stepInreplace(ctx: StepsCtx, obj: std.json.ObjectMap) bool { /// elsewhere belongs to something else and must survive. `readLinkAbsolute` /// doubles as the type check, and the link is unlinked rather than followed. fn stepRemove(ctx: StepsCtx, obj: std.json.ObjectMap) bool { - const needle = getString(obj, "symlink_target_contains") orelse { - // Without the guard this would be an unconditional recursive delete - // driven by formula data — not something to infer. - logUnsupported(ctx, "remove without symlink_target_contains"); - return false; - }; + // An unmet guard is the step declining to run, not a failure to report: + // a first install has no earlier payload to retire. + switch (evalGuards(ctx, obj)) { + .met => {}, + .unmet => return true, + .unsupported => { + logUnsupported(ctx, "remove with an unevaluable guard"); + return false; + }, + } + const paths_val = obj.get("paths") orelse { logUnsupported(ctx, "remove"); return false; @@ -653,6 +713,14 @@ fn stepRemove(ctx: StepsCtx, obj: std.json.ObjectMap) bool { return false; } + const needle = getString(obj, "symlink_target_contains") orelse { + if (getFlag(obj, "recursive")) return removeTrees(ctx, paths_val.array.items); + // Neither a symlink guard nor an explicit recursive intent leaves + // nothing to infer safely. + logUnsupported(ctx, "remove without symlink_target_contains"); + return false; + }; + for (paths_val.array.items) |item| { if (item != .object) continue; const path = resolveSpec(ctx, item.object, "paths") orelse continue; @@ -666,6 +734,32 @@ fn stepRemove(ctx: StepsCtx, obj: std.json.ObjectMap) bool { return true; } +/// Retire a subtree this formula owns, so the `copy` that follows starts +/// clean. Confinement keeps it inside the prefix; `sharedPrefixDir` keeps it +/// off the top-level directories every other package also writes into. +fn removeTrees(ctx: StepsCtx, items: []const std.json.Value) bool { + for (items) |item| { + if (item != .object) continue; + const path = resolveSpec(ctx, item.object, "paths") orelse continue; + if (!confined(ctx, path)) return false; + if (sharedPrefixDir(ctx, path)) { + logUnsupported(ctx, "recursive remove of a shared prefix directory"); + return false; + } + std.Io.Dir.cwd().deleteTree(ctx.io, path) catch {}; + } + return true; +} + +/// True for the prefix itself and its immediate children (`/lib`, +/// `/bin`, …) — shared ground, never one formula's to delete. +fn sharedPrefixDir(ctx: StepsCtx, path: []const u8) bool { + const trimmed = std.mem.trimEnd(u8, path, "/"); + if (std.mem.eql(u8, trimmed, ctx.prefix)) return true; + const parent = std.fs.path.dirname(trimmed) orelse return true; + return std.mem.eql(u8, parent, ctx.prefix); +} + fn stepLinkChildren(ctx: StepsCtx, obj: std.json.ObjectMap) bool { const source = resolvePathSpec(ctx, obj, "source") orelse return false; const target = resolvePathSpec(ctx, obj, "target") orelse return false; @@ -1619,6 +1713,151 @@ test "execute keeps a symlink whose target does not match the guard" { try std.Io.Dir.accessAbsolute(io, link, .{}); } +test "execute skips a remove whose if_exists guard is unmet" { + // A first install has nothing to clean up. Reporting the step as + // unsupported there downgrades the whole formula to the partial-skip + // envelope over work that was never going to happen. + var h = try TestHarness.init(); + defer h.deinit(); + + const json = try testFormulaJson(&h, + \\[{"type":"remove","recursive":true, + \\ "paths":[{"path":"{{HOMEBREW_PREFIX}}/lib/node_modules/npm"}], + \\ "guards":[{"condition":"if_exists","path":"{{HOMEBREW_PREFIX}}/lib/node_modules/npm"}]}] + ); + try testing.expect(execute(h.ctx(), json)); + try testing.expect(!h.flog.hasErrors()); +} + +test "execute removes a directory tree when the guard finds it" { + var h = try TestHarness.init(); + defer h.deinit(); + const io = h.io; + const a = h.arena.allocator(); + + const doomed = try std.fmt.allocPrint(a, "{s}/lib/node_modules/npm", .{h.prefix}); + try std.Io.Dir.cwd().createDirPath(io, doomed); + const child = try std.fmt.allocPrint(a, "{s}/index.js", .{doomed}); + (try std.Io.Dir.createFileAbsolute(io, child, .{})).close(io); + + const json = try testFormulaJson(&h, + \\[{"type":"remove","recursive":true, + \\ "paths":[{"path":"{{HOMEBREW_PREFIX}}/lib/node_modules/npm"}], + \\ "guards":[{"condition":"if_exists","path":"{{HOMEBREW_PREFIX}}/lib/node_modules/npm"}]}] + ); + try testing.expect(execute(h.ctx(), json)); + try testing.expect(!h.flog.hasErrors()); + try testing.expectError(error.FileNotFound, std.Io.Dir.accessAbsolute(io, doomed, .{})); +} + +test "execute refuses a recursive remove that would take a whole prefix dir" { + // Confinement alone would allow this: `/lib` is inside the + // boundary. Formula data may retire its own subtree, never a shared + // top-level directory holding every other package's files. + var h = try TestHarness.init(); + defer h.deinit(); + const io = h.io; + const a = h.arena.allocator(); + + const lib = try std.fmt.allocPrint(a, "{s}/lib", .{h.prefix}); + try std.Io.Dir.cwd().createDirPath(io, lib); + + const json = try testFormulaJson(&h, + \\[{"type":"remove","recursive":true, + \\ "paths":[{"path":"{{HOMEBREW_PREFIX}}/lib"}], + \\ "guards":[{"condition":"if_exists","path":"{{HOMEBREW_PREFIX}}/lib"}]}] + ); + try testing.expect(execute(h.ctx(), json)); + try testing.expect(h.flog.hasErrors()); + try std.Io.Dir.accessAbsolute(io, lib, .{}); +} + +test "a recursive remove unlinks a symlinked target instead of emptying it" { + // Confinement checks the parent chain, not the leaf, so a planted link can + // sit where the subtree is expected. Following it would delete a directory + // the formula never owned. + var h = try TestHarness.init(); + defer h.deinit(); + const io = h.io; + const a = h.arena.allocator(); + + const precious = try std.fmt.allocPrint(a, "{s}/etc/precious", .{h.prefix}); + try std.Io.Dir.cwd().createDirPath(io, precious); + const kept = try std.fmt.allocPrint(a, "{s}/keep.conf", .{precious}); + (try std.Io.Dir.createFileAbsolute(io, kept, .{})).close(io); + + const parent = try std.fmt.allocPrint(a, "{s}/lib/node_modules", .{h.prefix}); + try std.Io.Dir.cwd().createDirPath(io, parent); + const link = try std.fmt.allocPrint(a, "{s}/npm", .{parent}); + try std.Io.Dir.symLinkAbsolute(io, precious, link, .{}); + + const json = try testFormulaJson(&h, + \\[{"type":"remove","recursive":true, + \\ "paths":[{"path":"{{HOMEBREW_PREFIX}}/lib/node_modules/npm"}], + \\ "guards":[{"condition":"if_exists","path":"{{HOMEBREW_PREFIX}}/lib/node_modules/npm"}]}] + ); + try testing.expect(execute(h.ctx(), json)); + try std.Io.Dir.accessAbsolute(io, kept, .{}); +} + +test "execute still refuses a bare remove with neither guard nor recursive" { + var h = try TestHarness.init(); + defer h.deinit(); + + const json = try testFormulaJson(&h, + \\[{"type":"remove","paths":[{"path":"{{HOMEBREW_PREFIX}}/bin/glow-init"}]}] + ); + try testing.expect(execute(h.ctx(), json)); + try testing.expect(h.flog.hasErrors()); +} + +test "execute links every source_glob match into the target directory" { + var h = try TestHarness.init(); + defer h.deinit(); + const io = h.io; + const a = h.arena.allocator(); + + const man_src = try std.fmt.allocPrint(a, "{s}/share/glow/man1", .{h.keg}); + try std.Io.Dir.cwd().createDirPath(io, man_src); + for ([_][]const u8{ "npm.1", "npx.1", "package-json.1", "unrelated.1" }) |leaf| { + const p = try std.fmt.allocPrint(a, "{s}/{s}", .{ man_src, leaf }); + (try std.Io.Dir.createFileAbsolute(io, p, .{})).close(io); + } + const man_dst = try std.fmt.allocPrint(a, "{s}/share/man/man1", .{h.prefix}); + try std.Io.Dir.cwd().createDirPath(io, man_dst); + + const json = try testFormulaJson(&h, + \\[{"type":"symlink","force":true,"source_glob":true, + \\ "source":{"base":"prefix","path":"share/glow/man1/{npm,npx,package-}*"}, + \\ "target":{"path":"{{HOMEBREW_PREFIX}}/share/man/man1"}}] + ); + try testing.expect(execute(h.ctx(), json)); + try testing.expect(!h.flog.hasErrors()); + + for ([_][]const u8{ "npm.1", "npx.1", "package-json.1" }) |leaf| { + const p = try std.fmt.allocPrint(a, "{s}/{s}", .{ man_dst, leaf }); + try std.Io.Dir.accessAbsolute(io, p, .{}); + } + // A non-matching sibling must not be dragged along. + const stray = try std.fmt.allocPrint(a, "{s}/unrelated.1", .{man_dst}); + try testing.expectError(error.FileNotFound, std.Io.Dir.accessAbsolute(io, stray, .{})); +} + +test "globMatch honours alternation and wildcards without matching neighbours" { + var arena = std.heap.ArenaAllocator.init(testing.allocator); + defer arena.deinit(); + const a = arena.allocator(); + + try testing.expect(globMatch(a, "{npm,npx,package-}*", "npm.1")); + try testing.expect(globMatch(a, "{npm,npx,package-}*", "package-json.5")); + try testing.expect(!globMatch(a, "{npm,npx,package-}*", "unrelated.1")); + // A leading wildcard must not swallow the anchor that follows it. + try testing.expect(globMatch(a, "*.1", "npm.1")); + try testing.expect(!globMatch(a, "*.1", "npm.5")); + try testing.expect(globMatch(a, "literal", "literal")); + try testing.expect(!globMatch(a, "literal", "literals")); +} + test "execute runs the filesystem tier natively and leaves the log clean" { var h = try TestHarness.init(); defer h.deinit();