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

metadata: Supply local path for path dependencies #8994

Merged
merged 4 commits into from
Jan 6, 2021

Conversation

jonhoo
Copy link
Contributor

@jonhoo jonhoo commented Dec 17, 2020

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

/cc rust-lang/rustfmt#4247

Fixes #7483.

@rust-highfive
Copy link

r? @alexcrichton

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 17, 2020
@alexcrichton
Copy link
Member

To bikeshed a bit, perhaps we could call this path to emphasize that it's only defined for path sources? I think it's basically a historical bug that the source is null for path sources (and we can't change that now), but in some future-format this field would ideally be removed in favor of just being part of source.

With a generic name like manifest_path it indicatess to me that it should be defined for all dependency types since everything has a manifest at some point.

Could you also update the documentation at https://doc.rust-lang.org/cargo/commands/cargo-metadata.html to document this new field?

@jonhoo
Copy link
Contributor Author

jonhoo commented Dec 18, 2020

@alexcrichton I've updated it to path and added documentation in all the various places.

@alexcrichton alexcrichton added the T-cargo Team: Cargo label Dec 18, 2020
@alexcrichton
Copy link
Member

@rfcbot fcp merge

This is adding a new path key to the dependencies arrays for cargo metadata to determine where path sources point. Currently Cargo just simply says null for the path so consumers know that existing dependencies are path dependencies but they don't know where the edges point to. This means that non-workspace-members which are path dependencies cannot be found with cargo metadata --no-deps, which is a use-case that rustfmt would like to support.

@rfcbot
Copy link
Collaborator

rfcbot commented Dec 18, 2020

Team member @alexcrichton has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period An FCP proposal has started, but not yet signed off. disposition-merge FCP with intent to merge labels Dec 18, 2020
@jonhoo jonhoo changed the title Supply manifest path for path dependencies metadata: Supply local path for path dependencies Dec 19, 2020
@ehuss
Copy link
Contributor

ehuss commented Dec 19, 2020

I'm not sure if this completely solves the cargo fmt use case. If there is more than one level of path dependencies (like foo→bar→baz), this will only expose the first level of path dependencies with --no-deps. Is cargo fmt expected to still recursively run cargo metadata? That does not seem ideal, as it can be slow and tricky to do that correctly.

When I was looking at this in the past, I was considering a few different options:

  • Change Cargo to always include path dependencies as "members" of a workspace, even if there is no [workspace] table. The consequence of this is that all path dependencies will be displayed in cargo metadata --no-deps. I don't think there are too many other observable differences, though it is a pretty drastic change.

  • Alternatively, just change cargo metadata to include all path dependencies even with --no-deps (but otherwise don't treat them as "members").

Changing the behavior of cargo metadata --no-deps has some risk, but I'm not aware of anything in particular that would break with these changes.

@jonhoo
Copy link
Contributor Author

jonhoo commented Dec 19, 2020

@ehuss My impression from @calebcartwright was that these changes would be meaningful improvement for cargo fmt. Even though they would then have to recurse, it would at least immediately fix an issue that currently can make cargo fmt --all quite slow (much slower than recursion is likely to cause.

I agree that we probably want a more complete fix to this down the line to make the recursion unnecessary. However, such a fix would also need to include the path to any such local dependencies anyway, so this change seems (at least to me) to be a subset of the ultimate fix. And given that it gives tangible benefits in its current form, I feel like it's probably worthwhile to merge this now, and then make the larger adjustment subsequently? Especially since those larger adjustments you suggest probably also carry with them more risk, since they will cause additional dependencies to be listed that weren't before.

@ehuss
Copy link
Contributor

ehuss commented Dec 19, 2020

this change seems (at least to me) to be a subset of the ultimate fix.

I'm not sure I see that. If it listed all path-packages, then cargo fmt could just look at the existing manifest_path field of the Package.

@calebcartwright
Copy link
Member

Is cargo fmt expected to still recursively run cargo metadata? That does not seem ideal, as it can be slow and tricky to do that correctly.

Just want to reiterate for local context on this thread that we already do exactly this today in cargo fmt, the only difference on the rustfmt side with this patch would be that we'd be able plugin the --no-deps flag which in turn solves quite a few real problems our users are experiencing.

If there's a better holistic solution then I can understand the preference for that, though if such a solution is a long way off from being available then I do think the adage about not letting perfection be the enemy of the good (enough) is worth keeping in mind

@ehuss
Copy link
Contributor

ehuss commented Dec 22, 2020

I don't think the alternative is necessarily a long way off. I think I personally would be fine with changing cargo metadata --no-deps to include all path dependencies. There is some risk that could confuse some tools, but I think the risk is very small. That is the behavior in a [workspace], and the difference between the two is very minor.

I haven't looked at implementing it, but I would not expect it to be too difficult.

@jonhoo
Copy link
Contributor Author

jonhoo commented Dec 22, 2020

@ehuss So if I'm understanding you correctly, you'd want path dependencies to appear in the packages array? That seems fairly odd to me, given that they aren't actual packages in the current workspace. Or are you saying that transitive path dependencies should also appear in the dependencies array?

I could try to find some time to build your alternative proposal, but I don't have a good sense for how to even go about finding all the transitive path dependencies to inject into packages. Some pointers would be welcome :)

@alyssais
Copy link

I have implemented the alternative proposal (which I like much better as it avoids the user needing to recursively call cargo) in #9024.

@alexcrichton
Copy link
Member

I'm a little skeptical about a solution like #9024 myself. I realize that not having that forces a rustfmt-like-use-case to call cargo multiple times, however.

With #9024, though, where we try to provide information about path dependencies I'm worried about a package showing up with cargo metadata --no-deps and then not showing up with cargo metadata. As-written #9024 pulls in all workspace members which can include extra crates that aren't part of the original dependency graph. Even if we just iterated over package dependencies though we would perhaps consider optional dependencies or dev-dependencies not part of the original graph. Basically I'd expect that cargo metadata --no-deps prints a subset of the information of cargo metadata, but I don't think without constructing the resolve graph we can reasonably do that. I'm not sure this has huge practical ramifications but it does feel pretty weird to me.

That being said I don't know how to best implement the desired rustfmt behavior. Is the desired behavior to format everything on the local filesystem that you can? Or is it to only format the members in Cargo.lock (without actually having to compute Cargo.lock)?

@calebcartwright
Copy link
Member

That being said I don't know how to best implement the desired rustfmt behavior. Is the desired behavior to format everything on the local filesystem that you can?

I think the simplest way to describe the ask is for the ability to utilize cargo to get basic information about all members, implicit/local and explicit, within a workspace without necessarily requiring network access.

rustfmt wants this feature, because there's an assumption (IMO a reasonable one) users have that running cargo fmt --all will result in your entire workspace being formatted, regardless of implicit vs. explicit membership; they just want the project formatted. At the end of the day though, we don't particularly care how that capability gets surfaced.

Setting aside the explicit rustfmt use case though, as a user inspecting the cargo metadata output it seems a bit odd to me that source is set to null for local dependencies. There's of course an actual/known value for that but which is stripped/set to null in the metadata json, seemingly for unrelated (perhaps unintentional?) reasons based on #7483 (comment) 🤷‍♂️

@alexcrichton
Copy link
Member

It also sort of depends on the precise desires of rustfmt, if you're ok formatting packages not actually in the current project's compile graph, then something like #9024 would work alright. Otherwise I believe it's impossible to precisely know the set of path dependencies for a project without network access (e.g. without actually constructing the resolution graph). The resolution graph takes into account features that could be activated, and if you have [patch] pointing at a local package then we won't know what features are activated unless we touch the network.

Basically what I'm trying to say is that I think fundamentally rustfmt either needs to touch the network (calculate the full resolution graph) or be ok formatting packages which may not be part of the resolution graph. The latter capability, working with packages not part of the resolution graph, seems like it shouldn't be encoded or implemented in Cargo itself but instead rely on cargo fmt running cargo multiple times as an implementation-specific detail of rustfmt (acknowledging it may reformat code not part of the current project).

@jonhoo
Copy link
Contributor Author

jonhoo commented Jan 4, 2021

@alexcrichton My guess is that cargo fmt wants the latter, since formatting "too much" when --all is passed is probably not a case that users would complain about. In which case it sounds like you're more in favor of the approach this PR takes than #9024?

@rfcbot rfcbot added final-comment-period FCP — a period for last comments before action is taken and removed proposed-final-comment-period An FCP proposal has started, but not yet signed off. labels Jan 4, 2021
@rfcbot
Copy link
Collaborator

rfcbot commented Jan 4, 2021

🔔 This is now entering its final comment period, as per the review above. 🔔

@calebcartwright
Copy link
Member

It also sort of depends on the precise desires of rustfmt, if you're ok formatting packages not actually in the current project's compile graph, then something like #9024 would work alright. Otherwise I believe it's impossible to precisely know the set of path dependencies for a project without network access (e.g. without actually constructing the resolution graph). The resolution graph takes into account features that could be activated, and if you have [patch] pointing at a local package then we won't know what features are activated unless we touch the network.

@alexcrichton - Thank you for this! I think the way you worded it has helped me finally understand where the challenge/friction may be coming from.

From a "format everything" perspective (aka cargo fmt --all), rustfmt absolutely wants to format everything, even those things which may fall outside that compile/resolution graph in a given context. In my mind this is somewhat analogous to folks using cfg_if to utilize different mods, but they still want rustfmt to format all of those mods.

What we're really looking for to support the rustfmt use case is "for this given package, i want the path value for all { path = 'foo/bar' } instances within that package's Cargo.toml file", whereas cargo metadata is explicitly focused on the resolved dependencies for said package. rustfmt's trying to leverage cargo metadata to ultimately achieve the former, but that's obviously not what it's intended for.

be ok formatting packages which may not be part of the resolution graph.

Agreed (not only okay with it, but actually the desired behavior).

The latter capability, working with packages not part of the resolution graph, seems like it shouldn't be encoded or implemented in Cargo itself but instead rely on cargo fmt running cargo multiple times as an implementation-specific detail of rustfmt (acknowledging it may reformat code not part of the current project)

Also agreed. cargo fmt already runs cargo multiple times recursively as many times as needed, and no matter what does/doesn't happen we'd have to continue to support that for a long time anyway (to maintain BC with older versions of cargo)

@alexcrichton
Copy link
Member

Ok we got a chance to talk about this today in the Cargo team meeting, and the conclusion was that we're ok going ahead and landing this PR but would prefer to hold off on #9024. Our thinking is that rustfmt will continue to call cargo multiple times to learn about path dependencies but with --no-deps it will be much faster than it is today. If this is still too slow, though, then we could consider solutions like #9024. Does that sound ok for everyone?

(I'm gonna hold off on approving for @ehuss's final sign-off as well)

@ehuss
Copy link
Contributor

ehuss commented Jan 6, 2021

We talked about this in the team meeting and decided to go ahead and merge this, and maybe hold off on #9024 for now. Thanks @jonhoo for pushing this forward!

@jonhoo or @calebcartwright are you interested in updating cargo fmt to support this when it is available? If not, I can try to get to it, but it might be a while.

@bors r+

@bors
Copy link
Contributor

bors commented Jan 6, 2021

📌 Commit fd25f93 has been approved by ehuss

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 6, 2021
@bors
Copy link
Contributor

bors commented Jan 6, 2021

⌛ Testing commit fd25f93 with merge 329895f...

@calebcartwright
Copy link
Member

Does that sound ok for everyone?

Absolutely!

@jonhoo or @calebcartwright are you interested in updating cargo fmt to support this when it is available?

Updating cargo fmt to take advantage of this (once available) will be pretty high up on my rustfmt list, though if @jonhoo (or anyone else) gets around to a PR before me that'd be swell too 👍

@bors
Copy link
Contributor

bors commented Jan 6, 2021

☀️ Test successful - checks-actions
Approved by: ehuss
Pushing 329895f to master...

@bors bors merged commit 329895f into rust-lang:master Jan 6, 2021
@jonhoo
Copy link
Contributor Author

jonhoo commented Jan 6, 2021

@calebcartwright I will probably not beat you to it, though I did file a PR with cargo_metadata: oli-obk/cargo_metadata#149

@jonhoo jonhoo deleted the manifest-in-local-deps-meta branch January 6, 2021 00:47
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Jan 7, 2021
Update cargo

12 commits in 75d5d8cffe3464631f82dcd3c470b78dc1dda8bb..329895f5b52a358e5d9ecb26215708b5cb31d906
2020-12-22 18:10:56 +0000 to 2021-01-06 00:01:52 +0000
- metadata: Supply local path for path dependencies (rust-lang/cargo#8994)
- Add support for Rust edition 2021. (rust-lang/cargo#8922)
- Stabilize -Zfeatures and -Zpackage-features. (rust-lang/cargo#8997)
- Small refactor, adding a list of all kinds to BuildContext (rust-lang/cargo#9046)
- Fix git http.proxy config setting. (rust-lang/cargo#8986)
- Clarify the help text of `--aggressive` and `--precise` of `update` (rust-lang/cargo#9031)
- Assert that tests are run in the crate directory (rust-lang/cargo#9037)
- Update mdbook (rust-lang/cargo#9044)
- Bump to 0.52.0, update changelog (rust-lang/cargo#9042)
- Fix redundant semicolon. (rust-lang/cargo#9033)
- Clarify fingerprint log messages (rust-lang/cargo#9026)
- Update credential docs for gnome-secret. (rust-lang/cargo#9013)
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 8, 2021
Update cargo

12 commits in 75d5d8cffe3464631f82dcd3c470b78dc1dda8bb..329895f5b52a358e5d9ecb26215708b5cb31d906
2020-12-22 18:10:56 +0000 to 2021-01-06 00:01:52 +0000
- metadata: Supply local path for path dependencies (rust-lang/cargo#8994)
- Add support for Rust edition 2021. (rust-lang/cargo#8922)
- Stabilize -Zfeatures and -Zpackage-features. (rust-lang/cargo#8997)
- Small refactor, adding a list of all kinds to BuildContext (rust-lang/cargo#9046)
- Fix git http.proxy config setting. (rust-lang/cargo#8986)
- Clarify the help text of `--aggressive` and `--precise` of `update` (rust-lang/cargo#9031)
- Assert that tests are run in the crate directory (rust-lang/cargo#9037)
- Update mdbook (rust-lang/cargo#9044)
- Bump to 0.52.0, update changelog (rust-lang/cargo#9042)
- Fix redundant semicolon. (rust-lang/cargo#9033)
- Clarify fingerprint log messages (rust-lang/cargo#9026)
- Update credential docs for gnome-secret. (rust-lang/cargo#9013)
@rfcbot rfcbot added finished-final-comment-period FCP complete to-announce and removed final-comment-period FCP — a period for last comments before action is taken labels Jan 14, 2021
@ehuss ehuss added this to the 1.51.0 milestone Feb 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-merge FCP with intent to merge finished-final-comment-period FCP complete S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-cargo Team: Cargo to-announce
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Include path/url of local dependencies in cargo metadata --no-deps output
8 participants