Skip to content

Fix: add check before recursing into inputs when pruning rec follows. - #35

Merged
vic merged 1 commit into
denful:mainfrom
jbgi:main
Oct 28, 2026
Merged

vic merged 1 commit into
denful:mainfrom
jbgi:main

Conversation

@jbgi

@jbgi jbgi commented Oct 22, 2026

Copy link
Copy Markdown
Contributor

No description provided.

@vic
vic merged commit af30c91 into denful:main Oct 28, 2026
2 checks passed
@sini

sini commented Oct 28, 2026

Copy link
Copy Markdown
Contributor

Reviewed after the merge. The fix is right. follows recursed on inputs.${followRoot} even when the follow's root is not one of the overrides (in the new test, common-dep.follows = "test2/common-dep", where test2 comes from the npins sources rather than the override set), so the lookup threw. inputs ? ${followRoot} && stops the walk there, which means "this override is not part of the chain rooted at name". That is the right answer, and the new test covers that exact case.

One case still overflows. It predates this PR. When two overrides follow each other and neither chain reaches name, the walk never ends:

# fixtures as in test-complex-cross-overrides-variant
{ nixpkgs = mkSrc "/our-nixpkgs";
  nixpkgs3.follows = "common-dep/x";
  common-dep.follows = "nixpkgs3/y";
  test3.outPath = ./fixtures/with-inputs-project3; }
# error: stack overflow; max-call-depth exceeded

With the same setup, the variant test's overrides give /our-nixpkgs. Such a cycle is a user error, but a stack overflow is hard to read. A small change would carry the visited roots and stop, or throw by name:

follows = seen: input:
  isFollows inputs.${input}
  && (let followRoot = builtins.head (builtins.split "/" inputs.${input}.follows); in
      followRoot == name
      || (inputs ? ${followRoot}
          && (if builtins.elem followRoot seen
              then throw "with-inputs: follows cycle through ${followRoot}"
              else follows (seen ++ [ input ]) followRoot)));
# ... builtins.filter (follows [ ]) (builtins.attrNames inputs)

This is not blocking for #35.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants