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

warning: skipping duplicate package case found at ~\.cargo\git\checkouts\cargo-.. #10752

Closed
aminya opened this issue Jun 13, 2022 · 18 comments · Fixed by #10767 or #14239
Closed

warning: skipping duplicate package case found at ~\.cargo\git\checkouts\cargo-.. #10752

aminya opened this issue Jun 13, 2022 · 18 comments · Fixed by #10767 or #14239
Labels
A-diagnostics Area: Error and warning messages generated by Cargo itself. C-bug Category: bug S-needs-design Status: Needs someone to work further on the design for the feature or fix. NOT YET accepted.

Comments

@aminya
Copy link

aminya commented Jun 13, 2022

Problem

When using cargo install --git to install a package like cargo itself, a lot of warnings are emitted about duplicated packages. It seems that these packages are only used for the tests, and not sure if the warnings affect the final build.

warning: skipping duplicate package `cargo-list-test-fixture` found at `~\.cargo\git\checkouts\cargo-..`
...

It seems that this is caused by this commit:
e903f7d

image

Steps

Try to build cargo with itself:

cargo install --git https://github.com/rust-lang/cargo

Possible Solution(s)

No response

Notes

No response

Version

cargo 1.63.0-nightly (38472bc19 2022-05-31)
@aminya aminya added the C-bug Category: bug label Jun 13, 2022
@weihanglo
Copy link
Member

Thanks for the report. Those warnings won't affect any build, though flooding with warnings is definitely not good.

This situation happens when you

  • build something from git, and
  • the git dependency/package contains several Cargo.tomls with name conflict.

I am not sure whether this is rare or not. One simple way to solve it is reverting #10701.

@epage
Copy link
Contributor

epage commented Jun 13, 2022

Interesting. cargo is searching recursively through cargo's git repo and finding all of the test cases we have in the repo and reporting a warning for those as we duplicate the manifest names. These should not even be given as options for install for the warning to be activated.

Besides telling the user to run cargo install --git https://github.com/rust-lang/cargo cargo I wonder if there is some way we can allow repos to exclude directories from being searched for manifests.

@epage
Copy link
Contributor

epage commented Jun 14, 2022

We discussed this in the cargo team meeting and came up with a short term and long-term idea

Short term is to see if we can make the warning only appear when the package is publish = true

There is still the wider problem if packages being looked up that are internal. Ideas

  • a breadcrumb file that says "stop recursively walking for path/git sources"
  • a new manifest key that says "stop recursing and use workspace lookup rules"

@epage
Copy link
Contributor

epage commented Jun 14, 2022

@danilhendrasr since you worked on the warnings in #10701, I thought I'd first check if you would be interested in seeing if the warning could be restricted to packages with publish = true to reduce noise from the warning.

@danilhendrasr
Copy link
Contributor

danilhendrasr commented Jun 15, 2022

Sure, I'll take a stab at it.
@rustbot claim

@epage
Copy link
Contributor

epage commented Jun 21, 2022

Hmm, thinking about this, i suspect #10767 only gave people the tool to reduce the warnings but cargo still needs to opt-in to that tool, so going ahead and re-opening for now.

@epage epage reopened this Jun 21, 2022
@danilhendrasr danilhendrasr removed their assignment Jun 21, 2022
@ehuss
Copy link
Contributor

ehuss commented Jun 22, 2022

Would it be possible to restrict the warning to situations where the package is actually used? That is, when doing cargo install --git or having a git dependency, only issue a warning if that package was actually part of the selected packages? I suspect that won't be easy since read_packages is pretty low-level, but I think from a user experience, that is what would be expected. Warning about unrelated packages seems to be a false-positive. Also, forcing people to use a workaround like publish=false does not seem ideal.

