Auto merge of #149754 - jieyouxu:compiletest-cli, r=<try>

Retire `opt_str2` from compiletest cli parsing

try-job: arm-android
try-job: x86_64-mingw-1
try-job: test-various

Author: rust-bors[bot]

src/bootstrap/src/core/debuggers/android.rs renamed to src/bootstrap/src/core/android.rs

@@ -10,11 +10,15 @@ pub(crate) struct Android {
 }
 
 pub(crate) fn discover_android(builder: &Builder<'_>, target: TargetSelection) -> Option<Android> {
+    if !target.contains("android") {
+        return None;
+    }
+
     let adb_path = "adb";
     // See <https://github.com/rust-lang/rust/pull/102755>.
     let adb_test_dir = "/data/local/tmp/work";
 
-    let android_cross_path = if target.contains("android") && !builder.config.dry_run() {
+    let android_cross_path = if !builder.config.dry_run() {
         builder.cc(target).parent().unwrap().parent().unwrap().to_owned()
     } else {
         PathBuf::new()

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

@@ -29,7 +29,7 @@ use crate::core::builder::{
 };
 use crate::core::config::TargetSelection;
 use crate::core::config::flags::{Subcommand, get_completion, top_level_help};
-use crate::core::debuggers;
+use crate::core::{android, debuggers};
 use crate::utils::build_stamp::{self, BuildStamp};
 use crate::utils::exec::{BootstrapCommand, command};
 use crate::utils::helpers::{
@@ -2114,21 +2114,18 @@ Please disable assertions with `rust.debug-assertions = false`.
             builder.config.python.as_ref().expect("python is required for running rustdoc tests"),
         );
 
-        // FIXME(#148099): Currently we set these Android-related flags in all
-        // modes, even though they should only be needed in "debuginfo" mode,
-        // because the GDB-discovery code in compiletest currently assumes that
-        // `--android-cross-path` is always set for Android targets.
-        if let Some(debuggers::Android { adb_path, adb_test_dir, android_cross_path }) =
-            debuggers::discover_android(builder, target)
-        {
+        // Discover and set some flags related to running tests on Android targets.
+        let android = android::discover_android(builder, target);
+        if let Some(android::Android { adb_path, adb_test_dir, android_cross_path }) = &android {
             cmd.arg("--adb-path").arg(adb_path);
             cmd.arg("--adb-test-dir").arg(adb_test_dir);
             cmd.arg("--android-cross-path").arg(android_cross_path);
         }
 
         if mode == "debuginfo" {
-            if let Some(debuggers::Gdb { gdb }) = debuggers::discover_gdb(builder) {
-                cmd.arg("--gdb").arg(gdb);
+            if let Some(debuggers::Gdb { gdb }) = debuggers::discover_gdb(builder, android.as_ref())
+            {
+                cmd.arg("--gdb").arg(gdb.as_ref());
             }
 
             if let Some(debuggers::Lldb { lldb_exe, lldb_version }) =

src/bootstrap/src/core/debuggers/gdb.rs

@@ -1,13 +1,42 @@
+use std::borrow::Cow;
 use std::path::Path;
 
+use crate::core::android;
 use crate::core::builder::Builder;
+use crate::utils::exec::BootstrapCommand;
 
 pub(crate) struct Gdb<'a> {
-    pub(crate) gdb: &'a Path,
+    pub(crate) gdb: Cow<'a, Path>,
 }
 
-pub(crate) fn discover_gdb<'a>(builder: &'a Builder<'_>) -> Option<Gdb<'a>> {
-    let gdb = builder.config.gdb.as_deref()?;
+pub(crate) fn discover_gdb<'a>(
+    builder: &'a Builder<'_>,
+    android: Option<&android::Android>,
+) -> Option<Gdb<'a>> {
+    // If there's an explicitly-configured gdb, use that.
+    if let Some(gdb) = builder.config.gdb.as_deref() {
+        // FIXME(Zalathar): Consider returning None if gdb is an empty string,
+        // as a way to explicitly disable ambient gdb discovery.
+        let gdb = Cow::Borrowed(gdb);
+        return Some(Gdb { gdb });
+    }
 
-    Some(Gdb { gdb })
+    // Otherwise, fall back to whatever gdb is sitting around in PATH.
+    // (That's the historical behavior, but maybe we should require opt-in?)
+
+    let gdb: Cow<'_, Path> = match android {
+        Some(android::Android { android_cross_path, .. }) => {
+            android_cross_path.join("bin/gdb").into()
+        }
+        None => Path::new("gdb").into(),
+    };
+
+    // Check whether an ambient gdb exists, by running `gdb --version`.
+    let output = {
+        let mut gdb_command = BootstrapCommand::new(gdb.as_ref()).allow_failure();
+        gdb_command.arg("--version");
+        gdb_command.run_capture(builder)
+    };
+
+    if output.is_success() { Some(Gdb { gdb }) } else { None }
 }

src/bootstrap/src/core/debuggers/mod.rs

@@ -1,10 +1,8 @@
 //! Code for discovering debuggers and debugger-related configuration, so that
 //! it can be passed to compiletest when running debuginfo tests.
 
-pub(crate) use self::android::{Android, discover_android};
 pub(crate) use self::gdb::{Gdb, discover_gdb};
 pub(crate) use self::lldb::{Lldb, discover_lldb};
 
-mod android;
 mod gdb;
 mod lldb;

src/bootstrap/src/core/mod.rs

@@ -1,3 +1,4 @@
+pub(crate) mod android;
 pub(crate) mod build_steps;
 pub(crate) mod builder;
 pub(crate) mod config;

src/tools/compiletest/src/common.rs

@@ -566,7 +566,7 @@ pub struct Config {
     ///
     /// FIXME: take a look at this; this is piggy-backing off of gdb code paths but only for
     /// `arm-linux-androideabi` target.
-    pub android_cross_path: Utf8PathBuf,
+    pub android_cross_path: Option<Utf8PathBuf>,
 
     /// Extra parameter to run adb on `arm-linux-androideabi`.
     ///

src/tools/compiletest/src/debuggers.rs

@@ -54,15 +54,6 @@ pub(crate) fn configure_lldb(config: &Config) -> Option<Arc<Config>> {
     Some(Arc::new(Config { debugger: Some(Debugger::Lldb), ..config.clone() }))
 }
 
-/// Returns `true` if the given target is an Android target for the
-/// purposes of GDB testing.
-pub(crate) fn is_android_gdb_target(target: &str) -> bool {
-    matches!(
-        &target[..],
-        "arm-linux-androideabi" | "armv7-linux-androideabi" | "aarch64-linux-android"
-    )
-}
-
 /// Returns `true` if the given target is a MSVC target for the purposes of CDB testing.
 fn is_pc_windows_msvc_target(target: &str) -> bool {
     target.ends_with("-pc-windows-msvc")
@@ -129,35 +120,6 @@ pub(crate) fn extract_cdb_version(full_version_line: &str) -> Option<[u16; 4]> {
     Some([major, minor, patch, build])
 }
 
-pub(crate) fn discover_gdb(
-    gdb: Option<String>,
-    target: &str,
-    android_cross_path: &Utf8Path,
-) -> Option<Utf8PathBuf> {
-    #[cfg(not(windows))]
-    const GDB_FALLBACK: &str = "gdb";
-    #[cfg(windows)]
-    const GDB_FALLBACK: &str = "gdb.exe";
-
-    let fallback_gdb = || {
-        if is_android_gdb_target(target) {
-            let mut gdb_path = android_cross_path.to_string();
-            gdb_path.push_str("/bin/gdb");
-            gdb_path
-        } else {
-            GDB_FALLBACK.to_owned()
-        }
-    };
-
-    let gdb = match gdb {
-        None => fallback_gdb(),
-        Some(ref s) if s.is_empty() => fallback_gdb(), // may be empty if configure found no gdb
-        Some(ref s) => s.to_owned(),
-    };
-
-    Some(Utf8PathBuf::from(gdb))
-}
-
 pub(crate) fn query_gdb_version(gdb: &Utf8Path) -> Option<u32> {
     let mut version_line = None;
     if let Ok(output) = Command::new(&gdb).arg("--version").output() {

src/tools/compiletest/src/lib.rs

@@ -255,13 +255,23 @@ fn parse_config(args: Vec<String>) -> Config {
         }
     }
 
-    let target = opt_str2(matches.opt_str("target"));
-    let android_cross_path = opt_path(matches, "android-cross-path");
+    let host = matches.opt_str("host").expect("`--host` must be unconditionally specified");
+    let target = matches.opt_str("target").expect("`--target` must be unconditionally specified");
+
+    let android_cross_path = matches.opt_str("android-cross-path").map(Utf8PathBuf::from);
+
+    // FIXME: `adb_path` should be an `Option<Utf8PathBuf>`...
+    let adb_path = matches.opt_str("adb-path").map(Utf8PathBuf::from).unwrap_or_default();
+    // FIXME: `adb_test_dir` should be an `Option<Utf8PathBuf>`...
+    let adb_test_dir = matches.opt_str("adb-test-dir").map(Utf8PathBuf::from).unwrap_or_default();
+    let adb_device_status = target.contains("android")
+        && matches.opt_str("adb-test-dir").is_some_and(|p| !p.is_empty());
+
     // FIXME: `cdb_version` is *derived* from cdb, but it's *not* technically a config!
     let cdb = debuggers::discover_cdb(matches.opt_str("cdb"), &target);
     let cdb_version = cdb.as_deref().and_then(debuggers::query_cdb_version);
     // FIXME: `gdb_version` is *derived* from gdb, but it's *not* technically a config!
-    let gdb = debuggers::discover_gdb(matches.opt_str("gdb"), &target, &android_cross_path);
+    let gdb = matches.opt_str("gdb").map(Utf8PathBuf::from);
     let gdb_version = gdb.as_deref().and_then(debuggers::query_gdb_version);
     // FIXME: `lldb_version` is *derived* from lldb, but it's *not* technically a config!
     let lldb = matches.opt_str("lldb").map(Utf8PathBuf::from);
@@ -433,7 +443,7 @@ fn parse_config(args: Vec<String>) -> Config {
         optimize_tests: matches.opt_present("optimize-tests"),
         rust_randomized_layout: matches.opt_present("rust-randomized-layout"),
         target,
-        host: opt_str2(matches.opt_str("host")),
+        host,
         cdb,
         cdb_version,
         gdb,
@@ -443,11 +453,9 @@ fn parse_config(args: Vec<String>) -> Config {
         llvm_version,
         system_llvm: matches.opt_present("system-llvm"),
         android_cross_path,
-        adb_path: Utf8PathBuf::from(opt_str2(matches.opt_str("adb-path"))),
-        adb_test_dir: Utf8PathBuf::from(opt_str2(matches.opt_str("adb-test-dir"))),
-        adb_device_status: opt_str2(matches.opt_str("target")).contains("android")
-            && "(none)" != opt_str2(matches.opt_str("adb-test-dir"))
-            && !opt_str2(matches.opt_str("adb-test-dir")).is_empty(),
+        adb_path,
+        adb_test_dir,
+        adb_device_status,
         verbose: matches.opt_present("verbose"),
         only_modified: matches.opt_present("only-modified"),
         color,
@@ -493,13 +501,6 @@ fn parse_config(args: Vec<String>) -> Config {
     }
 }
 
-fn opt_str2(maybestr: Option<String>) -> String {
-    match maybestr {
-        None => "(none)".to_owned(),
-        Some(s) => s,
-    }
-}
-
 /// Called by `main` after the config has been parsed.
 fn run_tests(config: Arc<Config>) {
     debug!(?config, "run_tests");

src/tools/compiletest/src/runtest/debuginfo.rs

@@ -8,7 +8,7 @@ use tracing::debug;
 
 use super::debugger::DebuggerCommands;
 use super::{Debugger, Emit, ProcRes, TestCx, Truncated, WillExecute};
-use crate::debuggers::{extract_gdb_version, is_android_gdb_target};
+use crate::debuggers::extract_gdb_version;
 
 impl TestCx<'_> {
     pub(super) fn run_debuginfo_test(&self) {
@@ -122,13 +122,15 @@ impl TestCx<'_> {
         let exe_file = self.make_exe_name();
 
         let debugger_run_result;
-        if is_android_gdb_target(&self.config.target) {
+        // If bootstrap gave us an `--android-cross-path`, assume the target
+        // needs Android-specific handling.
+        if let Some(android_cross_path) = self.config.android_cross_path.as_deref() {
             cmds = cmds.replace("run", "continue");
 
             // write debugger script
             let mut script_str = String::with_capacity(2048);
             script_str.push_str(&format!("set charset {}\n", Self::charset()));
-            script_str.push_str(&format!("set sysroot {}\n", &self.config.android_cross_path));
+            script_str.push_str(&format!("set sysroot {android_cross_path}\n"));
             script_str.push_str(&format!("file {}\n", exe_file));
             script_str.push_str("target remote :5039\n");
             script_str.push_str(&format!(