feat(tree): show artifact dependency details#17075
Conversation
|
r? @ehuss rustbot has assigned @ehuss. Use Why was this reviewer chosen?The reviewer was selected based on:
|
| } | ||
| } | ||
|
|
||
| #[derive(Debug, Copy, Hash, Eq, Clone, PartialEq)] |
There was a problem hiding this comment.
tip: splitting changes like this into their own commit make it easier / faster for maintainers to review, see https://epage.github.io/dev/pr-style/#c-atomic
| .with_status(0) | ||
| .run(); | ||
|
|
||
| p.cargo("tree -Z bindeps -e features") |
There was a problem hiding this comment.
We ask for new test cases to be added in their own commit first, with tests passing in that commit, see https://epage.github.io/dev/pr-style/#c-test
| ws: &Workspace<'_>, | ||
| graph: &'a Graph<'_>, | ||
| node_index: NodeId, | ||
| incoming_edge: Option<&Edge>, |
There was a problem hiding this comment.
If we are relying on the incoming edge. Are there times where we have different edges but the same node? For example, what happens if a package has a regular dep and an artifact dep on a package?
#16192 also deals with edges and may have other relevant concerns.
| r#"... | ||
| foo v0.1.0 ([ROOT]/foo) | ||
| └── bar v0.1.0 ([ROOT]/foo/bar) | ||
| └── bar v0.1.0 ([ROOT]/foo/bar) (artifact: bin, target=[ALT_TARGET]) |
There was a problem hiding this comment.
This PR is marked as closing the issue but part of the issue is how to visually represent the graph.
2eb63a0 to
83d0988
Compare
|
I traced the design history through #10593, #14804, #14658, RFC 3028, The concrete proposal in #10593 / #14804 was to annotate artifact This PR takes that minimal textual approach, but spells the annotation I did not implement dashed-line rendering in this PR. Line style could If dashed-line rendering would be valuable, I am happy to implement that I also changed this from |
83d0988 to
78717bd
Compare
This comment has been minimized.
This comment has been minimized.
|
Updated this PR around the design discussion and the PR-structure feedback. Changes in this revision:
I left the review threads open for reviewer confirmation. Verified locally: |
78717bd to
4280c6a
Compare
This comment has been minimized.
This comment has been minimized.
|
We have other attempts at annotating edges which gets weird in some cases. I wonder if we should instead treat these like features: an artifact is a node that depends on the package / lib. |
|
Not dug into all the details of this implementation (particularly when there are bigger questions) but
|
This comment has been minimized.
This comment has been minimized.
4280c6a to
a45f3cc
Compare
|
This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
|
Updated this around the artifact-node design proposal. Artifact requests are now synthetic nodes, following the existing feature-node model: With The parent-to-artifact edge retains its normal/build/dev dependency kind. The first commit is a passing current-behavior test baseline, including format, inversion, pruning, selected binaries, targets, multiple artifact kinds, features, and Verified locally: |
42e78c1 to
cbf26b5
Compare
Add passing snapshots for how cargo tree currently renders artifact dependencies. The fixtures cover selected binaries, multiple artifact kinds, explicit targets, feature trees, custom formatting, inversion, pruning, dependency categories, and lib = true. Shared artifact fixtures record reversed artifact-kind declarations, depth limiting, deduplication, and negative dependency-kind filtering. These snapshots establish the behavior changed by the later feature commit.
The logic that routes a dependency edge through feature nodes was inline in `add_pkg`. Extract it into `add_dependency_edge`, alongside the existing `add_pkg` and `add_feature` free functions. The helper accepts the source node and edge so graph construction can reuse feature routing without maintaining a second package index. Also name the inherited compilation context `base_features_for` before applying an artifact target override. Tree output is unchanged and no tests are modified.
Represent each artifact request as an intermediate node between the dependent and the package that provides the artifact. The node records artifact kinds and target while the incoming edge preserves the dependency category. Artifact kinds use canonical ordering so requests that differ only in manifest order share one node. Feature trees route artifact requests through feature nodes before reaching the package. Inverted trees print requests before dependency section headings so their original category remains unambiguous. Custom package formats do not hide artifact details, and dependencies with lib = true keep a separate ordinary package edge. Artifact request nodes deliberately count as a level for --depth.
cbf26b5 to
e36bbbf
Compare
| .artifact() | ||
| .and_then(|artifact| artifact.target()) | ||
| .and_then(|target| target.to_resolved_compile_target(requested_kind)) | ||
| { |
There was a problem hiding this comment.
Also name the inherited compilation context
base_features_forbefore
applying an artifact target override.
"Also" and "and" are smells for a commit not being atomic. It looks like this change is independent of the function extraction. The fact that it takes closer inspection to determine whether the behavior changed and drops explanatory comments which take closer inspection to determine if that is fine, makes this require more reviewer attention.
| Node::Package { package_id, .. } => { | ||
| if filter_non_workspace_member && !ws.is_member_id(*package_id) { | ||
| return false; | ||
| } | ||
| !pkgs_to_prune.iter().any(|spec| spec.matches(*package_id)) | ||
| } | ||
| Node::Artifact { package_id, .. } => { | ||
| if filter_non_workspace_member && !ws.is_member_id(*package_id) { | ||
| return false; | ||
| } | ||
| !pkgs_to_prune.iter().any(|spec| spec.matches(*package_id)) | ||
| } |
There was a problem hiding this comment.
is there a reason these two aren't merged?
| └── right v0.0.0 ([ROOT]/foo/right) | ||
| [build-dependencies] | ||
| └── bar v0.5.0 ([ROOT]/foo/bar) | ||
| └── bar artifact "cdylib, staticlib" (*) |
There was a problem hiding this comment.
Shouldn't that be "cdylib", "staticlib" if we are quoting them?
|
Thanks for your continue work on this! |
What does this PR try to resolve?
cargo tree -Z bindepscurrently renders artifact dependencies as ordinary package edges. The output does not identify the requested artifact kinds, selected binaries, or artifact target, and it cannot distinguish a library relationship from an artifact relationship to the same package.This PR represents each artifact request as an intermediate node between the dependent and the package that provides the artifact:
The request remains in its original normal, build, or development dependency category. The package below it retains the compilation context and dependency subtree selected for the artifact target.
Refs #14804.
Design
Artifact details belong to a dependency request rather than globally to a package. An intermediate node preserves that request in both graph directions without adding hidden text to a formatted package line.
With feature edges enabled, the request precedes the features it activates:
In an inverted tree, the request remains visible between the package and its dependent:
--formatcontinues to format package nodes. Artifact request nodes, like feature nodes, use a fixed description so artifact information is not lost or appended outside the requested package format.Artifact request nodes are graph nodes and therefore count as a level for
--depth. For example, the provider package in the first example appears at depth 2 rather than depth 1.For
lib = true, Cargo has two relationships with the package: an ordinary Rust library dependency and an artifact request. The tree displays both:Scope
This PR does not add a dashed or otherwise distinct line style. The artifact node identifies the non-library request and provides its kinds and target; line styling can be considered separately without changing the graph model.
Dependency aliases are not added to the label. This matches existing
cargo treepackage-edge behavior. Requests that differ by selected artifacts, target, package compilation context, or activated features remain distinct.Commit structure
--no-dedupe, dependency filtering, targets, features, andlib = true.Each commit builds and passes the focused test suite independently.
How should we test and review this PR?
The expectation changes in the final commit show the display contract directly. The graph changes should preserve these invariants:
lib = trueretains a separate ordinary package relationship.Local verification: