Rollup merge of rust-lang#146119 - Zalathar:capture, r=jieyouxu

compiletest: Implement an experimental `--new-output-capture` mode

Thanks to the efforts on rust-lang#140192, compiletest no longer has an unstable dependency on libtest, but it still has an unstable dependency on `#![feature(internal_output_capture)]`. That makes building compiletest more complicated than for most other bootstrap tools.

This PR therefore adds opt-in support for an experimental compiletest mode that avoids the use of `internal_output_capture` APIs, and instead uses more mundane means to capture the output of individual test runners.

Each `TestCx` now contains `&dyn ConsoleOut` references for stdout and stderr. All print statements in `compiletests::runtest` have been replaced with `write!` or `writeln!` calls that explicitly write to one of those trait objects. The underlying implementation then forwards to `print!` or `eprint!` (for `--no-capture` or old-output-capture mode), or writes to a separate buffer (in new-output-capture mode).

---

Currently, new-output-capture is disabled by default. It can be explicitly enabled in one of two ways:

- When running `x test`, pass `--new-output-capture=on` as a *compiletest* argument (after `--`).
  - E.g. `x test ui -- --new-output-capture=on`.
  - The short form is `-Non` or `-Ny`.
- Set environment variable `COMPILETEST_NEW_OUTPUT_CAPTURE=on`.

After some amount of opt-in testing, new-output-capture will become the default (with a temporary opt-out). Eventually, old-output-capture and `#![feature(internal_output_capture)]` will be completely removed from compiletest.

r? jieyouxu

Author: jhpratt

src/tools/compiletest/src/common.rs

@@ -667,6 +667,10 @@ pub struct Config {
     /// to avoid `!nocapture` double-negatives.
     pub nocapture: bool,
 
+    /// True if the experimental new output-capture implementation should be
+    /// used, avoiding the need for `#![feature(internal_output_capture)]`.
+    pub new_output_capture: bool,
+
     /// Needed both to construct [`build_helper::git::GitConfig`].
     pub nightly_branch: String,
     pub git_merge_commit_email: String,
@@ -784,6 +788,7 @@ impl Config {
             builtin_cfg_names: Default::default(),
             supported_crate_types: Default::default(),
             nocapture: Default::default(),
+            new_output_capture: Default::default(),
             nightly_branch: Default::default(),
             git_merge_commit_email: Default::default(),
             profiler_runtime: Default::default(),

src/tools/compiletest/src/executor.rs

@@ -13,6 +13,7 @@ use std::sync::{Arc, Mutex, mpsc};
 use std::{env, hint, io, mem, panic, thread};
 
 use crate::common::{Config, TestPaths};
+use crate::output_capture::{self, ConsoleOut};
 use crate::panic_hook;
 
 mod deadline;
@@ -120,28 +121,28 @@ fn run_test_inner(
     runnable_test: RunnableTest,
     completion_sender: mpsc::Sender<TestCompletion>,
 ) {
-    let is_capture = !runnable_test.config.nocapture;
+    let capture = CaptureKind::for_config(&runnable_test.config);
 
     // Install a panic-capture buffer for use by the custom panic hook.
-    if is_capture {
+    if capture.should_set_panic_hook() {
         panic_hook::set_capture_buf(Default::default());
     }
-    let capture_buf = is_capture.then(|| Arc::new(Mutex::new(vec![])));
 
-    if let Some(capture_buf) = &capture_buf {
-        io::set_output_capture(Some(Arc::clone(capture_buf)));
+    if let CaptureKind::Old { ref buf } = capture {
+        io::set_output_capture(Some(Arc::clone(buf)));
     }
 
-    let panic_payload = panic::catch_unwind(move || runnable_test.run()).err();
+    let stdout = capture.stdout();
+    let stderr = capture.stderr();
+
+    let panic_payload = panic::catch_unwind(move || runnable_test.run(stdout, stderr)).err();
 
     if let Some(panic_buf) = panic_hook::take_capture_buf() {
         let panic_buf = panic_buf.lock().unwrap_or_else(|e| e.into_inner());
-        // For now, forward any captured panic message to (captured) stderr.
-        // FIXME(Zalathar): Once we have our own output-capture buffer for
-        // non-panic output, append the panic message to that buffer instead.
-        eprint!("{panic_buf}");
+        // Forward any captured panic message to (captured) stderr.
+        write!(stderr, "{panic_buf}");
     }
-    if is_capture {
+    if matches!(capture, CaptureKind::Old { .. }) {
         io::set_output_capture(None);
     }
 
@@ -152,11 +153,70 @@ fn run_test_inner(
             TestOutcome::Failed { message: Some("test did not panic as expected") }
         }
     };
-    let stdout = capture_buf.map(|mutex| mutex.lock().unwrap_or_else(|e| e.into_inner()).to_vec());
 
+    let stdout = capture.into_inner();
     completion_sender.send(TestCompletion { id, outcome, stdout }).unwrap();
 }
 
+enum CaptureKind {
+    /// Do not capture test-runner output, for `--no-capture`.
+    ///
+    /// (This does not affect `rustc` and other subprocesses spawned by test
+    /// runners, whose output is always captured.)
+    None,
+
+    /// Use the old output-capture implementation, which relies on the unstable
+    /// library feature `#![feature(internal_output_capture)]`.
+    Old { buf: Arc<Mutex<Vec<u8>>> },
+
+    /// Use the new output-capture implementation, which only uses stable Rust.
+    New { buf: output_capture::CaptureBuf },
+}
+
+impl CaptureKind {
+    fn for_config(config: &Config) -> Self {
+        if config.nocapture {
+            Self::None
+        } else if config.new_output_capture {
+            Self::New { buf: output_capture::CaptureBuf::new() }
+        } else {
+            // Create a capure buffer for `io::set_output_capture`.
+            Self::Old { buf: Default::default() }
+        }
+    }
+
+    fn should_set_panic_hook(&self) -> bool {
+        match self {
+            Self::None => false,
+            Self::Old { .. } => true,
+            Self::New { .. } => true,
+        }
+    }
+
+    fn stdout(&self) -> &dyn ConsoleOut {
+        self.capture_buf_or(&output_capture::Stdout)
+    }
+
+    fn stderr(&self) -> &dyn ConsoleOut {
+        self.capture_buf_or(&output_capture::Stderr)
+    }
+
+    fn capture_buf_or<'a>(&'a self, fallback: &'a dyn ConsoleOut) -> &'a dyn ConsoleOut {
+        match self {
+            Self::None | Self::Old { .. } => fallback,
+            Self::New { buf } => buf,
+        }
+    }
+
+    fn into_inner(self) -> Option<Vec<u8>> {
+        match self {
+            Self::None => None,
+            Self::Old { buf } => Some(buf.lock().unwrap_or_else(|e| e.into_inner()).to_vec()),
+            Self::New { buf } => Some(buf.into_inner().into()),
+        }
+    }
+}
+
 #[derive(Clone, Copy, Debug, PartialEq, Eq, Hash)]
 struct TestId(usize);
 
@@ -174,10 +234,12 @@ impl RunnableTest {
         Self { config, testpaths, revision }
     }
 
-    fn run(&self) {
+    fn run(&self, stdout: &dyn ConsoleOut, stderr: &dyn ConsoleOut) {
         __rust_begin_short_backtrace(|| {
             crate::runtest::run(
                 Arc::clone(&self.config),
+                stdout,
+                stderr,
                 &self.testpaths,
                 self.revision.as_deref(),
             );

src/tools/compiletest/src/lib.rs

@@ -15,6 +15,7 @@ pub mod directives;
 pub mod errors;
 mod executor;
 mod json;
+mod output_capture;
 mod panic_hook;
 mod raise_fd_limit;
 mod read2;
@@ -177,6 +178,12 @@ pub fn parse_config(args: Vec<String>) -> Config {
         // FIXME: Temporarily retained so we can point users to `--no-capture`
         .optflag("", "nocapture", "")
         .optflag("", "no-capture", "don't capture stdout/stderr of tests")
+        .optopt(
+            "N",
+            "new-output-capture",
+            "enables or disables the new output-capture implementation",
+            "off|on",
+        )
         .optflag("", "profiler-runtime", "is the profiler runtime enabled for this target")
         .optflag("h", "help", "show this message")
         .reqopt("", "channel", "current Rust channel", "CHANNEL")
@@ -461,6 +468,14 @@ pub fn parse_config(args: Vec<String>) -> Config {
         supported_crate_types: OnceLock::new(),
 
         nocapture: matches.opt_present("no-capture"),
+        new_output_capture: {
+            let value = matches
+                .opt_str("new-output-capture")
+                .or_else(|| env::var("COMPILETEST_NEW_OUTPUT_CAPTURE").ok())
+                .unwrap_or_else(|| "off".to_owned());
+            parse_bool_option(&value)
+                .unwrap_or_else(|| panic!("unknown `--new-output-capture` value `{value}` given"))
+        },
 
         nightly_branch: matches.opt_str("nightly-branch").unwrap(),
         git_merge_commit_email: matches.opt_str("git-merge-commit-email").unwrap(),
@@ -476,6 +491,19 @@ pub fn parse_config(args: Vec<String>) -> Config {
     }
 }
 
+/// Parses the same set of boolean values accepted by rustc command-line arguments.
+///
+/// Accepting all of these values is more complicated than just picking one
+/// pair, but has the advantage that contributors who are used to rustc
+/// shouldn't have to think about which values are legal.
+fn parse_bool_option(value: &str) -> Option<bool> {
+    match value {
+        "off" | "no" | "n" | "false" => Some(false),
+        "on" | "yes" | "y" | "true" => Some(true),
+        _ => None,
+    }
+}
+
 pub fn opt_str(maybestr: &Option<String>) -> &str {
     match *maybestr {
         None => "(none)",

src/tools/compiletest/src/output_capture.rs

@@ -0,0 +1,52 @@
+use std::fmt;
+use std::panic::RefUnwindSafe;
+use std::sync::Mutex;
+
+pub trait ConsoleOut: fmt::Debug + RefUnwindSafe {
+    fn write_fmt(&self, args: fmt::Arguments<'_>);
+}
+
+#[derive(Debug)]
+pub(crate) struct Stdout;
+
+impl ConsoleOut for Stdout {
+    fn write_fmt(&self, args: fmt::Arguments<'_>) {
+        print!("{args}");
+    }
+}
+
+#[derive(Debug)]
+pub(crate) struct Stderr;
+
+impl ConsoleOut for Stderr {
+    fn write_fmt(&self, args: fmt::Arguments<'_>) {
+        eprint!("{args}");
+    }
+}
+
+pub(crate) struct CaptureBuf {
+    inner: Mutex<String>,
+}
+
+impl CaptureBuf {
+    pub(crate) fn new() -> Self {
+        Self { inner: Mutex::new(String::new()) }
+    }
+
+    pub(crate) fn into_inner(self) -> String {
+        self.inner.into_inner().unwrap_or_else(|e| e.into_inner())
+    }
+}
+
+impl fmt::Debug for CaptureBuf {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        f.debug_struct("CaptureBuf").finish_non_exhaustive()
+    }
+}
+
+impl ConsoleOut for CaptureBuf {
+    fn write_fmt(&self, args: fmt::Arguments<'_>) {
+        let mut s = self.inner.lock().unwrap_or_else(|e| e.into_inner());
+        <String as fmt::Write>::write_fmt(&mut s, args).unwrap();
+    }
+}