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

Capture output from threads spawned in tests #78227

Merged
merged 3 commits into from
Oct 27, 2020

Conversation

SergioBenitez
Copy link
Contributor

@SergioBenitez SergioBenitez commented Oct 22, 2020

This is revival of #75172.

Original text:

Fixes #42474.

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


Closes #75172.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 22, 2020
@m-ou-se
Copy link
Member

m-ou-se commented Oct 22, 2020

Looks good. Thanks!

Did you look at this failure from the original PR? #75172 (comment) Would that still be a problem?

@m-ou-se
Copy link
Member

m-ou-se commented Oct 22, 2020

I think that failure is caused by the LOCAL_STDERR being destroyed before HANDLE is dropped in the spawned thread in that test:

thread_local!(static HANDLE: Handle = Handle(0));

Before this change, that thread would not have LOCAL_STDERR set. But now that it is set, the panic!() in the Drop of this test's Handle will cause the panic handler to access it after it was set (in thread::spawn) and already destroyed, triggering this panic:

self.try_with(f).expect(
"cannot access a Thread Local Storage value \
during or after destruction",
)

@m-ou-se
Copy link
Member

m-ou-se commented Oct 22, 2020

Looks like that test doesn't run in the test framework, so now that this change checks LOCAL_STREAMS before setting any streams, the failure should be fixed.

@m-ou-se m-ou-se added A-io Area: `std::io`, `std::fs`, `std::net` and `std::path` A-testsuite Area: The testsuite used to check the correctness of rustc T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Oct 22, 2020
Copy link
Member

@m-ou-se m-ou-se left a comment

Choose a reason for hiding this comment

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

Lgtm, with one tiny nit.

@m-ou-se
Copy link
Member

m-ou-se commented Oct 22, 2020

