rustdoc: set panic output before starting compiler thread pool #52181
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
|
(rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
QuietMisdreavus
added
the
beta-nominated
|
r? @rust-lang/rustdoc Also nominating for beta backport because the linked issue in the OP is on beta. EDIT: Also, i'm not sure how this can be properly tested. The |
|
bors=me with CI done EDIT: And please add a test. You can have one with |
tko
reviewed
Jul 9, 2018
| } | ||
| let data = Arc::new(Mutex::new(Vec::new())); | ||
|
|
||
| let old = io::set_panic(Some(box Sink(data.clone()))); |
There was a problem hiding this comment.
While this new arrangement delays the printing of the collected output to a later, more appropriate stage, doesn't this still leave the panic handler eating more data, if there is any? Should the panic handler also be restored? (Or maybe it's otherwise guaranteed to exit before that can happen? Sorry, not familiar with the code here.)
There was a problem hiding this comment.
My assumption is that because this is what it was doing beforehand, it's not much harm to leave it. Before I had this version, I tried calling set_panic to switch the sink back in this Drop call. It didn't change anything about the output, so I set it back. Each test will set the panic writer to a fresh buffer, so there's no chance of test outputs accumulating.
However, if any panic occurs outside of this function, the output will be swallowed. This may be what we want, but it's something to consider.
|
@GuillaumeGomez I can't test this in |
|
I think you can configure the output in order to be checked as is. We can't let this without a test. If needed, add a new option into the tester. |
|
Turns out, there is a compiletest flag, |
|
@QuietMisdreavus You should also return the |
|
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'm not really sure what's up with the test error? The UI test is complaining that the
@Zoxc Is that really necessary, though? It looks like it works anyway: $ cat e.rs
/// This is a function
///
/// ```
/// no
/// ```
pub fn foo() {}
#[cfg(test)]
#[test]
fn bar() { panic!("the disco"); }
$ rustdoc +local --test e.rs
running 1 test
test e.rs - foo (line 3) ... FAILED
failures:
---- e.rs - foo (line 3) stdout ----
error[E0425]: cannot find value `no` in this scope
--> e.rs:4:1
|
3 | no
| ^^ not found in this scope
thread 'e.rs - foo (line 3)' panicked at 'couldn't compile the test', librustdoc/test.rs:325:17
note: Run with `RUST_BACKTRACE=1` for a backtrace.
failures:
e.rs - foo (line 3)
test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out
|
With |
|
@Zoxc That's unfortunate. I moved those panics outside the thread pool, and added a test which makes sure that doctest compile failures also properly appear. I set parallel queries up on my local fork and made sure it worked. I also had to manually apply one kind of normalization to the test stdout file. This makes it work on travis, but because those files were made automatically, will that cause any problems with people running them locally? The test doesn't pass locally any more. |
|
I hope you don't mind me butting in, but I was really curious about the path issue. The Offhand I'm not sure of a good way to fix that. It's a little late for me, so I can't keep looking right now, but hopefully that gives you some ideas where to look. In particular, contrary to the comments, these two values are normally absolute paths (based on |
QuietMisdreavus
added
the
T-rustdoc
|
@ehuss I think the root of the problem is that i'm disabling all UI test output normalization so i can run the test in the first place, but some flag that travis is running (path remapping?) is causing it to output |
It's not a different flag, exactly. It's just running from a separate directory (see here). If you |
|
Aha, now i see it! Turns out, there's a bad interaction between how rustdoc prints paths in its output, and how compiletest normalizes paths: rust/src/tools/compiletest/src/runtest.rs Lines 2886 to 2898 in fc49152 Since that only runs the replace on the full path, it doesn't hit the partial path that occurs when you're in a parent folder of the file in question. A quick scan of the repo tells me that one existing test uses the I also went back and double-checked the test i added, and removing Possible solutions i can think of offhand:
|
|
Ping @rust-lang/rustdoc, release date is approaching and this PR fixes a beta regression. |
|
And it seems like it added the missing test I asked for so let's not prevent its merge any further. Thanks @QuietMisdreavus! @bors: r+ |
|
📌 Commit 460f05aab147129e297a641b6ca47e71fd18dd29 has been approved by |
|
@bors p=1 (fixes beta regression) Also, can someone approve this for backport? Thanks! |
|
⌛ Testing commit 460f05aab147129e297a641b6ca47e71fd18dd29 with merge 913bb6a3b955dd2672d77d040bca23a80761972d... |
|
💔 Test failed - status-travis |
|
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 |
bors
added
S-waiting-on-review
|
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 |
Does macOS name threads differently? Doctest executions are run in threads that (on linux) are named after the doctest they appear in. It looks like the threads are all named |
kennytm
added
S-waiting-on-author
|
Ping, 1.28 release is approaching (next week). |
|
I think i see how the thread naming is happening. When specifying a doctest, rustdoc gives libtest a closure that calls Lines 561 to 582 in a1e6bcb
rust/src/librustc_driver/lib.rs Lines 1566 to 1567 in a1e6bcb If it doesn't spawn a new thread, it's because it was able to set the current thread's stack size earlier in the function. This works on Linux because it's able to call ...now i need to figure out how to fix it. |
|
Okay, i was able to get that last commit to work locally (forcing the thread to spawn on linux by setting the |
|
And here we go again! @bors: r+ |
|
📌 Commit 76e33b4 has been approved by |
bors
added
S-waiting-on-bors
bors
added a commit
that referenced
this pull request
Jul 24, 2018
rustdoc: set panic output before starting compiler thread pool When the compiler was updated to run on a thread pool, rustdoc's processing of compiler/doctest stderr/stdout was moved into each compiler thread. However, this caused output of the test to be lost if the test failed at *runtime* instead of compile time. This change sets up the `set_panic` call and output bomb before starting the compiler thread pool, so that the `Drop` call that writes back to the test's stdout happens after the test runs, not just after it compiles. Fixes #51162
|
☀️ Test successful - status-appveyor, status-travis |
pietroalbini
removed
the
beta-nominated
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.
When the compiler was updated to run on a thread pool, rustdoc's processing of compiler/doctest stderr/stdout was moved into each compiler thread. However, this caused output of the test to be lost if the test failed at runtime instead of compile time. This change sets up the
set_paniccall and output bomb before starting the compiler thread pool, so that theDropcall that writes back to the test's stdout happens after the test runs, not just after it compiles.Fixes #51162