Rollup merge of #147952 - ferrocene:pvdrz/remote-test-client-timeout, r=Enselic,jieyouxu

Add a timeout to the `remote-test-client` connection

Currently, the `remote-test-client` doesn't have a timeout when connecting to the `remote-test-server`. This means that running tests using it can hang indefinitely which causes issues when running tests on CI, for example.

This PR now sets a default timeout of 5 minutes, meaning that if, for example, `TEST_DEVICE_ADDR=<IP:PORT> ./x test --target riscv64gc-unknown-linux-gnu tests/ui` is run and the `remote-test-server` is not reachable by the client, the client will panic after the timeout is reached.

Additionally, the `TEST_DEVICE_CONNECT_TIMEOUT` env variable can be used to set up the timeout to any value (in seconds).

This PR also wires up a test step for `remote-test-client`, which didn't previously have tool tests run in CI.

Author: Zalathar

Cargo.lock

@@ -229,6 +229,21 @@ dependencies = [
  "winnow 0.7.13",
 ]
 
+[[package]]
+name = "assert_cmd"
+version = "2.1.1"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "bcbb6924530aa9e0432442af08bbcafdad182db80d2e560da42a6d442535bf85"
+dependencies = [
+ "anstyle",
+ "bstr",
+ "libc",
+ "predicates",
+ "predicates-core",
+ "predicates-tree",
+ "wait-timeout",
+]
+
 [[package]]
 name = "autocfg"
 version = "1.5.0"
@@ -1128,6 +1143,12 @@ version = "0.1.13"
 source = "registry+https://github.com/rust-lang/crates.io-index"
 checksum = "56254986775e3233ffa9c4d7d3faaf6d36a2c09d30b20687e9f88bc8bafc16c8"
 
+[[package]]
+name = "difflib"
+version = "0.4.0"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "6184e33543162437515c2e2b48714794e37845ec9851711914eec9d308f6ebe8"
+
 [[package]]
 name = "digest"
 version = "0.10.7"
@@ -2949,6 +2970,33 @@ version = "0.1.1"
 source = "registry+https://github.com/rust-lang/crates.io-index"
 checksum = "925383efa346730478fb4838dbe9137d2a47675ad789c546d150a6e1dd4ab31c"
 
+[[package]]
+name = "predicates"
+version = "3.1.3"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "a5d19ee57562043d37e82899fade9a22ebab7be9cef5026b07fda9cdd4293573"
+dependencies = [
+ "anstyle",
+ "difflib",
+ "predicates-core",
+]
+
+[[package]]
+name = "predicates-core"
+version = "1.0.9"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "727e462b119fe9c93fd0eb1429a5f7647394014cf3c04ab2c0350eeb09095ffa"
+
+[[package]]
+name = "predicates-tree"
+version = "1.0.12"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "72dd2d6d381dfb73a193c7fca536518d7caee39fc8503f74e7dc0be0531b425c"
+dependencies = [
+ "predicates-core",
+ "termtree",
+]
+
 [[package]]
 name = "prettydiff"
 version = "0.7.0"
@@ -3229,6 +3277,9 @@ checksum = "2b15c43186be67a4fd63bee50d0303afffcef381492ebe2c5d87f324e1b8815c"
 [[package]]
 name = "remote-test-client"
 version = "0.1.0"
+dependencies = [
+ "assert_cmd",
+]
 
 [[package]]
 name = "remote-test-server"
@@ -5425,6 +5476,12 @@ dependencies = [
  "windows-sys 0.60.2",
 ]
 
+[[package]]
+name = "termtree"
+version = "0.5.1"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "8f50febec83f5ee1df3015341d8bd429f2d1cc62bcba7ea2076759d315084683"
+
 [[package]]
 name = "test-float-parse"
 version = "0.1.0"
@@ -6043,6 +6100,15 @@ version = "0.9.5"
 source = "registry+https://github.com/rust-lang/crates.io-index"
 checksum = "0b928f33d975fc6ad9f86c8f283853ad26bdd5b10b7f1542aa2fa15e2289105a"
 
+[[package]]
+name = "wait-timeout"
+version = "0.2.1"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "09ac3b126d3914f9849036f826e054cbabdc8519970b8998ddaf3b5bd3c65f11"
+dependencies = [
+ "libc",
+]
+
 [[package]]
 name = "walkdir"
 version = "2.5.0"

src/bootstrap/src/core/build_steps/test.rs

@@ -3986,3 +3986,40 @@ impl Step for CollectLicenseMetadata {
         dest
     }
 }
