diff --git a/DESIGN.md b/DESIGN.md index cfa001e..d58a96c 100644 --- a/DESIGN.md +++ b/DESIGN.md @@ -215,10 +215,24 @@ it uses a prefix-only `strip_out` rather than `strip_drv`) — and stores its matters more there than anywhere else: it's the one append-only, never-evicted table, so its per-row bytes are what compound over time (~15% off it, measured). +**Selector resolutions → SQLite too** (`sel_drv`, §6). A `-p` review evaluates +only the attrs it was named, so it writes no eval file — a partial eval cached +under a `(tree, system, profile)` key is the one thing that poisons every later +diff. But its _re-run_ has to stay instant like any other report's, so the +resolutions land in the same engine the `tests` cache uses, and for the same +reason: a keyed, incremental, partial fact, looked up by exact attr rather than +diffed as a whole. Unlike `test_pkg` it needs no completeness marker — nothing +ever asks for "all attrs at this key" — so a partial table is a partial cache +rather than a truncated fact. One consequence for `--clean` (below): a selector +key has no eval file, so it is not part of the corpus `--clean` measures or +evicts. That is the honest reading of `--clean`'s contract rather than an +oversight; the rows are tiny, keyed, and re-derivable in seconds, and a dated +eviction can be added additively if that ever stops being true. + ``` ~/.cache/nix-npb/ format-version # eval-cache format version (§1); a bump wipes the eval cache, keeps the log - npb.sqlite # observation log (tiny) + test cache (the bulk) + patch-tree cache (§8) + npb.sqlite # observation log (tiny) + test cache (the bulk) + selector resolutions + patch-tree cache (§8) //.tsv.zst # attr→drv maps (zstd), one file per (profile, system, eval) — evicted by --clean ``` @@ -656,7 +670,7 @@ as a bare `attr` line, distinct from a missing attr (no line at all). The diff is a set-diff on `(attr, drv_path)`, where a `None` drv means "threw": a package that starts or stops evaluating under the profile shows as a changed row (⏩↔build), while one that throws on _both_ sides is `None == None` — no change, -not shown, so ⏩→⏩ never appears (§8). A row present on only _one_ side is a +not shown, so ⏩→⏩ never appears in a delta report (§8). A row present on only _one_ side is a change whether or not it has a drv there, so a package that threw and then vanished (or appeared already throwing) still reports — ⏩→➖ and ➖→⏩ (§8). An earlier version dropped those two, reasoning that an attr nixpkgs-review could @@ -668,6 +682,147 @@ changed-by-this-side / by-the-other / by-both; it turned out not to matter in practice and was dropped. The merge base survives only as the `--no-merge` base of a report.) +**Reviewing less than the whole changed set: `-p`.** A change deep in the +dependency chain — a `gitMinimal` or `python3Packages.setuptools` bump — can put +tens of thousands of attrs in the changed set, which is more than a review +machine can build and more than a report can usefully say. So a review need not +cover the whole set: `-p`/`--package` reviews exactly the attrs it names +(`model::Coverage`, prompted by +[#2](https://github.com/samestep/npb/issues/2)). The question shifts from "what +did this change break?" — answered from the diff of two whole-set evals — to +"what does this change do to _these_ attrs?", which needs no whole-set eval at +all: npb evaluates the named attrs on both trees and diffs those. Its real value +is **reach** — it can review attrs a whole-set walk never enumerates — not a +different reporting rule. + +> **`-P`/`--skip-package` was implemented alongside it and then dropped**, before +> either shipped. The issue asked for both, for parity with nixpkgs-review, on +> the guess that one would come free with the other; it doesn't, and asked +> [directly](https://github.com/samestep/npb/issues/2#issuecomment-5385779137) +> the requester had never used `-P` there and couldn't construct a case for it +> that `-p` wouldn't cover. A flag nobody wants is a flag npb is committed to +> keeping working forever (§1), so it went. + +**Why a selection is not a filter — the feedback that forced this.** The first +implementation made `-p` a filter over the changed set, matched by attr name. +[The first person to try +it](https://github.com/samestep/npb/pull/3#issuecomment-5381463255) ran 18 `-p` +attrs against [nixpkgs#545428](https://github.com/NixOS/nixpkgs/pull/545428) and +got rows for two of them. Nothing was broken; the changed set genuinely +contained no `python3Packages.dirty-equals`, because a whole-set walk only +descends into attrsets marked `recurseForDerivations` and nixpkgs marks exactly +two of the python sets ([`all-packages.nix`](https://github.com/NixOS/nixpkgs/blob/4905842092e96ae11254a905ef9d46f1b0e29437/pkgs/top-level/all-packages.nix#L4485): +`python3Packages = dontRecurseIntoAttrs python314Packages`, with +`recurseIntoAttrs` on only `python313Packages`/`python314Packages`). Measured on +one cached eval: 11305 rows under `python313Packages`, 11304 under +`python314Packages`, **zero** under `python3Packages`, `python311Packages`, +`python312Packages`, `python315Packages`, or any of the pypy sets — while +`python311Packages.dirty-equals` evaluates perfectly well and is a real rebuild +of that PR. A name-matching filter can never reach those attrs, and neither can +any tool keyed on a recursing walk (ofborg and Hydra included). + +nixpkgs-review had already solved the near half of this: with `-p` it +**evaluates** the named attrs and intersects the changed set on **`drv_path`** +(`_join_packages_for_system`, whose own comment says so), dying when a named attr +resolves outside it — and on its local path it skips computing rebuilds +altogether and just builds what you named. Since drvpath keying is npb's founding +decision (§2), matching a filter on attr _names_ was keying on the wrong thing. +Evaluating the named attrs directly goes one better than intersecting: it reviews +attrs no changed set contains. + +**What a selection does.** For each named attr, npb resolves it on both trees — +one targeted `nix-eval-jobs` run per `(tree, system)` over a `lib.attrByPath` +selector (`eval::resolve_attrs`, the same `select_expr` the `instantiate` phase +uses, so the attr-path splitting stays on the Nix side and npb's Rust keeps +treating attrs as opaque keys) — and pairs the two sides into a row. Three +consequences: + +- **No whole-set eval at all.** A cold selector run costs seconds (a lazy + top-level lookup per side) where a delta review costs minutes for two + ~114k-attr walks. This is the difference between npb being usable on a + setuptools-class rebuild and not. +- **It replaces one step, not the pipeline.** npb picks the attrs to review, adds + their `passthru.tests` unless `--no-tests`, builds, and reports. `-p` replaces + the *first* step and nothing else — the way `--head` replaces the working-tree + guess without changing what happens to the revision it names. So a named + package's tests come along exactly as a changed package's would, and + `--no-tests` means the same thing it always did: skip the step that adds them. +- **Every named attr is reported**, whether or not the change touches it. That is + the point of naming it: "I think this might have moved, tell me it didn't" is a + question a diff can't answer, and the person who asked for the flag + [said so](https://github.com/samestep/npb/pull/3#issuecomment-5386738586) after + trying the other way. So `⏩→⏩` and `➖→➖` are reachable in a selection (§8) — + the latter being how a misspelled attr shows itself, which is why naming an + attr that resolves nowhere is a row and not an error. (Making it an error was + tried and backed out for a second reason: the condition can't be stated per + system, since a Linux-only package reviewed on Darwin legitimately resolves to + nothing there, so it needs a quantifier over every side of every `--system` — + and a rule that awkward to state is a rule about the wrong thing.) +- **A named attr must be a derivation, not a subtree.** `nix-eval-jobs` recurses + into a `recurseForDerivations` attrset, so `-p python313Packages` would quietly + become ~11k rows. Such a row arrives with a multi-element `attrPath` whose first + element is the attr that was asked for, so it is detected without parsing + anything and refused. Naming a whole set is a plausible want, but it is a + different feature with different costs, and it stays additive. + +**And the resolutions are cached, which is the whole reason this is affordable.** +A selection writes no eval file — it evaluated a handful of attrs, and caching +that under a `(tree, system, profile)` key would be a _partial_ eval, the one +thing that poisons every later diff with phantom "removed" packages (§4). But +npb's other stated guarantee is that a report's reproduction command re-runs +near-instantly, and a targeted eval with nowhere to live would re-import nixpkgs +on every run — exactly the cost `drvs_needing_instantiation` exists to avoid +elsewhere in this section. So a resolution is cached where a keyed, incremental, +partial fact belongs: SQLite (`sel_drv`, §4), holding the same trichotomy the +eval-file format carries — a drv, no drv but it threw (⏩), or absent (➖). A warm +selector re-run resolves nothing, imports nothing, and answers from the log. + +**Attrs are matched exactly.** No subtrees, no globs, no prefixes: nowhere in +npb's Rust is an attr path anything but an opaque key, and the string a matcher +would have to parse is ambiguous anyway — the eval file keeps `nix-eval-jobs`' +quoting, so a dotted _name_ (`rubyPackages."http_parser.rb"` — 34 such attrs in +one aarch64-linux eval) and a dotted _path_ look alike. + +**Tests follow the attrs a review covers, for free.** The `tests` phase expands +whatever the changed set holds (`changed_names`), so a selection expands every +attr it named and a delta expands the packages that changed — neither needs the +flag, or the diff, to know what a test row is called. The two modes then differ +only in how their test rows are filtered, which is the same difference as for +their packages: a delta reports changes, so an unchanged test is not a row +(`changed_tests`); a selection reports what it was given, and a named package's +tests were given with it (`paired_tests`). + +The delta's blind spot rides along: a change to a package's `passthru.tests` that +leaves the package itself untouched is invisible, since tests are only ever +enumerated from a package that is already being reviewed. A selection is the way +out, since it can name the test — `-p python313Packages.pydantic-core.tests.pytest` +reviews a test whose package didn't move (§6's `pydantic-core` case). Enumerating +`passthru.tests` for *every* package would close the hole and was measured at +roughly an order of magnitude on the whole-set eval, which is not a trade npb +makes by default; as an opt-in flag it stays a separate feature. + +One wrinkle *is* a selection's own. `-p` can name a `tests` attr directly, which +puts a test derivation in the changed set, and the phase then enumerates *that* +derivation's `passthru.tests` — which, for the `overrideAttrs`-with-`doCheck` +idiom, resolve straight back to itself (`x.tests.pytest.tests.pytest`). A +whole-set walk can never produce such a row, since it doesn't descend into +`passthru.tests`. npb drops it (`main::drop_self_tests`): a test whose derivation +*is* its package's, on both sides, is that package under a second name — no new +fact, facts being keyed on drvpath (§2) — and the report would only print the +pair as an alias. The redundant enumeration still happens; recognising the named +attr as a test up front would mean reading structure out of an attr path, which +npb doesn't do. + +**Leaving an attr out removes a target, not a dependency.** An unnamed package +that a named target needs is still built by nix inside that target's closure — +matching nixpkgs-review — and the observation it produces is keyed on its +drvpath (§2) like any other, so it lands in the log all the same. A later run +without the flag finds it already decided, for free. The visible consequence is a +one-sided `🚫` whose culprit isn't in the report; that is unavoidable and mild, +since `🚫` already means "something in the closure broke" (§8). Narrowing a +review narrows a report; it never discards knowledge — which is the difference +between doing this over a durable fact store and doing it one-shot. + **Eval does not instantiate; the changed set is materialized before building.** `nix-eval-jobs` runs with `--no-instantiate`: npb needs only the `drvPath` and `outputs` (both emitted regardless), so it skips writing the `.drv` files — ~40% @@ -1017,10 +1172,14 @@ accepted gap of §5: a target nix never reached with nothing verifiably failing in its closure). A section is one `(base, head)` state pair, and its header **is** a composable `before → after` token (one emoji per side) — no per-row glyphs; the section a row lands in carries all the meaning. -Of the 6 × 6 = 36 pairs, **34** can appear in a report: every combination except -the two the diff can't see at all — ⏩→⏩ (a `None` drv on both sides is no +Of the 6 × 6 = 36 pairs, **34** can appear in a _delta_ report: every combination +except the two the diff can't see at all — ⏩→⏩ (a `None` drv on both sides is no change, §6) and ➖→➖ (an attr in neither eval is never a row). Every other pair, -including ⏩→➖ and ➖→⏩, is reachable. +including ⏩→➖ and ➖→⏩, is reachable. A **selection** (`-p`, §6) reaches all +**36**, because it reports every attr it was named rather than every attr that +changed: an attr that throws on both sides is still the answer to a question +someone asked, and ➖→➖ is how a `-p` that resolved to nothing — a typo, or a path +that isn't on either tree — shows itself. Sections are ordered **worst-delta-first**: each state has a goodness on the build-outcome axis (`✅` > `⏩` > `🚫` > `❌`, with `➖` absent slotted just under `✅` as _new_/_gone_), and a section sorts by the signed delta @@ -1062,10 +1221,10 @@ tree-keyed and the synthetic merge is deterministic (§6), that reproduces the review byte-for-byte, and npb re-mints the merge itself — the command never names a synthetic (local-only) commit. Only report-shaping flags are echoed (`--no-merge`, the profile's `--allow-broken`/`--allow-unsupported`/`--allow-insecure`, -`--no-tests`, and an explicit `-s` per system, since the default system is -host-specific); `--retry` and the eval-sizing knobs don't change the changeset, -so they're omitted. What varies is only how the _head_'s tree is recovered on -another machine: +`--no-tests`, each `-p` of a selection (§6), and an explicit `-s` per system, +since the default system is host-specific); `--retry` and the eval-sizing +knobs don't change the changeset, so they're omitted. What varies is only how the +_head_'s tree is recovered on another machine: - a committed / explicit head is already a fetchable commit → `--head `; - otherwise (a `--pr` head or an uncommitted working tree) the head has no diff --git a/README.md b/README.md index b3b1ffc..02b2c9b 100644 --- a/README.md +++ b/README.md @@ -116,6 +116,7 @@ That command may not give you the same result if you run it yourself, but `npb`' - [`--patch`](#--patch) - [`--no-merge`](#--no-merge) - [`--no-tests`](#--no-tests) + - [`-p`, `--package`](#-p---package) - [`--allow-unsupported`](#--allow-unsupported) - [`--allow-broken`](#--allow-broken) - [`--allow-insecure`](#--allow-insecure) @@ -250,6 +251,14 @@ By default, `npb` includes all `passthru.tests` for packages that changed betwee npb --no-tests ``` +### `-p`, `--package` + +By default, `npb` determines the set of non-test packages by finding all changes. To provide a custom set of packages instead: + +```sh +npb -ppython3{,{11..15}}Packages.dirty-equals +``` + ### `--allow-unsupported` By default, `npb` sets [`allowUnsupportedSystem`](https://nixos.org/manual/nixpkgs/stable/#sec-allow-unsupported-system) to `false`. To set it to `true` instead: diff --git a/src/clean.rs b/src/clean.rs index 11de2ad..8763a62 100644 --- a/src/clean.rs +++ b/src/clean.rs @@ -7,9 +7,11 @@ //! The last-*used* time is the file's mtime, which `eval::eval_pairs` re-stamps //! on every cache hit (`evalfile::touch_eval`) so a reused base eval stays warm. //! -//! Evicting an eval file also purges that `(tree, system)`'s `tests` rows -//! (`store::Store::purge_tests`): the tests cache is keyed on the same tree, so -//! the two travel together and the DB stays proportional to the eval corpus. +//! Evicting an eval file also purges that `(tree, system)`'s cached rows +//! (`store::Store::purge_key` — `tests` and selector resolutions alike): both are +//! keyed on the same tree, so they travel with it and the DB stays proportional +//! to the eval corpus. A selector run (`-p`) writes no eval file, so its key is +//! not part of that corpus and nothing here evicts it (DESIGN §4). //! The append-only observation log is left untouched — it's keyed on drvpath (no //! tree to evict by), tiny, and the one thing expensive to re-derive (it //! remembers *failures*, DESIGN.md §5). @@ -243,7 +245,7 @@ fn gather(root: &std::path::Path) -> Result> { Ok(out) } -/// Evict eval files per `spec`, purge each evicted `(tree, system)`'s `tests` +/// Evict eval files per `spec`, purge each evicted `(tree, system)`'s cached /// rows, and vacuum the DB once. This is the whole `--clean` action — it reviews /// nothing. It first prints exactly what it *would* remove and asks for /// confirmation on stdin, deleting only on a yes. Nothing is touched until confirmed. @@ -284,7 +286,7 @@ pub fn clean(spec: &CleanSpec) -> Result<()> { for &i in &victims { let f = &files[i]; fs::remove_file(&f.path).with_context(|| format!("removing {}", f.path.display()))?; - rows += store.purge_tests(&f.tree, &f.system)?; + rows += store.purge_key(&f.tree, &f.system)?; } store.vacuum()?; diff --git a/src/eval.rs b/src/eval.rs index 0b78839..48b7c1e 100644 --- a/src/eval.rs +++ b/src/eval.rs @@ -24,7 +24,7 @@ use serde::Deserialize; use crate::evalfile::{eval_path, write_eval}; use crate::live; -use crate::model::{AttrEval, Profile, Rev, TestJob}; +use crate::model::{AttrEval, Profile, Resolved, Rev, TestJob}; /// The nixpkgs `config` attrset for a [`Profile`] (DESIGN.md §6). npb evaluates /// under the profile the user means — strict by default — so a @@ -747,6 +747,164 @@ fn select_expr(repo: &Path, rev: &str, system: &str, paths: &[String], config: & ) } +/// Which of [`resolve_attrs`]' requests get a node in the progress tree: the ones +/// with something left to resolve. +/// +/// A side whose attrs are all cached has no items, hence no shard — and a group +/// with no shards is never marked running or done, so it would sit blue at +/// `0 / 0` for the whole run while its phase line showed yellow, since the parent +/// rollup reads a waiting child as "still running" (`live::eff_state`). Both +/// sibling phases already avoid this: `instantiate_prepare` drops its empty +/// requests, and `reveal_system_tests` never creates the leaf at all. The +/// returned indices map a group back to its request. +fn live_requests(requests: &[(Rev, String, Vec)]) -> Vec { + requests + .iter() + .enumerate() + .filter(|(_, (_, _, attrs))| !attrs.is_empty()) + .map(|(i, _)| i) + .collect() +} + +/// Resolve the attrs a selector review named (`-p`) to their drvs at each +/// `(tree, system)`, through the same shard scheduler as every other targeted +/// eval (DESIGN §6). `requests` is `(rev, system, attrs)` — callers pass only the +/// attrs not already cached (`store::sel_drv`), so a warm re-run calls this with +/// nothing and does no work at all, which is what keeps a selector report's +/// reproduction command instant. +/// +/// Returns one [`Resolved`] per *requested* attr, in the request's order, filling +/// in the attrs `nix-eval-jobs` said nothing about: a `lib.attrByPath` miss +/// evaluates to `null`, which yields no job line, and that silence is exactly the +/// `➖ absent` fact (a typo'd `-p`, or a real attr path that isn't there on this +/// side). An attr that *threw* does produce a line — with an `error` and no +/// `drvPath` — so ⏩ and ➖ stay distinct, as they are in the eval-file format. +/// +/// One shard per request, never sub-sliced up front, exactly like +/// [`eval_tests`]/[`instantiate_execute`]: the cost is the per-key nixpkgs +/// import, so splitting a handful of attrs across shards would only re-pay it. +/// +/// **A named attr must resolve to a derivation, not a subtree.** `nix-eval-jobs` +/// recurses into an attrset marked `recurseForDerivations`, so `-p +/// python313Packages` would quietly become ~11k rows; such a row comes back with +/// a multi-element `attrPath` (`["python313Packages", "APScheduler"]`) whose +/// first element is the attr that was asked for, so it is detected without +/// parsing anything, and refused. Naming a whole set is a plausible thing to +/// want, but it is a different feature with different costs. +pub fn resolve_attrs( + repo: &Path, + requests: &[(Rev, String, Vec)], + profile: Profile, + tree: &live::Tree, + handle: live::LiveHandle<'_>, +) -> Result>> { + let live = live_requests(requests); + if live.is_empty() { + return Ok(requests.iter().map(|_| Vec::new()).collect()); + } + // Light workers (no `.drv` writes, a handful of attrs), so the full-set eval's + // per-slot budget fits — unlike `tests`, this never forces a `nixosTest`. + let slots = default_slots(SLOT_MEM_MB); + let groups: Vec<(String, String)> = live + .iter() + .map(|&i| (requests[i].1.clone(), requests[i].0.display.clone())) + .collect(); + let nodes = add_phase(tree, "resolve", &groups, Leaf::Count); + let labels: Vec = live + .iter() + .map(|&i| format!("{} {}", requests[i].0.display, requests[i].1)) + .collect(); + let items: Vec> = live.iter().map(|&i| requests[i].2.clone()).collect(); + let meta: Vec<(&Rev, &str)> = live + .iter() + .map(|&i| (&requests[i].0, requests[i].1.as_str())) + .collect(); + let shard_size = items.iter().map(Vec::len).max().unwrap_or(1).max(1); + let results: Vec>> = + live.iter().map(|_| Mutex::new(Vec::new())).collect(); + + run_shards( + "resolve", + nodes, + labels, + items, + shard_size, + slots, + // One streamed job per requested attr — except the absent ones, which + // stream nothing, so the count can legitimately stop short of the total. + true, + handle, + |gi, label, attrs, on_item| { + let (rev, system) = meta[gi]; + let expr = select_expr(repo, &rev.commit, system, attrs, &profile_config(profile)); + let rows = stream_jobs( + &expr, + 1, + DEFAULT_WORKER_MEM_MB, + false, + label, + |raw| raw, + || on_item(1), + )?; + rows.into_iter() + .map(|raw| { + // The wrapper attrset's name *is* the attr that was asked for, + // so element 0 identifies the request even when the attr path + // it names contains dots. More than one element means + // nix-eval-jobs recursed into a set (above). + let asked = raw.attr_path.first().cloned().unwrap_or_default(); + if raw.attr_path.len() > 1 { + bail!( + "-p {asked} names a package set, not a package: it contains \ + {} (and others). Name the packages you want reviewed.", + raw.attr_path.join(".") + ); + } + let threw = raw.drv_path.is_none(); + Ok(( + asked, + Resolved { + drv_path: raw.drv_path, + threw, + }, + )) + }) + .collect() + }, + |gi, rows| { + results[gi].lock().unwrap().extend(rows); + Ok(()) + }, + // No salvage. The rows a partial pass streamed are individually true, but + // *absence* is only meaningful for a stream that ran to completion — a + // truncated one is indistinguishable from "that attr isn't there" — so the + // shard is re-run whole rather than concluding ➖ from a dead worker. + None, + )?; + + // Back to one entry per request, in the caller's order: a side that had + // nothing to resolve contributes nothing (its attrs were already cached). + let mut resolved: Vec> = + requests.iter().map(|_| HashMap::new()).collect(); + for (&i, rows) in live.iter().zip(results) { + resolved[i] = rows.into_inner().unwrap().into_iter().collect(); + } + // Fill in the attrs that streamed nothing: silence means absent. + Ok(requests + .iter() + .zip(resolved) + .map(|((_, _, asked), by_attr)| { + asked + .iter() + .map(|a| { + let r = by_attr.get(a).cloned().unwrap_or_else(Resolved::absent); + (a.clone(), r) + }) + .collect() + }) + .collect()) +} + /// A prepared instantiate: its (blue) tree nodes and the non-empty requests they /// pair with, from [`instantiate_prepare`], ready for [`instantiate_execute`]. pub struct Instantiate { @@ -1595,4 +1753,41 @@ mod tests { assert!(e.contains(r#""hello" "#)); assert!(e.contains(r#""with\"quote" "#)); } + + #[test] + fn only_sides_with_work_get_a_resolve_node() { + let rev = |label: &str| Rev { + tree: format!("tree-{label}"), + commit: format!("commit-{label}"), + label: label.into(), + display: label.into(), + }; + let req = |label: &str, system: &str, attrs: &[&str]| { + ( + rev(label), + system.to_string(), + attrs.iter().map(|a| a.to_string()).collect::>(), + ) + }; + // A system whose attrs were all cached (both sides empty) contributes no + // group at all — otherwise its leaves would sit blue at `0 / 0` forever + // and drag the phase line to yellow after everything finished. + let requests = vec![ + req("base", "x86_64-linux", &["git"]), + req("head", "x86_64-linux", &["git"]), + req("base", "aarch64-linux", &[]), + req("head", "aarch64-linux", &[]), + // ...and a system cached on one side only keeps the other side. + req("base", "aarch64-darwin", &[]), + req("head", "aarch64-darwin", &["git"]), + ]; + assert_eq!(live_requests(&requests), vec![0, 1, 5]); + + // Nothing to do anywhere: no phase, which `resolve_attrs` short-circuits on. + let cached = vec![ + req("base", "x86_64-linux", &[]), + req("head", "x86_64-linux", &[]), + ]; + assert!(live_requests(&cached).is_empty()); + } } diff --git a/src/evalfile.rs b/src/evalfile.rs index 7425976..8f95531 100644 --- a/src/evalfile.rs +++ b/src/evalfile.rs @@ -451,6 +451,35 @@ pub fn changed_tests( diff(&rows(base), &rows(head)) } +/// Pair two `test_attr → drv` maps into a row per test, *keeping* the ones whose +/// drv is identical on both sides — [`changed_tests`] without the diff. +/// +/// This is what a selection wants (DESIGN §6). `-p` replaces the step that finds +/// the packages to review; the step that adds their `passthru.tests` still runs, +/// so those tests are part of what was asked for, and "this test is unaffected" +/// is an answer to that question rather than a non-event. A delta review keeps +/// [`changed_tests`], where a row *is* a change and an unchanged test isn't one. +pub fn paired_tests( + base: &std::collections::HashMap, + head: &std::collections::HashMap, +) -> Vec { + let mut attrs: Vec<&String> = base.keys().chain(head.keys()).collect(); + attrs.sort_unstable(); + attrs.dedup(); + attrs + .into_iter() + .map(|attr| ChangedAttr { + attr: attr.clone(), + base_drv: base.get(attr).cloned(), + head_drv: head.get(attr).cloned(), + // Only tests that resolved to a drv are cached, so a side without a + // row is genuinely absent (➖) rather than a throw (⏩). + base_threw: false, + head_threw: false, + }) + .collect() +} + #[cfg(test)] mod tests { use super::*; @@ -696,6 +725,38 @@ mod tests { let _ = fs::remove_dir_all(&dir); } + #[test] + fn paired_tests_keeps_the_unchanged_rows() { + let m = |kv: &[(&str, &str)]| { + kv.iter() + .map(|(a, d)| (a.to_string(), format!("/nix/store/{d}.drv"))) + .collect::>() + }; + let base = m(&[ + ("pkg.tests.dropped", "d1"), + ("pkg.tests.same", "s1"), + ("pkg.tests.bumped", "b0"), + ]); + let head = m(&[ + ("pkg.tests.added", "a1"), + ("pkg.tests.same", "s1"), + ("pkg.tests.bumped", "b1"), + ]); + // Every test in either side gets a row, sorted by attr — `same` included, + // which `changed_tests` drops. That row is the point: it says the test a + // selection asked about is unaffected. + let got = paired_tests(&base, &head); + let want = vec![ + ca("pkg.tests.added", None, Some("a1"), false, false), + ca("pkg.tests.bumped", Some("b0"), Some("b1"), false, false), + ca("pkg.tests.dropped", Some("d1"), None, false, false), + ca("pkg.tests.same", Some("s1"), Some("s1"), false, false), + ]; + assert_eq!(got, want); + // The diff over the same input keeps only the three that changed. + assert_eq!(changed_tests(&base, &head).len(), 3); + } + #[test] fn changed_tests_matches_diff_semantics() { let m = |kv: &[(&str, &str)]| { diff --git a/src/main.rs b/src/main.rs index e021455..5764ef5 100644 --- a/src/main.rs +++ b/src/main.rs @@ -27,7 +27,7 @@ use anyhow::{Context, Result, bail}; use clap::{CommandFactory, Parser}; use clap_complete::Shell; -use crate::model::{BuildPolicy, Profile, Rev}; +use crate::model::{BuildPolicy, Coverage, Profile, Rev}; /// The npb source tree this binary was built from, as a GitHub URL: the tag of /// the release it was cut from, so a report and the binary that produced it name @@ -79,6 +79,9 @@ struct Cli { /// Don't add passthru.tests #[arg(long)] no_tests: bool, + /// Every non-test package to build [default: all changes] + #[arg(short = 'p', long, value_name = "ATTR")] + package: Vec, /// Enable allowUnsupportedSystem in Nixpkgs config #[arg(long)] allow_unsupported: bool, @@ -830,14 +833,15 @@ enum HeadRepro { /// and the `--patch` flag), so npb does the git plumbing internally and the /// command calls no external binary. Only flags that change *what the report /// contains* are echoed (`--no-merge`, the profile's `--allow-*`, `--no-tests`, -/// the systems); `--retry` and the eval-sizing knobs don't change the changeset, -/// so they're omitted. +/// each `-p` of a selection, the systems); `--retry` and the eval-sizing +/// knobs don't change the changeset, so they're omitted. fn repro_command( base_sha: &str, head: &HeadRepro, no_merge: bool, profile: Profile, no_tests: bool, + coverage: &Coverage, systems: &[String], ) -> String { let mut flags = String::new(); @@ -856,6 +860,13 @@ fn repro_command( if no_tests { flags.push_str(" --no-tests"); } + // A selection decides *what* the report covers, so a repro that omitted it + // would reproduce a different report (DESIGN §8). + if let Some(attrs) = coverage.only() { + for a in attrs { + flags.push_str(&format!(" -p {a}")); + } + } for s in systems { flags.push_str(&format!(" -s {s}")); } @@ -904,6 +915,48 @@ fn changed_names(changed: &[evalfile::ChangedAttr]) -> (Vec, Vec ) } +/// Flatten both sides' `test_attr → (pkg_attr, drv)` maps to `test_attr → drv`, +/// dropping any test whose derivation *is* the derivation of the package it +/// hangs off — on both sides (DESIGN §6). +/// +/// Such a test is that package under a second name: no new fact, since npb keys +/// on drvpath (§2) and the package's own row already carries it, and the report +/// would only print the pair as an alias (`pkg = pkg.tests.x`). It became +/// reachable with `-p`: naming a `tests` attr directly puts a test derivation in +/// the changed set, and the `tests` phase then enumerates *its* `passthru.tests`, +/// which for the `overrideAttrs`-with-`doCheck` idiom resolve straight back to +/// itself (`x.tests.pytest.tests.pytest`). A whole-set walk can't produce that, +/// since it never descends into `passthru.tests`. +/// +/// Both sides must match, so a test that shares one side's derivation with its +/// package but not the other's is a real difference and stays — as does a test +/// present on only one side. +fn drop_self_tests( + bmap: &HashMap, + hmap: &HashMap, + changed: &[evalfile::ChangedAttr], +) -> (HashMap, HashMap) { + let parents: HashMap<&str, &evalfile::ChangedAttr> = + changed.iter().map(|c| (c.attr.as_str(), c)).collect(); + let is_alias = |test: &str| -> bool { + let Some((pkg, _)) = bmap.get(test).or_else(|| hmap.get(test)) else { + return false; + }; + let Some(parent) = parents.get(pkg.as_str()) else { + return false; + }; + let drv = |m: &HashMap| m.get(test).map(|(_, d)| d.clone()); + drv(bmap) == parent.base_drv && drv(hmap) == parent.head_drv + }; + let flat = |m: &HashMap| -> HashMap { + m.iter() + .filter(|(test, _)| !is_alias(test)) + .map(|(test, (_, drv))| (test.clone(), drv.clone())) + .collect() + }; + (flat(bmap), flat(hmap)) +} + /// Per-system state accumulated as each platform's eval lands (DESIGN §9). Its /// `Store` lives here rather than being shared by `&` because `rusqlite`'s /// connection is `!Sync` and this is touched from eval worker threads (behind the @@ -993,6 +1046,103 @@ fn reveal_system_tests( Ok(()) } +/// A selector review's changed set: resolve every `-p` attr on both trees and +/// pair the two sides into rows (DESIGN §6). +/// +/// This is the whole difference between a selection and a delta. There is no +/// whole-set eval and no diff — npb evaluates exactly the attrs it was told to +/// review, so a cold selector run costs seconds rather than the minutes two +/// ~114k-attr walks take. Resolutions are cached per `(tree, system, profile)` +/// (`store::sel_drv`), so the re-run of a report's reproduction command resolves +/// nothing and imports nothing, which is the property the eval files buy for a +/// delta review. +/// +/// Every named attr becomes a row, including the ones a diff would drop: +/// identical drvs on both sides (the change doesn't affect it — a real answer to +/// "what does this do to `git`?"), and nothing on either side (➖→➖, which is how +/// a typo'd `-p` shows itself). Rows keep the order they were named in, deduped; +/// the report sorts its own output anyway. +#[allow(clippy::too_many_arguments)] +fn selected_changed_sets( + repo: &std::path::Path, + base: &Rev, + head: &Rev, + systems: &[String], + profile: Profile, + attrs: &[String], + store: &mut store::Store, + tree: &live::Tree, + handle: live::LiveHandle<'_>, +) -> Result>> { + // Dedup, first-seen order: `-p git -p git` is one row, not two. + let mut wanted: Vec = Vec::new(); + for a in attrs { + if !wanted.contains(a) { + wanted.push(a.clone()); + } + } + + // Cache first, per side. A warm re-run finds everything here and asks the + // evaluator for nothing at all. + let sides: Vec<(&Rev, &String)> = systems + .iter() + .flat_map(|sys| [(base, sys), (head, sys)]) + .collect(); + let mut cached: Vec> = Vec::new(); + let mut requests: Vec<(Rev, String, Vec)> = Vec::new(); + for (rev, sys) in &sides { + let hits = store.resolved_attrs(&rev.tree, &profile.qualify(sys), &wanted)?; + let misses: Vec = wanted + .iter() + .filter(|a| !hits.contains_key(*a)) + .cloned() + .collect(); + requests.push(((*rev).clone(), (*sys).clone(), misses)); + cached.push(hits); + } + + // Resolve the misses (nothing to do when every side was cached), then record + // them — including the ones that resolved to nothing, so a `-p` naming an attr + // that isn't there costs one evaluation ever, not one per run. + let fresh = eval::resolve_attrs(repo, &requests, profile, tree, handle)?; + for ((rev, sys, _), rows) in requests.iter().zip(&fresh) { + store.cache_resolutions(&rev.tree, &profile.qualify(sys), rows)?; + } + + // Pair the sides back up per system, in `systems` order (two sides each, in + // the order they were pushed above). + let resolved: Vec> = (0..sides.len()) + .map(|n| { + let mut m = cached[n].clone(); + m.extend(fresh[n].iter().cloned()); + m + }) + .collect(); + + let mut out = HashMap::new(); + for (i, sys) in systems.iter().enumerate() { + let (b, h) = (&resolved[2 * i], &resolved[2 * i + 1]); + let rows: Vec = wanted + .iter() + .map(|attr| { + let get = |m: &HashMap| { + m.get(attr).cloned().unwrap_or_else(model::Resolved::absent) + }; + let (base, head) = (get(b), get(h)); + evalfile::ChangedAttr { + attr: attr.clone(), + base_drv: base.drv_path, + head_drv: head.drv_path, + base_threw: base.threw, + head_threw: head.threw, + } + }) + .collect(); + out.insert(sys.clone(), rows); + } + Ok(out) +} + /// The pre-build phases — everything that runs behind the one live progress tree /// (DESIGN §6, §9): evaluate both sides, diff to the changed set, expand /// `tests`, instantiate the `.drv`s the build will touch, and probe the cache. @@ -1007,6 +1157,7 @@ fn run_phases( profile: Profile, policy: BuildPolicy, tests: bool, + coverage: &Coverage, tree: &live::Tree, handle: live::LiveHandle<'_>, ) -> Result<(PerSystemChanged, Vec)> { @@ -1057,13 +1208,42 @@ fn run_phases( } }; - // Cold systems fire `process` as their eval lands; systems already cached - // when eval starts fire once `eval_two` has created the eval nodes (so `tests` - // still sorts below `evaluate`). The sweep then catches the fully-cached run, - // where `eval_two` creates no nodes and fires nothing at all. - eval::eval_two(repo, base, head, systems, profile, tree, handle, &process)?; - for sys in systems { - process(sys); + match coverage.only() { + // A delta review: evaluate both sides whole and diff them. Cold systems + // fire `process` as their eval lands; systems already cached when eval + // starts fire once `eval_two` has created the eval nodes (so `tests` still + // sorts below `evaluate`). The sweep then catches the fully-cached run, + // where `eval_two` creates no nodes and fires nothing at all. + None => { + eval::eval_two(repo, base, head, systems, profile, tree, handle, &process)?; + for sys in systems { + process(sys); + } + } + // A selection: resolve exactly the named attrs on both trees — no + // whole-set eval, no diff (DESIGN §6) — then reveal each system's `tests` + // the way the delta path does as its eval lands. + Some(attrs) => { + let mut acc = accum.lock().unwrap(); + let changed = selected_changed_sets( + repo, + base, + head, + systems, + profile, + attrs, + &mut acc.store, + tree, + handle, + )?; + for sys in systems { + let rows = changed.get(sys).cloned().unwrap_or_default(); + if tests { + reveal_system_tests(&mut acc, tree, systems, base, head, profile, sys, &rows)?; + } + acc.changed.insert(sys.clone(), rows); + } + } } // Assemble the diffs in system order; surface any callback error. @@ -1094,7 +1274,16 @@ fn run_phases( let key = profile.qualify(sys); let bmap = acc.store.tests_drvs_for(&base.tree, &key, &base_names)?; let hmap = acc.store.tests_drvs_for(&head.tree, &key, &head_names)?; - changed.extend(evalfile::changed_tests(&bmap, &hmap)); + let (bmap, hmap) = drop_self_tests(&bmap, &hmap, changed); + // A delta reports changes, so an unchanged test isn't one. A + // selection reports the attrs it was given, and a named package's + // tests are given with it — "no regression here" is the answer it was + // asked for (DESIGN §6). + changed.extend(if coverage.only().is_some() { + evalfile::paired_tests(&bmap, &hmap) + } else { + evalfile::changed_tests(&bmap, &hmap) + }); } } @@ -1207,6 +1396,14 @@ fn run(cli: Cli) -> Result<()> { insecure: cli.allow_insecure, }; let policy = BuildPolicy { retry: cli.retry }; + // What this review covers (DESIGN §6): the whole changed set, or exactly the + // attrs `-p` named. The report's reproduction command echoes the flag + // (DESIGN §8), like every other report-shaping flag. + let coverage = if cli.package.is_empty() { + Coverage::All + } else { + Coverage::Only(cli.package) + }; let repo = resolve_repo(cli.path)?; let systems = resolve_systems(cli.system); @@ -1312,7 +1509,7 @@ fn run(cli: Cli) -> Result<()> { ensure_distinct_trees(&base, &head)?; let (per_system_changed, targets) = run_phases( - &repo, &base, &head, &systems, profile, policy, tests, &tree, handle, + &repo, &base, &head, &systems, profile, policy, tests, &coverage, &tree, handle, )?; Ok(( base, @@ -1445,6 +1642,7 @@ fn run(cli: Cli) -> Result<()> { cli.no_merge, profile, !tests, + &coverage, &systems, ); print!( @@ -1952,6 +2150,7 @@ mod tests { false, strict, false, + &Coverage::All, &["x86_64-linux".into()], ); assert_eq!(cmd, "npb --base aaa --head bbb -s x86_64-linux"); @@ -1962,6 +2161,7 @@ mod tests { true, all, true, + &Coverage::All, &["a".into(), "b".into()], ); assert_eq!( @@ -1980,6 +2180,7 @@ mod tests { false, strict, false, + &Coverage::All, &["sys".into()], ); assert_eq!(cmd, "npb --base m1 --head fork --patch fork...m2 -s sys"); @@ -1994,12 +2195,73 @@ mod tests { false, strict, false, + &Coverage::All, &["sys".into()], ); assert_eq!( cmd, "npb --base b --head h --patch /dev/stdin -s sys <<'PATCH'\n--- a\n+++ b\nPATCH" ); + + // A selection decides what the report covers, so it's echoed — one token + // per attr, in the order they were named. + let cmd = repro_command( + "aaa", + &HeadRepro::Commit("bbb".into()), + false, + strict, + false, + &Coverage::Only(vec!["git".into(), "hello".into()]), + &["sys".into()], + ); + assert_eq!(cmd, "npb --base aaa --head bbb -p git -p hello -s sys"); + } + + #[test] + fn drop_self_tests_removes_a_test_that_is_its_own_package() { + let m = |rows: &[(&str, &str, &str)]| { + rows.iter() + .map(|(t, p, d)| (t.to_string(), (p.to_string(), d.to_string()))) + .collect::>() + }; + // The `-p ` case: the named row is a test derivation, and + // the `tests` phase enumerates its own `passthru.tests` right back to it. + let changed = vec![ca( + "p.tests.pytest", + Some("/d/b"), + Some("/d/h"), + false, + false, + )]; + let bmap = m(&[ + ("p.tests.pytest.tests.pytest", "p.tests.pytest", "/d/b"), + ("p.tests.pytest.tests.other", "p.tests.pytest", "/d/other-b"), + ]); + let hmap = m(&[ + ("p.tests.pytest.tests.pytest", "p.tests.pytest", "/d/h"), + ("p.tests.pytest.tests.other", "p.tests.pytest", "/d/other-h"), + ]); + let (b, h) = drop_self_tests(&bmap, &hmap, &changed); + // The self-alias goes; a genuinely different test of the same attr stays. + let keys = |m: &HashMap| m.keys().cloned().collect::>(); + assert_eq!(keys(&b), ["p.tests.pytest.tests.other"]); + assert_eq!(keys(&h), ["p.tests.pytest.tests.other"]); + + // Matching on only one side is a real difference, so it stays. + let one_sided = m(&[("p.tests.pytest.tests.pytest", "p.tests.pytest", "/d/b")]); + let (b, _) = drop_self_tests(&one_sided, &HashMap::new(), &changed); + assert!(b.contains_key("p.tests.pytest.tests.pytest")); + + // An ordinary package's tests are untouched, even when another changed + // attr happens to share the derivation (that's an alias worth printing). + let changed = vec![ + ca("pkg", Some("/d/p0"), Some("/d/p1"), false, false), + ca("other", Some("/d/t0"), Some("/d/t1"), false, false), + ]; + let bmap = m(&[("pkg.tests.x", "pkg", "/d/t0")]); + let hmap = m(&[("pkg.tests.x", "pkg", "/d/t1")]); + let (b, h) = drop_self_tests(&bmap, &hmap, &changed); + assert!(b.contains_key("pkg.tests.x") && h.contains_key("pkg.tests.x")); } #[test] diff --git a/src/model.rs b/src/model.rs index 7871170..50aa006 100644 --- a/src/model.rs +++ b/src/model.rs @@ -100,6 +100,91 @@ impl Profile { } } +/// What a review covers: the whole changed set, or exactly the attrs +/// `-p`/`--package` named (DESIGN §6). +/// +/// A *delta* review asks "what did this change break?", and derives its attrs +/// from the diff of two whole-set evals. A *selection* asks "what does this +/// change do to _these_ attrs?", so npb evaluates just those on both trees and +/// diffs them. Its value is **reach**: a whole-set walk only descends into +/// attrsets marked `recurseForDerivations`, so plenty of real attrs — a +/// `python311Packages.foo`, a `.tests.` — are in no changed set and +/// can be reviewed no other way. +/// +/// A selection needs no whole-set eval at all, which is the other half of the +/// point: on a cold cache it turns minutes of enumerating ~114k attrs into +/// seconds of evaluating the handful you named. What it does *not* give up is +/// the warm re-run — those resolutions are cached like any other pure eval fact +/// (`store::sel_drv`), so re-running a report's reproduction command stays +/// near-instant, with no nixpkgs import at all. +/// +/// It replaces exactly one step, and nothing downstream. npb's pipeline is: pick +/// the attrs to review, add their `passthru.tests` unless `--no-tests`, build, +/// report. `-p` replaces the *first* step — the whole-set diff that would +/// otherwise pick them — the way `--head` replaces the working-tree guess. So a +/// named package's tests come along exactly as a changed package's would, and +/// every named attr is reported whether or not the change touches it: "nothing +/// here regressed" is the answer the flag asked for, and it is the one a diff +/// cannot give. That makes `⏩→⏩` and `➖→➖` reachable in a selection (§8), the +/// latter being how a misspelled attr shows itself. +/// +/// **Attrs are matched exactly**, with no subtrees or globs, because nothing in +/// npb's Rust reads structure out of an attr path — attrs are opaque keys here, +/// and where structure is needed it comes from the evaluator, as `nix-eval-jobs`' +/// pre-split `attrPath` ([`TestJob`]) or a `lib.attrByPath (lib.splitString ".")` +/// inside a generated expression. The string a matcher would have to parse is +/// ambiguous anyway: the eval file keeps `nix-eval-jobs`' quoting, so a dotted +/// *name* (`rubyPackages."http_parser.rb"`) and a dotted *path* look alike. +/// +/// Leaving an attr out removes a *target*, never a dependency: an unnamed package +/// that some named target needs is still built by nix as part of that target's +/// closure, and the observation it produces is keyed on its drvpath like any +/// other (DESIGN §2), so it lands in the log all the same. A later run without +/// the flag finds it already decided. Narrowing a review narrows a report; it +/// doesn't discard knowledge. +#[derive(Debug, Clone, Default, PartialEq, Eq)] +pub enum Coverage { + /// The whole changed set: diff two whole-set evals. + #[default] + All, + /// Exactly these attrs, resolved on both trees (`-p`). Never empty. + Only(Vec), +} + +impl Coverage { + /// The attrs a selection named, or `None` for a delta review. This is the + /// one question that changes which phases run at all (DESIGN §6). + pub fn only(&self) -> Option<&[String]> { + match self { + Coverage::Only(attrs) => Some(attrs), + Coverage::All => None, + } + } +} + +/// What one attr named with `-p` evaluates to at one `(tree, system, profile)`. +/// +/// A pure fact, cached in `store::sel_drv` — which is what keeps a selector +/// review's re-run instant without an eval file (DESIGN §4). It carries the same +/// trichotomy the eval-file format does: a drv; no drv but `threw`, meaning it is +/// broken/unsupported/insecure under the profile or forces something that is +/// (⏩); or no drv and not `threw`, meaning the attr path isn't there at all (➖). +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct Resolved { + pub drv_path: Option, + pub threw: bool, +} + +impl Resolved { + /// The `➖` case: nothing at that attr path on this tree. + pub fn absent() -> Self { + Self { + drv_path: None, + threw: false, + } + } +} + /// Result of evaluating one attribute on one platform at one commit under a /// given [`Profile`]. /// @@ -322,4 +407,31 @@ mod tests { assert_eq!(strict.qualify("x86_64-linux"), "---/x86_64-linux"); assert_eq!(all.qualify("aarch64-darwin"), "ubi/aarch64-darwin"); } + + #[test] + fn coverage_defaults_to_the_whole_delta() { + // The default covers the whole changed set and names nothing. + assert_eq!(Coverage::default(), Coverage::All); + assert!(Coverage::All.only().is_none()); + + // A selection carries exactly the attrs it was given, in order. + let only = Coverage::Only(vec!["git".into(), "python313Packages.requests".into()]); + assert_eq!( + only.only(), + Some(&["git".to_string(), "python313Packages.requests".to_string()][..]) + ); + } + + #[test] + fn resolved_distinguishes_absent_from_threw() { + // The trichotomy the report needs: a drv, ⏩ threw, ➖ absent. + let absent = Resolved::absent(); + assert_eq!(absent.drv_path, None); + assert!(!absent.threw); + let threw = Resolved { + drv_path: None, + threw: true, + }; + assert_ne!(absent, threw); + } } diff --git a/src/schema.sql b/src/schema.sql index 820a18e..9cb2f73 100644 --- a/src/schema.sql +++ b/src/schema.sql @@ -81,6 +81,32 @@ CREATE TABLE IF NOT EXISTS test_drv ( PRIMARY KEY (key_id, pkg_attr, test_attr) ) STRICT, WITHOUT ROWID; +-- The selector-resolution cache (DESIGN.md §4, §6): what an attr named with `-p` +-- evaluates to at one `(tree, system)` key. A selector run evaluates only the +-- attrs it was given, so it writes no eval file — and this is what keeps its +-- *re-run* near-instant all the same, the property the eval files provide for a +-- whole-set review. Sound for the same reason they are: a resolution is a pure +-- function of `(tree, system, profile)`, and the profile rides in the +-- `eval_key.system` value. +-- +-- One row per attr *asked for*, holding the trichotomy the eval-file format +-- carries too: a drv (stored stripped, `evalfile::strip_drv`); no drv with +-- `threw` set, meaning it evaluated to an error under this profile (⏩); or no +-- drv with `threw` clear, meaning the attr path isn't there at all (➖). So a +-- *present row* means resolved, and an absent row means not yet asked — never +-- "absent attr", which is a cached fact of its own. +-- +-- Unlike `test_pkg`, this needs no completeness marker: every read is a lookup by +-- exact attr, so nothing ever asks for "all attrs at this key" and a partial +-- table is simply a partial cache rather than a truncated fact. +CREATE TABLE IF NOT EXISTS sel_drv ( + key_id INTEGER NOT NULL REFERENCES eval_key (id), + attr TEXT NOT NULL, + drv_path TEXT, + threw INTEGER NOT NULL, + PRIMARY KEY (key_id, attr) +) STRICT, WITHOUT ROWID; + -- The patch-tree cache (DESIGN.md §8): maps a `--patch ` compare — its -- anchor commit and sha-pinned expression — to the head *tree* npb reconstructed -- by applying that compare's diff onto the anchor. It lets a *reproduction* diff --git a/src/store.rs b/src/store.rs index 2315c0e..15c774a 100644 --- a/src/store.rs +++ b/src/store.rs @@ -10,7 +10,7 @@ use anyhow::{Context, Result}; use rusqlite::{Connection, OptionalExtension, params}; use crate::evalfile::{restore_drv, strip_drv}; -use crate::model::{Observation, Outcome, TestJob}; +use crate::model::{Observation, Outcome, Resolved, TestJob}; // npb is public and writes no migration code (DESIGN.md §1, CLAUDE.md): this // schema evolves only *additively* — a new `CREATE TABLE IF NOT EXISTS`, or a @@ -329,15 +329,17 @@ impl Store { Ok(()) } - /// All cached test drvs for `pkgs` at this key, as `test_attr → drv_path` - /// (only tests that resolved to a derivation), with drv paths restored to - /// their full `/nix/store/…​.drv` form. One query for the whole set. + /// All cached test drvs for `pkgs` at this key, as + /// `test_attr → (pkg_attr, drv_path)` (only tests that resolved to a + /// derivation), with drv paths restored to their full `/nix/store/…​.drv` + /// form. One query for the whole set. The package a test hangs off rides + /// along so the caller can spot a test that *is* its package (`run_phases`). pub fn tests_drvs_for( &self, tree: &str, system: &str, pkgs: &[String], - ) -> Result> { + ) -> Result> { let mut out = std::collections::HashMap::new(); let Some(key_id) = self.key_id(tree, system)? else { return Ok(out); @@ -347,7 +349,7 @@ impl Store { for chunk in pkgs.chunks(IN_CHUNK) { let placeholders = placeholders(chunk.len()); let sql = format!( - "SELECT test_attr, drv_path FROM test_drv \ + "SELECT test_attr, pkg_attr, drv_path FROM test_drv \ WHERE key_id = ?1 AND pkg_attr IN ({placeholders})", ); let mut stmt = self.conn.prepare(&sql)?; @@ -355,35 +357,124 @@ impl Store { std::iter::once(key_id.to_string()).chain(chunk.iter().cloned()), ); let rows = stmt.query_map(params, |r| { - Ok((r.get::<_, String>(0)?, r.get::<_, String>(1)?)) + Ok(( + r.get::<_, String>(0)?, + r.get::<_, String>(1)?, + r.get::<_, String>(2)?, + )) })?; for row in rows { - let (test_attr, stored) = row?; + let (test_attr, pkg_attr, stored) = row?; let drv = restore_drv(Some(&stored)).expect("Some maps to Some"); - out.insert(test_attr, drv); + out.insert(test_attr, (pkg_attr, drv)); + } + } + Ok(out) + } + + // --- the selector-resolution cache (DESIGN.md §4, §6) ------------------ + + /// The cached resolutions for `attrs` at this key, as `attr → resolution`. + /// A *missing* entry means the attr has never been resolved here; an entry + /// present with no drv is itself a cached fact — it threw (⏩) or the attr + /// path isn't there (➖). Drv paths come back in full `/nix/store/…​.drv` form. + pub fn resolved_attrs( + &self, + tree: &str, + system: &str, + attrs: &[String], + ) -> Result> { + let mut out = std::collections::HashMap::new(); + let Some(key_id) = self.key_id(tree, system)? else { + return Ok(out); // key never recorded ⇒ nothing cached + }; + // Chunked under SQLite's bound-parameter cap (see `IN_CHUNK`). + for chunk in attrs.chunks(IN_CHUNK) { + let placeholders = placeholders(chunk.len()); + let sql = format!( + "SELECT attr, drv_path, threw FROM sel_drv \ + WHERE key_id = ?1 AND attr IN ({placeholders})", + ); + let mut stmt = self.conn.prepare(&sql)?; + let params = rusqlite::params_from_iter( + std::iter::once(key_id.to_string()).chain(chunk.iter().cloned()), + ); + let rows = stmt.query_map(params, |r| { + Ok(( + r.get::<_, String>(0)?, + r.get::<_, Option>(1)?, + r.get::<_, i64>(2)?, + )) + })?; + for row in rows { + let (attr, stored, threw) = row?; + out.insert( + attr, + Resolved { + drv_path: restore_drv(stored.as_deref()), + threw: threw != 0, + }, + ); } } Ok(out) } + /// Record a completed resolution of `resolutions` at this key, in one + /// transaction — including the ones that resolved to nothing, which are facts + /// worth caching too (a typo'd attr shouldn't cost an eval every run). + /// Idempotent (`INSERT OR REPLACE`). + pub fn cache_resolutions( + &mut self, + tree: &str, + system: &str, + resolutions: &[(String, Resolved)], + ) -> Result<()> { + if resolutions.is_empty() { + return Ok(()); + } + let tx = self.conn.transaction()?; + let key_id = Self::key_id_get_or_create(&tx, tree, system)?; + for (attr, r) in resolutions { + tx.execute( + "INSERT OR REPLACE INTO sel_drv (key_id, attr, drv_path, threw) \ + VALUES (?1, ?2, ?3, ?4)", + params![ + key_id, + attr, + r.drv_path.as_deref().map(strip_drv), + i64::from(r.threw) + ], + )?; + } + tx.commit()?; + Ok(()) + } + /// Drop the caches for one `(tree, system)` — its `eval_key` row and the - /// `test_pkg`/`test_drv` rows that reference it — when its eval file is - /// evicted (`--clean`, DESIGN.md §4). Returns the number of `test_drv` rows + /// `test_pkg`/`test_drv`/`sel_drv` rows that reference it — when its eval file + /// is evicted (`--clean`, DESIGN.md §4). Returns the number of `test_drv` rows /// removed (the bulk); a no-op if the key was never recorded. The caller /// [`Store::vacuum`]s once after a batch of these to return the pages. - pub fn purge_tests(&mut self, tree: &str, system: &str) -> Result { + /// + /// Note what this *doesn't* reach: a selector run (`-p`) writes no eval file, + /// so its key has nothing for `--clean` to evict and its rows live on. That's + /// deliberate — `--clean`'s budget is over the eval-file corpus, and a + /// selector key isn't part of it (DESIGN §4) — not an oversight. + pub fn purge_key(&mut self, tree: &str, system: &str) -> Result { let Some(key_id) = self.key_id(tree, system)? else { return Ok(0); }; let tx = self.conn.transaction()?; let drvs = tx.execute("DELETE FROM test_drv WHERE key_id = ?1", [key_id])?; tx.execute("DELETE FROM test_pkg WHERE key_id = ?1", [key_id])?; + tx.execute("DELETE FROM sel_drv WHERE key_id = ?1", [key_id])?; tx.execute("DELETE FROM eval_key WHERE id = ?1", [key_id])?; tx.commit()?; Ok(drvs) } - /// Rebuild the database file to reclaim the pages freed by [`Store::purge_tests`] + /// Rebuild the database file to reclaim the pages freed by [`Store::purge_key`] /// (a `DELETE` only moves them to the freelist). Run once after an eviction batch. pub fn vacuum(&self) -> Result<()> { self.conn.execute_batch("VACUUM").context("vacuuming")?; @@ -584,16 +675,17 @@ mod tests { .unwrap(); assert!(done.contains("hello") && done.contains("ripgrep") && !done.contains("curl")); - // hello resolves to its two drv'd tests (the errored one is not stored). + // hello resolves to its two drv'd tests (the errored one is not stored), + // each paired with the package it hangs off. let hd = s.tests_drvs_for(c, sys, &pkgs(&["hello"])).unwrap(); assert_eq!(hd.len(), 2); assert_eq!( hd.get("hello.tests.run"), - Some(&"/nix/store/a.drv".to_string()) + Some(&("hello".to_string(), "/nix/store/a.drv".to_string())) ); assert_eq!( hd.get("hello.tests.version"), - Some(&"/nix/store/b.drv".to_string()) + Some(&("hello".to_string(), "/nix/store/b.drv".to_string())) ); // ripgrep is cached-done but has no test drvs. assert!( @@ -612,7 +704,62 @@ mod tests { } #[test] - fn purge_tests_drops_one_key_only() { + fn resolutions_round_trip_the_trichotomy() { + let dir = std::env::temp_dir().join(format!("npb-sel-{}", std::process::id())); + let _ = fs::remove_dir_all(&dir); + let mut s = Store::open(&dir.join("npb.sqlite")).unwrap(); + let sys = "---/aarch64-linux"; + let want = |attr: &str, drv: Option<&str>, threw: bool| { + ( + attr.to_string(), + Resolved { + drv_path: drv.map(str::to_string), + threw, + }, + ) + }; + let rows = vec![ + want("git", Some("/nix/store/aaa-git.drv"), false), + // Present but throws under this profile (⏩)... + want("broken", None, true), + // ...versus no such attr path at all (➖). Both are cached facts, so + // neither costs an evaluation again. + want("typo", None, false), + ]; + s.cache_resolutions("treeA", sys, &rows).unwrap(); + + let asked: Vec = ["git", "broken", "typo", "never-asked"] + .iter() + .map(|a| a.to_string()) + .collect(); + let got = s.resolved_attrs("treeA", sys, &asked).unwrap(); + // Stored drvs come back with their `/nix/store/….drv` affixes restored. + assert_eq!( + got.get("git").unwrap().drv_path.as_deref(), + Some("/nix/store/aaa-git.drv") + ); + assert_eq!(got.get("broken").unwrap(), &rows[1].1); + assert_eq!(got.get("typo").unwrap(), &rows[2].1); + // An attr never resolved has no entry — distinct from one resolved to + // nothing, which is why a missing row can't mean "absent". + assert!(!got.contains_key("never-asked")); + + // Keys don't leak across trees or profile-qualified systems. + assert!(s.resolved_attrs("treeB", sys, &asked).unwrap().is_empty()); + assert!( + s.resolved_attrs("treeA", "ubi/aarch64-linux", &asked) + .unwrap() + .is_empty() + ); + + // Evicting the key takes the resolutions with it (`--clean` lockstep). + s.purge_key("treeA", sys).unwrap(); + assert!(s.resolved_attrs("treeA", sys, &asked).unwrap().is_empty()); + let _ = fs::remove_dir_all(&dir); + } + + #[test] + fn purge_key_drops_one_key_only() { let dir = std::env::temp_dir().join(format!("npb-purge-{}", std::process::id())); let _ = fs::remove_dir_all(&dir); let mut s = Store::open(&dir.join("npb.sqlite")).unwrap(); @@ -641,7 +788,7 @@ mod tests { .unwrap(); // Evicting treeA removes exactly its rows (1 test_drv) and leaves treeB. - assert_eq!(s.purge_tests("treeA", sys).unwrap(), 1); + assert_eq!(s.purge_key("treeA", sys).unwrap(), 1); assert!( s.tests_cached_pkgs("treeA", sys, &pkgs(&["hello"])) .unwrap() @@ -665,7 +812,7 @@ mod tests { ); // Purging an unknown key is a no-op, and VACUUM after a batch is fine. - assert_eq!(s.purge_tests("treeA", sys).unwrap(), 0); + assert_eq!(s.purge_key("treeA", sys).unwrap(), 0); s.vacuum().unwrap(); let _ = fs::remove_dir_all(&dir);