Skip to content
Draft
Show file tree
Hide file tree
Changes from 4 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
9 changes: 9 additions & 0 deletions src/cargo/ops/tree/format/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ enum Chunk {
Repository,
Features,
LibName,
VersionRequirement,
}

pub struct Pattern(Vec<Chunk>);
Expand All @@ -30,6 +31,7 @@ impl Pattern {
RawChunk::Argument("r") => Chunk::Repository,
RawChunk::Argument("f") => Chunk::Features,
RawChunk::Argument("lib") => Chunk::LibName,
RawChunk::Argument("ver-req") => Chunk::VersionRequirement,
RawChunk::Argument(a) => {
bail!("unsupported pattern `{}`", a);
}
Expand Down Expand Up @@ -111,6 +113,13 @@ impl<'a> fmt::Display for Display<'a> {
write!(fmt, "{}", target.crate_name())?;
}
}
Chunk::VersionRequirement => {
if let Some(version_req) =
self.graph.version_req_for_id(package.package_id())
{
write!(fmt, "{}", version_req)?;
}
}
}
}
}
Expand Down
122 changes: 118 additions & 4 deletions src/cargo/ops/tree/format/parse.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
use std::iter;
use std::str;

#[derive(Debug, PartialEq, Eq)]
pub enum RawChunk<'a> {
/// Raw text to include in the output.
Text(&'a str),
Expand All @@ -23,9 +24,9 @@ pub enum RawChunk<'a> {
/// (and optionally source), and the `{l}` will be the license from
/// `Cargo.toml`.
///
/// Substitutions are alphabetic characters between curly braces, like `{p}`
/// or `{foo}`. The actual interpretation of these are done in the `Pattern`
/// struct.
/// Substitutions are alphabetic characters or hyphens between curly braces,
/// like `{p}`, {foo} or `{bar-baz}`. The actual interpretation of these is
/// done in the `Pattern` struct.
///
/// Bare curly braces can be included in the output with double braces like
/// `{{` will include a single `{`, similar to Rust's format strings.
Expand Down Expand Up @@ -67,7 +68,7 @@ impl<'a> Parser<'a> {

loop {
match self.it.peek() {
Some(&(_, ch)) if ch.is_alphanumeric() => {
Some(&(_, ch)) if ch.is_alphanumeric() || ch == '-' => {
self.it.next();
}
Some(&(end, _)) => return &self.s[start..end],
Expand Down Expand Up @@ -121,3 +122,116 @@ impl<'a> Iterator for Parser<'a> {
}
}
}

#[cfg(test)]
mod tests {
use super::{Parser, RawChunk};

#[test]
fn plain_text() {
let chunks: Vec<_> = Parser::new("Hello World").collect();
assert_eq!(chunks, vec![RawChunk::Text("Hello World")]);
}

#[test]
fn basic_argument() {
let chunks: Vec<_> = Parser::new("{pkg}").collect();
assert_eq!(chunks, vec![RawChunk::Argument("pkg")]);
}

#[test]
fn mixed_content() {
let chunks: Vec<_> = Parser::new("Package {p} version:{v}").collect();
assert_eq!(
chunks,
vec![
RawChunk::Text("Package "),
RawChunk::Argument("p"),
RawChunk::Text(" version:"),
RawChunk::Argument("v"),
]
);
}

#[test]
fn escaped_braces() {
let chunks: Vec<_> = Parser::new("{{text}} in {{braces}}").collect();
assert_eq!(
chunks,
vec![
RawChunk::Text("{"),
RawChunk::Text("text"),
RawChunk::Text("}"),
RawChunk::Text(" in "),
RawChunk::Text("{"),
RawChunk::Text("braces"),
RawChunk::Text("}"),
]
);
}

#[test]
fn hyphenated_argument() {
let chunks: Vec<_> = Parser::new("{foo-bar}").collect();
assert_eq!(chunks, vec![RawChunk::Argument("foo-bar")]);
}

#[test]
fn unclosed_brace() {
let chunks: Vec<_> = Parser::new("{unclosed").collect();
assert_eq!(chunks, vec![RawChunk::Error("expected '}'")])
}

#[test]
fn unexpected_close_brace() {
let chunks: Vec<_> = Parser::new("unexpected}").collect();
assert_eq!(
chunks,
vec![
RawChunk::Text("unexpected"),
RawChunk::Error("unexpected '}'"),
]
);
}

#[test]
fn empty_argument() {
let chunks: Vec<_> = Parser::new("{}").collect();
assert_eq!(chunks, vec![RawChunk::Argument("")]);
}

#[test]
fn invalid_argument_chars() {
let chunks: Vec<_> = Parser::new("{a-b} {123}").collect();
assert_eq!(
chunks,
vec![
RawChunk::Argument("a-b"),
RawChunk::Text(" "),
RawChunk::Error("expected '}'"),
]
);
}

#[test]
fn complex_format() {
let format = "Pkg{{name}}: {p} [{v}] (License: {l})";
let chunks: Vec<_> = Parser::new(format).collect();
assert_eq!(
chunks,
vec![
RawChunk::Text("Pkg"),
RawChunk::Text("{"),
RawChunk::Text("name"),
RawChunk::Text("}"),
RawChunk::Text(": "),
RawChunk::Argument("p"),
RawChunk::Text(" ["),
RawChunk::Argument("v"),
RawChunk::Text("] (License: "),
RawChunk::Argument("l"),
RawChunk::Text(")"),
]
);
}
}
17 changes: 16 additions & 1 deletion src/cargo/ops/tree/graph.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,8 @@ use crate::core::dependency::DepKind;
use crate::core::resolver::Resolve;
use crate::core::resolver::features::{CliFeatures, FeaturesFor, ResolvedFeatures};
use crate::core::{FeatureMap, FeatureValue, Package, PackageId, PackageIdSpec, Workspace};
use crate::util::CargoResult;
use crate::util::interning::{INTERNED_DEFAULT, InternedString};
use crate::util::{CargoResult, OptVersionReq};
use std::collections::{HashMap, HashSet};

#[derive(Debug, Copy, Clone)]
Expand Down Expand Up @@ -161,6 +161,8 @@ pub struct Graph<'a> {
/// Key is the index of a package node, value is a map of `dep_name` to a
/// set of `(pkg_node_index, is_optional)`.
dep_name_map: HashMap<NodeId, HashMap<InternedString, HashSet<(NodeId, bool)>>>,
/// Map for looking up version requirements for dependency packages.
version_req_map: HashMap<PackageId, OptVersionReq>,
}

impl<'a> Graph<'a> {
Expand All @@ -172,6 +174,7 @@ impl<'a> Graph<'a> {
package_map,
cli_features: HashSet::new(),
dep_name_map: HashMap::new(),
version_req_map: HashMap::new(),
}
}

Expand Down Expand Up @@ -240,6 +243,12 @@ impl<'a> Graph<'a> {
}
}

/// Returns the version requirement for the given package ID. Returns `None`
/// if no version requirement is recorded (e.g., root packages).
pub fn version_req_for_id(&self, package_id: PackageId) -> Option<&OptVersionReq> {
self.version_req_map.get(&package_id)
}

/// Returns `true` if the given feature node index is a feature enabled
/// via the command-line.
pub fn is_cli_feature(&self, index: NodeId) -> bool {
Expand Down Expand Up @@ -523,6 +532,12 @@ fn add_pkg(
requested_kind,
opts,
);
// Store the version requirement for this dependency for
// later use in formatting.
graph.version_req_map.insert(
graph.package_id_for_index(dep_index),
dep.version_req().clone(),
);
let new_edge = Edge {
kind: EdgeKind::Dep(dep.kind()),
node: dep_index,
Expand Down
46 changes: 42 additions & 4 deletions tests/testsuite/cargo_tree/deps.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1128,7 +1128,10 @@ foo v0.1.0 ([ROOT]/foo)
#[cargo_test]
fn format() {
Package::new("dep", "1.0.0").publish();
Package::new("other-dep", "1.0.0").publish();
Package::new("dep", "2.0.0").publish();
Package::new("other-dep", "1.0.0")
.dep("dep", "^1.0")
.publish();

Package::new("dep_that_is_awesome", "1.0.0")
.file(
Expand All @@ -1140,6 +1143,10 @@ fn format() {

[lib]
name = "awesome_dep"

[dependencies]
dep1 = {package="dep", version="<2.0"}
dep2 = {package="dep", version="2.0"}
"#,
)
.file("src/lib.rs", "pub struct Straw;")
Expand All @@ -1156,9 +1163,9 @@ fn format() {
repository = "https://github.com/rust-lang/cargo"

[dependencies]
dep = {version="1.0", optional=true}
dep = {version="=1.0"}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is removing coverage.

I suspect the test changes here should be purely additive rather than mutating existing stuff.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also question our having a single format test and wonder if we should break out your new cases into a separate test or even multiple tests

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes that's a good callout, I can work on splitting this out.

other-dep = {version="1.0", optional=true}
dep_that_is_awesome = {version="1.0", optional=true}
dep_that_is_awesome = {version=">=1.0, <2", optional=true}


[features]
Expand All @@ -1173,6 +1180,7 @@ fn format() {
p.cargo("tree --format <<<{p}>>>")
.with_stdout_data(str![[r#"
<<<foo v0.1.0 ([ROOT]/foo)>>>
└── <<<dep v1.0.0>>>

"#]])
.run();
Expand All @@ -1191,6 +1199,7 @@ Caused by:
p.cargo("tree --format {p}-{{hello}}")
.with_stdout_data(str![[r#"
foo v0.1.0 ([ROOT]/foo)-{hello}
└── dep v1.0.0-{hello}

"#]])
.run();
Expand All @@ -1199,6 +1208,7 @@ foo v0.1.0 ([ROOT]/foo)-{hello}
.arg("{p} {l} {r}")
.with_stdout_data(str![[r#"
foo v0.1.0 ([ROOT]/foo) MIT https://github.com/rust-lang/cargo
└── dep v1.0.0

"#]])
.run();
Expand All @@ -1207,17 +1217,19 @@ foo v0.1.0 ([ROOT]/foo) MIT https://github.com/rust-lang/cargo
.arg("{p} {f}")
.with_stdout_data(str![[r#"
foo v0.1.0 ([ROOT]/foo) bar,default,foo
└── dep v1.0.0

"#]])
.run();

p.cargo("tree --all-features --format")
.arg("{p} [{f}]")
.with_stdout_data(str![[r#"
foo v0.1.0 ([ROOT]/foo) [bar,default,dep,dep_that_is_awesome,foo,other-dep]
foo v0.1.0 ([ROOT]/foo) [bar,default,dep_that_is_awesome,foo,other-dep]
├── dep v1.0.0 []
├── dep_that_is_awesome v1.0.0 []
└── other-dep v1.0.0 []
└── dep v1.0.0 []

"#]])
.run();
Expand All @@ -1227,8 +1239,34 @@ foo v0.1.0 ([ROOT]/foo) [bar,default,dep,dep_that_is_awesome,foo,other-dep]
.arg("--format={lib}")
.with_stdout_data(str![[r#"

├── dep
├── awesome_dep
└── other_dep
└── dep

"#]])
.run();

p.cargo("tree --all-features")
.arg("--format={p} satisfies {ver-req}")
.with_stdout_data(str![[r#"
foo v0.1.0 ([ROOT]/foo) satisfies
├── dep v1.0.0 satisfies ^1.0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Am I misreading things? This says foo has ^1.0 when it instead has:

dep = {version="=1.0"}

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, this version requirement seems to belong to other-dep. I think I see what's happening. We don't consider the parent when registering the version requirement in the lookup map, so it's probably replacing previous entries for the same package?

I think using NodeID as index should make this unique.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a forewarning, a problem I suspect we'll see when we have two different, compatible version requirements on the same package and --invert it is that we'll only see one of them.

version requirements are more of a property of edges and not nodes. We don't annotate edges. Is that good enough, a blocker for this feature, or a cause to re-think some base assumptions?

As an example of the latter, maybe for --invert, we put the version requirement on the dependent package and not the dependency.

├── dep_that_is_awesome v1.0.0 satisfies >=1.0, <2
└── other-dep v1.0.0 satisfies ^1.0
└── dep v1.0.0 satisfies ^1.0

"#]])
.run();

p.cargo("tree --all-features")
.arg("--format={p} satisfies {ver-req}")
.arg("--invert=dep")
.with_stdout_data(str![[r#"
dep v1.0.0 satisfies ^1.0
├── foo v0.1.0 ([ROOT]/foo) satisfies
└── other-dep v1.0.0 satisfies ^1.0
└── foo v0.1.0 ([ROOT]/foo) satisfies

"#]])
.run();
Expand Down