ReviewOS
Reload

also looking at this

pantry-pm/pantry

fix(install): keep .env-inferred services out of pantry.lock

#254
Merged glennmichael123 wants to merge fix/253-env-deps-lockfile into main
4 files +286 -70
packages/zig/src/cli/commands/install/env_deps.zigmodified+177-45
@@ -19,9 +19,15 @@
1919//! is added and its pin is untouched.
2020//! - Only those keys are read, and only a recognised driver name is ever
2121//! printed; no other `.env` value leaves this file.
22//! - Under `--frozen-lockfile` the lockfile is the contract: a package is
23//! inferred only if `pantry.lock` already records it, so CI never drifts
24//! from the committed lock.
22//! - An inference never reaches `pantry.lock`, neither as a constraint nor
23//! as a package entry, and neither does anything it drags in. The lock
24//! records what the workspace declares, so it has to read the same on
25//! every machine; a service read out of someone's local `.env` does not.
26//! Writing them in made a lock cut on a Postgres machine carry
27//! postgresql.org, gnome.org/libxslt and unicode.org, which a checkout
28//! without that `.env` then removed again (#253).
29//! - Because of that rule, `--frozen-lockfile` needs no special case here:
30//! an inference cannot move the lock, so it cannot drift from it either.
2531//! - `PANTRY_ENV_DEPS=0` (or `false`/`off`) turns the whole thing off.
2632
2733const std = ;
@@ -119,15 +125,21 @@ pub fn inferFromEnv(content: []const u8, out: *[max_inferences]Inference) []Infe
119125 return out[0..n];
120126}
121127
128/// The registry domain a dependency name refers to, in any spelling
129/// (`postgres`, `auto:redis`, `postgresql.org@17`).
130pub fn canonicalDomain(dep_name: []const u8) []const u8 {
131 var name = helpers.normalizePackageName(dep_name);
132 // `postgresql.org@17` written as a single key.
133 if (std.mem.lastIndexOfScalar(u8, name, '@')) |at| {
134 if (at > 0) name = name[0..at];
135 }
136 return helpers.resolvePackageAlias(name);
137}
138
122139/// Does the dependency list already name `domain`, in any spelling?
123140pub fn isDeclared(declared: []const parser.PackageDependency, domain: []const u8) bool {
124141 for (declared) |dep| {
125 var name = helpers.normalizePackageName(dep.name);
126 // `postgresql.org@17` written as a single key.
127 if (std.mem.lastIndexOfScalar(u8, name, '@')) |at| {
128 if (at > 0) name = name[0..at];
129 }
130 if (std.mem.eql(u8, helpers.resolvePackageAlias(name), domain)) return true;
142 if (std.mem.eql(u8, canonicalDomain(dep.name), domain)) return true;
131143 }
132144 return false;
133145}
@@ -137,22 +149,7 @@ pub fn isEnabled() bool {
137149 return !(std.mem.eql(u8, val, "0") or std.ascii.eqlIgnoreCase(val, "false") or std.ascii.eqlIgnoreCase(val, "off") or std.ascii.eqlIgnoreCase(val, "no"));
138150}
139151
140fn lockfileHas(allocator: std.mem.Allocator, project_dir: []const u8, domain: []const u8) bool {
141 const lockfile_reader = ;
142 const path = std.fs.path.join(allocator, &.{ project_dir, "pantry.lock" }) catch return false;
143 defer allocator.free(path);
144 var lock = lockfile_reader.readLockfile(allocator, path) catch return false;
145 defer lock.deinit(allocator);
146 if (lock.packages.get(domain) != null) return true;
147 var it = lock.packages.iterator();
148 while (it.next()) |entry| {
149 if (std.mem.eql(u8, entry.value_ptr.name, domain)) return true;
150 }
151 return false;
152}
153
154152pub const Options = struct {
155 frozen_lockfile: bool = false,
156153 /// Print what was added and why.
157154 report: bool = true,
158155};
@@ -184,7 +181,6 @@ pub fn infer(
184181 for (found) |inf| {
185182 if (isDeclared(declared, inf.domain)) continue;
186183 if (isDeclared(added.items, inf.domain)) continue;
187 if (options.frozen_lockfile and !lockfileHas(allocator, project_dir, inf.domain)) continue;
188184
189185 const name = try allocator.dupe(u8, inf.domain);
190186 errdefer allocator.free(name);
@@ -199,6 +195,106 @@ pub fn infer(
199195 return added.toOwnedSlice(allocator);
200196}
201197
198/// A set of registry domains, owning its keys.
199pub const DomainSet = struct {
200 allocator: std.mem.Allocator,
201 map: std.StringHashMap(void),
202
203 pub fn contains(self: DomainSet, domain: []const u8) bool {
204 return self.map.contains(domain);
205 }
206
207 pub fn deinit(self: *DomainSet) void {
208 var it = self.map.keyIterator();
209 while (it.next()) |key| self.allocator.free(key.*);
210 self.map.deinit();
211 }
212};
213
214/// Walk `seeds` and everything the catalog says they need. `visit` is called
215/// with each domain reached, seeds included, once each.
216fn walkCatalog(
217 allocator: std.mem.Allocator,
218 seeds: []const []const u8,
219 context: anytype,
220 comptime visit: fn (@TypeOf(context), []const u8) anyerror!void,
221) !void {
222 const generated = ;
223 const pipeline = ;
224
225 var seen = std.StringHashMap(void).init(allocator);
226 defer seen.deinit();
227
228 var frontier: std.ArrayList([]const u8) = .empty;
229 defer frontier.deinit(allocator);
230 for (seeds) |seed| try frontier.append(allocator, seed);
231
232 while (frontier.pop()) |domain| {
233 if ((try seen.getOrPut(domain)).found_existing) continue;
234 try visit(context, domain);
235 const info = generated.getPackageByDomain(domain) orelse continue;
236 for (info.dependencies) |spec| {
237 // Any OS's guard: a lock is written on one platform and read on
238 // another, so what the other one would pull in counts too.
239 const parsed = pipeline.parseAnyOsDepSpec(spec) orelse continue;
240 try frontier.append(allocator, parsed.domain);
241 }
242 }
243}
244
245/// Domains that are in this install ONLY because `.env` named a service.
246///
247/// The inferred roots, plus everything the package catalog says they pull in,
248/// minus everything a declared dependency pulls in. The subtraction is the
249/// point: `unicode.org` arrives with postgresql.org here, but a workspace that
250/// declares something else needing it has it on its own merits, and leaving
251/// that out of the lock would make the lock depend on the local `.env` in the
252/// opposite direction.
253///
254/// The transitive half is what made this a set rather than a name check: the
255/// lock cut on a Postgres machine gained gnome.org/libxslt and unicode.org as
256/// well as postgresql.org itself (#253).
257///
258/// The caller owns the result.
259pub fn inferredOnlyDomains(
260 allocator: std.mem.Allocator,
261 inferred: []const parser.PackageDependency,
262 declared: []const parser.PackageDependency,
263) !DomainSet {
264 var set = DomainSet{ .allocator = allocator, .map = std.StringHashMap(void).init(allocator) };
265 errdefer set.deinit();
266 if (inferred.len == 0) return set;
267
268 var inferred_roots: std.ArrayList([]const u8) = .empty;
269 defer inferred_roots.deinit(allocator);
270 for (inferred) |dep| try inferred_roots.append(allocator, canonicalDomain(dep.name));
271
272 const Add = struct {
273 fn visit(target: *DomainSet, domain: []const u8) anyerror!void {
274 const gop = try target.map.getOrPut(domain);
275 if (gop.found_existing) return;
276 gop.key_ptr.* = target.allocator.dupe(u8, domain) catch |err| {
277 _ = target.map.remove(domain);
278 return err;
279 };
280 }
281 };
282 try walkCatalog(allocator, inferred_roots.items, &set, Add.visit);
283
284 var declared_roots: std.ArrayList([]const u8) = .empty;
285 defer declared_roots.deinit(allocator);
286 for (declared) |dep| try declared_roots.append(allocator, canonicalDomain(dep.name));
287
288 const Remove = struct {
289 fn visit(target: *DomainSet, domain: []const u8) anyerror!void {
290 if (target.map.fetchRemove(domain)) |removed| target.allocator.free(removed.key);
291 }
292 };
293 try walkCatalog(allocator, declared_roots.items, &set, Remove.visit);
294
295 return set;
296}
297
202298pub fn freeDeps(allocator: std.mem.Allocator, deps: []const parser.PackageDependency) void {
203299 for (deps) |dep| {
204300 var owned = dep;
@@ -317,7 +413,7 @@ test "infer reads the project's .env and never overrides an explicit pin" {
317413 try testing.expectEqualStrings("redis.io", with_pin[0].name);
318414}
319415
320test "under --frozen-lockfile only packages the lockfile records are inferred" {
416test "inference is independent of the lockfile, in frozen runs too" {
321417 const allocator = testing.allocator;
322418 var tmp = testing.tmpDir(.{});
323419 defer tmp.cleanup();
@@ -326,23 +422,59 @@ test "under --frozen-lockfile only packages the lockfile records are inferred" {
326422
327423 try writeFixture(tmp, ".env", "DB_CONNECTION=pgsql\nQUEUE_CONNECTION=redis\n");
328424
329 // No lockfile at all: frozen infers nothing.
330 const without_lock = try infer(allocator, dir, &.{}, .{ .frozen_lockfile = true, .report = false });
331 defer freeDeps(allocator, without_lock);
332 try testing.expectEqual(@as(usize, 0), without_lock.len);
333
334 try writeFixture(tmp, "pantry.lock",
335 \\{
336 \\ "version": "1",
337 \\ "lockfileVersion": 1,
338 \\ "generatedAt": "2026-10-03T00:00:00Z",
339 \\ "packages": {
340 \\ "postgresql.org@17.6.0": { "name": "postgresql.org", "version": "17.6.0", "source": "pantry" }
341 \\ }
342 \\}
343 );
344 const with_lock = try infer(allocator, dir, &.{}, .{ .frozen_lockfile = true, .report = false });
345 defer freeDeps(allocator, with_lock);
346 try testing.expectEqual(@as(usize, 1), with_lock.len);
347 try testing.expectEqualStrings("postgresql.org", with_lock[0].name);
425 // No lockfile at all. Inference used to be gated on the lock recording the
426 // package, which only held because inference itself had put it there; now
427 // nothing it produces reaches the lock, so there is nothing to gate on and
428 // a --frozen-lockfile run infers exactly what any other run does (#253).
429 const inferred = try infer(allocator, dir, &.{}, .{ .report = false });
430 defer freeDeps(allocator, inferred);
431 try testing.expectEqual(@as(usize, 2), inferred.len);
432 try testing.expectEqualStrings("postgresql.org", inferred[0].name);
433 try testing.expectEqualStrings("redis.io", inferred[1].name);
434}
435
436test "inferred-only domains cover what an inference drags in" {
437 const allocator = testing.allocator;
438 const mk = struct {
439 fn dep(name: []const u8) parser.PackageDependency {
440 return .{ .name = name, .version = "latest" };
441 }
442 }.dep;
443
444 var set = try inferredOnlyDomains(allocator, &.{mk("postgresql.org")}, &.{mk("bun.sh")});
445 defer set.deinit();
446
447 // The root itself, and the transitive entries that appeared in the lock
448 // alongside it in the reported failure.
449 try testing.expect(set.contains("postgresql.org"));
450 try testing.expect(set.contains("gnome.org/libxslt"));
451 try testing.expect(set.contains("unicode.org"));
452 try testing.expect(!set.contains("bun.sh"));
453}
454
455test "a domain a declared package also needs is not inferred-only" {
456 const allocator = testing.allocator;
457 const mk = struct {
458 fn dep(name: []const u8) parser.PackageDependency {
459 return .{ .name = name, .version = "latest" };
460 }
461 }.dep;
462
463 // Declaring postgres outright leaves nothing attributable to the .env...
464 var declared = try inferredOnlyDomains(allocator, &.{mk("postgresql.org")}, &.{mk("postgres")});
465 defer declared.deinit();
466 try testing.expect(!declared.contains("postgresql.org"));
467 try testing.expect(!declared.contains("unicode.org"));
468
469 // ...and so does declaring something that needs the same library: the lock
470 // must not lose an entry that package needs on its own merits.
471 var shared = try inferredOnlyDomains(allocator, &.{mk("postgresql.org")}, &.{mk("gnome.org/libxslt")});
472 defer shared.deinit();
473 try testing.expect(shared.contains("postgresql.org"));
474 try testing.expect(!shared.contains("gnome.org/libxslt"));
475
476 // Nothing inferred, nothing to exclude.
477 var empty = try inferredOnlyDomains(allocator, &.{}, &.{mk("postgresql.org")});
478 defer empty.deinit();
479 try testing.expectEqual(@as(u32, 0), empty.map.count());
348480}