Skip to content
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

Include path/url of local dependencies in cargo metadata --no-deps output #7483

Closed
calebcartwright opened this issue Oct 4, 2019 · 5 comments · Fixed by #8994
Closed

Include path/url of local dependencies in cargo metadata --no-deps output #7483

calebcartwright opened this issue Oct 4, 2019 · 5 comments · Fixed by #8994
Labels
C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted` Command-metadata

Comments

@calebcartwright
Copy link
Member

calebcartwright commented Oct 4, 2019

Describe the problem you are trying to solve

We're running into some challenges over in rustfmt due to the output of the cargo metadata command missing the source path for local dependencies when using the --no-deps flag with cargo metadata.

Describe the solution you'd like

Ideally, in the output of cargo metadata --no-deps, the source field for local dependencies would include the path value instead of being null.

Notes

cargo fmt uses the cargo metadata command to get info about the workspace to pass as inputs to rustfmt. One of the modes cargo fmt supports is --all which allows rustfmt to also format local dependencies. In order to be able to do this, cargo fmt needs to be able locate the manifest path for the local dependency.

However, the serialization for Dependency results in the path value for local dependencies being stripped, so in the output from cargo metadata, local dependencies have "source": null, and the output of cargo metadata --no-deps does not contain any other info cargo fmt can use to definitively determine the manifest path of the local dependency. This is problematic for cargo fmt, especially since the directory name where the local dependency resides and the name of the dependency can differ - rust-lang/rustfmt#3643

We've been working around the missing data by running cargo metadata (without the --no-deps flag) since that output does allow us to correctly determine the manifest_path value for local dependencies. However, running cargo metadata without --no-deps is creating some other challenges for us, and we'd really like to be able to get everything needed via cargo metadata --no-deps.

AFAICT, for path-based dependencies, the value is stripped deliberately from the serialized output:

impl ser::Serialize for SourceId {
fn serialize<S>(&self, s: S) -> Result<S::Ok, S::Error>
where
S: ser::Serializer,
{
if self.is_path() {
None::<String>.serialize(s)
} else {
s.collect_str(&self.into_url())
}
}
}

I'm sure that's done for good reason, though I'm wondering/hoping that those reasons may not be applicable for cargo metadata output and that it would be possible to keep that value for local path-based dependencies included just in the cargo metadata output (perhaps by using different struct representations)?

cc @topecongiro

@calebcartwright calebcartwright added the C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted` label Oct 4, 2019
@alexcrichton
Copy link
Member

Thanks for the report! That serialization of SourceId was actually done for Cargo.lock where we don't want to encode paths but otherwise including paths for cargo metadata seems fine to me! We probably just need to switch Cargo.lock serialization to use a custom serialization and the default serialization would include path information.

@sfackler
Copy link
Member

You can back the directory out of the manifest_path field, but it would be nice for source to be properly populated.

@sschuberth
Copy link

sschuberth commented Dec 8, 2020

You can back the directory out of the manifest_path field, but it would be nice for source to be properly populated.

I don't see a manifest_path field in the output of cargo metadata... what am I missing?

Edit: Nevermind, I was looking at the nodes instead of the packages section.

@calebcartwright
Copy link
Member Author

Just to clarify on the original ask here, we're in need of the source information not being stripped for local dependencies of the packages. I know there's a manifest_path on the packages nodes, but I see no such manifest_path on the dependency nodes, just a null source field.

A null source field on a dependency lets us know it's local, but we've no idea from that metadata output where to locate that dependency. This happens often with implicit workspace members.

@jonhoo
Copy link
Contributor

jonhoo commented Dec 17, 2020

I'm digging into this at the moment and will submit a PR shortly. Interestingly, it looks like none of the tests fail if I simply remove the is_path() clause and update the tests that currently expect null in the output from cargo metadata? @alexcrichton what specifically is the concern with lockfile output? I can't seem to find a way to have the path source appear in a lockfile (or get any tests to fail) if I simply comment out the nulling logic.

jonhoo pushed a commit to jonhoo/cargo that referenced this issue Dec 17, 2020
Previously, the location for path-based (i.e., local) packages was
redacted from metadata output. This was done way back when so that
`Cargo.lock` would not include path sources:
rust-lang#7483 (comment)

This PR makes this redaction optional so that `cargo metadata` and
friends _will_ output the source location, which is useful for tools
like `rustfmt` which need to know the paths to local packages (see rust-lang#7483
and rust-lang/rustfmt#4599).

Interestingly enough, no tests fail even though
`disable_path_serialization` is never currently called. I don't know if
that's because there's no test that cover where redaction is needed, or
because redaction is simply no longer needed (presumably because
lockfile generation now uses custom logic). If it is the latter, this PR
can be simplified to not support opting into redaction.

Fixes rust-lang#7483
alyssais added a commit to alyssais/cargo that referenced this issue Dec 29, 2020
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
bors added a commit that referenced this issue Jan 6, 2021
metadata: Supply local path for path dependencies

This is potentially a simpler way to address #7483 than #8988.

/cc rust-lang/rustfmt#4247

Fixes #7483.
@bors bors closed this as completed in 17b129a Jan 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted` Command-metadata
Projects
None yet
6 participants