Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

cargo add refuses to add new non-vendored packages due to source replacement #10729

Closed
k3d3 opened this issue Jun 4, 2022 · 13 comments
Closed
Assignees
Labels
C-bug Category: bug Command-add S-accepted Status: Issue or feature is accepted, and has a team member available to help mentor or review

Comments

@k3d3
Copy link

k3d3 commented Jun 4, 2022

Problem

When a vendored directory is established, cargo add no longer adds new packages. Instead, it tries to translate a package name into a package that already exists in the vendored directory.

In this case, I've added xml-rs, then proceeded to vendor my dependencies. Now, no matter what I try to cargo add, it always tries to add xml-rs.

I've even tried to run cargo +beta add cbindgen --registry crates-io, and it still gives me the same behaviour. The only way I can get cargo add to work is to remove .cargo/config.toml file and place it back afterwards.

image

Steps

Commands to run:

cargo new example
cd example
cargo +beta add xml-rs  # The same behaviour is exhibited in +nightly as well
cargo vendor ./vendor

# (add the output to .cargo/config.toml)

cargo +beta add --build cbindgen  # Doesn't need to be a build dependency, and "cbindgen" can be anything, even crates that don't exist
cargo +beta add cbindgen --registry crates-io  # This also does not work

Contents of .cargo/config.toml:

[source.crates-io]
replace-with = "vendored-sources"

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

Possible Solution(s)

I assume the reason for this is because what's added in .cargo/config.toml is source replacement, meaning any dependencies that try to access crates-io will access the vendored directory instead. I think I understand why cargo add does what it does here, however I feel it's fairly unintuitive combined with vendoring.

I haven't figured out any way to work around this (at least, in a way that I can continue to use cargo add). I've tried to add to config.toml:

[source.actual-crates]
registry = "https://github.com/rust-lang/crates.io-index"