I would also expect this to actually be an error in the long-run. In the original issue (#10669), this leads to non-deterministic behavior. A warning just says "something's broken" and then proceeds doing something random, which doesn't seem good.

Also, the current warning does not seem to be very helpful (particularly for an end-user who may have no idea where this is coming from, or is not well-versed in cargo). There is no actionable advice given to the user, or any context as to how cargo got into this bind. I would expect in the situation with a git dependency that has a duplicate, it would say something like:

error: multiple packages named `foo` found in git repository 
https://github.com/rust-lang/example for dependency `foo` specified in `path/to/Cargo.toml`
    First instance found at `path/in/repo/Cargo.toml`
    Second instance found at `path/in/repo/other-foo/Cargo.toml`
    A git repository must only contain a single package with the name `foo`.

With similar errors for paths overrides and install --git.

(Without the ability to restrict a git repo to a specific path, the solution of being unique isn't great, but should be fine in most situations.)

@epage
Copy link
Contributor

epage commented Jun 22, 2022

Would it be possible to restrict the warning to situations where the package is actually used? That is, when doing cargo install --git or having a git dependency, only issue a warning if that package was actually part of the selected packages? I suspect that won't be easy since read_packages is pretty low-level, but I think from a user experience, that is what would be expected. Warning about unrelated packages seems to be a false-positive. Also, forcing people to use a workaround like publish=false does not seem ideal.

The big issue with the case in question is that the user is doing

$ cargo install --git https://github.com/rust-lang/cargo

and not

$ cargo install --git https://github.com/rust-lang/cargo -p cargo

The most we can filter it down is by whether the package has an installable artifact which I think would be an additional useful thing to do.

Overall, this still leaves the big hole of cargo install installing everything in the source regardless of the source author's intent.

@ehuss
Copy link
Contributor

ehuss commented Jun 23, 2022

The most we can filter it down is by whether the package has an installable artifact which I think would be an additional useful thing to do.

Overall, this still leaves the big hole of cargo install installing everything in the source regardless of the source author's intent.

I believe the former command only works if there is a single package with a binary (see here). With cargo's repo, it errors with:

error: multiple packages with binaries found: capture, cargo, cargo-credential-1password, cargo-credential-gnome-secret, cargo-credential-macos-keychain, cargo-credential-wincred, foo, mdman, semver-check. When installing a git repository, cargo will always search the entire repo for any Cargo.toml. Please specify which to install.

bors added a commit to rust-lang/rust-clippy that referenced this issue Jul 21, 2022
Add `ui_cargo_toml_metadata` test

This PR adds a test to check the metadata of packages in the `ui_cargo` directory.

A recent change to Cargo causes it to warn when it finds multiple packages with the same name in a git dependency (the issue is described [here](rust-lang/cargo#10752)).

Many (if  not all) Dylint libraries depend upon `clippy_utils`. As a result of the change, one now sees the following when building a Dylint library:
```
warning: skipping duplicate package `fail` found at `/home/smoelius/.cargo/git/checkouts/rust-clippy-4b72815e96774b3d/0cb0f76/tests/ui-cargo/module_style/pass_mod`
warning: skipping duplicate package `fail` found at `/home/smoelius/.cargo/git/checkouts/rust-clippy-4b72815e96774b3d/0cb0f76/tests/ui-cargo/module_style/fail_no_mod`
warning: skipping duplicate package `cargo_common_metadata` found at `/home/smoelius/.cargo/git/checkouts/rust-clippy-4b72815e96774b3d/0cb0f76/tests/ui-cargo/cargo_common_metadata/fail_publish_true`
warning: skipping duplicate package `fail-cargo` found at `/home/smoelius/.cargo/git/checkouts/rust-clippy-4b72815e96774b3d/0cb0f76/tests/ui-cargo/cargo_rust_version/pass_cargo`
warning: skipping duplicate package `fail-clippy` found at `/home/smoelius/.cargo/git/checkouts/rust-clippy-4b72815e96774b3d/0cb0f76/tests/ui-cargo/cargo_rust_version/fail_clippy`
warning: skipping duplicate package `fail-both-same` found at `/home/smoelius/.cargo/git/checkouts/rust-clippy-4b72815e96774b3d/0cb0f76/tests/ui-cargo/cargo_rust_version/fail_both_same`
warning: skipping duplicate package `fail-file-attr` found at `/home/smoelius/.cargo/git/checkouts/rust-clippy-4b72815e96774b3d/0cb0f76/tests/ui-cargo/cargo_rust_version/fail_file_attr`
```
There appear to be two contributing factors:
- Some packages in `ui_cargo` could have a `publish = false` added to them.
- Some packages in `ui_cargo` seem to be inconsistently named.

The new test checks that each package in the `ui_cargo` directory has a name matching one of its parent directories, and `publish = false` in its metadata (with a few exceptions).

Note that the packages in `cargo_common_metadata` require special care because `publish` is the subject of some of the `cargo_common_metadata` tests.

Also note that this PR adds `walkdir` as a dev dependency to the `clippy` package. However, it was already a dependency of `clippy_dev` and `lintcheck`. So hopefully this is acceptable.

Our continued thanks for making `clippy_utils` available, BTW. :)

r? `@flip1995`

changelog: none
irh added a commit to irh/cbindgen that referenced this issue Apr 13, 2023
When specifying `cbindgen` as a dependency via git, several 'skipping duplicate
package' warnings pop up regarding some of the test crates.

The warning seems to be spurious given that the test packages aren't needed when
depending on `cbindgen` (see rust-lang/cargo#10752),
but while a fix is being considered for Cargo, this commit disambiguates the
duplicated package names by referring to their relative paths.
irh added a commit to irh/cbindgen that referenced this issue Apr 13, 2023
When specifying `cbindgen` as a dependency via git, several 'skipping duplicate
package' warnings pop up regarding some of the test crates.

The warning seems to be spurious given that the test packages aren't needed when
depending on `cbindgen` (see rust-lang/cargo#10752),
but while a fix is being considered for Cargo, this commit disambiguates the
duplicated package names by referring to their relative paths.
@DianaNites
Copy link

I'm hitting this, as an end-user, with kani as a tagged git dev dependency, every single time i build/check my project.

kani = { git = "https://github.com/model-checking/kani", tag = "kani-0.25.0", package = "kani" }

Technically speaking, the only reason I need it as a dependency is because they don't publish to crates.io and their tool adds it at runtime, but that doesnt play nice with Rust Analyzer, hence adding the dependency.

Is there any way to silence these warnings?

I'm not installing a binary so I can't just use -p kani, though i did try package = "kani" to no avail, not that I really expected that to work.

2023-04-17-173140

@weihanglo
Copy link
Member

Should we reconsider redirecting error to a log file? Or at minimal, Cargo can log some parts of the error that is not meaingful to end users. We then can provide a way to retrieve a verbose error report.

The workflow might look like this,

warning: error: multiple packages with the same name found in git repository 
This may cause non-deterministic result of the compilation.
Run `cargo report errors --id 168` to see the error report.

@weihanglo weihanglo added S-needs-design Status: Needs someone to work further on the design for the feature or fix. NOT YET accepted. and removed E-help-wanted labels Apr 18, 2023
@PaulDance

This comment was marked as resolved.

epage added a commit to epage/cargo that referenced this issue May 31, 2024
These are dramatically different and the shared implementation is
getting in the way of rust-lang#10752.
epage added a commit to epage/cargo that referenced this issue May 31, 2024
These are dramatically different and the shared implementation is
getting in the way of rust-lang#10752.
epage added a commit to epage/cargo that referenced this issue Jun 1, 2024
These are dramatically different and the shared implementation is
getting in the way of rust-lang#10752.
epage added a commit to epage/cargo that referenced this issue Jun 3, 2024
These are dramatically different and the shared implementation is
getting in the way of rust-lang#10752.
epompeii added a commit to bencherdev/bencher that referenced this issue Jun 22, 2024
bors added a commit that referenced this issue Jul 1, 2024
refactor(source): Clean up after PathSource/RecursivePathSource split

### What does this PR try to resolve?

This is a follow up to #13993 and prep for future improvements (e.g. cargo script, #10752)

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

### Additional information
bors added a commit that referenced this issue Jul 11, 2024
fix(overrides): Don't warn on duplicate packages from using '..'

### What does this PR try to resolve?

As part of #10752, I was changing the "duplicate package" warning to be like:
```
[WARNING] skipping duplicate package `a2 v0.5.0 ([ROOT]/foo/b/../a)`:
  [ROOT]/foo/b/../a/a2/Cargo.toml
in favor of [ROOT]/foo/a/a2/Cargo.toml
```
and it showed that we were considering two paths to the same package to be duplicates.

This suppresses that warning.

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

### Additional information
bors added a commit that referenced this issue Jul 11, 2024
refactor(source): More RecursivePathSource clean up

### What does this PR try to resolve?

This is a follow up to #13993 and #14169 and is part of my work towards #10752.

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

### Additional information
epage added a commit to epage/cargo that referenced this issue Jul 12, 2024
epage added a commit to epage/cargo that referenced this issue Jul 12, 2024
epage added a commit to epage/cargo that referenced this issue Jul 12, 2024
epage added a commit to epage/cargo that referenced this issue Jul 12, 2024
epage added a commit to epage/cargo that referenced this issue Jul 12, 2024
epage added a commit to epage/cargo that referenced this issue Jul 13, 2024
bors added a commit that referenced this issue Jul 15, 2024
fix(source): Don't warn about unreferenced duplicate packages

### What does this PR try to resolve?

This also improves the message, consolidating multiple duplicates and saying which was loaded instead, as it naturally fell out of the design

Fixes #10752

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

### Additional information

We're still subject to #13724 and fully load every manifest, even if we don't use it.  I'm exploring that topic at https://rust-lang.zulipchat.com/#narrow/stream/246057-t-cargo/topic/Redundant.20code.20in.20.60GitSouce.60.3F/near/450783427

This change builds on
- #13993
- #14169
- #14231
- #14234
@bors bors closed this as completed in ed56f1e Jul 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Error and warning messages generated by Cargo itself. C-bug Category: bug S-needs-design Status: Needs someone to work further on the design for the feature or fix. NOT YET accepted.
Projects
None yet
8 participants