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.

Sign up for GitHub

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

Jump to bottom

Capture output from threads spawned in tests #75172

Closed
wants to merge 1 commit into from
Closed

Capture output from threads spawned in tests #75172

wants to merge 1 commit into from

Conversation

tmandry
Copy link
Member

@tmandry tmandry commented Aug 5, 2020

Fixes #42474.

r? @dtolnay since you expressed interest in this, but feel free to redirect if you aren't the right person anymore.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 5, 2020
dtolnay
dtolnay approved these changes Aug 7, 2020
View reviewed changes
Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

@dtolnay
Copy link
Member

dtolnay commented Aug 7, 2020

Er, CI failed though:

--- [ui] ui/panic-while-printing.rs stdout ----

error: test compilation failed although it shouldn't!
status: exit code: 1
command: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "/checkout/src/test/ui/panic-while-printing.rs" "-Zthreads=1" "--target=x86_64-unknown-linux-gnu" "--error-format" "json" "-Zui-testing" "-Zdeduplicate-diagnostics=no" "-C" "prefer-dynamic" "-o" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/panic-while-printing/a" "-Crpath" "-O" "-Cdebuginfo=0" "-Zunstable-options" "-Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/panic-while-printing/auxiliary"
stdout:
------------------------------------------

------------------------------------------
stderr:
------------------------------------------
error[E0277]: the trait bound `std::vec::Vec<_>: std::io::LocalOutput` is not satisfied
  --> /checkout/src/test/ui/panic-while-printing.rs:19:20
   |
LL |     set_panic(Some(Box::new(Vec::new())));
   |                    ^^^^^^^^^^^^^^^^^^^^ the trait `std::io::LocalOutput` is not implemented for `std::vec::Vec<_>`
   |
   = note: required for the cast to the object type `dyn std::io::LocalOutput`



--- [ui] ui/threads-sendsync/task-stderr.rs stdout ----

error: test compilation failed although it shouldn't!
status: exit code: 1
command: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "/checkout/src/test/ui/threads-sendsync/task-stderr.rs" "-Zthreads=1" "--target=x86_64-unknown-linux-gnu" "--error-format" "json" "-Zui-testing" "-Zdeduplicate-diagnostics=no" "-C" "prefer-dynamic" "-o" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/threads-sendsync/task-stderr/a" "-Crpath" "-O" "-Cdebuginfo=0" "-Zunstable-options" "-Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/threads-sendsync/task-stderr/auxiliary"
stdout:
------------------------------------------

------------------------------------------
stderr:
------------------------------------------
error[E0277]: the trait bound `Sink: std::io::LocalOutput` is not satisfied
  --> /checkout/src/test/ui/threads-sendsync/task-stderr.rs:24:28
   |
LL |         io::set_panic(Some(Box::new(sink)));
   |                            ^^^^^^^^^^^^^^ the trait `std::io::LocalOutput` is not implemented for `Sink`
   |
   = note: required for the cast to the object type `dyn std::io::LocalOutput`

@tmandry
Copy link
Member Author

tmandry commented Aug 18, 2020

Tests fixed!

@dtolnay
Copy link
Member

dtolnay commented Aug 18, 2020 

error[E0277]: the trait bound `dyn realstd::io::LocalOutput: io::Write` is not satisfied
   --> library/std/src/panicking.rs:216:15
    |
216 |         write(&mut local);
    |               ^^^^^^^^^^ the trait `io::Write` is not implemented for `dyn realstd::io::LocalOutput`
    |
    = note: required because of the requirements on the impl of `io::Write` for `realstd::boxed::Box<dyn realstd::io::LocalOutput>`
    = note: required for the cast to the object type `dyn io::Write`

@tmandry
Copy link
Member Author

tmandry commented Aug 18, 2020

Yep, working on this now. Sorry for the noise.

@tmandry
Copy link
Member Author

tmandry commented Aug 18, 2020

CI passed.

@dtolnay
Copy link
Member

dtolnay commented Aug 18, 2020

@bors r

@bors
Copy link
Contributor

bors commented Aug 18, 2020

📌 Commit 38b920d has been approved by dtolnay

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 18, 2020
tmandry added a commit to tmandry/rust that referenced this pull request Aug 18, 2020
@tmandry
Rollup merge of rust-lang#75172 - tmandry:test-thread-capture, r=dtolnay
ad4610d 
Capture output from threads spawned in tests

Fixes rust-lang#42474.

r? @dtolnay since you expressed interest in this, but feel free to redirect if you aren't the right person anymore.
@tmandry tmandry mentioned this pull request Aug 18, 2020
Closed
tmandry added a commit to tmandry/rust that referenced this pull request Aug 18, 2020
@tmandry
Rollup merge of rust-lang#75172 - tmandry:test-thread-capture, r=dtolnay
03922c6 
Capture output from threads spawned in tests

Fixes rust-lang#42474.

r? @dtolnay since you expressed interest in this, but feel free to redirect if you aren't the right person anymore.
@tmandry tmandry mentioned this pull request Aug 18, 2020
Closed
@tmandry
Copy link
Member Author

tmandry commented Aug 19, 2020

@bors rollup=iffy

