Make cargo metadata --no-deps print all path deps (including other workspaces) #9024
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
rust-highfive
added
the
S-waiting-on-review
Including those outside the current workspace. This allows for finding all local code without having to recursively call Cargo, which is useful for rustfmt and other tools. Fixes: rust-lang#7483
Workspace::new is very expensive. Doing it over and over again like the initial implementation made cargo metadata --no-deps take several minutes in a big workspace. By only calling Workspace::new once per workspace, we can save a lot of time. Now the same command in the same workspace takes only 0.16 seconds.
|
Thanks for the PR! We discussed this in the team meeting, and decided to move forward with #8994 for now, and to put this on hold. There are some concerns noted in #8994 (comment) that this may not be so straightforward. @alyssais will #8994 address what you are interested in? I'm fine with leaving this PR open for now if you'd like, until we have more experience with #8994 to see how the performance is. |
|
@alyssais will #8994 address what you are interested in? I'm fine with
leaving this PR open for now if you'd like, until we have more
experience with #8994 to see how the performance is.
Let me share a little about my use case:
I'm working on tooling to provide better packaging of Cargo projects in
Nixpkgs. Part of this is that given a git checkout that I know contains
the crate I'm interested in, find the Cargo.toml for that crate. I
figure "look up the Cargo.toml for a local crate given its name and
version" is probably something that would be useful for lots of tooling
built on Cargo -- I don't think it matters to users what workspace it's
in, as long as it's a local (path-only) dependency somewhere in the
dependency graph, and this seems to mirror what the rustfmt people in
#8994 say is their use case too.
#8994 certainly makes it possible for me to find this information, but
in a very roundabout way. First, I'm going to now need to call Cargo
once per workspace to hunt for my crate until I find it, which doesn't
sound so bad, except that there's no way to actually get a list of local
workspaces, so now I have to construct that. And constructing that is
very complicated, because I have to iterate over the dependencies, and
run cargo metadata on those to discover their dependencies, recursively.
And to do this at all efficiently, I have to keep a cache of which
workspaces I've visited, so when I visit them again I don't iterate
through all of their dependencies over and over again.
And now what was a simple cargo | jq shell pipeline becomes a custom
program that I have to write and maintain, and I have to think about
recursion and caching and get that all right. And rustfmt, seperately,
also has to do all those things. And the next tool that wants to
operate on an entire local project _also_ has to do all these things.
And lots of neat things probably just can't happen, because if I want to
write a quick shell script or something to loop through all my local
crates, there's just no way to do that. This is something I've wanted
to do before and given up on because the complexity of having to write
that code myself for a one-off is just too high.
So #8994 makes it possible to do this, but it doesn't make it
accessible. It'll be fine for me, since the problem I want to solve is
very important for us and I can spend a great deal of time implementing
all the fiddly logic correctly. I didn't have to implement this in
Cargo -- my reasons for doing so were partially that Cargo already has
types and stuff for this, but mostly because there's a clear need from
other people for the same functionality and I wanted it to be easily
accessible to them as well. And so even though it sounds like I will
have to go and write that program, I would still like to see Cargo offer
a way to do this to prevent different people having to figure out the
same complicated tree-walking code over and over again.
Other things Cargo could do to enable this:
- Provide a way to print all local workspaces. That turns a tree
traversal into a simple iteration. But I don't know where it would
fit into the Cargo CLI, whereas the approach in this PR felt very
natural.
- Create a new mode for cargo metadata, maybe cargo metadata
--local-workspaces or something. I'll admit it's a bit strange to
include local dependencies when a flag called --no-deps is given, and
I actually thought about proposing to rename it to --no-remote-deps or
something.
|
|
I've just run into a problem that is very similar to what @alyssais solved with this PR. Do you have any thoughts regarding rethinking the decision you took here? I also want to known the path of a given workspace member within a hermetic build. |
|
☔ The latest upstream changes (presumably #10269) made this pull request unmergeable. Please resolve the merge conflicts. |
|
Since #8994 has landed, I think this PR can be closed. |
|
☔ The latest upstream changes (possibly 081d7ba) made this pull request unmergeable. Please resolve the merge conflicts. |
Yet another solution to #7483. (See also #8988, #8994.)
This solution avoids users to get all the information they need with a single cargo invocation, rather than having to recursively call cargo themselves.
The first commit implements the functionality, and the second speeds it up to the point of feasibility, at the cost of some code complexity.