diff --git a/cargo-soothfast/src/gate.rs b/cargo-soothfast/src/gate.rs index 703facb..32114ec 100644 --- a/cargo-soothfast/src/gate.rs +++ b/cargo-soothfast/src/gate.rs @@ -67,6 +67,7 @@ struct GateArgs { against_ref: Option, deps: bool, allow_gone: bool, + allow_harness_change: bool, matrix: String, save_baseline: Option, reuse_base: bool, @@ -87,6 +88,7 @@ pub fn run(args: &[String]) -> i32 { against_ref: None, deps: false, allow_gone: false, + allow_harness_change: false, matrix: "default".into(), save_baseline: None, reuse_base: true, @@ -120,6 +122,7 @@ pub fn run(args: &[String]) -> i32 { "--no-reuse-base" => g.reuse_base = false, "--deps" => g.deps = true, "--allow-gone" => g.allow_gone = true, + "--allow-harness-change" => g.allow_harness_change = true, other => return err(&format!("unknown gate arg {other:?}")), } } @@ -128,11 +131,16 @@ pub fn run(args: &[String]) -> i32 { return err(&e); } - let mut failures = 0u32; + let mut regressions = 0u32; let mut failing_ids: Vec = Vec::new(); - let (reference, current, short_circuited) = match resolve(&g) { - Ok(Some(triple)) => triple, + let Resolved { + reference, + current, + short_circuited, + harness, + } = match resolve(&g) { + Ok(Some(resolved)) => resolved, // Newly measured crate: nothing at the ref to regress against. // Passing is the honest answer; failing would make a crate's // first bench permanently ungateable until it is already merged. @@ -154,6 +162,7 @@ pub fn run(args: &[String]) -> i32 { if let Some(b) = ¤t.gating_backend { println!("gate: gating backend = {b}"); } + harness_note(&harness); let root = invoke::workspace_root().ok(); let accepted = root .as_deref() @@ -169,7 +178,7 @@ pub fn run(args: &[String]) -> i32 { accepted, headroom_pct: None, }; - failures += compare( + regressions += compare( &reference, ¤t, "", @@ -189,7 +198,7 @@ pub fn run(args: &[String]) -> i32 { deps_mode: false, ..ctx.clone() }; - failures += compare( + regressions += compare( &r, ¤t, "RATCHET ", @@ -204,16 +213,31 @@ pub fn run(args: &[String]) -> i32 { } } - // Checked claims from the runner. + // Checked claims from the runner. They hold HEAD against its own + // declared numbers, so the reference harness has no say over them and + // --allow-harness-change never reaches them. + let mut assertion_failures = 0u32; for a in ¤t.assertions { let verdict = if a.ok { "ok" } else { "FAIL" }; if !a.ok { - failures += 1; + assertion_failures += 1; failing_ids.push(a.id.clone()); } println!("{verdict:<5} {} assert {}: {}", a.id, a.kind, a.detail); } + if harness_waiver(&harness, g.allow_harness_change, regressions) { + println!( + "gate: {regressions} regression(s) allowed by --allow-harness-change ({})", + harness.unpinned().join(", ") + ); + regressions = 0; + } + let failures = regressions + assertion_failures; + if let Some(r) = &root { + triage_harness_note(&r.join(".soothfast").join("triage"), &harness); + } + write_gate_status(failures); if let Some(name) = &g.save_baseline { match save_verdict(failures, short_circuited) { @@ -241,14 +265,88 @@ pub fn run(args: &[String]) -> i32 { // buildcost pseudo-items have no runnable body to profile. failing_ids.retain(|id| !id.starts_with("buildcost::")); triage(&g.common, &failing_ids); - println!("gate: FAILED ({failures} regression(s))"); + println!( + "gate: FAILED ({failures} regression(s)){}", + harness_verdict(&harness, regressions > 0) + ); 1 } else { - println!("gate: passed ({} item(s))", current.items.len()); + println!( + "gate: passed ({} item(s)){}", + current.items.len(), + harness_verdict(&harness, false) + ); 0 } } +/// Put the mismatch in front of the deltas it may have produced: a +/// reference built against another harness measures another protocol, so +/// its numbers are not this change's in either direction. +fn harness_note(harness: &invoke::HarnessSync) { + if harness.is_mismatched() { + println!( + "gate: HARNESS MISMATCH — the merge-base could not be pinned to HEAD's soothfast ({}); \ + deltas below may be the harness, not this change", + harness.unpinned().join(", ") + ); + } +} + +/// What the verdict line adds about the mismatch. The PR comment carries +/// only the tail of the output, so the header alone would not reach it. +fn harness_verdict(harness: &invoke::HarnessSync, regressed: bool) -> String { + if !harness.is_mismatched() { + return String::new(); + } + let unpinned = harness.unpinned().join(", "); + if regressed { + format!( + " — measured against a different harness ({unpinned}); \ + pass --allow-harness-change if the harness bump explains it" + ) + } else { + format!( + " — but measured against a different harness ({unpinned}), \ + so the comparison is not conclusive" + ) + } +} + +/// Whether a regression measured against an unpinned harness is waived. +/// Only comparison failures qualify: an assertion holds HEAD against its +/// own declared numbers, which the reference harness has no say over. +fn harness_waiver(harness: &invoke::HarnessSync, allow: bool, regressions: u32) -> bool { + regressions > 0 && harness.is_mismatched() && allow +} + +/// Leave the mismatch beside the triage reports in `dir`, so a downloaded +/// artifact carries the reason its numbers may not be the code's. Removed +/// when the pin held: a stale file would mislabel a valid comparison. +fn triage_harness_note(dir: &std::path::Path, harness: &invoke::HarnessSync) { + let path = dir.join("harness-mismatch.txt"); + if !harness.is_mismatched() { + let _ = std::fs::remove_file(&path); + return; + } + let _ = std::fs::create_dir_all(dir); + let unpinned: String = harness + .unpinned() + .iter() + .map(|u| format!(" {u}\n")) + .collect(); + let _ = std::fs::write( + &path, + format!( + "harness mismatch\n\n\ + The merge-base worktree could not be pinned to HEAD's soothfast versions, so the\n\ + reference bench embeds a different measurement harness:\n\n\ + {unpinned}\n\ + Deltas in this run may be that harness change rather than the measured code.\n" + ), + ); +} + /// Gate each package in turn, after building every bench target together. fn run_each(args: &[String], packages: &[String]) -> i32 { let mut common = CommonArgs::default(); @@ -288,6 +386,8 @@ fn prebuild_both_sides(packages: &[String], common: &CommonArgs, args: &[String] }; let _ = invoke::with_merge_base_worktree(&refname, |wt| { invoke::sync_untracked_cargo_config(wt)?; + // Warming a build cache decides no verdict. The per-package leg + // syncs this worktree again and its outcome is what gets reported. invoke::sync_harness_versions(wt)?; let _ = invoke::prebuild_benches(packages, common, Some(wt), Some(&target)); Ok(()) @@ -334,11 +434,21 @@ fn with_package(args: &[String], pkg: &str) -> Vec { out } -/// Resolve the comparison pair for a gate run: reference doc, current run, -/// and whether the current run was a short-circuited identical-binaries -/// pass. `Ok(None)` only in `--against-ref` mode: a crate newly measured has -/// nothing at the ref to compare against. -fn resolve(g: &GateArgs) -> Result, String> { +/// The two sides of one comparison, plus what the reader needs to know +/// about how much the comparison is worth. +struct Resolved { + reference: Value, + current: Run, + /// The identical-binaries short circuit ran one timing-only pass, so + /// `current` has no gating counters and is not baseline material. + short_circuited: bool, + harness: invoke::HarnessSync, +} + +/// Measure or load both sides of a gate run as a [`Resolved`]. `Ok(None)` +/// only in `--against-ref` mode: a crate newly measured has nothing at the +/// ref to compare against. +fn resolve(g: &GateArgs) -> Result, String> { if g.common.backend.as_deref() == Some("buildcost") { let Some(pkg) = g.common.pkg.clone() else { return Err("--backend buildcost requires -p PKG".into()); @@ -364,13 +474,15 @@ fn resolve(g: &GateArgs) -> Result, String> { } baseline }; - return Ok(Some((reference, current, false))); + return Ok(Some(Resolved { + reference, + current, + short_circuited: false, + harness: invoke::HarnessSync::Matched, + })); } if let Some(refname) = &g.against_ref { - return match measure_ref_interleaved(&g.common, refname, g.reuse_base)? { - Some(pair) => Ok(Some((pair.reference, pair.current, pair.short_circuited))), - None => Ok(None), - }; + return measure_ref_interleaved(&g.common, refname, g.reuse_base); } let records = invoke::run_bench(&g.common, &[]).map_err(|e| e.to_string())?; let mut current = invoke::collect(&records); @@ -386,7 +498,12 @@ fn resolve(g: &GateArgs) -> Result, String> { g.baseline, g.baseline ) })?; - Ok(Some((baseline, current, false))) + Ok(Some(Resolved { + reference: baseline, + current, + short_circuited: false, + harness: invoke::HarnessSync::Matched, + })) } struct AcceptArgs { @@ -465,18 +582,25 @@ fn accept_cmd(args: &[String]) -> i32 { against_ref: a.against_ref, deps: false, allow_gone: false, + allow_harness_change: false, matrix: a.matrix, save_baseline: None, reuse_base: true, }; - let (reference, current, _) = match resolve(&g) { - Ok(Some(triple)) => triple, + let Resolved { + reference, + current, + harness, + .. + } = match resolve(&g) { + Ok(Some(resolved)) => resolved, Ok(None) => { println!("gate accept: nothing to compare — nothing to accept"); return 0; } Err(e) => return err(&e), }; + harness_note(&harness); if current.items.is_empty() { return err("measured 0 items — check --filter / registry setup; refusing to accept"); } @@ -632,15 +756,6 @@ fn write_gate_status(failures: u32) { ); } -/// A measured (reference, head) pair from the merge-base worktree. -struct RefPair { - reference: Value, - current: Run, - /// The identical-binaries short circuit ran one timing-only pass, so - /// `current` has no gating counters and is not baseline material. - short_circuited: bool, -} - /// Measure the merge-base with HEAD in a temp worktree, interleaving rounds /// (head, base, head, base) tango-style so slow environmental drift cancels; /// per-metric minima across each side's rounds form the comparison values. @@ -655,7 +770,7 @@ fn measure_ref_interleaved( common: &CommonArgs, refname: &str, reuse: bool, -) -> Result, String> { +) -> Result, String> { const TIMING_ONLY: &[&str] = &["--skip-gating-counters"]; enum Ref { NoBenchTarget, @@ -703,7 +818,8 @@ fn measure_ref_interleaved( None => measure(None, &[])?, }; cache_head(common, &head_stamp, &head, head_dig.as_deref()); - return Ok(Some(RefPair { + return Ok(Some(Resolved { + harness: harness_of(&doc), reference: doc, current: head, short_circuited: false, @@ -719,7 +835,7 @@ fn measure_ref_interleaved( if let Some(pkg) = &pkg && !workspace::has_bench_target(pkg, &target, Some(wt)).map_err(|e| e.to_string())? { - return Ok((Ref::NoBenchTarget, None)); + return Ok((Ref::NoBenchTarget, None, invoke::HarnessSync::Matched)); } // A local .cargo/config.toml (untracked, e.g. a linker pin) wouldn't // otherwise reach the worktree and would read as a false mismatch. @@ -728,7 +844,7 @@ fn measure_ref_interleaved( // measurement-protocol change between the two locked versions would // otherwise be reported as a regression in the measured project. // Pinning must precede the base build the binary comparison does. - invoke::sync_harness_versions(wt)?; + let harness = invoke::sync_harness_versions(wt)?; let digests = bench_text_digests(common, wt, wt_target.as_deref()); // A run measured from byte-identical machine code under the same // conditions is that binary's measurement, whatever commit built it. @@ -749,11 +865,15 @@ fn measure_ref_interleaved( cache_head_doc(common, &head_stamp, doc, head_dig.as_deref()); } let args = identical_pass_args(common.backend.as_deref()); - return Ok((Ref::IdenticalBinaries(measure(None, args)), digests)); + return Ok(( + Ref::IdenticalBinaries(measure(None, args)), + digests, + harness, + )); } if let Some(doc) = by_binary { println!("gate: reusing a run measured from the same merge-base binary"); - return Ok((Ref::Cached(doc), digests)); + return Ok((Ref::Cached(doc), digests, harness)); } Ok(( Ref::Rounds(Box::new([ @@ -763,9 +883,10 @@ fn measure_ref_interleaved( measure(Some(wt), TIMING_ONLY), ])), digests, + harness, )) })?; - let (outcome, digests) = outcome; + let (outcome, digests, harness) = outcome; let (base, head) = match outcome { Ref::NoBenchTarget => return Ok(None), @@ -773,10 +894,11 @@ fn measure_ref_interleaved( // zero deltas by construction. Ref::IdenticalBinaries(run) => { let head = run?; - return Ok(Some(RefPair { + return Ok(Some(Resolved { reference: ref_doc(&head), current: head, short_circuited: true, + harness, })); } Ref::Cached(doc) => { @@ -785,10 +907,11 @@ fn measure_ref_interleaved( None => measure(None, &[])?, }; cache_head(common, &head_stamp, &head, head_dig.as_deref()); - return Ok(Some(RefPair { + return Ok(Some(Resolved { reference: doc, current: head, short_circuited: false, + harness, })); } Ref::Rounds(rounds) => { @@ -796,7 +919,8 @@ fn measure_ref_interleaved( (combine_rounds(base1?, base2), combine_rounds(head1?, head2)) } }; - let reference = ref_doc(&base); + let mut reference = ref_doc(&base); + mark_harness(&mut reference, &harness); if let Some(k) = &cache_key { runcache::store(k, &reference); } @@ -810,13 +934,37 @@ fn measure_ref_interleaved( ); } cache_head(common, &head_stamp, &head, head_dig.as_deref()); - Ok(Some(RefPair { + Ok(Some(Resolved { reference, current: head, short_circuited: false, + harness, })) } +/// Where a stored reference remembers that its harness could not be pinned, +/// so a run served from the cache carries the mark of the run that measured +/// it rather than reading as a clean comparison. +const HARNESS_KEY: &str = "harness_unpinned"; + +fn mark_harness(doc: &mut Value, harness: &invoke::HarnessSync) { + if harness.is_mismatched() { + doc[HARNESS_KEY] = serde_json::json!(harness.unpinned()); + } +} + +fn harness_of(doc: &Value) -> invoke::HarnessSync { + let labels = doc[HARNESS_KEY] + .as_array() + .map(|a| { + a.iter() + .filter_map(|v| v.as_str().map(String::from)) + .collect() + }) + .unwrap_or_default(); + invoke::HarnessSync::from_unpinned(labels) +} + /// HEAD's own measurement, if a run already cached one under this binary and /// these conditions, e.g. the `gate` run `gate accept` follows moments later. /// Avoids a second full measurement of the same code just to build the @@ -1518,7 +1666,7 @@ mod tests { }; use crate::buildstamp; use crate::gate_lock; - use crate::invoke::{ItemMetrics, Run}; + use crate::invoke::{HarnessSync, ItemMetrics, Run}; use serde_json::{Value, json}; fn ctx() -> CompareCtx { @@ -2242,4 +2390,62 @@ mod tests { .output() .is_ok_and(|o| o.status.success()) } + + fn mismatched() -> HarnessSync { + HarnessSync::Mismatched(vec!["soothfast 0.2.0 -> 0.3.1".into()]) + } + + #[test] + fn a_regression_on_a_mismatched_harness_names_the_flag_that_waives_it() { + let text = super::harness_verdict(&mismatched(), true); + assert!(text.contains("--allow-harness-change")); + assert!(text.contains("soothfast 0.2.0 -> 0.3.1")); + } + + #[test] + fn a_clean_run_on_a_mismatched_harness_still_says_so() { + let text = super::harness_verdict(&mismatched(), false); + assert!(text.contains("different harness")); + assert!(text.contains("soothfast 0.2.0 -> 0.3.1")); + assert!(!text.contains("--allow-harness-change")); + assert!(super::harness_verdict(&HarnessSync::Matched, false).is_empty()); + } + + #[test] + fn the_flag_waives_a_regression_and_nothing_else() { + assert!(super::harness_waiver(&mismatched(), true, 1)); + assert!(!super::harness_waiver(&mismatched(), false, 1)); + // A pinned reference means a regression is the code's, flag or not. + assert!(!super::harness_waiver(&HarnessSync::Matched, true, 1)); + // Nothing regressed, so there is nothing to waive. + assert!(!super::harness_waiver(&mismatched(), true, 0)); + } + + #[test] + fn a_reference_served_from_the_cache_keeps_its_mismatch() { + let mut doc = json!({ "version": 1, "items": {} }); + super::mark_harness(&mut doc, &mismatched()); + assert_eq!(super::harness_of(&doc), mismatched()); + let clean = json!({ "version": 1, "items": {} }); + assert_eq!(super::harness_of(&clean), HarnessSync::Matched); + } + + #[test] + fn a_stored_reference_is_only_marked_when_the_pin_missed() { + let mut doc = json!({ "version": 1, "items": {} }); + super::mark_harness(&mut doc, &HarnessSync::Matched); + assert_eq!(doc, json!({ "version": 1, "items": {} })); + } + + #[test] + fn the_triage_artifact_carries_the_mismatch_and_drops_it_again() { + let dir = std::env::temp_dir().join("soothfast-test-triage-harness"); + let _ = std::fs::remove_dir_all(&dir); + super::triage_harness_note(&dir, &mismatched()); + let note = std::fs::read_to_string(dir.join("harness-mismatch.txt")).unwrap(); + assert!(note.contains("soothfast 0.2.0 -> 0.3.1")); + super::triage_harness_note(&dir, &HarnessSync::Matched); + assert!(!dir.join("harness-mismatch.txt").exists()); + let _ = std::fs::remove_dir_all(&dir); + } } diff --git a/cargo-soothfast/src/invoke.rs b/cargo-soothfast/src/invoke.rs index 4f24ce6..64eccb9 100644 --- a/cargo-soothfast/src/invoke.rs +++ b/cargo-soothfast/src/invoke.rs @@ -1176,44 +1176,257 @@ pub fn tree_is_clean() -> bool { git(&["status", "--porcelain"]).is_ok_and(|s| s.trim().is_empty()) } +/// Whether the reference side ended up embedding HEAD's measurement harness. +/// A reference built against another harness measures a different protocol, +/// so the outcome travels with the run. +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum HarnessSync { + /// Every soothfast-family crate on the reference side is at HEAD's version. + Matched, + /// Crates cargo would not move, each spelled `name 0.2.0 -> 0.3.1`. + Mismatched(Vec), +} + +impl HarnessSync { + /// Rebuild an outcome from the crate labels a stored run kept. + pub fn from_unpinned(labels: Vec) -> Self { + if labels.is_empty() { + Self::Matched + } else { + Self::Mismatched(labels) + } + } + + /// The crates left at a version of their own, empty when the pin held. + pub fn unpinned(&self) -> &[String] { + match self { + Self::Matched => &[], + Self::Mismatched(labels) => labels, + } + } + + /// Whether the two sides embed different measurement harnesses. + pub fn is_mismatched(&self) -> bool { + matches!(self, Self::Mismatched(_)) + } +} + +fn sync_outcome(mismatches: &[(String, String, String)]) -> HarnessSync { + HarnessSync::from_unpinned( + mismatches + .iter() + .map(|(name, from, to)| format!("{name} {from} -> {to}")) + .collect(), + ) +} + /// Pin the merge-base worktree's soothfast crates to HEAD's locked versions -/// so the reference bench binary embeds the same measurement harness. Best -/// effort: a pin that cargo rejects (offline, incompatible requirement) -/// warns and leaves the reference side as its own lock resolved it. -pub fn sync_harness_versions(wt: &Path) -> Result<(), String> { +/// so the reference bench binary embeds the same measurement harness. A pin +/// the worktree's own requirement excludes is retried after rewriting that +/// requirement; anything still unpinned comes back as `Mismatched` for the +/// caller to put in front of the verdict. +pub fn sync_harness_versions(wt: &Path) -> Result { let head_path = workspace_root() .map_err(|e| e.to_string())? .join("Cargo.lock"); let Ok(head) = std::fs::read_to_string(&head_path) else { - return Ok(()); + return Ok(HarnessSync::Matched); }; // One crate per pass, re-reading the lock: pinning the facade drags its // family along, and already-moved entries must not be warned about. for _ in 0..16 { let Ok(base) = std::fs::read_to_string(wt.join("Cargo.lock")) else { - return Ok(()); + return Ok(HarnessSync::Matched); }; - let Some((name, from, to)) = harness_mismatches(&head, &base).into_iter().next() else { - return Ok(()); + let mismatches = harness_mismatches(&head, &base); + let Some((name, from, to)) = mismatches.first().cloned() else { + return Ok(HarnessSync::Matched); }; println!( "gate: pinning {name} {from} -> {to} in the merge-base worktree (harness must match HEAD)" ); - let out = Command::new("cargo") - .args(["update", "-p", &format!("{name}@{from}"), "--precise", &to]) - .current_dir(wt) - .output() - .map_err(|e| e.to_string())?; - if !out.status.success() { - let stderr = String::from_utf8_lossy(&out.stderr); + let mut refused = pin_precise(wt, &name, &from, &to)?; + // A caret-incompatible bump is refused by the merge-base's own + // requirement. The worktree is disposable, so widen it and retry. + if refused.as_deref().is_some_and(is_requirement_conflict) + && relax_harness_requirements(wt, &mismatches)? + { + println!("gate: widened the merge-base's soothfast requirements to retry the pin"); + refused = pin_precise(wt, &name, &from, &to)?; + } + if let Some(stderr) = refused { eprintln!( "WARN: could not pin {name} ({}); the reference keeps its own harness — deltas may reflect the harness change itself", stderr.trim().lines().last().unwrap_or("no error output") ); - return Ok(()); + return Ok(sync_outcome(&mismatches)); } } - Ok(()) + let remaining = std::fs::read_to_string(wt.join("Cargo.lock")) + .map(|base| harness_mismatches(&head, &base)) + .unwrap_or_default(); + Ok(sync_outcome(&remaining)) +} + +/// Cargo's precise pin, run in the worktree. `Ok(None)` when it landed, +/// `Ok(Some(stderr))` when cargo refused it. +fn pin_precise(wt: &Path, name: &str, from: &str, to: &str) -> Result, String> { + let out = Command::new("cargo") + .args(["update", "-p", &format!("{name}@{from}"), "--precise", to]) + .current_dir(wt) + .output() + .map_err(|e| e.to_string())?; + if out.status.success() { + return Ok(None); + } + Ok(Some(String::from_utf8_lossy(&out.stderr).into_owned())) +} + +/// Whether cargo refused the pin because a requirement excludes the wanted +/// version, rather than because the index or network was unreachable. +fn is_requirement_conflict(stderr: &str) -> bool { + stderr.contains("didn't match") + || stderr.contains("does not match") + || stderr.contains("failed to select a version") +} + +/// Point the worktree's soothfast-family requirements at the versions HEAD +/// locks, so the pin above is no longer refused. Returns whether any +/// manifest changed. +fn relax_harness_requirements( + wt: &Path, + mismatches: &[(String, String, String)], +) -> Result { + let wanted: BTreeMap<&str, &str> = mismatches + .iter() + .map(|(name, _, to)| (name.as_str(), to.as_str())) + .collect(); + let mut changed = false; + for path in manifests(wt) { + let Ok(text) = std::fs::read_to_string(&path) else { + continue; + }; + let Some(next) = retarget_requirements(&text, &wanted) else { + continue; + }; + std::fs::write(&path, next).map_err(|e| format!("{}: {e}", path.display()))?; + changed = true; + } + Ok(changed) +} + +/// Every `Cargo.toml` under `dir`. `target`/`.git` hold nothing the +/// resolver reads. +fn manifests(dir: &Path) -> Vec { + let mut out = Vec::new(); + let Ok(entries) = std::fs::read_dir(dir) else { + return out; + }; + for entry in entries.flatten() { + let name = entry.file_name(); + if name == "target" || name == ".git" { + continue; + } + let path = entry.path(); + if path.is_dir() { + out.extend(manifests(&path)); + } else if name == "Cargo.toml" { + out.push(path); + } + } + out +} + +/// Rewrite the `version` requirement of each dependency `wanted` names, in +/// one manifest's text. `None` when nothing matched. Only the requirement +/// string moves: re-specifying the dependency would drop `features`, +/// `optional` or `default-features` beside it. +fn retarget_requirements(text: &str, wanted: &BTreeMap<&str, &str>) -> Option { + let mut out = String::with_capacity(text.len()); + let mut table = String::new(); + let mut changed = false; + for raw in text.split_inclusive('\n') { + let line = raw.trim_end_matches('\n').trim_end_matches('\r'); + let ending = &raw[line.len()..]; + let trimmed = line.trim(); + if trimmed.starts_with('[') && trimmed.ends_with(']') { + table = trimmed + .trim_start_matches('[') + .trim_end_matches(']') + .trim() + .to_string(); + out.push_str(raw); + continue; + } + let rewritten = match dep_table_name(&table) { + Some(name) => version_key(line) + .zip(wanted.get(name)) + .and_then(|(at, to)| retarget_line(line, at, to)), + None => dep_line(line, &table, wanted).and_then(|(at, to)| retarget_line(line, at, to)), + }; + match rewritten { + Some(next) => { + out.push_str(&next); + out.push_str(ending); + changed = true; + } + None => out.push_str(raw), + } + } + changed.then_some(out) +} + +/// Where a `version = "..."` line's value starts. +fn version_key(line: &str) -> Option { + let (key, rest) = line.split_once('=')?; + (key.trim().trim_matches('"') == "version").then(|| line.len() - rest.len()) +} + +/// The dependency a `[…dependencies.NAME]` header names. +fn dep_table_name(table: &str) -> Option<&str> { + let (parent, name) = table.rsplit_once('.')?; + is_dep_table(parent).then_some(name) +} + +fn is_dep_table(table: &str) -> bool { + matches!( + table.rsplit('.').next(), + Some("dependencies" | "dev-dependencies" | "build-dependencies") + ) +} + +/// Where a dependency line's requirement starts, and what it should become. +/// A path or git dependency is left alone: its version is not what the +/// resolver picks. +fn dep_line<'a>( + line: &str, + table: &str, + wanted: &BTreeMap<&str, &'a str>, +) -> Option<(usize, &'a str)> { + if !is_dep_table(table) { + return None; + } + let (key, rest) = line.split_once('=')?; + let to = wanted.get(key.trim().trim_matches('"'))?; + let rest_at = line.len() - rest.len(); + let value = rest.trim_start(); + if value.starts_with('"') { + return Some((rest_at, to)); + } + if !value.starts_with('{') || value.contains("path =") || value.contains("git =") { + return None; + } + let version_at = rest_at + rest.find("version")?; + Some((version_at, to)) +} + +/// Replace the first quoted string at or after `from` with `to`, leaving +/// the rest of the line (other keys, a trailing comment) untouched. +fn retarget_line(line: &str, from: usize, to: &str) -> Option { + let open = from + line.get(from..)?.find('"')?; + let close = open + 1 + line.get(open + 1..)?.find('"')?; + (&line[open + 1..close] != to) + .then(|| format!("{}\"{to}\"{}", &line[..open], &line[close + 1..])) } /// Copy an untracked `.cargo/config.toml` (or legacy `.cargo/config`) into @@ -1302,6 +1515,8 @@ pub fn git_in(dir: &Path, args: &[&str]) -> io::Result { #[cfg(test)] mod tests { + use std::collections::BTreeMap; + use super::{ CommonArgs, ItemMetrics, Run, SaveScope, harness_mismatches, run_from_items_value, run_to_items_value, @@ -1423,6 +1638,92 @@ source = "registry+https://github.com/rust-lang/crates.io-index" assert!(harness_mismatches(&no_source, &base).is_empty()); } + #[test] + fn a_caret_incompatible_bump_is_still_a_mismatch() { + let base = HEAD.replace("0.1.7", "0.2.0"); + assert_eq!( + harness_mismatches(HEAD, &base), + vec![ + ("soothfast".into(), "0.2.0".into(), "0.1.7".into()), + ("soothfast-measure".into(), "0.2.0".into(), "0.1.7".into()), + ] + ); + } + + fn wanted() -> BTreeMap<&'static str, &'static str> { + BTreeMap::from([("soothfast", "0.3.1")]) + } + + #[test] + fn a_requirement_conflict_is_told_apart_from_an_unreachable_index() { + assert!(super::is_requirement_conflict( + "error: failed to select a version for the requirement `soothfast = \"^0.2.0\"`\n\ + candidate versions found which didn't match: 0.3.1\n" + )); + assert!(!super::is_requirement_conflict( + "error: failed to get `soothfast` as a dependency\n\ + Caused by: network failure seems to have happened\n" + )); + } + + #[test] + fn every_requirement_shape_moves_to_heads_version() { + let manifest = r#"[package] +name = "demo" + +[dependencies] +soothfast = "0.2.0" +serde = "1.0.200" + +[dev-dependencies] +soothfast = { version = "0.2.0", features = ["runner"], default-features = false } + +[target.'cfg(unix)'.dependencies.soothfast] +version = "0.2.0" +optional = true +"#; + let got = super::retarget_requirements(manifest, &wanted()).expect("rewritten"); + assert_eq!(got.matches("\"0.3.1\"").count(), 3); + assert!(!got.contains("0.2.0")); + assert!(got.contains("features = [\"runner\"], default-features = false")); + assert!(got.contains("serde = \"1.0.200\"")); + assert!(got.contains("optional = true")); + } + + #[test] + fn a_manifest_with_nothing_to_move_is_left_alone() { + let manifest = "[dependencies]\nsoothfast = \"0.3.1\"\nserde = \"1\"\n"; + assert!(super::retarget_requirements(manifest, &wanted()).is_none()); + // A path dep resolves from the tree, not from a requirement. + let path_dep = + "[dependencies]\nsoothfast = { path = \"../soothfast\", version = \"0.2.0\" }\n"; + assert!(super::retarget_requirements(path_dep, &wanted()).is_none()); + // `soothfast` outside a dependency table names something else. + let other = "[features]\nsoothfast = \"0.2.0\"\n"; + assert!(super::retarget_requirements(other, &wanted()).is_none()); + } + + #[test] + fn rewriting_keeps_the_rest_of_the_line() { + let manifest = "[dependencies]\nsoothfast = \"0.2.0\" # the harness\n"; + let got = super::retarget_requirements(manifest, &wanted()).expect("rewritten"); + assert_eq!(got, "[dependencies]\nsoothfast = \"0.3.1\" # the harness\n"); + } + + #[test] + fn an_unpinned_crate_survives_as_the_runs_verdict_context() { + let matched = super::sync_outcome(&[]); + assert!(!matched.is_mismatched()); + assert!(matched.unpinned().is_empty()); + let missed = super::sync_outcome(&[("soothfast".into(), "0.2.0".into(), "0.3.1".into())]); + assert!(missed.is_mismatched()); + assert_eq!(missed.unpinned(), ["soothfast 0.2.0 -> 0.3.1"]); + assert_eq!( + super::HarnessSync::from_unpinned(missed.unpinned().to_vec()), + missed + ); + } + #[test] fn each_rustdoc_configuration_gets_its_own_cache_slot() { let json = std::path::Path::new("/t/doc/soothfast_spec.json"); diff --git a/cargo-soothfast/src/main.rs b/cargo-soothfast/src/main.rs index 8e4137c..dc4e7ca 100644 --- a/cargo-soothfast/src/main.rs +++ b/cargo-soothfast/src/main.rs @@ -44,8 +44,11 @@ commands: gate [-p PKG] [--filter S] [--backend B] [--samples N] [--features F] [--baseline NAME] [--ratchet NAME] [--against-ref REF] [--deps] [--features-matrix M] [--target NAME] [--save-baseline NAME] - [--codegen-units N|inherit] [--no-reuse-base] + [--codegen-units N|inherit] [--no-reuse-base] [--allow-gone] + [--allow-harness-change] (--save-baseline persists the measured head run after a pass) + (--allow-harness-change passes a run whose reference could not be + pinned to HEAD's soothfast, where the delta may be the harness) (--codegen-units pins both sides' partitioning; default 1) (--no-reuse-base re-measures the reference instead of reusing it) gate accept -p PKG --against-ref REF --justification \"...\" diff --git a/docs/gating.md b/docs/gating.md index cb4669b..fc24b49 100644 --- a/docs/gating.md +++ b/docs/gating.md @@ -155,6 +155,48 @@ A baseline saved before build stamps existed compares the same way, and says to re-save it. `--against-ref` never hits that: both sides are measured fresh in the same run. +## When the harness moves under a PR + +The same argument applies to soothfast itself. A `--against-ref` run builds +the merge-base's bench binary from the merge-base's lockfile, so a PR that +bumps soothfast has the reference embedding the old measurement harness and +HEAD embedding the new one. The harness is inside the measured loop, and a +protocol change of a few hundred instructions is several percent of a 30K +bench. That delta is the harness, not your code. + +So the gate pins it. Before building the reference it rewrites the +worktree's lockfile to HEAD's soothfast versions, and when the merge-base's +own requirement excludes them (`soothfast = "0.2"` cannot take `0.3.1`) it +widens that requirement in the worktree's manifests and retries. The +worktree is a throwaway checkout under `.soothfast/worktrees/`, so nothing +in your tree is touched. + +```console +gate: pinning soothfast 0.2.0 -> 0.3.1 in the merge-base worktree (harness must match HEAD) +gate: widened the merge-base's soothfast requirements to retry the pin +``` + +Pinning can still fail: an offline runner, a yanked version, a requirement +the rewrite does not recognise. The gate then says so, in the banner, in the +verdict line, and in `.soothfast/triage/harness-mismatch.txt`: + +```console +gate: HARNESS MISMATCH — the merge-base could not be pinned to HEAD's soothfast (soothfast 0.2.0 -> 0.3.1); deltas below may be the harness, not this change +FAIL demo::parse instructions 30104.0 -> 31680.0 (+5.2%) +gate: FAILED (1 regression(s)) — measured against a different harness (soothfast 0.2.0 -> 0.3.1); pass --allow-harness-change if the harness bump explains it +``` + +A regression on such a run fails, and `--allow-harness-change` is what lets +it pass. Reach for it once you have read the deltas and they are the shape +of a harness bump: a small uniform shift across every item, nothing +concentrated in what the PR touched. It waives comparison failures only. +Checked claims hold HEAD against its own declared numbers, so they fail +through it. + +A run that is otherwise clean still reports the mismatch. Both sides being +measured with different instruments is not evidence in either direction, +and a quiet pass would be as wrong as a red verdict. + ## Features A bench target declaring `required-features`, or a crate whose hot paths sit