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

Move libtest out of tree #59775

Open
Manishearth opened this issue Apr 7, 2019 · 10 comments
Open

Move libtest out of tree #59775

Manishearth opened this issue Apr 7, 2019 · 10 comments
Labels
A-libtest Area: #[test] related C-enhancement Category: An issue proposing an enhancement or a PR with one. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@Manishearth
Copy link
Member

See: https://internals.rust-lang.org/t/a-path-forward-towards-re-usable-libtest-functionality-custom-test-frameworks-and-a-stable-bench-macro

An attempt for moving libtest out of tree was made in #57842 , however it broke clippy in weird ways that involve interactions with the rustc build system.

It has been reverted in #59766 so that the beta is fixed in time.

We should still work towards making this happen.

A potential plan is to:

  • Continue to improve http://github.com/rust-lang/libtest/
  • Migrate compiletest-rs to using http://github.com/rust-lang/libtest/, on pure stable
  • Make sure rustc compiletest and compiletest-rs match
  • Update clippy, miri to use new compiletest-rs, ensure this works within the rustc build system
  • Update rustc compiletest to be compiletest-rs (optional, but nice to have)
  • Update rustc libtest to reexport libtest

thoughts? @gnzlbg @xales

@Manishearth Manishearth added the A-libtest Area: #[test] related label Apr 7, 2019
@Manishearth
Copy link
Member Author

A couple unresolved questions:

  • We still don't know why the rust build system doesn't like out-of-tree libtest. I suspect it's because it's a runtime dependency that has a name clash, but I'm not sure
  • Dealing with stability is going to be tricky.

@phansch
Copy link
Member

phansch commented Apr 7, 2019

Make sure rustc compiletest and compiletest-rs match

Just curious, why is this a requirement if using compiletest-rs in rustc is listed as optional? It seems like a lot of work to align both.

@Manishearth
Copy link
Member Author

That's optional too.

compiletest-rs is a fork of rustc compiletest made to work out of tree, it's not that much work. We regularly sync compiletest-rs from rustc, IIRC.

@gnzlbg
Copy link
Contributor

gnzlbg commented Apr 7, 2019

The main problem was that, for some reason, the dependencies of the test crate get put in the sysroot, can be accessed via rustc_private, and #[test] needs to be able to access them.

None of this should be necessary, and IIUC @xales figured out that this was happening because test re-exports some items from libtest, such that one of the options would be to add a thin shim to test that uses newtype wrappers to avoid this issue.

Otherwise we'd need to add more magic to the build system to handle these dependencies specially.

@gnzlbg
Copy link
Contributor

gnzlbg commented Apr 7, 2019

Also, we probably want to move to use compiletest-rs from crates.io in tree before moving libtest out of tree. Is there any reason not to do that?

@Manishearth
Copy link
Member Author

Also, we probably want to move to use compiletest-rs from crates.io in tree before moving libtest out of tree. Is there any reason not to do that?

We should definitely try. Two challenges:

  • the two crates may have diverged a bit. this needs some manual verification, basically. Overall clippy doesn't use functionality that in-tree compiletest uses, so with luck it should be a straightforward copy out
  • rustc sysroot may not like us. hopefully it's fine

@djrenren
Copy link
Contributor

We might as well start tracking the feature flags that are keeping libtest from being stable:

  • asm (unsure what it's used for)
  • test (used for std::hint::blackbox)
  • termination_trait_lib (used to describe TestFn type)
  • panic_unwind (used for output capture)
  • set_stdio (used for output capture)

panic_unwind should be easily replaceable with std::panic::set_hook,
and I think we can replace termination_trait_lib with our own Termination type.

set_stdio seems to be the big one. As it will require either rearchitecting libtest with some multi-process setup or will require stabilizing a new feature. What are people's thoughts?

@gnzlbg
Copy link
Contributor

gnzlbg commented Aug 27, 2019

Only the latter three are really needed, and they are only needed to support features that could be optional, so we can put them behind a cargo unstable feature, and allow them only on nightly initially.

@djrenren
Copy link
Contributor

That's fair... but really asm & test should be put behind a feature for like bench. Because bench without blackbox can just be lies.

IMO I/O capture is a pretty essential feature, so I'm hesistant to just punt on it, but I guess it's still a step forward from the status quo

@gnzlbg
Copy link
Contributor

gnzlbg commented Aug 27, 2019

@djrenren I think we might be talking past each other. When writing a benchmark, users would need to enable some feature to access std::hint::bench_black_box, but the libtest bench implementation itself does not need to always use that. It could use it, e.g., if a nightly Rust version is used, and an unstable cargo feature is enabled. But if that is not the case, libtest could do something else (e.g. volatile pointer store/loads hacks to emulate bench_black_box).

@jonas-schievink jonas-schievink added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. C-enhancement Category: An issue proposing an enhancement or a PR with one. labels Mar 31, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-libtest Area: #[test] related C-enhancement Category: An issue proposing an enhancement or a PR with one. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
Status: No status
Development

No branches or pull requests

5 participants