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

vendor: Add --include-path-deps to include path dependencies #12858

Closed

Conversation

stevenroose
Copy link

@stevenroose stevenroose commented Oct 19, 2023

Fixes #9172, #10134.
Replacement of #10135.

I would just like to say that I personally think it's incorrect that path dependencies are skipped by default (especially without this being documented explicitly anywhere). I think #10135 is a more correct fix for this behavior that I consider a bug. However, since it seems that #10135 is not considered a bugfix by the cargo devs, I would like to propose this alternative solution to make a flag that explicitly includes path dependencies.

This means that using the flag will break with existing behavior, but using the flag will also fix the current documented behavior.

@rustbot
Copy link
Collaborator

rustbot commented Oct 19, 2023

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @weihanglo (or someone else) soon.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

  • @rustbot author: the review is finished, PR author should check the comments and take action accordingly
  • @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

@rustbot rustbot added A-cli Area: Command-line interface, option parsing, etc. Command-vendor S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 19, 2023
This changes the erroneous behavior to skip path dependencies and adds a
command option to keep the current behavior.
@weihanglo
Copy link
Member

While the effort is pretty much appreciated, the current approach doesn't seem to work.

As ehuss mentioned earlier, source replacement doesn't support path dependencies. The current output vendor-config is like

[source."path+file:///home/user/cargo/target/tmp/cit/t0/foo/bar"]
directory = "file:///home/user/cargo/target/tmp/cit/t0/foo/bar"
replace-with = "vendored-sources"

[source.vendored-sources]
directory = "vendor"

It makes bar a directory source at ~/cargo/target/tmp/cit/t0/foo/bar, but the bar is not really a directory source. If you add the config to your .cargo/config.toml and run cargo build --verbose, you'll see that cargo won't pick the vendored bar from vendor/bar.

We'd happy to discuss use cases, workflows and design first in #10134, and see which approach fits better with the community need and the current architecture of Cargo.

@weihanglo weihanglo added S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 29, 2023
@bors
Copy link
Collaborator

bors commented Nov 12, 2023

☔ The latest upstream changes (presumably #12959) made this pull request unmergeable. Please resolve the merge conflicts.

@weihanglo
Copy link
Member

I am going to close this and keep the alternative #13347 open. #13347 is a proof-of-concept that addresses the issue mentioned in #12858 (comment). It supports path sources in source replacement so that Cargo can pick up correct sources.

Thanks for the pull request!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-cli Area: Command-line interface, option parsing, etc. Command-vendor S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cargo vendor with patch section and local sources does not vendor the local files
4 participants