use ui_test dependency builder for test dependencies

[RalfJung]

changelog: none

This tries to make progress on rust-lang/rust#78717 by using the ui_test dependency handling instead of linking in the dependencies of clippy itself with the tests. This partially reverts #11045. However, we still use the old style of dealing with dependencies for clippy's own crates and the "internal" tests, as otherwise those would get rebuilt which takes too long.

[rustbot]

r? @Alexendoo

rustbot has assigned @Alexendoo.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[Alexendoo]

No clue about the ui_test error, but ui-internal tests still depend on clippy_utils

[RalfJung]

Oh I see, and those are driven by the same runner. So depending on the test suite we'll need to use one or the other logic.

[oli-obk]

This effectively reverts #11045.

that PR was made because it was prohibitively slow for many maintainers and contributors to build the deps in the test suite instead of in the main build. I don't see how that issue can be avoided.

[oli-obk]

I am not sure which exact dependencies ui_test is referring to here -- clippy_test_deps does not have any build-dependencies.

@oli-obk do you know what could be done here?

this is about quote, which iirc I could not differentiate in cargo_metadata output by rustc and cargo to properly detect which ones are build as proc macro deps or as direct deps

[RalfJung]

I don't see how that issue can be avoided.

The issue is avoided by noting having the clippy crates in clippy_test_deps. Only the ui-internal tests need them; those will still use the old style of adding dependencies -- which is fine as they don't run in the rustc repo at all.

I didn't implement this part yet; it's what I referred to here.

[RalfJung]

this is about quote, which iirc I could not differentiate in cargo_metadata output by rustc and cargo to properly detect which ones are build as proc macro deps or as direct deps

Ah so the problem is that we pull in serde which pulls in a proc macro which has quote as a proc macro dep? Hm...

They look different in what you print, one is in the x86_64-unknown-linux-gnu folder and one is not. Not sure if there's a good way to use that.

Do clippy tests need serde with the proc macro or could we disable that feature?

[flip1995]

A lot of Clippy tests use derive(Deserialize), which needs those proc macros. The tests/ui/unsafe_derive_deserialize.rs specifically is about this derive attribute.

[RalfJung]

Okay, damn. Then ui_test has to somehow figure out which dependencies belong where...

[oli-obk]

I can give it another shot, but I may have to ask cargo to emit more data

[RalfJung]

I tried to understand the problem and created an issue for further discussion: oli-obk/ui_test#327.

[RalfJung]

After adding the internal dependencies back for the internal tests, this PR now passes when using a ui_test that includes oli-obk/ui_test#329. :)

[RalfJung]

Okay this should be good to go now :)

[samueltardieu]

Even if the main is empty today and may stay empty, could you add clippy_test_deps to the list of packages in test/dogfood.rs so that it gets linted as well when running cargo dev dogfood? (for cargo dev fmt, this is handled already as it looks for all *.rs files, so there is nothing to do there)

[RalfJung]

I don't understand what that has to do with my PR...?

[RalfJung]

Oh, because I added a new crate, and that's supposed to be a list of all crates there, I see. Will do.

That crate really should stay empty though, it's code is not run or tested by anything anyway.

[flip1995]

Thanks! LGTM overall. I would not add that test crate to the dogfood list though. See below.

[flip1995]

I disagree here. If this crate is empty and should stay empty, we just add time to the dogfood tests waiting for the dependencies to be compiled. This will just increase the time of dogfood tests without any benefit.

[RalfJung]

I'm fine either way, I hope the clippy maintainers can come to a consensus here. :)

[flip1995]

@samueltardieu Do you feel strongly about adding it to the dogfood list?

If not, r=me

[samueltardieu]

Nope, I thought it was more "belts and suspensers" here in case something crept it, but I'm fine without it!

[flip1995]

rustc bootstrap doesn't run Clippy's internal tests anyway. Which is sometimes annoying during the sync, so I'd like to enable this at some point. This kinda closes the door on that. At least for stage 1. IIUC on a stage 2 build, this would still be possible?

Anyway, the benefits of this is way bigger than the slight annoyance of not running ui-internal in the Rust repo.

[RalfJung]

This kinda closes the door on that. At least for stage 1. IIUC on a stage 2 build, this would still be possible?

I think so, yeah. Or alternatively, when running in ./x, the internal dependencies could also be managed by the clippy_test_deps crate. But that seems messy... so only doing it in stage 2 is probably best, similar to the "fulldep" tests in rustc.

It doesn't close any door on stage 1, the internal tests are equally broken in stage 1 before and after this PR. What the PR changes is that it hopefully un-breaks the other tests.

[flip1995]

Understood! Thanks for the explanation!