+
+#[derive(Debug, Clone, PartialEq, Eq, Hash)]
+pub struct RemoteTestClientTests {
+    host: TargetSelection,
+}
+
+impl Step for RemoteTestClientTests {
+    type Output = ();
+    const IS_HOST: bool = true;
+    const DEFAULT: bool = true;
+
+    fn should_run(run: ShouldRun<'_>) -> ShouldRun<'_> {
+        run.path("src/tools/remote-test-client")
+    }
+
+    fn make_run(run: RunConfig<'_>) {
+        run.builder.ensure(Self { host: run.target });
+    }
+
+    fn run(self, builder: &Builder<'_>) {
+        let bootstrap_host = builder.config.host_target;
+        let compiler = builder.compiler(0, bootstrap_host);
+
+        let cargo = tool::prepare_tool_cargo(
+            builder,
+            compiler,
+            Mode::ToolBootstrap,
+            bootstrap_host,
+            Kind::Test,
+            "src/tools/remote-test-client",
+            SourceType::InTree,
+            &[],
+        );
+
+        run_cargo_test(cargo, &[], &[], "remote-test-client", bootstrap_host, builder);
+    }
+}

src/bootstrap/src/core/builder/mod.rs

@@ -891,6 +891,7 @@ impl<'a> Builder<'a> {
                 test::CrateRustdoc,
                 test::CrateRustdocJsonTypes,
                 test::CrateBootstrap,
+                test::RemoteTestClientTests,
                 test::Linkcheck,
                 test::TierCheck,
                 test::Cargotest,

src/doc/rustc-dev-guide/src/tests/running.md

@@ -319,6 +319,10 @@ Tests are built on the machine running `x` not on the remote machine. Tests
 which fail to build unexpectedly (or `ui` tests producing incorrect build
 output) may fail without ever running on the remote machine.
 
+There is a default timeout of 30 minutes in case the `remote-test-server`
+cannot be reached by the `x` command. This timeout can be modified by using the
+`TEST_DEVICE_TIMEOUT_SECONDS` environment variable.
+
 ## Testing on emulators
 
 Some platforms are tested via an emulator for architectures that aren't readily

src/tools/remote-test-client/Cargo.toml

@@ -4,3 +4,4 @@ version = "0.1.0"
 edition = "2021"
 
 [dependencies]
+assert_cmd = "2"

src/tools/remote-test-client/src/main.rs

@@ -11,12 +11,16 @@ use std::io::{self, BufWriter};
 use std::net::TcpStream;
 use std::path::{Path, PathBuf};
 use std::process::{Command, Stdio};
-use std::time::Duration;
+use std::time::{Duration, Instant};
 use std::{env, thread};
 
 const REMOTE_ADDR_ENV: &str = "TEST_DEVICE_ADDR";
 const DEFAULT_ADDR: &str = "127.0.0.1:12345";
 
+const CONNECT_TIMEOUT_ENV: &str = "TEST_DEVICE_CONNECT_TIMEOUT_SECONDS";
+/// The default timeout is high to not break slow CI or slow device starts.
+const DEFAULT_CONNECT_TIMEOUT: Duration = Duration::from_mins(30);
+
 macro_rules! t {
     ($e:expr) => {
         match $e {
@@ -56,6 +60,17 @@ fn main() {
     }
 }
 
+fn connect_timeout() -> Duration {
+    match env::var(CONNECT_TIMEOUT_ENV).ok() {
+        Some(timeout) => timeout.parse().map(Duration::from_secs).unwrap_or_else(|e| {
+            panic!(
+                "error: parsing `{CONNECT_TIMEOUT_ENV}` value \"{timeout}\" as seconds failed: {e}"
+            )
+        }),
+        None => DEFAULT_CONNECT_TIMEOUT,
+    }
+}
+
 fn spawn_emulator(target: &str, server: &Path, tmpdir: &Path, rootfs: Option<PathBuf>) {
     let device_address = env::var(REMOTE_ADDR_ENV).unwrap_or(DEFAULT_ADDR.to_string());
 
@@ -69,20 +84,28 @@ fn spawn_emulator(target: &str, server: &Path, tmpdir: &Path, rootfs: Option<Pat
     }
 
     // Wait for the emulator to come online
-    loop {
+    let timeout = connect_timeout();
+    let mut successful_read = false;
+    let start_time = Instant::now();
+    while start_time.elapsed() < timeout {
         let dur = Duration::from_millis(2000);
         if let Ok(mut client) = TcpStream::connect(&device_address) {
             t!(client.set_read_timeout(Some(dur)));
             t!(client.set_write_timeout(Some(dur)));
             if client.write_all(b"ping").is_ok() {
                 let mut b = [0; 4];
                 if client.read_exact(&mut b).is_ok() {
+                    successful_read = true;
                     break;
                 }
             }
         }
         thread::sleep(dur);
     }
+
+    if !successful_read {
+        panic!("Gave up trying to connect to test device at {device_address} after {timeout:?}");
+    }
 }
 
 fn start_android_emulator(server: &Path) {

src/tools/remote-test-client/tests/lib.rs

@@ -0,0 +1,14 @@
+#[test]
+fn test_timeout() {
+    let mut cmd = assert_cmd::cargo::cargo_bin_cmd!();
+    cmd.env("TEST_DEVICE_CONNECT_TIMEOUT_SECONDS", "1");
+    cmd.env("TEST_DEVICE_ADDR", "0.0.0.0:6969");
+    cmd.args(["spawn-emulator", "riscv64-unknown-linux-gnu", "./"]);
+    cmd.arg(std::env::temp_dir());
+
+    let assert = cmd.assert().failure();
+    let output = assert.get_output();
+
+    let stderr = String::from_utf8(output.stderr.clone()).unwrap();
+    assert!(stderr.contains("Gave up trying to connect to test device"));
+}