Allow ui tests to have dependencies in a reliable way #2373

oli-obk · 2022-07-14T13:58:13Z

This completely sidesteps the issue that compiletest-rs has where old artifacts of a dependency cause multiple available crates of name XXX errors. At this point I think we've reached feature parity for clippy, too, so I'm going to try publishing a version once this is merged.

miri

RalfJung · 2022-07-14T21:58:28Z

Could you describe the design at a high level? That would help with reviewing. :)

oli-obk · 2022-07-15T13:18:31Z

There is a crate that we build once to generate all dependencies
we grab all the artifacts that were built for that crate
we link every test against all those artifacts

coincidentally this removes the implicit trick of using dev-dependencies from the main miri crate for tests via extern crate imports. the dev-deps scheme can require cargo clean when updating dependencies as you'd then get "multiple imports found" when there are two versions available

RalfJung · 2022-07-15T13:32:58Z

this removes the implicit trick of using dev-dependencies from the main miri crate for tests via extern crate imports

I never heard of this, no idea what you are talking about here.

oli-obk · 2022-07-15T13:52:57Z

I never heard of this, no idea what you are talking about here.

we have lots of tests that use extern crate libc; which just accidentally works by picking the libc from the sysroot. But with compiletest-rs you could have picked any dev-dependency and used that (clippy tests against usages of serde types and derives in its ui tests this way)

RalfJung · 2022-07-15T13:59:23Z

Oh lol, I had no idea. Like, taking the sysroot libc seems fine, but I didn't know the other thing would have worked. Good thing I didn't.^^

bors · 2022-07-18T02:54:04Z

☔ The latest upstream changes (presumably #2380) made this pull request unmergeable. Please resolve the merge conflicts.

oli-obk · 2022-07-18T15:00:28Z

Do you want to review this? Or can I merge myself?

RalfJung · 2022-07-18T15:10:09Z

It's in my queue :)

RalfJung

Wow this is clever, I like it a lot. :)

miri

test_dependencies/Cargo.toml

tests/compiletest.rs

ui_test/Cargo.toml

ui_test/README.md

ui_test/src/dependencies.rs

RalfJung · 2022-07-19T00:30:43Z

We will probably want to move some tests from test-cargo-miri to here, in particular the getrandom tests. Those use the renaming feature of cargo:

getrandom_1 = { package = "getrandom", version = "0.1" }
getrandom_2 = { package = "getrandom", version = "0.2" }

Will that work?

RalfJung · 2022-07-19T00:31:11Z

Oh also please check that this works in the rustc workspace via x.py. The ./miri will probably not be the right call there...

oli-obk · 2022-07-19T07:44:49Z

Will that work?

I was afraid of this ^^ it doesn't work with my current scheme. The fix is obvious and I knew I should've done it this way, but 🤷

Basically what I need to do is to also run cargo metadata to get these renames and how they map to the artifacts

EDIT: works now correctly

ui_test/src/lib.rs

oli-obk · 2022-07-19T11:25:50Z

Oh also please check that this works in the rustc workspace via x.py. The ./miri will probably not be the right call there...

even the latest version doesn't work from rustc yet, so this PR is not ready yet

test-cargo-miri/tests/test.rs

test-cargo-miri/run-test.py

ui_test/src/lib.rs

ui_test/src/dependencies.rs

RalfJung · 2022-07-19T11:34:19Z

even the latest version doesn't work from rustc yet, so this PR is not ready yet

@rustbot author

tests/pass/random.rs

oli-obk · 2022-07-19T15:05:40Z

status update

I can reproduce the failure with x.py now: running

cargo run --manifest-path=cargo-miri/Cargo.toml -- miri run --manifest-path=test_dependencies/Cargo.toml

via the bootstrap machinery will suceed to build cargo-miri (that already worked for cargo miri setup), but fail when cargo-miri tries to build build scripts (or proc macros), as they can't find the libstd.

edit: manually adding --sysroot = build/...../stage1 makes it compile, now I gotta figure out why/where we're losing that

tests/pass/libc.rs

miri

oli-obk · 2022-07-20T08:36:54Z

@rustbot ready

cargo-miri/bin.rs

miri

test-cargo-miri/run-test.py

RalfJung · 2022-07-20T13:19:39Z

@rustbot author

cargo-miri/bin.rs

oli-obk · 2022-07-20T13:59:14Z

@rustbot ready

test-cargo-miri/run-test.py

RalfJung · 2022-07-20T14:38:43Z

r=me with run-test.py fixed and commits squashed

oli-obk · 2022-07-20T15:59:17Z

@bors r=RalfJung

bors · 2022-07-20T15:59:18Z

📌 Commit ab6fb9d has been approved by RalfJung

It is now in the queue for this repository.

bors · 2022-07-20T15:59:24Z

⌛ Testing commit ab6fb9d with merge 1366bf6...

bors · 2022-07-20T16:34:39Z

☀️ Test successful - checks-actions
Approved by: RalfJung
Pushing 1366bf6 to master...

RalfJung · 2022-07-21T11:45:18Z

tests/fail/tokio_mvp.rs

+//@only-target-linux: the errors differ too much between platforms
+
+#[tokio::main]
+async fn main() {}


Btw would be nice to also have a 'pass' test for #602 (comment).

Should we have these tests of non-stdlib crates in a subdir in our test folders? The root dirs are already quite messy, so something like {pass,fail}/crates or so might make sense?

yea grouping them into a custom folder makes sense.

#2408 does these changes.

RalfJung reviewed Jul 14, 2022

View reviewed changes

miri Outdated Show resolved Hide resolved

oli-obk force-pushed the test_dependencies branch from 5c97ff3 to cf74b1f Compare July 18, 2022 08:04

oli-obk changed the title ~~Allow ui tests to have reliable dependencies~~ Allow ui tests to have dependencies in a reliable way Jul 18, 2022

oli-obk force-pushed the test_dependencies branch from cf74b1f to e5736f9 Compare July 18, 2022 08:38