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

Fix: cargo vendor can't handle duplicates. #13271

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

junjihashimoto
Copy link

@junjihashimoto junjihashimoto commented Jan 9, 2024

What does this PR try to resolve?

part of #10310
Add --no-merge-sources options for cargo-vendor to handle duplicates and offline-build.
This is a reimplementation of alexcrichton/cargo-vendor#96.
CC: @ljli @alexcrichton.

The vendor directory and cargo.toml with --no-merge-sources become as follows.

vendor/
├── git-2214abe90aa91c00
│   └── weechat
├── git-e800b3691875ab00
│   └── serenity
└── registry-40351f815f426200
    ├── addr2line
    xxx
[source.crates-io]
replace-with = "vendor+crates-io"

[source."git+https://github.com/terminal-discord/rust-weechat?rev=d49cdd0"]
git = "https://github.com/terminal-discord/rust-weechat"
rev = "d49cdd0"
replace-with = "vendor+git+https://github.com/terminal-discord/rust-weechat?rev=d49cdd0"

[source."git+https://github.com/terminal-discord/serenity?rev=c4ae4c61"]
git = "https://github.com/terminal-discord/serenity"
rev = "c4ae4c61"
replace-with = "vendor+git+https://github.com/terminal-discord/serenity?rev=c4ae4c61"

[source."vendor+crates-io"]
directory = "vendor/registry-40351f815f426200"

[source."vendor+git+https://github.com/terminal-discord/rust-weechat?rev=d49cdd0"]
directory = "vendor/git-2214abe90aa91c00"

[source."vendor+git+https://github.com/terminal-discord/serenity?rev=c4ae4c61"]
directory = "vendor/git-e800b3691875ab00"

The vendor directory without --no-merge-sources becomes as follows.

vendor/
├── weechat
├── serenity
├── addr2line
xxx

How should we test and review this PR?

Check out the unit tests.

Additional information

None.

@rustbot
Copy link
Collaborator

rustbot commented Jan 9, 2024

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. 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 Jan 9, 2024
@junjihashimoto junjihashimoto force-pushed the feature/no-merge-sources branch from edf2ba7 to 3ee1557 Compare January 9, 2024 14:50
Copy link
Member

@weihanglo weihanglo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you brief the design in the PR description? such as how the layout of vendor directory look like, and the difference between with and without --no-merge-sources?

Note that the flag was proposed but at that time the Cargo team prefered to do it automatically without a flag: #10344 (review)

@junjihashimoto
Copy link
Author

junjihashimoto commented Jan 9, 2024

In the case of automation, the use cases may be different and are not compatible for current users.

@junjihashimoto
Copy link
Author

If it is better to use #10344 as a base, I will consider that.

@junjihashimoto
Copy link
Author

@weihanglo Do you actually have a schedule to fix this issue?

@weihanglo
Copy link
Member

For myself, I would wait for FCP in rust-lang/rfcs#3243 (comment) and see if we can avoid adding an extra flag. We can perhaps think ahead how the layout should be like with namespaced package feature.

That being said, if there is any proposal with better UX, feel free to share.

(General design discussions are expected to happen in the issue #10310 and leave PR for specific implementation btw)

@junjihashimoto
Copy link
Author

junjihashimoto commented Jan 15, 2024

@weihanglo Thank you for pointing it out.
rust-lang/rfcs#3243 (comment) looks like the individual packages themselves need to be modified with using namespaces when using the feature.
If such a fix is possible, the feature like this PR is not needed.
This PR is effective when the dependencies of the package we are using are complex and it is difficult to adjust the version for each package.

The rfc does not include what namespace to use when linking git like this NPM.
It seems difficult to resolve this issue without a new RFC.

@haraldh
Copy link

haraldh commented Feb 16, 2024

@junjihashimoto Thank you very much! This has been a "life" saver.

@bors
Copy link
Contributor

bors commented Feb 23, 2024

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

@junjihashimoto junjihashimoto force-pushed the feature/no-merge-sources branch from 3ee1557 to 663eea9 Compare February 23, 2024 19:35
@junjihashimoto
Copy link
Author

@bors The merge is resolved. Thx!

@junjihashimoto junjihashimoto force-pushed the feature/no-merge-sources branch from 663eea9 to 96303f1 Compare February 23, 2024 19:41
@junjihashimoto junjihashimoto force-pushed the feature/no-merge-sources branch from 96303f1 to f97a61c Compare February 23, 2024 19:42
@bors
Copy link
Contributor

bors commented Mar 19, 2024

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

@ehuss
Copy link
Contributor

ehuss commented Apr 22, 2024

I'm not quite clear, why does this need a new flag? Why can't it just use separate directories when there is a conflict?

@junjihashimoto
Copy link
Author

I think it's due to different use cases.
With the current default, if a user wants to find a conflicting package, it can be detected as an error.
Automatic collision avoidance will not find conflicting packages.

I would like to be able to build with automatic collision avoidance.
Should I change to that?

@weihanglo
Copy link
Member

With the current default, if a user wants to find a conflicting package, it can be detected as an error.
Automatic collision avoidance will not find conflicting packages.

In this case they should use tools like cargo-deny instead of cargo vendor.

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-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants