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

Simplify output capturing #78714

Merged
merged 5 commits into from
Nov 17, 2020
Merged

Conversation

m-ou-se
Copy link
Member

@m-ou-se m-ou-se commented Nov 3, 2020

This is a sequence of incremental improvements to the unstable/internal set_panic and set_print mechanism used by the test crate:

  1. Remove the LocalOutput trait and use Arc<Mutex<dyn Write>> instead of Box<dyn LocalOutput>. In practice, all implementations of LocalOutput were just Arc<Mutex<..>>. This simplifies some logic and removes all custom Sink implementations such as library/test/src/helpers/sink.rs. Also removes a layer of indirection, as the outermost Box is now gone. It also means that locking now happens per write_fmt, not per individual write within. (So "{} {}\n" now results in one lock(), not four or more.)

  2. Since in all cases the dyn Writes were just Vec<u8>s, replace the type with Arc<Mutex<Vec<u8>>>. This simplifies things more, as error handling and flushing can be removed now. This also removes the hack needed in the default panic handler to make this work with ::realstd, as (unlike Write) Vec<u8> is from alloc, not std.

  3. Replace the RefCells by regular Cells. The RefCells were mostly used as mem::replace(&mut *cell.borrow_mut(), something), which is just Cell::replace. This removes an unecessary bookkeeping and makes the code a bit easier to read.

  4. Merge set_panic and set_print into a single set_output_capture. Neither the test crate nor rustc (the only users of this feature) have a use for using these separately. Merging them simplifies things even more. This uses a new function name and feature name, to make it clearer this is internal and not supposed to be used by other crates.

Might be easier to review per commit.

@m-ou-se m-ou-se added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. A-libtest Area: `#[test]` / the `test` library A-io Area: `std::io`, `std::fs`, `std::net` and `std::path` labels Nov 3, 2020
@rust-highfive
Copy link
Collaborator

r? @KodrAus

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 3, 2020
@KodrAus
Copy link
Contributor

KodrAus commented Nov 6, 2020

Merge set_panic and set_print into a single set_output_capture

I think these are two unstable APIs that are actually relied upon out in the wild. We have reserved the right to tinker with them, but I'd be interested to see how widely they're actually used! I'll kick off a crater run to see what impact this has.

@craterbot check

@craterbot
Copy link
Collaborator

🚨 Error: missing start toolchain

🆘 If you have any trouble with Crater please ping @rust-lang/infra!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@KodrAus
Copy link
Contributor

KodrAus commented Nov 6, 2020

@bors try

@bors
Copy link
Contributor

bors commented Nov 6, 2020

⌛ Trying commit b85533df13416e98e40f4ce7ac3661b72f24f109 with merge 3e22d1f9053c0fc87629b3c6d5d9567856368e5d...

@m-ou-se
Copy link
Member Author

m-ou-se commented Nov 6, 2020

I think these are two unstable APIs that are actually relied upon out in the wild.

Those APIs were already broken recently by #75172 / #78227. Those didn't get a crater run, though. Searching for set_print and set_panic on GitHub mostly gives results inside forks of Rust itself.