I'm trying to track down the cause of #75684 (comment) and while I don't think this can cause it (nor can I reproduce locally), nothing else in that change seems related.

@bors
Copy link
Contributor

bors commented Aug 19, 2020

⌛ Testing commit 38b920d with merge 7b86e1eeec51cc8ce5e8edf130f2df638605da97...

@tmandry
Copy link
Member Author

tmandry commented Aug 19, 2020

@bors retry
Yielding to rollup

@bors
Copy link
Contributor

bors commented Aug 19, 2020

⌛ Testing commit 38b920d with merge 049ea2e42ed0a109e27a83fa7636690d19e3bfc8...

@tmandry
Copy link
Member Author

tmandry commented Aug 19, 2020

@bors retry

@bors
Copy link
Contributor

bors commented Aug 19, 2020

⌛ Testing commit 38b920d with merge ea7c21aedf7c1bbab87df466b39bb79968f6804f...

@bors
Copy link
Contributor

bors commented Aug 19, 2020

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Aug 19, 2020
@tmandry
Copy link
Member Author

tmandry commented Aug 19, 2020

Same failure. Seems to only happen on the dist-i586-gnu-i586-i686-musl bot, I'll investigate more tomorrow.

@bors
Copy link
Contributor

bors commented Aug 30, 2020

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

@crlf0710 crlf0710 added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 18, 2020
@crlf0710
Copy link
Member

Triage: There's merge conflicts now.

@crlf0710 crlf0710 added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 8, 2020
@m-ou-se
Copy link
Member

m-ou-se commented Oct 20, 2020

This PR partially breaks the effect of std::io::stdio::LOCAL_STREAMS:

rust/library/std/src/io/stdio.rs

Lines 33 to 37 in 981346f

/// Flag to indicate LOCAL_STDOUT and/or LOCAL_STDERR is used.
///
/// If both are None and were never set on any thread, this flag is set to
/// false, and both LOCAL_STDOUT and LOCAL_STDOUT can be safely ignored on all
/// threads, saving some time and memory registering an unused thread local.

clone_io should first check LOCAL_STREAMS.load(Relaxed), and return (None, None) if it was false. Otherwise, spawning a thread in non-test program will needlessly register thread local destructors for LOCAL_STDOUT and LOCAL_STDERR.

@SergioBenitez SergioBenitez mentioned this pull request Oct 22, 2020
Merged
@dtolnay
Copy link
Member

dtolnay commented Oct 23, 2020

Closing in favor of #78227 which includes a rebase of this commit.

@dtolnay dtolnay closed this Oct 23, 2020
@m-ou-se m-ou-se mentioned this pull request Oct 23, 2020
Closed
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Oct 25, 2020
@Dylan-DPC
Rollup merge of rust-lang#78227 - SergioBenitez:test-stdout-threading…
0c44b23 
…, r=m-ou-se

Capture output from threads spawned in tests

This is revival of rust-lang#75172.

Original text:
> Fixes rust-lang#42474.
>
> r? `@dtolnay` since you expressed interest in this, but feel free to redirect if you aren't the right person anymore.

---

Closes rust-lang#75172.
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Oct 25, 2020
@Dylan-DPC
Rollup merge of rust-lang#78227 - SergioBenitez:test-stdout-threading…
117ce17 
…, r=m-ou-se

Capture output from threads spawned in tests

This is revival of rust-lang#75172.

Original text:
> Fixes rust-lang#42474.
>
> r? `@dtolnay` since you expressed interest in this, but feel free to redirect if you aren't the right person anymore.

---

Closes rust-lang#75172.
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Oct 26, 2020
@Dylan-DPC
Rollup merge of rust-lang#78227 - SergioBenitez:test-stdout-threading…
489c4b0 
…, r=m-ou-se

Capture output from threads spawned in tests

This is revival of rust-lang#75172.

Original text:
> Fixes rust-lang#42474.
>
> r? `@dtolnay` since you expressed interest in this, but feel free to redirect if you aren't the right person anymore.

---

Closes rust-lang#75172.
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Oct 26, 2020
@Dylan-DPC
Rollup merge of rust-lang#78227 - SergioBenitez:test-stdout-threading…
6b28db3 
…, r=m-ou-se

Capture output from threads spawned in tests

This is revival of rust-lang#75172.

Original text:
> Fixes rust-lang#42474.
>
> r? `@dtolnay` since you expressed interest in this, but feel free to redirect if you aren't the right person anymore.

---

Closes rust-lang#75172.
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 27, 2020
@bors
Auto merge of rust-lang#78227 - SergioBenitez:test-stdout-threading, …
56d288f 
…r=m-ou-se

Capture output from threads spawned in tests

This is revival of rust-lang#75172.

Original text:
> Fixes rust-lang#42474.
>
> r? `@​dtolnay` since you expressed interest in this, but feel free to redirect if you aren't the right person anymore.

---

Closes rust-lang#75172.
@m-ou-se m-ou-se mentioned this pull request Nov 6, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dtolnay dtolnay dtolnay approved these changes

Assignees

@dtolnay dtolnay

Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

cargo test doesn't capture print from threads
6 participants
@tmandry @dtolnay @bors @crlf0710 @m-ou-se @rust-highfive