(I've also tried git in place of https, and adding .git to the end of the URL)

and if I try to use cargo add cbindgen --registry actual-crates, I get:

error: source `actual-crates` defines source registry `crates-io`, but that source is already defined by `crates-io`
note: Sources are not allowed to be defined multiple times.

If I put the following into cargo.toml:

[registries]
actual-crates = { index = "https://github.com/rust-lang/crates.io-index" }

and run the same cargo add command, I get the same behaviour:

warning: translating `cbindgen` to `xml-rs`
      Adding xml-rs v0.8.4 to dependencies.

For now, I can easily add my dependencies by modifying Cargo.toml myself, however the cargo add command is very convenient, especially when introducing Rust to a team that's new to the language.

Notes

While I am identifying this as a bug (or at least, a problem), I'm not sure what the best way is to fix it.

Making cargo add always grab from crates.io, despite a configured source replacement, feels like the wrong way to go about it because it has the potential to break other setups, especially ones that use mirrored (or alternative) registries.

Perhaps a flag could be given to cargo add? Maybe cargo add --no-source-replacement or something less wordy?

Alternatively, maybe there should be a new command cargo vendor-add that calls cargo-add code, but without the source replacement? Though that would likely have to be special-cased only to "directory" source-replacement, or you'd need to pass the vendor path as well, like you do with cargo vendor.

Version

I've tested this on both beta and nightly cargo.

Beta's output:
cargo 1.62.0-beta.3 (4751950cc 2022-05-27)
release: 1.62.0-beta.3
commit-hash: 4751950ccc948c07047e62c20adf423d7e5f668c
commit-date: 2022-05-27
host: x86_64-pc-windows-msvc
libgit2: 1.4.2 (sys:0.14.2 vendored)
libcurl: 7.80.0-DEV (sys:0.4.51+curl-7.80.0 vendored ssl:Schannel)
os: Windows 10.0.19044 (Windows 10 Pro) [64-bit]

Nightly's output:
cargo 1.63.0-nightly (38472bc19 2022-05-31)
release: 1.63.0-nightly
commit-hash: 38472bc19f2f76e245eba54a6e97ee6821b3c1db
commit-date: 2022-05-31
host: x86_64-pc-windows-msvc
libgit2: 1.4.2 (sys:0.14.2 vendored)
libcurl: 7.83.1-DEV (sys:0.4.55+curl-7.83.1 vendored ssl:Schannel)
os: Windows 10.0.19044 (Windows 10 Pro) [64-bit]
@k3d3 k3d3 added the C-bug Category: bug label Jun 4, 2022
@Muscraft
Copy link
Member

Muscraft commented Jun 4, 2022

It looks like this happens because a fuzzy search for a PathSoruce returns all packages and cargo_add enables fuzzy searching in get_latest_dependency.

The simple solution would be to turn off fuzzy searching but I don't know if that is the correct course of action as it would stop linked_hash_map from being turned into linked-hash-map.

@k3d3
Copy link
Author

k3d3 commented Jun 4, 2022

Hmm, yeah that would at least get rid of the warning message, though you're right, that would cause other issues.

Though even if that were changed, wouldn't that cause cargo add to outright fail, rather than adding the crate dependency to Cargo.toml?

@Muscraft
Copy link
Member

Muscraft commented Jun 4, 2022

Yeah it would cause it to outright fail. I think throwing an error here is the best course of action as bypassing a source replace feels wrong as it wouldn't be vendored then. You would have to run cargo add then cargo vendor ./vendor.

I personally think throwing an error if their is a source replace may be the best course of action and then have a community subcommand to deal with vendor. This is just my opinion and one of the maintainers will probably have a better idea for the path forward.

@epage
Copy link
Contributor

epage commented Jun 4, 2022

I'm a bit annoyed with the disparate implementations of fuzzy searching. Its difficult to provide a consistent user experience and is easy to make mistakes like this where you assume "its a registry, of course fuzzy searching is safe" when its easy to overlook the interaction with other features like source replacements.

Fundamentally, I think that is what needs to be fixed though to fix that would probably require a major refactoring to keep the current, expected behavior working. This would also make solving #10680 less ambiguous

As for vendoring in general, we had not yet fleshed out that use case (see #10472). Mind creating a separate issue for this? I had missed that when creating the initial batch of cargo-add issues.

@k3d3
Copy link
Author

k3d3 commented Jun 5, 2022

Sure! So just to make sure I'm on the same page, this issue should be related to the fuzzy searching, and a new issue should be made for the cargo-add-with-vendoring use case?

Do you want me to rewrite this current issue, or keep it as-is?

@epage
Copy link
Contributor

epage commented Jun 6, 2022

So just to make sure I'm on the same page, this issue should be related to the fuzzy searching, and a new issue should be made for the cargo-add-with-vendoring use case?

Yes, let's keep this issue about the fuzzy searching

@epage
Copy link
Contributor

epage commented Jun 14, 2022

I'd propose we do the following

  1. Combine the query with fuzzy query methods, where separate
  2. Switch from a bool to an enum (Strict, Fuzzy)
  3. Add the following cases to the enum: Normalize and Suggestions
  4. Explore replacing Fuzzy with a mixture of other enum cases

bors added a commit that referenced this issue Jul 21, 2022
refactor(source): Open query API for adding more types of queries

### What does this PR try to resolve?

This refactors the Source/Registry traits from accepting a `fuzzy: bool` to accepting an enum so we can add alternative query styles in the future, as discussed in the Cargo team meeting for fixing #10729

The expected fix for `cargo add` at this point would be
- Add `QueryKind::Normalized`
  - Initially, we can make it like Exact for path sources and like Fuzzy for registry sources
- Switch cargo-add to using that kind everywhere (both where `Exact` and `Fuzzy` are used)
- A test to ensure this fixed it
- Rename `Fuzzy` to `Alternatives` or something to clarify its actual intent

### How should we test and review this PR?

The refactor is broken down into multiple commits, so ideally review a commit at a time to more easily see how it evolved.

### Additional information

Future possibilities
- Supporting normalized search on all sources
- Doing version / source matching for normalized results (probably not needed for cargo-add but will make the API less surprising for future users)
@epage
Copy link
Contributor

epage commented Jul 21, 2022

#10883 was a refactor that laid the ground work for fixing this.

Remaining steps:

  • Add QueryKind::Normalized
    • Initially, we can make it like Exact for path sources and like Fuzzy for registry sources
  • Switch cargo-add to using that Kind everywhere (both where Exact and Fuzzy are used)
  • A test to ensure this fixed it
  • Rename Fuzzy to Alternatives, Suggestions or something else to clarify its actual intent

@weihanglo weihanglo added E-mentor S-accepted Status: Issue or feature is accepted, and has a team member available to help mentor or review and removed E-help-wanted labels Apr 18, 2023
@JosiahParry
Copy link

I am presently running into this issue and I have no idea how to find my way out.

@LuuuXXX
Copy link
Contributor

LuuuXXX commented Dec 5, 2023

claim first, question it later~ @rustbot claim

bors added a commit that referenced this issue Feb 19, 2024
fix(add): Improve error when adding registry packages while vendored

### **What does this PR try to resolve?**

When a vendored directory is established, cargo add no longer adds new packages. Instead, it tries to translate a package name into a package that already exists in the vendored directory.
[More details](#10729 (comment))

Since `@epage` has done most of the work, here I do the rest of the finishing work.

Improves the error from #10729

### **How should we test and review this PR?**

The implementation procedure is as follows:
#10729 (comment)

Test steps:
1. Try to get an arbitrary crate and execute `cargo vendor` command.
2. Configure the vendor directory in .cargo/config.toml.
3. Add `alter-registry` to the config.toml file.
```
[registries]
alter-registry= { index = "XXX" }
```
4. run the same `cargo add` command.
```
cargo add another-crate --registry alter-registry
```
bors added a commit that referenced this issue Feb 22, 2024
fix(add): Improve error when adding registry packages while vendored

### **What does this PR try to resolve?**

When a vendored directory is established, cargo add no longer adds new packages. Instead, it tries to translate a package name into a package that already exists in the vendored directory.
[More details](#10729 (comment))

Since `@epage` has done most of the work, here I do the rest of the finishing work.

Improves the error from #10729

### **How should we test and review this PR?**

The implementation procedure is as follows:
#10729 (comment)

Test steps:
1. Try to get an arbitrary crate and execute `cargo vendor` command.
2. Configure the vendor directory in .cargo/config.toml.
3. Add `alter-registry` to the config.toml file.
```
[registries]
alter-registry= { index = "XXX" }
```
4. run the same `cargo add` command.
```
cargo add another-crate --registry alter-registry
```
@weihanglo
Copy link
Member

I didn't quite follow. Does #13281 resolve this?

@weihanglo
Copy link
Member

Thanks for the reply. #10729 (comment) seems to scope this issue within fuzzy search. Close as resolved via #13281.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
C-bug Category: bug Command-add S-accepted Status: Issue or feature is accepted, and has a team member available to help mentor or review
Projects
None yet
Development

No branches or pull requests

6 participants