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

Tests fail with conflicting external dependencies in the target directory #7343

Closed
camsteffen opened this issue Jun 10, 2021 · 3 comments · Fixed by #7631
Closed

Tests fail with conflicting external dependencies in the target directory #7343

camsteffen opened this issue Jun 10, 2021 · 3 comments · Fixed by #7631
Labels
C-an-interesting-project Category: Interesting projects, that usually are more involved design/code wise. C-tracking-issue Category: Tracking Issue

Comments

@camsteffen
Copy link
Contributor

camsteffen commented Jun 10, 2021

This is about the "multiple matching crates found" error or "multiple rlibs found" error that occurs rather frequently when running Clippy UI tests.

There are already multiple issues opened, but I want to explain the problem, tell what has been done to try and fix it, centralize discussion and possibly find a solution.

In short, the current state of things is okay/working, but fragile. For instance, every time I run cargo check during local development, or build with a different set of feature flags, I bump into this error and have to rm target/debug/deps/lib* and try again. The error also occurs now and then in CI and it's a real show stopper.

One thing to keep in mind is that this error can be seen as a good thing as it may prevent rustc bootstrapping from needlessly recompiling external crates, adding more time to a process that we want to quicken in any way we can. So a fix along the lines of "just rebuild in another target directory" may not be desirable.

Detailed Explanation

Clippy UI tests use external crate dependencies from crates.io. It is the only project in the rustc repository with this feature. For instance, we have some lints that are specific to serde. We use (a fork of) compiletest like rustc. And compiletest does not have first-class support of this feature. In order to use external dependencies with compiletest, you need to have them compiled beforehand (by cargo) and provide -L dir or --extern file rust flags to compiletest. This is what Clippy does. We provide the Clippy target directory (e.g. target/debug/deps) so that any crates listed in the dependencies of the main Cargo.toml will be compiled and available for use in UI tests with extern crate.

This fails when there are multiple versions of the same library in the -L target dir(s) because rustc won't know which one to use and will fail with a "multiple matching crates" error. As long as only one version of every library is compiled, with the same features, everything is fine. That is basically the current solution. But a variety of scenarios can break this requirement. It's hard to maintain.

We have one partial fix where we give preference to libs in the clippy (or stage-tools) target dir by scanning the directory and using --extern with libs found there. This is where we have our dreaded "multiple rlibs found" panic case.

Of course, Cargo knows how to handle (and create) the "multiple rlibs" problem. So UI tests should just use Cargo? Well FWIW that's what trybuild does. But I don't think we want to add a third-party solution to the mix when we already have compiletest.

One potential solution is to run cargo build something --message-format=json and parse the output to get specific rlib paths and then use those paths as --extern flags with compiletest. This was previously done in #5121 but then reverted (#5145) when complications arose with rustc bootstrapping. I think that attempt was on the right track. The main problem though, I think, is that rustc bootstrapping has a very complex setup for running cargo commands. If clippy tests themselves run cargo commands during rustc bootstrapping but outside of the rustc bootstrapping framework, we're bound to do it wrong.

Goal

We should have a way to build UI test dependencies in the same target directory as Clippy itself and be able to use those dependencies without any risk of "multiple crates found". I think we should leverage cargo's dependency resolution abilities in some way. And it has to work within rustc bootstrapping.

Questions

  • Should compiletest itself run cargo or resolve external dependencies more intelligently somehow?
  • Can we use cargo build --message-format within Clippy tests (as in Improve compile_test and dogfood tests #5121) and make it work in rustc bootstrapping with the right set of env vars? (see FIXME)
  • Should rustc bootstrap run cargo build --message-format instead and somehow provide the dependency info to Clippy tests?
  • Maybe instead of having UI test dependencies in the main Cargo.toml, we should have another crate for UI tests and the Cargo.toml for that crate should have UI test dependencies declared instead. I'm not sure if that solves anything, but it's an idea that can be integrated with other solutions.

Related: #6809, #3643, rust-lang/rust#76495, rust-lang/rust#78717

@flip1995 flip1995 added C-an-interesting-project Category: Interesting projects, that usually are more involved design/code wise. C-tracking-issue Category: Tracking Issue labels Jun 10, 2021
@jyn514
Copy link
Member

jyn514 commented Jun 10, 2021

One thing to keep in mind is that this error can be seen as a good thing as it may prevent rustc bootstrapping from needlessly recompiling external crates, adding more time to a process that we want to quicken in any way we can. So a fix along the lines of "just rebuild in another target directory" may not be desirable.

Again, I strongly encourage you to measure the performance difference here before ruling it out. It is both the simplest and easiest fix.

@camsteffen
Copy link
Contributor Author

I don't see that as a fleshed out solution that could be tested or ruled out.

@fpoli
Copy link

fpoli commented Jun 11, 2021

I don't know how much effort is needed to run cargo with rustc bootstrapping, but since you mentioned trybuild, an alternative might be the cargo-test-support library that is used by cargo's test suite. I wonder if they also need to run their tests with rustc bootstrapping. If, so there might already be a solution.

bors added a commit that referenced this issue Jun 21, 2021
Improve panic message on "Found multiple rlibs" error in compile-test

Related to #7343

When I first met this error I was pretty much confused, so I thought it may be a good idea to:

- Give a hint on what to do to users that don't want to dig into specifics and just want to quickly resolve the issue.
- Give a link for those who are interested in details.

## Old appearance:

<img width="1121" alt="Screenshot 2021-06-20 at 08 30 34" src="https://user-images.githubusercontent.com/12111581/122663361-df8ae780-d1aa-11eb-9236-775b4fd754d5.png">

## New appearance:

<img width="1121" alt="Screenshot 2021-06-20 at 08 32 18" src="https://user-images.githubusercontent.com/12111581/122663363-e4e83200-d1aa-11eb-9c46-f62d83eb79e2.png">

*Please write a short comment explaining your change (or "none" for internal only changes)*

changelog: none
bors added a commit that referenced this issue Jul 26, 2021
Improve conflicting rlibs error again

changelog: none

Now you can do `rm <paste>` and 🐇💨

```text
thread 'compile_test' panicked at '
----------------------------------------------------------------------
ERROR: Found multiple rlibs for crates: `clippy_lints`, `clippy_utils`
Try running `cargo clean` or remove the following files:

target/debug/deps/libclippy_lints-9117c875159004e0.rlib \
target/debug/deps/libclippy_lints-fe45157be7ff9444.rlib \
target/debug/deps/libclippy_utils-5eba1e07a9846ed0.rlib \
target/debug/deps/libclippy_utils-ccbc08fcf64de262.rlib

For details on this error see #7343
----------------------------------------------------------------------
```
@bors bors closed this as completed in 27afd6a Sep 8, 2021
bors added a commit that referenced this issue Sep 13, 2021
Target directory cleanup

changelog: none

* .cargo/config now has `target-dir` specified so that it is inherited by child projects. The target directory needs to be shared with clippy_dev, but not necessarily at the project root. (cc #7625)
* Uses `std::env::current_exe` (and its parent directories) whenever possible
* `CLIPPY_DRIVER_PATH` and `TARGET_LIBS` are no longer required from rustc bootstrap (but `HOST_LIBS` still is). These can be removed from the rustc side after merging.
* `CLIPPY_DOGFOOD` and the separate target directory are removed. This was originally added to mitigate #7343.

r? `@flip1995`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-an-interesting-project Category: Interesting projects, that usually are more involved design/code wise. C-tracking-issue Category: Tracking Issue
Projects
None yet
4 participants