feat(fetch): resolve a nested workspace's members against their own root - #137
Open
justin13888 wants to merge 6 commits into
Open
feat(fetch): resolve a nested workspace's members against their own root#137justin13888 wants to merge 6 commits into
justin13888 wants to merge 6 commits into
Conversation
A scan can span more than one workspace: a `fuzz/` or `examples/` tree with its own `[workspace]` is a root in its own right, and the walk descends into it because Cargo already ignores such a subtree, so nobody lists it in the outer root's `exclude`. The shallow graph resolved every member against one root's tables — the scan root's. Refusing to hand the outer root's `[workspace.package] version` to a nested crate stopped it reporting a number that was never its own, but left it with no version at all, when the version it does have sits one directory up in its own root. The same single-root assumption routed every member's `dep.workspace = true` through the outer root's `[workspace.dependencies]`, so a name that a nested root vendors by path was classified from whatever the outer root happened to say about that name, or from nothing. Carry a `Scope` per workspace root instead — both inheritance tables together, since a member's `version.workspace = true` and its `dep.workspace = true` name the same root — and give each member the index of the nearest `[workspace]` ancestor that governs it. A nested root declaring no version still yields none: an absent table resolves to nothing rather than to some other root's number, so a crate can never borrow a version from a workspace it is not in. The walk's growing state moves onto a `Walk` struct so the recursion carries context rather than an argument list.
…dary The member walk deduplicates by `[package] name` and takes the first crate it meets. Its `read_dir` yields filesystem order, so two crates sharing a name — one in the scanned workspace, one under a nested, independent root — were settled by whichever the filesystem happened to hand back first. That was invisible while both resolved against the same tables. Now that each resolves against its own root, the two answers differ, and the one reported would differ between machines holding identical contents. Descend in sorted order and keep the outer crate: a nested root is only pushed onto the scope arena after the root containing it, so the smaller scope index is the enclosing one. Sorting alone would only make a wrong answer stable — it is here so that the tie between two crates in sibling nested workspaces, where neither encloses the other, is fixed rather than arbitrary.
The member walk read a directory's `Cargo.toml` with `read_to_string(..).ok()` and asked `parse_workspace` whether it opened a new scope. Both collapse failure into `None`, so a nested root that could not be read — mode 000, or a TOML syntax error — was indistinguishable from a directory with no manifest at all, and every crate beneath it kept the *outer* scope. Such a crate resolved `version.workspace = true` against a root with no authority over it, reporting a number the outer root's `[workspace.package]` happened to carry. A file that exists and cannot be read is not evidence that the enclosing root governs what is below it: it may well declare a `[workspace]`, and nothing can rule that out. Classify the manifest into absent, readable, or opaque, and give an opaque one a scope of its own with both inheritance tables empty. Crates below it now inherit nothing, in either direction, while a directory with no `Cargo.toml` keeps inheriting the enclosing scope as before.
Two crates can also share a `[package] name` without a workspace boundary between them — `crates/dup` and `examples/dup` in one workspace — and the scope comparison cannot settle that: both index the same root, so first-wins decides. What "first" means is the walk's sorted descent, and nothing pinned it. The existing cross-boundary test passes with or without `paths.sort()`, because the scope comparison picks the same winner in either encounter order. Assert that the alphabetically earlier path wins, with the two directories created in reverse alphabetical order so a filesystem reporting entries in creation order hands the walk the wrong crate first. Removing `paths.sort()` fails this test on every run rather than on some machines.
`tree`'s private `SKIP_DIRS` lists the same four names as `discover::SKIP_DIRS`, and the inline filter beside it is `discover::is_skipped_dir` verbatim, which reads as an oversight worth unifying. It is not: the two bound different scans with different consequences — adding a name to `discover`'s list narrows what `list` and `check` report on, adding one here silently drops crates from the graph. Say so where the next reader will be tempted.
… feat/110-nested-workspace-scope
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #110.
dependable treefalls back to a shallow, manifest-only graph when the scan root has noCargo.lock. That walk collects everyCargo.tomlbeneath the root, which sweeps in crates belonging to a nested, independent workspace — acargo fuzztree, anexamples/inner, anxtaskwith its own[workspace]. Cargo ignores such a subtree entirely, so nobody lists it in the outer root'sexclude, and the walk reaches it.Those crates were resolved against a single set of workspace tables: the scan root's. Refusing to hand the outer root's
[workspace.package] versionto a nested crate stopped it reporting a number that was never its own, but left it with no version at all — while the version it does have sits one directory up, in its own root's[workspace.package], unread. The same single-root assumption routed every member'sdep.workspace = truethrough the outer root's[workspace.dependencies], so a name a nested root vendors by path was classified from whatever the outer root happened to say about that name, or from nothing.This carries a scope per workspace root instead.
What changed
Scopeholds one root's two inheritance tables —[workspace.package]and[workspace.dependencies]— because a member'sversion.workspace = trueand itsdep.workspace = truename the same root; answering them from different manifests is the defect.Scopearena seeded with the scan root at index 0. On meeting a manifest that declares a[workspace], it pushes that root's scope and uses it for that directory and everything below. The switch happens before the directory's own[package]is read, so a manifest that is both a[workspace]and a[package]— thecargo fuzzshape — resolves against itself.Membercarries that scope index in place of agoverned_by_root: bool;shallow_graphresolves both kinds of inheritance against it and no longer takes the root's content at all.[workspace]under a[workspace]pushes another scope. The guarantee that a crate never borrows an unrelated root's version is preserved by construction, not by a special case: a nested root declaring no[workspace.package] versionyields a table with noversionkey, which resolves to nothing. There is a test for exactly that.Cargo.tomlis classified into absent, readable, or opaque, where opaque means the file exists but could not be read (permissions, a device error) or is not valid TOML. Both failures used to collapse into "not a workspace" —read_to_string(..).ok()andImDocument::parse(..).ok()?each yieldNone— so no scope was pushed and every crate below an unreadable nested root kept the outer scope and resolvedversion.workspace = trueagainst a root with no authority over it. An opaque boundary now pushes a scope of its own with both tables empty: a file that exists and cannot be read is not evidence that the enclosing root governs what is beneath it. A directory with noCargo.tomlat all still inherits the enclosing scope, which is correct and unchanged. (The old behaviour was not introduced here — the base branch'sdeclares_workspacecollapsed the same two cases — but the claim above is only true once it is fixed.)[package] nameduplicated across a nested-workspace boundary in favour of the outer crate. This change is the first to make the outcome differ by which scope won, so leaving the existing first-wins-over-unsorted-read_dirrule would have shipped a version that varied with filesystem iteration order between machines holding identical contents.All private to
tree.rs. No public API, noTreeErrororGraphSourcevariant, no CLI flag, no config key, no exit code, no JSON schema field, no dependency change.Decisions taken
Recorded under the autonomy contract for a run the user declared unattended, in their own words: "autonomously. create everything end-to-end" and "finish everything and finalize me all the final products I will test myself". There was no channel to ask on; each fork below was settled rather than raised, and each names what would reverse it.
1. Deliverable boundary — how the nested scope is resolved
Taken: a full scope stack carrying BOTH
[workspace.package]and[workspace.dependencies]. The walk carries each member's governing scope;version.workspace = trueresolves against it, andresolve_workspace_inheritancereceives that member's own scope's declarations.Rejected: a scope stack for
[workspace.package]only — it closes the issue's literal complaint and knowingly leaves the second authority leak in place, becauseshallow_graphcallsresolve_workspace_inheritance(&mut items, &declarations)for EVERY member withdeclarationsderived from the outerroot_contentalone. A nested crate'sdep.workspace = truewould still take the outer root's declaration and therefore itsPackageSourceandNodeKind. Shipping aScopestruct that carries one of the two tables invites the reader to ask why. The marginal cost is one field and one argument, and the code that buildsdeclarationsalready exists verbatim at the top ofshallow_graphand is being lifted anyway.Rejected: excluding nested-workspace crates from the outer graph entirely, as Cargo does — it is the largest behaviour change of the four: nodes present in today's output disappear, and
treeis a discovery tool where a user may well want to see thefuzzcrate they forgot about. It answers the issue by deleting the question rather than by reading the0.0.0sitting one directory up, and it requires rewriting the base branch's newest test into its negation.Rejected: growing
SKIP_DIRS— strictly less precise than machinery the file already has. Detecting a nested workspace from its[workspace]table identifies one STRUCTURALLY; a name list guesses.xtaskis canonically a real member of the outer workspace, so listing it would drop a governed crate — the mirror image of the bug being fixed — andexamples/<name>/Cargo.tomlis likewise a member in many workspaces. The issue's owntests/fixturescandidate is not even expressible, because matching is on the bare directory name. The issue speculates this "may make the first unnecessary"; it cannot, because a nested workspace in an unlisted directory still needs the flag.Reverses: drop
declarationsfromScopeand restore the single outer-rootresolve_workspace_inheritancecall (gives the version-only variant); or return early at a nested[workspace]in the walk (gives the Cargo-like exclusion).Filed: the "should a nested crate be visually distinguishable" question, as #138.
2. Which crate wins a duplicate
[package] nameacross a nested boundaryTaken: the outer crate, kept as one node. The scan root is the workspace the user asked about, so its member keeps its own name. Concretely: the walk deduplicates by name with a
seenindex, and a later member replaces an earlier one only when its scope index is smaller — a nested root can only be pushed after the root containing it, so a smaller index is the enclosing scope. Directory entries are also sorted before recursing; this change is the first that makes the OUTCOME differ by which crate won, so leaving the answer toread_dirorder would ship a version that varies between machines holding identical contents.Rejected: dropping the nested crate from the graph once its name collides. It is the reading Cargo itself takes — the nested subtree is simply not part of this workspace — but
treeis a discovery tool, and silently omitting a crate is the opposite of what a user runs it for.Rejected: keeping both as distinct nodes, the way the lockfile path already keeps duplicate versions of one crate apart. The shallow graph keys nodes by bare name with no path component, so two nodes named
dupwould be indistinguishable in every renderer — the tree, the JSON, the TUI — and the ambiguity would move from the builder to the reader rather than being settled.Rejected: sorting alone — determinism is not correctness; alphabetical order does not systematically favour the outer root, it only makes the wrong answer stable.
Reverses: drop the
scope < members[idx].scopecomparison for plain first-wins, or keyseenon the member's path rather than its name.3. Between two SIBLING nested workspaces, the alphabetically earlier directory wins
Taken: arbitrary, but fixed. Neither sibling encloses the other, so
scope < members[idx].scopecompares two indices with no enclosure relation between them, and the smaller one is simply whichever the sorted walk reached first. No scope has a claim here; stability across machines is the only property that matters, and the sorted descent supplies it.Rejected: reporting the collision.
treehas no diagnostic channel for a graph-construction ambiguity — the builder returns a graph or aTreeError, and neither carries warnings — and adding one for a case that requires two nested workspaces sharing a crate name is disproportionate to the case.Reverses: emit a notice when
seenrejects a member whose scope neither encloses nor is enclosed by the winner's.4. A nested root is authoritative for its inheritance tables but not for its
exclude, and its crates still count as members of the outer workspaceTaken: half-honouring the nested root, deliberately, as the current step rather than the end state. The two inheritance tables are what #110 is about. Honouring a nested root's
[workspace] excludeand reconsidering whether its crates belong in the outer graph at all are separate questions with their own blast radii, and pretending otherwise would widen this change into both.Rejected: honouring the nested root's
excludein the same change.collect_membersdocuments that it treats a crate as in-workspace iff its[package] nameappears under the root, precisely to sidestep a glob engine;excludeentries are globs, so honouring them reintroduces exactly what that design avoids — for the nested root and then, by symmetry, for the scan root too.Filed: the visual half — whether a nested crate should be distinguishable in the rendered tree — is #138.
Reverses: honour the nested root's
[workspace] excludeinexcluded_dirs.5. Test surface
Taken: TempDir tests in
crates/dependable-fetch/tests/tree.rsonly.Rejected: adding a committed lockfile-less fixture plus CLI assertions —
crates/dependable/tests/fixture_tree.rsis documented as "the graph comes from the fixture'sCargo.lock", which a lockfile-less fixture would make untrue, and it widens the change intocrates/dependable/. The reachability risk that would have justified it does not apply:tests/tree.rsalready drivesshallow_graphthrough TempDirs, and the earlier nested-workspace test reaches both branches of the governed flag.Reverses: add the fixture directory and
fixture_tree.rsassertions.6. Taken during implementation, not planned
clippy::too_many_argumentsfires at the 8th argument, which the scope arena added to the recursive walk. Rather than suppress the lint, the walk's cross-recursion state — the scan root, the exclude set, the dedup index, the members, and the arena — moved onto a privateWalkstruct with the recursion as a method, leaving only what varies per directory (dir,depth_left,scope) as arguments. This is a private restructuring insidetree.rs; nothing re-exported moves. Reverses: restore the free function and thread the arena through its parameters, which requires an#[allow]the repository's-D warningsposture does not otherwise carry.Residual, knowingly left
tree.rskeeps a privateSKIP_DIRSthat lists the same four names asdiscover::SKIP_DIRS, and its inlineSKIP_DIRS.contains(&name) || name.starts_with('.')isdiscover::is_skipped_dirverbatim. The two can drift: addingdisttodiscover's list would change whatlistandcheckscan without changing whattreewalks. Leaving the duplication is the right call for this PR — the two bound different scans with genuinely different consequences, since a name added todiscovernarrows a report while a name added here silently drops crates from the graph — so unifying them would couple two decisions that should stay separate. A comment ontree.rs'sSKIP_DIRSnow records that the divergence is intentional, so the next reader does not helpfully merge them.Also settled, and why it is not here
Whether a declared pin should be distinguishable from a lockfile resolution was settled NO on the base branch (#120, decision 4), for the same reason it is declined here: it needs a new field on
Node, which spansdependable-core, the TUI, and the JSON schema.Tests
Seven tests in
crates/dependable-fetch/tests/tree.rs. Each was confirmed to discriminate by mutating the implementation back toward the old behaviour and observing the failure — a test that passes against both is evidence of nothing.a_nested_independent_workspaces_crate_resolves_against_its_own_root(rewritten from..._does_not_inherit_the_outer_roots_version)a= 1.0.0,a-fuzz= 0.0.0 (wasNone),a-fuzz-stated= 7.7.7a_nested_root_declaring_no_version_leaves_its_crate_without_one[workspace]with no[workspace.package]yieldsNone, never the outer root's1.0.0the_innermost_workspace_root_governs_when_workspaces_nest_twicea_nested_workspaces_crate_inherits_dependencies_from_its_own_rootNodeKind::Patheven though the outer root declares the same name as a registry entrya_name_shared_across_a_nested_boundary_keeps_the_outer_crates_version9.9.9in the second layout)a_name_shared_within_one_scope_settles_on_the_alphabetically_earlier_pathaaa/andzzz/both name a cratedupin the same (root) scope, with different literal versions, so first-wins decides and the sorted walk is the only thing that makes "first" mean anything.aaa's1.1.1wins. The two directories are created in reverse alphabetical order on purpose, so a filesystem reporting entries in creation order hands the walk the wrong crate firstpaths.sort()is removed — observed failing 30 out of 30 runs on the development machine, returning9.9.9a_crate_under_an_unparseable_nested_root_inherits_nothing[workspacetable header as a nested root'sCargo.toml, with a crate below it declaringversion.workspace = trueandshared-dep.workspace = true. Both resolve toNone; the outer root's1.0.0and its[workspace.dependencies]do not reach across. The outer root's own member still gets1.0.0Absentinstead ofOpaque— the crate then reports the outer root's1.0.0falls_back_to_shallow_graph_without_lockfileanda_member_inheriting_its_version_from_the_workspace_root_still_reports_oneare unchanged and pass.Validation
Every command below completed in this run, colour disabled (
env -u FORCE_COLOR -u COLORTERM), on the final tree. No failures, so nothing to classify.cargo test -p dependable-fetch --test treecargo test -p dependable-fetchmise run testmise run fmt:checkmise run lint(clippy --workspace --all-targets -D warnings)convco checkover the seriesNot proven
treeCLI rendering of a nested crate's newly present version is not asserted at the CLI level, per decision 3; the graph the renderer consumes is.[workspace] membersglobs are still not evaluated, a nested root's ownexcludeis still not honoured (decision 4), and the depth bound of 64 and the scan root'sexcludeare untouched. All were out of scope by design.Cargo.tomlthat exists but cannot be read — mode 000, an EIO — takes the same branch inboundary_at, but no test creates one, because a permission-denied fixture is not reliable across the environments this suite runs in (root, containers, and filesystems that ignore the mode bits). The classification is onematcharm away from the tested one.Base
Opened against
feat/107-exact-pin-version(#120), which is where the exact-pin narrowing in this same file lives. Its arm ofshallow_graph— the external-dependency loop — is untouched here.