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

Adding a dependency from a github repository with features seems to be failing/disambiguation command is not properly parsed #13121

Closed
IceTDrinker opened this issue Dec 6, 2023 · 6 comments · Fixed by #13213
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

@IceTDrinker
Copy link

Problem

Adding a workspace member from a github repository as a dependency via cargo add specifying existing features triggers an issue

Steps

  1. cargo add --git https://github.com/zama-ai/tfhe-rs.git tfhe --features=x86_64-unix

output:

    Updating git repository `https://github.com/zama-ai/tfhe-rs.git`
      Adding tfhe (git) to dependencies.
error: unrecognized feature for crate tfhe: x86_64-unix
no features available for crate tfhe

As one can see here the feature does exist in the tfhe workspace member Cargo.toml:

https://github.com/zama-ai/tfhe-rs/blob/ad41fdf5a5060c0a981cd0c35bf998feafe68e02/tfhe/Cargo.toml#L138

Possible Solution(s)

Unsure, I don't know the cargo code base, but my guess would be to parse the workspace member Cargo.toml to get some metadata/information about the crate

Notes

this is probably an edge case but can prove interesting for automated workflows that may be doing some automatic api breaks/checks by adding a work in progress version of a project to a test/regression project and run tests from that project

Version

cargo 1.76.0-nightly (9b13310ca 2023-11-24)
release: 1.76.0-nightly
commit-hash: 9b13310ca596020a737aaa47daa4ed9ff8898a2f
commit-date: 2023-11-24
host: x86_64-unknown-linux-gnu
libgit2: 1.7.1 (sys:0.18.1 vendored)
libcurl: 8.4.0-DEV (sys:0.4.68+curl-8.4.0 vendored ssl:OpenSSL/1.1.1u)
ssl: OpenSSL 1.1.1u  30 May 2023
os: Ubuntu 22.04 (jammy) [64-bit]
@IceTDrinker IceTDrinker added C-bug Category: bug S-triage Status: This issue is waiting on initial triage. labels Dec 6, 2023
@IceTDrinker
Copy link
Author

ok looks like Cargo is not interpreting the tfhe in the cargo add call as the workspace member to add but it was what the help suggested 🤔

@IceTDrinker
Copy link
Author

cargo add --git https://github.com/zama-ai/tfhe-rs.git
    Updating git repository `https://github.com/zama-ai/tfhe-rs.git`
error: multiple packages found at `https://github.com/zama-ai/tfhe-rs.git`:
    concrete-csprng, tasks, tfhe, tfhe-trivium
To disambiguate, run `cargo add --git https://github.com/zama-ai/tfhe-rs.git <package>`

@IceTDrinker IceTDrinker changed the title Adding a workspace member from a github repository with features seems to be failing Adding a workspace member from a github repository with features seems to be failing/disambiguation command is not properly parsed Dec 6, 2023
@epage epage changed the title Adding a workspace member from a github repository with features seems to be failing/disambiguation command is not properly parsed Adding a dependency from a github repository with features seems to be failing/disambiguation command is not properly parsed Dec 6, 2023
@epage epage added Command-add S-accepted Status: Issue or feature is accepted, and has a team member available to help mentor or review and removed S-triage Status: This issue is waiting on initial triage. labels Dec 6, 2023
@stupendoussuperpowers
Copy link
Contributor

I can take a look at this if it's not already claimed. I'm starting with digging around here - https://github.com/rust-lang/cargo/blob/master/src/cargo/ops/cargo_add/mod.rs#L280, please suggest if you think the issue might be in a different source.

@stupendoussuperpowers
Copy link
Contributor

Hi @epage, need some input on how to proceed here.

I've found an easy fix for this, but I reckon it might cause regressions, I can't be sure since I am not sure what exactly the existing code does.

In the populate_available_features, here

If we change the QueryKind from Fuzzy to Exact, the possibilities vec only contains the explicitly stated workspace instead of all of them in the repo. After which we look up the required feature in only the explicitly entered workspace instead of all the available ones.

After this change I am able to add the dependency stated in the original issue, as well as from a repo without any workspaces. Both of these work:

cargo add --git <url_w_workspaces> <workspace> --features=<features>
cargo add --git <url_wo_workspaces> --features=<features>

Is there some case for which we need the QueryKind to be Fuzzy? If not we should be able to just make this change. Otherwise we might need to do some boundary checks to determine the QueryKind. Unsure if this fix makes sense.

@epage
Copy link
Contributor

epage commented Dec 27, 2023

We use Fuzzy in another place to help with normalizing registry packages with insignificant differences

match registry.query_vec(&query, QueryKind::Fuzzy) {

However, we shouldn't need to be doing that by the time we do populate_available_features and should be able to use Exact, so switching should be fine.

If you can, I'd recommend having a commit that has a test that shows the bug and then a follow up commit switches to Exact and updates the test so it remains passing and the change in the test helps show what the change in behavior is.

@stupendoussuperpowers
Copy link
Contributor

Sure, thanks. Will make these changes.

stupendoussuperpowers added a commit to stupendoussuperpowers/cargo that referenced this issue Dec 28, 2023
stupendoussuperpowers added a commit to stupendoussuperpowers/cargo that referenced this issue Dec 28, 2023
stupendoussuperpowers added a commit to stupendoussuperpowers/cargo that referenced this issue Dec 28, 2023
bors added a commit that referenced this issue Dec 29, 2023
…epage

`cargo add` - fix for adding features from repository with multiple packages.

Fixes #13121

As discussed in the issue, when using `cargo add` to add a package with features from a git repository from one of it's members, the command might fail due to improper target for querying for said features.

This PR adds a test for this edge-case where we expect it to fail with current code. It also adds a fix for this, and updates the test to expect success.

While populating available features, the current code does a `Fuzzy` search which might lead to searching for features in a wrong member package. If we change it to an `Exact` query, we get back the proper member to search within.
@bors bors closed this as completed in e46d80e Dec 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in 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
3 participants