feat(pins,resolver): omit fetch of specified pins#91
Conversation
NotAShelf
left a comment
NotAShelf
left a comment
There was a problem hiding this comment.
3/10 this PR was like having a piano being dropped on me
The feature seems to work as intended though. I've verified lazy omission on both flake and tack sides. CI is failing, obviously, and should be fixed (obviously).
| let table = table | ||
| .as_table_like() | ||
| .with_context(|| "omit_inputs must be a table")?; |
There was a problem hiding this comment.
This is what's breaking the CI, I think. Try let table_like = table..
| path: vec![inp.name.clone()], | ||
| source: SourceRef::Locked(node.clone()), | ||
| submodules: inp.submodules, | ||
| omit: OmitPolicy::for_input(&omit_inputs, inp, &all_follow), |
There was a problem hiding this comment.
Attaching one OmitPolicy per top-level pin makes dedup results depend on pin declaration order. visited below is keyed only on item.source.key(), so when two pins reach the same transitive source, whichever target is dequeued first wins and the other is dropped. BUT, queue_tack_transitive propagates each pin's policy into the shared subtree.
I'd suggest either including the policy in the visited key (re-scan shared subtrees once per distinct policy), or, union semantics (only omit an input from the report if every pin reaching that subtree omits it.) Order dependence should be a proper, deliberate choice and not... this.
| in | ||
| { | ||
| omitted = all_omit_inputs ++ (pin.omit_inputs or [ ]); | ||
| kept = (pin.keep_inputs or [ ]) ++ (attrNames f.level); |
There was a problem hiding this comment.
I think this kept-set disagrees with the dedup implementation on exclude_follow semantics? It's either that, or I'm missing something.
Looks to me like that here the resolver removes follows from all_follow by exact key (removeAttrs all_follow (pin.exclude_follow or [ ]) and whatnot), but then keeps attrNames f.level. OmitPolicy::for_input filters all_follow keys by FollowAlias::bare_name(key) ∉ excludes, i.e., a bare-name match.
I don't have a fix I can propose to you, but I'm sure you'll know what to do.
| pins = fromTOML (readFile ./pins.toml); | ||
| lock = fromJSON (readFile ./pins.lock.json); | ||
| all_follow_raw = pins.all_follow or { }; | ||
| all_omit_inputs = (pins.omit_inputs or { }).names or [ ]; |
There was a problem hiding this comment.
Writing omit_inputs = ["flake-compat"] as an array instead of a [omit_inputs] table is a plausible mistake (bcs per-pin omit_inputs is an array), and (pins.omit_inputs or { }).names or [ ] would silently evaluate to []. Tthe resolver omits nothing with no indication why, while tack dedup hard-errors with "omit_inputs must be a table" for the same file. Similarly, names = "somestring" surfaces later as an opaque ++ error. I'd probably throw with a clear validation error/hint here like if isAttrs ... then ... else throw "tack: omit_inputs must be a table with a names array".
| tackOmitOverrides = listToAttrs ( | ||
| map | ||
| (name: { | ||
| inherit name; | ||
| value = omittedInput { | ||
| side = "tack"; | ||
| inherit name; | ||
| }; | ||
| }) | ||
| ( | ||
| filter ( | ||
| name: | ||
| shouldOmit { | ||
| inherit omit; | ||
| side = "tack"; | ||
| inherit name; | ||
| } | ||
| ) (attrNames (upPins.inputs or { })) | ||
| ) | ||
| ); |
There was a problem hiding this comment.
Sorry for the nit but isn't this basically evalFetch?
| }, | ||
| }; | ||
| for (key, locked) in doc.locked_nodes() { | ||
| if omit.omits(Side::Flake, strip_disambiguator(key)) { |
There was a problem hiding this comment.
This skips the omitted node's own finding, but the rest of the upstream lock is still flat-scanned.
E.g., in the README's omit_inputs = ["pre-commit-hooks"] example any dep exclusive to pre-commit-hooks still shows up in tack dedup. This may be an accepted approximation, though, README says omission applies to tack dedup.
2 participants
No description provided.