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 --skip-path-deps to skip path dependencies #10135

Closed

Conversation

stevenroose
Copy link

This changes the erroneous behavior to skip path dependencies and adds a
command option to keep the current behavior.

Fixes #10134 and #9172.

@rust-highfive
Copy link

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

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 29, 2021
This changes the erroneous behavior to skip path dependencies and adds a
command option to keep the current behavior.
@stevenroose
Copy link
Author

Added test, fixed build and optimized something I oversaw.

I can understand that it might be good behavior to not automatically duplicate path dependencies that are inside the current path.

So let's look at some different options for behavior:

  1. never vendor path deps (current behavior pre-patch)
  2. never vendor path deps, but add option to vendor all path deps
  3. always vendor all path deps, but add option to skip vendoring (behavior in this patch right now)
  4. vendor all path deps outside of current dir, don't vendor any path deps inside current dir
  5. by default, vendor all path deps outside of current dir and don't vendor any path deps inside current dir, but have flags to vendor all/none

@stevenroose
Copy link
Author

stevenroose commented Nov 30, 2021

Currently, it still suffers from the .cargo/config output being filled with absolute paths instead of relative ones.

EDIT: I can't seem to find a way to work around that. It seems that all "sources" internally get converted right away into full Url's with absolute paths and the original relative URL used is nowhere maintained.

@bors
Copy link
Collaborator

bors commented Dec 13, 2021

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

@joshtriplett
Copy link
Member

In its current form, this would break backwards compatibility, by changing the existing behavior. That's not something we can do lightly; it would take a transition of some kind, assuming we wanted to make that change.

Given that, based on our conversation in today's @rust-lang/cargo meeting, I'm going to close this PR. However, we'd be happy to talk through details and use cases more on #10134.

@hbina
Copy link
Contributor

hbina commented Oct 2, 2023

Would you be opposed to adding an option to do the reverse? i.e. include local paths?

@stevenroose
Copy link
Author

Oh, I missed older notifications to this thread.

In its current form, this would break backwards compatibility, by changing the existing behavior. That's not something we can do lightly; it would take a transition of some kind, assuming we wanted to make that change.

Given that, based on our conversation in today's @rust-lang/cargo meeting, I'm going to close this PR. However, we'd be happy to talk through details and use cases more on #10134.

IMO this is a bug, because this behavior is nowhere documented whatsoever. So are bug fixes considered "breaking backwards compatibility"?

Also, having additional folders in someone's vendor/ directory will almost never bother anyone, we're not removing any vendor directories, which would be way more likely to break anyone's workflow.

I would prefer to have this MR reopened, but I could try convert it into a version where we add --include-path-deps but that's weird as there is no such need for --include-git-deps and --include-crate-io-deps.

@stevenroose
Copy link
Author

stevenroose commented Oct 19, 2023

I pushed an updated version of this branch that is rebased on master.
(It seems like changes to the branch aren't being shows here, I supposed because the MR is closed.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow vendoring of path dependencies
6 participants