Run doctests via out-of-process rustc #63827
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This comment has been minimized.
This comment has been minimized.
andjo403
commented
Aug 23, 2019
andjo403
commented
Aug 23, 2019
andjo403
commented
Aug 23, 2019
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
I'll try to make some time tomorrow to review this patch and leave some notes here. |
JohnTitor
added
the
S-waiting-on-review
|
cc @rust-lang/rustdoc as y'all might have thoughts on this / should be aware |
Mark-Simulacrum
added
S-blocked
andjo403
force-pushed
the
out-of-process-rustc-in-rustdoc
branch
from
2661f79
to
ad6b322
Compare
This comment has been minimized.
This comment has been minimized.
|
We probably need to set the current directory of the compiler to the same as rustdoc's, all other paths (in the input that is) should be relative to that anyway. |
|
tried to set current dir to rustdocs current dir but that it did not work and according to the doc the path shall be relative the current file |
|
I'll try to get this branch built locally and get back to you after some debugging then. |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
This patch fixes the test failure for me: diff --git a/src/librustdoc/test.rs b/src/librustdoc/test.rs
index f27d6c3597e..e5b38a8f84f 100644
--- a/src/librustdoc/test.rs
+++ b/src/librustdoc/test.rs
@@ -199,7 +199,7 @@ fn run_test(
_ => PathBuf::from(r"doctest.rs"),
};
- let file_and_line = format!("{:?}:{}",path,line as isize - line_offset as isize);
+ let file_and_line = format!("{}:{}", path.display(), line as isize - line_offset as isize);
enum DirState {
Temp(tempfile::TempDir),
@@ -245,7 +245,6 @@ fn run_test(
compiler.arg("--sysroot").arg(sysroot);
}
compiler.arg("--edition").arg(&edition.to_string());
- compiler.current_dir(outdir.path());
compiler.env("UNSTABLE_RUSTDOC_TEST_FILE_AND_LINE", file_and_line);
compiler.arg("-o").arg(&output_file);
if as_test_harness { |
andjo403
force-pushed
the
out-of-process-rustc-in-rustdoc
branch
from
ad6b322
to
fde794c
Compare
|
thanks for finding that |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
This comment has been minimized.
This comment has been minimized.
andjo403
force-pushed
the
out-of-process-rustc-in-rustdoc
branch
from
fde794c
to
67e18bf
Compare
bors
added a commit
that referenced
this pull request
Aug 30, 2019
…rk-Simulacrum Run doctests via out-of-process rustc closes #63638
I think out-of-process test runs are better for a number of reasons:
Furthermore, I think the in-process model is inherently more complicated and painful than this: we need to track defaults over time, setup globals correctly, etc. This model, running |
I think it's possible there's more performance gains to be had here: e.g., I believe we still have a global, system-wide, lock on diagnostic output on Windows, which could contribute to slowdown here. Regardless, I think a performance difference of 0.9x on Windows is more than worth the 2x performance gain on Linux. |
|
think that the regression for cpu with few cores can be due to the there is no jobserver at the time and the the codegen-units is not 1 by default. so for a 8 threaded cpu 8 test will start and each test have the possibility as there is no jobserver to start 32 jobbs at the same time resulting in the possibility that 256 threads is concurrently running, this is counted high hoppfully not 32 codegen-units is created for the small tests. @ollie27 can you test to add |
|
☀️ Test successful - checks-azure |
That would not work in incremental mode. |
This was referenced Aug 30, 2019
Merged
Merged
@bjorn3 I do not understand what you mean with incremental mode |
Yes
I don't know. |
|
We should be able to set |
|
working on reproducing the regression and will check if my theory about codegen-units solves the problem but it takes for ever to compile on my laptop |
|
I'm not able to reproduce the regression on my computer with windows10 and 2 core 4 thread cpu
so no change with this PR and with codegen-units=1 about 10% faster tests |
That does improve the time to 203s from 214s but it's still not quite as fast as the 194s before this PR. |
|
(I'll note here that I continue to think this regression is, if not expected, not too bad. However, if we decide to revert, that's also largely acceptable to me, though I would personally be against such a revert). |
netbsd-srcmastr
pushed a commit
to NetBSD/pkgsrc
that referenced
this pull request
Nov 11, 2019
Pkgsrc changes:
* Remove patch which no longer applies (but what about RPATH?)
* Adapt a few patches to changed files upstream.
Upstream changes:
Version 1.39.0 (2019-11-07)
===========================
Language
--------
- [You can now create `async` functions and blocks with `async fn`,
`async move {}`, and `async {}` respectively, and you can now call
`.await` on async expressions.][63209]
- [You can now use certain attributes on function, closure, and function
pointer parameters.][64010] These attributes include `cfg`, `cfg_attr`,
`allow`, `warn`, `deny`, `forbid` as well as inert helper attributes used
by procedural macro attributes applied to items. e.g.
```rust
fn len(
#[cfg(windows)] slice: &[u16],
#[cfg(not(windows))] slice: &[u8],
) -> usize {
slice.len()
}
```
- [You can now take shared references to bind-by-move patterns in the
`if` guards of `match` arms.][63118] e.g.
```rust
fn main() {
let array: Box<[u8; 4]> = Box::new([1, 2, 3, 4]);
match array {
nums
// ---- `nums` is bound by move.
if nums.iter().sum::<u8>() == 10
// ^------ `.iter()` implicitly takes a reference to `nums`.
=> {
drop(nums);
// ----------- Legal as `nums` was bound by move and so we have ownership.
}
_ => unreachable!(),
}
}
```
Compiler
--------
- [Added tier 3\* support for the `i686-unknown-uefi` target.][64334]
- [Added tier 3 support for the `sparc64-unknown-openbsd` target.][63595]
- [rustc will now trim code snippets in diagnostics to fit in your terminal.]
[63402] **Note** Cargo currently doesn't use this feature. Refer to
[cargo#7315][cargo/7315] to track this feature's progress.
- [You can now pass `--show-output` argument to test binaries to print the
output of successful tests.][62600]
\* Refer to Rust's [platform support page][forge-platform-support] for more
information on Rust's tiered platform support.
Libraries
---------
- [`Vec::new` and `String::new` are now `const` functions.][64028]
- [`LinkedList::new` is now a `const` function.][63684]
- [`str::len`, `[T]::len` and `str::as_bytes` are now `const` functions.][63770]
- [The `abs`, `wrapping_abs`, and `overflowing_abs` numeric functions are
now `const`.][63786]
Stabilized APIs
---------------
- [`Pin::into_inner`]
- [`Instant::checked_duration_since`]
- [`Instant::saturating_duration_since`]
Cargo
-----
- [You can now publish git dependencies if supplied with a `version`.]
[cargo/7237]
- [The `--all` flag has been renamed to `--workspace`.][cargo/7241] Using
`--all` is now deprecated.
Misc
----
- [You can now pass `-Clinker` to rustdoc to control the linker used
for compiling doctests.][63834]
Compatibility Notes
-------------------
- [Code that was previously accepted by the old borrow checker, but rejected by
the NLL borrow checker is now a hard error in Rust 2018.][63565] This was
previously a warning, and will also become a hard error in the Rust 2015
edition in the 1.40.0 release.
- [`rustdoc` now requires `rustc` to be installed and in the same directory to
run tests.][63827] This should improve performance when running a large
amount of doctests.
- [The `try!` macro will now issue a deprecation warning.][62672] It is
recommended to use the `?` operator instead.
- [`asinh(-0.0)` now correctly returns `-0.0`.][63698] Previously this
returned `0.0`.
[62600]: rust-lang/rust#62600
[62672]: rust-lang/rust#62672
[63118]: rust-lang/rust#63118
[63209]: rust-lang/rust#63209
[63402]: rust-lang/rust#63402
[63565]: rust-lang/rust#63565
[63595]: rust-lang/rust#63595
[63684]: rust-lang/rust#63684
[63698]: rust-lang/rust#63698
[63770]: rust-lang/rust#63770
[63786]: rust-lang/rust#63786
[63827]: rust-lang/rust#63827
[63834]: rust-lang/rust#63834
[63927]: rust-lang/rust#63927
[63933]: rust-lang/rust#63933
[63934]: rust-lang/rust#63934
[63938]: rust-lang/rust#63938
[63940]: rust-lang/rust#63940
[63941]: rust-lang/rust#63941
[63945]: rust-lang/rust#63945
[64010]: rust-lang/rust#64010
[64028]: rust-lang/rust#64028
[64334]: rust-lang/rust#64334
[cargo/7237]: rust-lang/cargo#7237
[cargo/7241]: rust-lang/cargo#7241
[cargo/7315]: rust-lang/cargo#7315
[`Pin::into_inner`]: https://doc.rust-lang.org/std/pin/struct.Pin.html#method.into_inner
[`Instant::checked_duration_since`]: https://doc.rust-lang.org/std/time/struct.Instant.html#method.checked_duration_since
[`Instant::saturating_duration_since`]: https://doc.rust-lang.org/std/time/struct.Instant.html#method.saturating_duration_since
This was referenced Aug 27, 2020
bors
added a commit
to rust-lang/cargo
that referenced
this pull request
Aug 28, 2020
Fix LTO with doctests. This fixes an issue where `cargo test --release` would fail to run doctests if LTO is set in `profile.release` or `profile.bench`. The issue is that dependencies were built with `-Clinker-plugin-lto`, but the final rustdoc invocation did not issue `-C lto`. This causes a link failure (or crash!) because the necessary object code was missing. This is because rustdoc historically did not support codegen flags, so Cargo has never passed them in. Rustdoc now supports codegen flags (via rust-lang/rust#63827), so it should be safe to start passing them in. For now, I am only adding LTO, but more should be added in the future. There are two bugs here. One is that LTO flags aren't passed to rustdoc. The other is that the "doctest" unit was using the wrong profile (it was using dev/release when it should be using test/bench). There are two distinct scenarios here. One where just `release` has `lto` set. And one where both `release` and `bench` have `lto` set. This is relevant because LTO mostly cares about what the final artifact wants, and in the case where `bench` does not have `lto` set, then LTO will not be used for tests. This will hopefully be a little cleaner in the future when #6988 is stabilized, which causes the test/bench profiles to *inherit* from the dev/bench profiles, which means you won't need to manually synchronize the test/bench profiles with dev/release. Fixes #8654
ehuss
pushed a commit
to ehuss/cargo
that referenced
this pull request
Aug 28, 2020
Fix LTO with doctests. This fixes an issue where `cargo test --release` would fail to run doctests if LTO is set in `profile.release` or `profile.bench`. The issue is that dependencies were built with `-Clinker-plugin-lto`, but the final rustdoc invocation did not issue `-C lto`. This causes a link failure (or crash!) because the necessary object code was missing. This is because rustdoc historically did not support codegen flags, so Cargo has never passed them in. Rustdoc now supports codegen flags (via rust-lang/rust#63827), so it should be safe to start passing them in. For now, I am only adding LTO, but more should be added in the future. There are two bugs here. One is that LTO flags aren't passed to rustdoc. The other is that the "doctest" unit was using the wrong profile (it was using dev/release when it should be using test/bench). There are two distinct scenarios here. One where just `release` has `lto` set. And one where both `release` and `bench` have `lto` set. This is relevant because LTO mostly cares about what the final artifact wants, and in the case where `bench` does not have `lto` set, then LTO will not be used for tests. This will hopefully be a little cleaner in the future when rust-lang#6988 is stabilized, which causes the test/bench profiles to *inherit* from the dev/bench profiles, which means you won't need to manually synchronize the test/bench profiles with dev/release. Fixes rust-lang#8654
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
closes #63638