Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 45 additions & 5 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -602,11 +602,51 @@ path = "src/lib.rs"
#
# Fix: disable debug-assertions for the proptest crate ONLY. This removes the
# false panic from 3rd-party code while keeping OUR debug_asserts fully active
# (overflow-checks left on). Verified: `PROPTEST_CASES=2000000` no longer panics.
# Profiles must live in the workspace-root manifest, so they live here.
[profile.test.package.proptest]
debug-assertions = false

# (overflow-checks left on). Profiles must live in the workspace-root manifest,
# so they live here.
#
# #2571 — WHY THERE IS ONLY ONE BLOCK NOW, AND WHY IT CANNOT BE ZERO.
# This manifest is BOTH the workspace root AND the published `aprender` package,
# so every profile block here is shipped to crates.io and re-validated by
# `cargo install aprender`. That resolve excludes dev-dependencies entirely, so a
# spec naming a dev-only crate matches nothing and cargo opens the install with
# warning: profile package spec `proptest` in profile `<p>` did not match any packages
# There used to be two such blocks, hence two warnings on every install.
#
# The `test`-profile block was DEAD and is deleted. proptest is only ever built as
# a *dependency*, and dependencies build under `dev` even when the test target
# builds under `test`. Measured on the full workspace test graph
# (`cargo +nightly test --no-run --workspace --lib -Z unstable-options --unit-graph`):
# exactly ONE proptest unit exists among 1110 units, and with this `dev` block
# alone it already carries `debug_assertions: false`. Deleting the `test` block
# changed no unit and removed one install warning (verified end-to-end: a real
# `cargo install --path .` printed 2 warnings before, 1 after).
#
# The `dev` block is LOAD-BEARING — do not "clean it up". Measured both ways with
# `PROPTEST_CASES=1000000 cargo test -p aprender-train --lib config::validate::proptests`
# (the module #1989 named; PROPTEST_CASES engagement proven by 10x time scaling,
# 5.45s at 100k -> ~55s at 1M):
# block removed -> rc=101, "assertion failed: self.low - result < self.intervals.step"
# at proptest-1.11.0/src/num/float_samplers.rs:466, in
# prop_negative_lr_fails (unit graph: debug_assertions=true)
# block present -> rc=0, 21 passed in 57.33s (unit graph: debug_assertions=false)
# Downgrading is not an escape either: float_samplers.rs is byte-identical between
# proptest 1.8.0 and 1.11.0 apart from the f16 feature, and 1.11.0 is still newest.
#
# The one remaining warning is irreducible without paying somewhere worse; four
# alternatives were measured and rejected:
# - declaring proptest in the root [dev-dependencies]: still warns (cargo install
# skips dev-deps), so the spec still matches nothing.
# - `[target.'cfg(any())'.dependencies] proptest`: silences it, but injects 39
# phantom packages (proptest, rustix, libc, getrandom, ...) into EVERY
# downstream consumer's Cargo.lock, and so into their cargo-audit/deny/vendor
# surface, for a crate that is never built.
# - `[profile.dev.package."*"]`: silences it (cargo never validates `*`), but
# disables debug-assertions for ALL third-party crates in our dev/test builds —
# weakening a real check to remove a cosmetic line.
# - a no-op `"*"` block alongside the named spec: still warns.
# The only cost-free fix is structural: split the `aprender` facade out of the
# workspace root so dev-only profiles stop being published. Tracked in #2571.
[profile.dev.package.proptest]
debug-assertions = false

Expand Down
183 changes: 183 additions & 0 deletions crates/aprender-core/tests/monorepo_invariants.rs
Original file line number Diff line number Diff line change
Expand Up @@ -691,3 +691,186 @@ fn falsify_pub_007_gate5_leaf_and_hub_differ() {
rather than left as folklore."
);
}

// ---------------------------------------------------------------------------
// FALSIFY-INSTALL-001 (#2571): the root manifest is ALSO the published package.
// ---------------------------------------------------------------------------
// Every `[profile.<p>.package.<spec>]` block in the workspace-root manifest is
// shipped to crates.io and re-validated by `cargo install aprender`. That resolve
// excludes dev-dependencies, so a spec naming a dev-only crate matches nothing and
// cargo greets the user with
// warning: profile package spec `<spec>` in profile `<p>` did not match any packages
// #2571 shipped two of those in 0.63.0. This gate keeps the count from growing.

/// Package specs that are knowingly absent from the install graph, with the reason
/// each one cannot simply be deleted. An entry here is a debt, not a blessing:
/// delete the entry the moment the spec is removable or the package becomes
/// reachable, and this gate fails if you leave a stale one behind.
const PROFILE_SPEC_ALLOWLIST: &[(&str, &str)] = &[
// proptest 1.11.0's uniform float sampler carries an over-strict debug_assert
// (num/float_samplers.rs). Removing this block is RED, not cosmetic: measured at
// `PROPTEST_CASES=1000000 cargo test -p aprender-train --lib config::validate::proptests`,
// rc=101 with "assertion failed: self.low - result < self.intervals.step" without
// it, rc=0 with it. Downgrading is no escape (float_samplers.rs is byte-identical
// 1.8.0..=1.11.0 apart from the f16 feature). The cost-free fix is structural —
// split the facade out of the workspace root — and is tracked in #2571.
("dev", "proptest"),
];

