-
Notifications
You must be signed in to change notification settings - Fork 2.7k
Add format option to cargo tree to print the package version requirement
#16192
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Add format option to cargo tree to print the package version requirement
#16192
Conversation
29b53e0 to
92544e2
Compare
src/cargo/ops/tree/graph.rs
Outdated
| } | ||
|
|
||
| #[derive(Debug, Clone, Eq, PartialEq, Hash)] | ||
| pub struct NodePackage { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why was this split out?
Also, best if splitting something out like this be left to a refactor commit before the main commit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just pushed this change as an attempt to scope down the Ord-related changes. Trying to implement Ord for Node was quite ugly because of the enum branches. Before that, I also tried wrapping the version requirement into another type and directly implementing Ord on OptVersionReq.
I'm open to suggestions on the cleanest way to achieve this, what do you think we should do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not particularly happy with the latest attempt using a new struct NodePackage. Feels rather verbose simply to ignore a field. I can think of two other ways to approach this: either implement Ord/Eq on OptVersionReq, or wrap OptVersionReq in a type that does. There are also third party crate that allows ignoring fields in derived impl, such as "derivative" (https://docs.rs/derivative/latest/derivative/) but unsure we need another dependency just for that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not particularly happy with the latest attempt using a new struct NodePackage. Feels rather verbose simply to ignore a field.
Looking closer, ignoring a field is likely not the right approach.
For instance, in one place we rely on Ord we have the comment
// Created a sorted list for consistent output.
let mut edges = edges.to_owned();
edges.sort_unstable_by(|a, b| self.node(a.node()).cmp(&self.node(b.node())));If you added a test case for multiple deps to the same package, like below, I expect it could end up with less pleasant results
[dependencies]
clap3 = { package = "clap", version = "3" }
clap4 = { package = "clap", version = "4" }Also, best if splitting something out like this be left to a refactor commit before the main commit.
FYI the refactor commit I was looking for was to have no behavior or logic changes, which means it wouldn't have the version requirement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting. Alright I can take a look at implementing Ord on OptVersionReq in a way that makes the most sense.
Also, best if splitting something out like this be left to a refactor commit before the main commit.
FYI the refactor commit I was looking for was to have no behavior or logic changes, which means it wouldn't have the version requirement.
Sounds good, will move the version requirement changes to the next commit where we update the tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I pushed an alternate implementation that does not require storing additional data on the Node itself.
Instead, we record the version requirement in the Graph mapped by PackageID. We have already other data recorded the same way so I thought it was fitting for this use-case as well.
One thing I am not sure is whether we should default to OptVersionReq::Any for packages without version requirement (root packages). This was the behavior I went with previously, but looking back now, I noticed that some format args like license conditionally write a value, therefore I replicated here as well. Only thing is if we use a format string like the one in tests with {p} satisfies {ver-req}, it shows as: dep satisfies which doesn't feel great.
I'd like to know what people think of this. I personally am divided 😄
src/cargo/ops/tree/format/mod.rs
Outdated
| RawChunk::Argument("r") => Chunk::Repository, | ||
| RawChunk::Argument("f") => Chunk::Features, | ||
| RawChunk::Argument("lib") => Chunk::LibName, | ||
| RawChunk::Argument("c") => Chunk::Constraint, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thats unfortunate that r is taken as we tend to refer to this as a version requirement, not constraint. As it corresponds to the version field, v might be ok but that could mean something else
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah naming is hard, thanks for the clarification 😄 We have lib as precedent for multi-letters, we could use vr as version requirement if v could be ambiguous with version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or just ver-req, less ambiguous . I don't feel like we need to pursue shortness here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I hadn't done that because of the precedent for single characters.
@ehuss why are the single characters used? Should we treat this like the CLI and offer long forms while short forms are only used as shortcuts in very specific cases?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know. I assume it is because shorter forms are more convenient. One would need to look through the history in the original repo to see if that was ever mentioned.
I think using longer forms makes sense, with shortcuts only for common options.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I posted #16204.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For myself, I tend to shorten "version requirement" to "version-req" but I could see us going in a lot of different directions.
54706e8 to
f28f30f
Compare
cargo tree to print the package version constraintcargo tree to print the package version requirement
f28f30f to
bf10d3c
Compare
tests/testsuite/cargo_tree/deps.rs
Outdated
| foo v0.1.0 ([ROOT]/foo) satisfies * | ||
| ├── dep v1.0.0 satisfies =1.0 | ||
| ├── 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
both paths to dep involve the same version requirement. It would help if they were different.
bf10d3c to
5dbbb18
Compare
Signed-off-by: Alexandre Barone <[email protected]>
Update cargo tree format tests and introduce different version requirements and new placeholder tests to prepare for the next commits which will add a new formatting option to display package version requirements. Signed-off-by: Alexandre Barone <[email protected]>
Also update unit tests with test cases for normal and inverted output. Signed-off-by: Alexandre Barone <[email protected]>
Signed-off-by: Alexandre Barone <[email protected]>
5dbbb18 to
9381123
Compare
| [dependencies] | ||
| dep = {version="1.0", optional=true} | ||
| dep = {version="=1.0"} |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 v1.0.0 | ||
| └── dep v1.0.0 | ||
| foo v0.1.0 ([ROOT]/foo) satisfies | ||
| ├── dep v1.0.0 satisfies ^1.0 |
There was a problem hiding this comment.
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"}There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Description
{ver-req}tocargo treethat prints the resolved version requirement of packages as specified in Cargo.toml.cargo treecommand documentation with the new option.As per the issue linked below, I implemented the second suggestion which was to add a new formatting option.
Adding a new flag would also be relatively simple, but I'd like to know what people think before adding a commit.
Fixes: #16163
Example
Example output using the cargo repo:
cargo tree output