By the way: Panicking in the Drop implementaiton of a thread local in a thread in a unit test is probably not very common, but that situation could be partially fixed by making the panic handler use LOCAL_STDERR.try_with instead of .with. (It wouldn't capture the panic, but at least it would no longer trigger a double-panic.) Might be good as a follow-up PR (including a test that that works).

@jyn514 jyn514 added A-libtest Area: `#[test]` / the `test` library and removed A-testsuite Area: The testsuite used to check the correctness of rustc labels Oct 22, 2020
@jyn514
Copy link
Member

jyn514 commented Oct 22, 2020

(FYI A-testsuite is for src/test, i.e. tests for the rust compiler itself, not for #[test])

@SergioBenitez
Copy link
Contributor Author

Looks good. Thanks!

Did you look at this failure from the original PR? #75172 (comment) Would that still be a problem?

I thought there was a chance that the LOCAL_STREAMS check would resolve that issue. We'll see what the CI says.

@m-ou-se
Copy link
Member

m-ou-se commented Oct 23, 2020

@bors r+

@bors
Copy link
Contributor

bors commented Oct 23, 2020

📌 Commit db15596 has been approved by m-ou-se

@bors
Copy link
Contributor

bors commented Oct 23, 2020

🌲 The tree is currently closed for pull requests below priority 1000, this pull request will be tested once the tree is reopened

@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 Oct 23, 2020
@dtolnay dtolnay assigned m-ou-se and unassigned dtolnay Oct 23, 2020
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Oct 25, 2020
…, 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
…, 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
…, 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
Copy link

failed in rollup log (on wasm)

@bors r-

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Oct 26, 2020
@m-ou-se
Copy link
Member

m-ou-se commented Oct 26, 2020

@bors r+

@bors
Copy link
Contributor

bors commented Oct 26, 2020

📌 Commit 7c4fe00 has been approved by m-ou-se

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 26, 2020
@bors
Copy link
Contributor

bors commented Oct 27, 2020

⌛ Testing commit 7c4fe00 with merge 56d288f...

@bors
Copy link
Contributor

bors commented Oct 27, 2020

☀️ Test successful - checks-actions
Approved by: m-ou-se
Pushing 56d288f to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 27, 2020
@bors bors merged commit 56d288f into rust-lang:master Oct 27, 2020
@rustbot rustbot added this to the 1.49.0 milestone Oct 27, 2020
@m-ou-se
Copy link
Member

m-ou-se commented Oct 27, 2020

This is now merged. Thanks for reviving this!

@SergioBenitez
Copy link
Contributor Author

This is now merged. Thanks for reviving this!

Thank you for the swift help!

@jebrosen jebrosen mentioned this pull request Nov 1, 2020
9 tasks
@sfackler sfackler added the relnotes Marks issues that should be documented in the release notes of the next release. label Nov 5, 2020
netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request Feb 14, 2021
Pkgsrc changes:
 * Adjust patches, convert tabs to spaces so that tests pass.
 * Remove patches which are no longer needed (upstream changed)
 * Minor adjustments for SunOS, e.g. disable stack probes.
 * Adjust cargo checksum patching accordingly.
 * Remove commented-out use of PATCHELF on NetBSD, which doesn't work anyway...

Upstream changes:

Version 1.49.0 (2020-12-31)
============================

Language
-----------------------

- [Unions can now implement `Drop`, and you can now have a field in a union
  with `ManuallyDrop<T>`.][77547]
- [You can now cast uninhabited enums to integers.][76199]
- [You can now bind by reference and by move in patterns.][76119] This
  allows you to selectively borrow individual components of a type. E.g.
  ```rust
  #[derive(Debug)]
  struct Person {
      name: String,
      age: u8,
  }

  let person = Person {
      name: String::from("Alice"),
      age: 20,
  };

  // `name` is moved out of person, but `age` is referenced.
  let Person { name, ref age } = person;
  println!("{} {}", name, age);
  ```

Compiler
-----------------------

- [Added tier 1\* support for `aarch64-unknown-linux-gnu`.][78228]
- [Added tier 2 support for `aarch64-apple-darwin`.][75991]
- [Added tier 2 support for `aarch64-pc-windows-msvc`.][75914]
- [Added tier 3 support for `mipsel-unknown-none`.][78676]
- [Raised the minimum supported LLVM version to LLVM 9.][78848]
- [Output from threads spawned in tests is now captured.][78227]
- [Change os and vendor values to "none" and "unknown" for some targets][78951]

\* Refer to Rust's [platform support page][forge-platform-support] for more
information on Rust's tiered platform support.

Libraries
-----------------------

- [`RangeInclusive` now checks for exhaustion when calling `contains`
  and indexing.][78109]
- [`ToString::to_string` now no longer shrinks the internal buffer
  in the default implementation.][77997]
- [`ops::{Index, IndexMut}` are now implemented for fixed sized
  arrays of any length.][74989]

Stabilized APIs
---------------

- [`slice::select_nth_unstable`]
- [`slice::select_nth_unstable_by`]
- [`slice::select_nth_unstable_by_key`]

The following previously stable methods are now `const`.

- [`Poll::is_ready`]
- [`Poll::is_pending`]

Cargo
-----------------------
- [Building a crate with `cargo-package` should now be independently
  reproducible.][cargo/8864]
- [`cargo-tree` now marks proc-macro crates.][cargo/8765]
- [Added `CARGO_PRIMARY_PACKAGE` build-time environment
  variable.]  [cargo/8758] This variable will be set if the crate
  being built is one the user selected to build, either with `-p`
  or through defaults.
- [You can now use glob patterns when specifying packages &
  targets.][cargo/8752]


Compatibility Notes
-------------------
- [Demoted `i686-unknown-freebsd` from host tier 2 to target tier
  2 support.][78746]
- [Macros that end with a semi-colon are now treated as statements
  even if they expand to nothing.][78376]
- [Rustc will now check for the validity of some built-in attributes
  on enum variants.][77015] Previously such invalid or unused
  attributes could be ignored.
- Leading whitespace is stripped more uniformly in documentation
  comments, which may change behavior. You read [this post about
  the changes][rustdoc-ws-post] for more details.
- [Trait bounds are no longer inferred for associated types.][79904]

Internal Only
-------------
These changes provide no direct user facing benefits, but represent
significant improvements to the internals and overall performance
of rustc and related tools.

- [rustc's internal crates are now compiled using the `initial-exec` Thread
  Local Storage model.][78201]
- [Calculate visibilities once in resolve.][78077]
- [Added `system` to the `llvm-libunwind` bootstrap config option.][77703]
- [Added `--color` for configuring terminal color support to bootstrap.][79004]


[75991]: rust-lang/rust#75991
[78951]: rust-lang/rust#78951
[78848]: rust-lang/rust#78848
[78746]: rust-lang/rust#78746
[78376]: rust-lang/rust#78376
[78228]: rust-lang/rust#78228
[78227]: rust-lang/rust#78227
[78201]: rust-lang/rust#78201
[78109]: rust-lang/rust#78109
[78077]: rust-lang/rust#78077
[77997]: rust-lang/rust#77997
[77703]: rust-lang/rust#77703
[77547]: rust-lang/rust#77547
[77015]: rust-lang/rust#77015
[76199]: rust-lang/rust#76199
[76119]: rust-lang/rust#76119
[75914]: rust-lang/rust#75914
[74989]: rust-lang/rust#74989
[79004]: rust-lang/rust#79004
[78676]: rust-lang/rust#78676
[79904]: rust-lang/rust#79904
[cargo/8864]: rust-lang/cargo#8864
[cargo/8765]: rust-lang/cargo#8765
[cargo/8758]: rust-lang/cargo#8758
[cargo/8752]: rust-lang/cargo#8752
[`slice::select_nth_unstable`]: https://doc.rust-lang.org/nightly/std/primitive.slice.html#method.select_nth_unstable
[`slice::select_nth_unstable_by`]: https://doc.rust-lang.org/nightly/std/primitive.slice.html#method.select_nth_unstable_by
[`slice::select_nth_unstable_by_key`]: https://doc.rust-lang.org/nightly/std/primitive.slice.html#method.select_nth_unstable_by_key
[`hint::spin_loop`]: https://doc.rust-lang.org/stable/std/hint/fn.spin_loop.html
[`Poll::is_ready`]: https://doc.rust-lang.org/stable/std/task/enum.Poll.html#method.is_ready
[`Poll::is_pending`]: https://doc.rust-lang.org/stable/std/task/enum.Poll.html#method.is_pending
[rustdoc-ws-post]: https://blog.guillaume-gomez.fr/articles/2020-11-11+New+doc+comment+handling+in+rustdoc
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-io Area: `std::io`, `std::fs`, `std::net` and `std::path` A-libtest Area: `#[test]` / the `test` library merged-by-bors This PR was explicitly merged by bors. relnotes Marks issues that should be documented in the release notes of the next release. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cargo test doesn't capture print from threads
10 participants