/// Parse `[profile.<profile>.package.<spec>]` headers out of the root manifest.
/// `"*"` is skipped: cargo never validates the wildcard, so it cannot warn.
fn root_profile_package_specs() -> Vec<(String, String)> {
let manifest = std::fs::read_to_string(workspace_root().join("Cargo.toml"))
.expect("workspace-root Cargo.toml must be readable");

let mut specs = Vec::new();
for line in manifest.lines() {
let line = line.trim();
let Some(inner) = line
.strip_prefix("[profile.")
.and_then(|rest| rest.strip_suffix(']'))
else {
continue;
};
let Some((profile, spec)) = inner.split_once(".package.") else {
continue;
};
let spec = spec.trim_matches('"');
if spec == "*" || profile.contains('.') {
continue;
}
specs.push((profile.to_string(), spec.to_string()));
}
specs
}

/// Every package reachable from the published root package over NON-dev edges —
/// i.e. the package set `cargo install aprender` actually resolves.
fn install_graph_package_names() -> HashSet<String> {
use std::collections::HashMap;

let root = workspace_root();
let output = Command::new("cargo")
.args(["metadata", "--format-version", "1"])
.current_dir(&root)
.output()
.expect("cargo metadata failed");
let metadata: serde_json::Value =
serde_json::from_slice(&output.stdout).expect("failed to parse cargo metadata");

let root_manifest = root.join("Cargo.toml");
let mut name_of: HashMap<String, String> = HashMap::new();
let mut root_id: Option<String> = None;
for pkg in metadata["packages"].as_array().expect("no packages") {
let id = pkg["id"].as_str().unwrap_or_default().to_string();
let name = pkg["name"].as_str().unwrap_or_default().to_string();
if pkg["manifest_path"].as_str().map(Path::new) == Some(root_manifest.as_path()) {
root_id = Some(id.clone());
}
name_of.insert(id, name);
}
let root_id = root_id.expect("the workspace-root Cargo.toml must be a package in the metadata");

let mut non_dev_deps: HashMap<String, Vec<String>> = HashMap::new();
for node in metadata["resolve"]["nodes"]
.as_array()
.expect("cargo metadata carried no resolve graph")
{
let id = node["id"].as_str().unwrap_or_default().to_string();
let mut edges = Vec::new();
for dep in node["deps"].as_array().into_iter().flatten() {
let kinds = dep["dep_kinds"].as_array();
// An absent/empty dep_kinds means a plain (normal) dependency.
let reached_without_dev = kinds.is_none_or(|k| {
k.is_empty() || k.iter().any(|entry| entry["kind"].as_str() != Some("dev"))
});
if reached_without_dev {
if let Some(pkg) = dep["pkg"].as_str() {
edges.push(pkg.to_string());
}
}
}
non_dev_deps.insert(id, edges);
}

let mut seen: HashSet<String> = HashSet::new();
let mut queue = vec![root_id];
while let Some(id) = queue.pop() {
for next in non_dev_deps.get(&id).into_iter().flatten() {
if seen.insert(next.clone()) {
queue.push(next.clone());
}
}
}

seen.iter()
.filter_map(|id| name_of.get(id).cloned())
.collect()
}

/// FALSIFY-INSTALL-001: no profile package spec in the root manifest may name a
/// package that `cargo install aprender` does not resolve. Each such spec costs the
/// user one `did not match any packages` warning on every install (#2571).
#[test]
fn falsify_install_001_no_dead_profile_package_specs() {
let reachable = install_graph_package_names();

// Non-vacuity control. If the walk ever follows dev edges, every spec would
// "match" and this gate would pass unconditionally — which is exactly the state
// that let #2571 ship. proptest is the canary: it is a dev-dependency of many
// members, so it is present in `cargo metadata` but absent from an install.
assert!(
reachable.len() > 10,
"FALSIFY-INSTALL-001 is vacuous: the install graph resolved to {} package(s). \
The non-dev walk from the root package is broken.",
reachable.len()
);
assert!(
!reachable.contains("proptest"),
"FALSIFY-INSTALL-001 is vacuous: proptest is reachable over non-dev edges, so the walk is \
following dev-dependencies and would bless any spec. Either the walk regressed or \
proptest genuinely became a runtime dependency — in the latter case drop it from \
PROFILE_SPEC_ALLOWLIST instead."
);

let specs = root_profile_package_specs();
let mut dead = Vec::new();
let mut used_allowlist = Vec::new();

for (profile, spec) in &specs {
if reachable.contains(spec) {
continue;
}
let entry = (profile.as_str(), spec.as_str());
if PROFILE_SPEC_ALLOWLIST.contains(&entry) {
used_allowlist.push(entry);
continue;
}
dead.push(format!("[profile.{profile}.package.{spec}]"));
}

assert!(
dead.is_empty(),
"FALSIFY-INSTALL-001 (#2571): {dead:?} name package(s) absent from the `cargo install \
aprender` resolve. Each one makes cargo print `warning: profile package spec ... did not \
match any packages` to every user on every install. Delete the block if it is dead (a \
dependency of a dependency builds under `dev`, never `test`), or — if it is load-bearing \
— add it to PROFILE_SPEC_ALLOWLIST with the measurement that proves removing it is RED."
);

// A stale allowlist is its own defect: it hides that the debt is already paid.
for (profile, spec) in PROFILE_SPEC_ALLOWLIST {
assert!(
specs.iter().any(|(p, s)| p == profile && s == spec),
"FALSIFY-INSTALL-001 (#2571): PROFILE_SPEC_ALLOWLIST still excuses \
[profile.{profile}.package.{spec}], but the root manifest no longer declares it. \
Delete the allowlist entry."
);
assert!(
used_allowlist.contains(&(profile, spec)),
"FALSIFY-INSTALL-001 (#2571): [profile.{profile}.package.{spec}] is allowlisted as \
unreachable, but it now resolves in the install graph and warns about nothing. \
Delete the allowlist entry."
);
}
}
Loading