(These functions aren't just unstable, they are doc(hidden) too.)

@bors
Copy link
Contributor

bors commented Nov 6, 2020

☀️ Try build successful - checks-actions
Build commit: 3e22d1f9053c0fc87629b3c6d5d9567856368e5d (3e22d1f9053c0fc87629b3c6d5d9567856368e5d)

@m-ou-se
Copy link
Member Author

m-ou-se commented Nov 10, 2020

Starting crater with start=beta instead of the parent commit of this PR, because the function signatures were already changed/broken two weeks ago. (Hopefully that won't report too many unrelated changes.)

@craterbot check start=beta

@craterbot
Copy link
Collaborator

🚨 Error: missing end toolchain

🆘 If you have any trouble with Crater please ping @rust-lang/infra!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@m-ou-se
Copy link
Member Author

m-ou-se commented Nov 10, 2020

@craterbot check start=beta end=try#3e22d1f9053c0fc87629b3c6d5d9567856368e5d

@craterbot
Copy link
Collaborator

👌 Experiment pr-78714 created and queued.
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 10, 2020
It was only ever used with Vec<u8> anyway. This simplifies some things.

- It no longer needs to be flushed, because that's a no-op anyway for
  a Vec<u8>.

- Writing to a Vec<u8> never fails.

- No #[cfg(test)] code is needed anymore to use `realstd` instead of
  `std`, because Vec comes from alloc, not std (like Write).
There were no use cases for setting them separately.
Merging them simplifies some things.
@m-ou-se m-ou-se force-pushed the simplify-local-streams branch from b85533d to aff7bd6 Compare November 10, 2020 20:59
@craterbot
Copy link
Collaborator

🚧 Experiment pr-78714 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot
Copy link
Collaborator

🎉 Experiment pr-78714 is completed!
📊 379 regressed and 4349 fixed (129870 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the blacklist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels Nov 14, 2020
@m-ou-se
Copy link
Member Author

m-ou-se commented Nov 14, 2020

That crater run resulted in quite a few irrelevant failures, because it compaired against beta. These are the only four that had errors in their logs about missing set_print or set_panic:

  • wasm_glue:
    This crate's main purpose is to use set_print and set_panic to work around some issues with stdout/stderr on wasm. This crate doesn't seem to be very popular though.
  • cis198-2016s/demo-xcompile-android:
    Fails because it depends on an old version of the deprecated android-rs-glue project that used set_print/set_panic. Newer versions and the project that superseded it don't use those functions.
  • reitermarkus/lndir and rvolz/rustrt:
    These both fail because of their dependency on an old version of cucumber_rust. Versions since may 2019 don't use set_print or set_panic anymore.

@Mark-Simulacrum
Copy link
Member

FWIW we have access to by-commit artifacts so you can get a smaller commit range, but ultimately it probably wouldn't help too much. If we want a better run I would probably revert the breakage in a pr, try commit that, then compare against that in crater.

Let me know if you need more help here.

@m-ou-se
Copy link
Member Author

m-ou-se commented Nov 15, 2020

In this case I knew what the error would be, so it was as simple as just downloading the build-fail tar.gz from crater and grepping for set_panic and set_print. Only four crates matched.

@KodrAus
Copy link
Contributor

KodrAus commented Nov 16, 2020

That's a lot fewer than I was expecting! Thanks for looking into this @m-ou-se

@KodrAus
Copy link
Contributor

KodrAus commented Nov 16, 2020

Ok, this looks good to me. I was worried about churning an unstable hack that I thought was pretty widely relied on, but it turns out I was wrong!

@bors r+

@bors
Copy link
Contributor

bors commented Nov 16, 2020

📌 Commit aff7bd6 has been approved by KodrAus

@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 Nov 16, 2020
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 16, 2020
Rollup of 11 pull requests

Successful merges:

 - rust-lang#74989 (Implement `Index` and `IndexMut` for arrays)
 - rust-lang#76339 (Test structural matching for all range types)
 - rust-lang#77691 (Rename/Deprecate LayoutErr in favor of LayoutError)
 - rust-lang#78364 (Update RELEASES.md for 1.48.0)
 - rust-lang#78678 (Add tests and improve rendering of cfgs on traits)
 - rust-lang#78714 (Simplify output capturing)
 - rust-lang#78769 (Remove unneeded lifetimes in array/mod.rs)
 - rust-lang#78903 (BTreeMap: test chaotic ordering & other bits & bobs)
 - rust-lang#79032 (improve type const mismatch errors)
 - rust-lang#79061 (Make all rustdoc functions and structs crate-private)
 - rust-lang#79087 (Update E0744 about control flow in `const` contexts to accurately describe when the error is triggered and why)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 11ce918 into rust-lang:master Nov 17, 2020
@rustbot rustbot added this to the 1.50.0 milestone Nov 17, 2020
@m-ou-se m-ou-se deleted the simplify-local-streams branch March 31, 2021 18:38
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 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.

7 participants