cargo icon indicating copy to clipboard operation
cargo copied to clipboard

Fix: cargo vendor can't handle duplicates.

Open junjihashimoto opened this issue 5 months ago • 12 comments

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 https://github.com/alexcrichton/cargo-vendor/pull/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.

junjihashimoto avatar Jan 09 '24 14:01 junjihashimoto

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 avatar Jan 09 '24 14:01 rustbot

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

junjihashimoto avatar Jan 09 '24 15:01 junjihashimoto

If it is better to use https://github.com/rust-lang/cargo/pull/10344 as a base, I will consider that.

junjihashimoto avatar Jan 09 '24 15:01 junjihashimoto

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

junjihashimoto avatar Jan 09 '24 15:01 junjihashimoto

For myself, I would wait for FCP in https://github.com/rust-lang/rfcs/pull/3243#issuecomment-1879610048 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)

weihanglo avatar Jan 15 '24 00:01 weihanglo

@weihanglo Thank you for pointing it out. https://github.com/rust-lang/rfcs/pull/3243#issuecomment-1879610048 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.

junjihashimoto avatar Jan 15 '24 08:01 junjihashimoto

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

haraldh avatar Feb 16 '24 13:02 haraldh

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

bors avatar Feb 23 '24 18:02 bors

@bors The merge is resolved. Thx!

junjihashimoto avatar Feb 23 '24 19:02 junjihashimoto

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

bors avatar Mar 19 '24 16:03 bors

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?

ehuss avatar Apr 22 '24 16:04 ehuss

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?

junjihashimoto avatar Apr 22 '24 16:04 junjihashimoto