Merge #4375

4375: Cleanup toolchain handling r=matklad a=matklad

bors r+
🤖

Co-authored-by: Aleksey Kladov <aleksey.kladov@gmail.com>

Cargo.lock

@@ -957,14 +957,6 @@ dependencies = [
  "test_utils",
 ]
 
-[[package]]
-name = "ra_env"
-version = "0.1.0"
-dependencies = [
- "anyhow",
- "home",
-]
-
 [[package]]
 name = "ra_flycheck"
 version = "0.1.0"
@@ -975,7 +967,7 @@ dependencies = [
  "jod-thread",
  "log",
  "lsp-types",
- "ra_env",
+ "ra_toolchain",
  "serde_json",
 ]
 
@@ -1180,8 +1172,8 @@ dependencies = [
  "ra_arena",
  "ra_cfg",
  "ra_db",
- "ra_env",
  "ra_proc_macro",
+ "ra_toolchain",
  "rustc-hash",
  "serde",
  "serde_json",
@@ -1213,6 +1205,13 @@ dependencies = [
  "text-size",
 ]
 
+[[package]]
+name = "ra_toolchain"
+version = "0.1.0"
+dependencies = [
+ "home",
+]
+
 [[package]]
 name = "ra_tt"
 version = "0.1.0"

crates/ra_env/src/lib.rs

@@ -1,66 +0,0 @@
-//! This crate contains a single public function
-//! [`get_path_for_executable`](fn.get_path_for_executable.html).
-//! See docs there for more information.
-
-use anyhow::{bail, Result};
-use std::env;
-use std::path::{Path, PathBuf};
-use std::process::Command;
-
-/// Return a `PathBuf` to use for the given executable.
-///
-/// E.g., `get_path_for_executable("cargo")` may return just `cargo` if that
-/// gives a valid Cargo executable; or it may return a full path to a valid
-/// Cargo.
-pub fn get_path_for_executable(executable_name: impl AsRef<str>) -> Result<PathBuf> {
-    // The current implementation checks three places for an executable to use:
-    // 1) Appropriate environment variable (erroring if this is set but not a usable executable)
-    //      example: for cargo, this checks $CARGO environment variable; for rustc, $RUSTC; etc
-    // 2) `<executable_name>`
-    //      example: for cargo, this tries just `cargo`, which will succeed if `cargo` is on the $PATH
-    // 3) `~/.cargo/bin/<executable_name>`
-    //      example: for cargo, this tries ~/.cargo/bin/cargo
-    //      It seems that this is a reasonable place to try for cargo, rustc, and rustup
-    let executable_name = executable_name.as_ref();
-    let env_var = executable_name.to_ascii_uppercase();
-    if let Ok(path) = env::var(&env_var) {
-        if is_valid_executable(&path) {
-            Ok(path.into())
-        } else {
-            bail!(
-                "`{}` environment variable points to something that's not a valid executable",
-                env_var
-            )
-        }
-    } else {
-        if is_valid_executable(executable_name) {
-            return Ok(executable_name.into());
-        }
-        if let Some(mut path) = ::home::home_dir() {
-            path.push(".cargo");
-            path.push("bin");
-            path.push(executable_name);
-            if is_valid_executable(&path) {
-                return Ok(path);
-            }
-        }
-        // This error message may also be caused by $PATH or $CARGO/$RUSTC/etc not being set correctly
-        // for VSCode, even if they are set correctly in a terminal.
-        // On macOS in particular, launching VSCode from terminal with `code <dirname>` causes VSCode
-        // to inherit environment variables including $PATH, $CARGO, $RUSTC, etc from that terminal;
-        // but launching VSCode from Dock does not inherit environment variables from a terminal.
-        // For more discussion, see #3118.
-        bail!(
-            "Failed to find `{}` executable. Make sure `{}` is in `$PATH`, or set `${}` to point to a valid executable.",
-            executable_name, executable_name, env_var
-        )
-    }
-}
-
-/// Does the given `Path` point to a usable executable?
-///
-/// (assumes the executable takes a `--version` switch and writes to stdout,
-/// which is true for `cargo`, `rustc`, and `rustup`)
-fn is_valid_executable(p: impl AsRef<Path>) -> bool {
-    Command::new(p.as_ref()).arg("--version").output().is_ok()
-}

crates/ra_flycheck/Cargo.toml

@@ -14,7 +14,7 @@ log = "0.4.8"
 cargo_metadata = "0.9.1"
 serde_json = "1.0.48"
 jod-thread = "0.1.1"
-ra_env = { path = "../ra_env" }
+ra_toolchain = { path = "../ra_toolchain" }
 
 [dev-dependencies]
 insta = "0.16.0"

crates/ra_flycheck/src/lib.rs

@@ -16,7 +16,6 @@ use lsp_types::{
     CodeAction, CodeActionOrCommand, Diagnostic, Url, WorkDoneProgress, WorkDoneProgressBegin,
     WorkDoneProgressEnd, WorkDoneProgressReport,
 };
-use ra_env::get_path_for_executable;
 
 use crate::conv::{map_rust_diagnostic_to_lsp, MappedRustDiagnostic};
 
@@ -216,10 +215,10 @@ impl FlycheckThread {
 
         let mut cmd = match &self.config {
             FlycheckConfig::CargoCommand { command, all_targets, all_features, extra_args } => {
-                let mut cmd = Command::new(get_path_for_executable("cargo").unwrap());
+                let mut cmd = Command::new(ra_toolchain::cargo());
                 cmd.arg(command);
-                cmd.args(&["--workspace", "--message-format=json", "--manifest-path"]);
+                cmd.args(&["--workspace", "--message-format=json", "--manifest-path"])
-                cmd.arg(self.workspace_root.join("Cargo.toml"));
+                    .arg(self.workspace_root.join("Cargo.toml"));
                 if *all_targets {
                     cmd.arg("--all-targets");
                 }

crates/ra_project_model/Cargo.toml

@@ -16,7 +16,7 @@ cargo_metadata = "0.9.1"
 ra_arena = { path = "../ra_arena" }
 ra_cfg = { path = "../ra_cfg" }
 ra_db = { path = "../ra_db" }
-ra_env = { path = "../ra_env" }
+ra_toolchain = { path = "../ra_toolchain" }
 ra_proc_macro =  { path = "../ra_proc_macro" }
 
 serde = { version = "1.0.106", features = ["derive"] }

crates/ra_project_model/src/cargo_workspace.rs

@@ -11,7 +11,6 @@ use anyhow::{Context, Result};
 use cargo_metadata::{BuildScript, CargoOpt, Message, MetadataCommand, PackageId};
 use ra_arena::{Arena, Idx};
 use ra_db::Edition;
-use ra_env::get_path_for_executable;
 use rustc_hash::FxHashMap;
 
 /// `CargoWorkspace` represents the logical structure of, well, a Cargo
@@ -147,7 +146,7 @@ impl CargoWorkspace {
         cargo_features: &CargoConfig,
     ) -> Result<CargoWorkspace> {
         let mut meta = MetadataCommand::new();
-        meta.cargo_path(get_path_for_executable("cargo")?);
+        meta.cargo_path(ra_toolchain::cargo());
         meta.manifest_path(cargo_toml);
         if cargo_features.all_features {
             meta.features(CargoOpt::AllFeatures);
@@ -289,7 +288,7 @@ pub fn load_extern_resources(
     cargo_toml: &Path,
     cargo_features: &CargoConfig,
 ) -> Result<ExternResources> {
-    let mut cmd = Command::new(get_path_for_executable("cargo")?);
+    let mut cmd = Command::new(ra_toolchain::cargo());
     cmd.args(&["check", "--message-format=json", "--manifest-path"]).arg(cargo_toml);
     if cargo_features.all_features {
         cmd.arg("--all-features");

crates/ra_project_model/src/lib.rs

@@ -8,13 +8,12 @@ use std::{
     fs::{read_dir, File, ReadDir},
     io::{self, BufReader},
     path::{Path, PathBuf},
-    process::Command,
+    process::{Command, Output},
 };
 
 use anyhow::{bail, Context, Result};
 use ra_cfg::CfgOptions;
 use ra_db::{CrateGraph, CrateName, Edition, Env, ExternSource, ExternSourceId, FileId};
-use ra_env::get_path_for_executable;
 use rustc_hash::FxHashMap;
 use serde_json::from_reader;
 
@@ -568,25 +567,18 @@ pub fn get_rustc_cfg_options(target: Option<&String>) -> CfgOptions {
         }
     }
 
-    match (|| -> Result<String> {
+    let rustc_cfgs = || -> Result<String> {
         // `cfg(test)` and `cfg(debug_assertion)` are handled outside, so we suppress them here.
-        let mut cmd = Command::new(get_path_for_executable("rustc")?);
+        let mut cmd = Command::new(ra_toolchain::rustc());
         cmd.args(&["--print", "cfg", "-O"]);
         if let Some(target) = target {
             cmd.args(&["--target", target.as_str()]);
         }
-        let output = cmd.output().context("Failed to get output from rustc --print cfg -O")?;
+        let output = output(cmd)?;
-        if !output.status.success() {
-            bail!(
-                "rustc --print cfg -O exited with exit code ({})",
-                output
-                    .status
-                    .code()
-                    .map_or(String::from("no exit code"), |code| format!("{}", code))
-            );
-        }
         Ok(String::from_utf8(output.stdout)?)
-    })() {
+    }();
+
+    match rustc_cfgs {
         Ok(rustc_cfgs) => {
             for line in rustc_cfgs.lines() {
                 match line.find('=') {
@@ -599,8 +591,16 @@ pub fn get_rustc_cfg_options(target: Option<&String>) -> CfgOptions {
                 }
             }
         }
-        Err(e) => log::error!("failed to get rustc cfgs: {}", e),
+        Err(e) => log::error!("failed to get rustc cfgs: {:#}", e),
     }
 
     cfg_options
 }
+
+fn output(mut cmd: Command) -> Result<Output> {
+    let output = cmd.output().with_context(|| format!("{:?} failed", cmd))?;
+    if !output.status.success() {
+        bail!("{:?} failed, {}", cmd, output.status)
+    }
+    Ok(output)
+}

crates/ra_project_model/src/sysroot.rs

@@ -1,14 +1,15 @@
 //! FIXME: write short doc here
 
-use anyhow::{bail, Context, Result};
 use std::{
     env, ops,
     path::{Path, PathBuf},
-    process::{Command, Output},
+    process::Command,
 };
 
+use anyhow::{bail, Result};
 use ra_arena::{Arena, Idx};
-use ra_env::get_path_for_executable;
+
+use crate::output;
 
 #[derive(Default, Debug, Clone)]
 pub struct Sysroot {
@@ -85,50 +86,22 @@ impl Sysroot {
     }
 }
 
-fn create_command_text(program: &str, args: &[&str]) -> String {
-    format!("{} {}", program, args.join(" "))
-}
-
-fn run_command_in_cargo_dir(
-    cargo_toml: impl AsRef<Path>,
-    program: impl AsRef<Path>,
-    args: &[&str],
-) -> Result<Output> {
-    let program = program.as_ref().as_os_str().to_str().expect("Invalid Unicode in path");
-    let output = Command::new(program)
-        .current_dir(cargo_toml.as_ref().parent().unwrap())
-        .args(args)
-        .output()
-        .context(format!("{} failed", create_command_text(program, args)))?;
-    if !output.status.success() {
-        match output.status.code() {
-            Some(code) => bail!(
-                "failed to run the command: '{}' exited with code {}",
-                create_command_text(program, args),
-                code
-            ),
-            None => bail!(
-                "failed to run the command: '{}' terminated by signal",
-                create_command_text(program, args)
-            ),
-        };
-    }
-    Ok(output)
-}
-
 fn get_or_install_rust_src(cargo_toml: &Path) -> Result<PathBuf> {
     if let Ok(path) = env::var("RUST_SRC_PATH") {
         return Ok(path.into());
     }
-    let rustc = get_path_for_executable("rustc")?;
+    let current_dir = cargo_toml.parent().unwrap();
-    let rustc_output = run_command_in_cargo_dir(cargo_toml, &rustc, &["--print", "sysroot"])?;
+    let mut rustc = Command::new(ra_toolchain::rustc());
+    rustc.current_dir(current_dir).args(&["--print", "sysroot"]);
+    let rustc_output = output(rustc)?;
     let stdout = String::from_utf8(rustc_output.stdout)?;
     let sysroot_path = Path::new(stdout.trim());
     let src_path = sysroot_path.join("lib/rustlib/src/rust/src");
 
     if !src_path.exists() {
-        let rustup = get_path_for_executable("rustup")?;
+        let mut rustup = Command::new(ra_toolchain::rustup());
-        run_command_in_cargo_dir(cargo_toml, &rustup, &["component", "add", "rust-src"])?;
+        rustup.current_dir(current_dir).args(&["component", "add", "rust-src"]);
+        let _output = output(rustup)?;
     }
     if !src_path.exists() {
         bail!(

crates/ra_toolchain/Cargo.toml

@@ -1,9 +1,8 @@
 [package]
 edition = "2018"
-name = "ra_env"
+name = "ra_toolchain"
 version = "0.1.0"
 authors = ["rust-analyzer developers"]
 
 [dependencies]
-anyhow = "1.0.26"
 home = "0.5.3"

crates/ra_toolchain/src/lib.rs

@@ -0,0 +1,64 @@
+//! This crate contains a single public function
+//! [`get_path_for_executable`](fn.get_path_for_executable.html).
+//! See docs there for more information.
+use std::{env, iter, path::PathBuf};
+
+pub fn cargo() -> PathBuf {
+    get_path_for_executable("cargo")
+}
+
+pub fn rustc() -> PathBuf {
+    get_path_for_executable("rustc")
+}
+
+pub fn rustup() -> PathBuf {
+    get_path_for_executable("rustup")
+}
+
+/// Return a `PathBuf` to use for the given executable.
+///
+/// E.g., `get_path_for_executable("cargo")` may return just `cargo` if that
+/// gives a valid Cargo executable; or it may return a full path to a valid
+/// Cargo.
+fn get_path_for_executable(executable_name: &'static str) -> PathBuf {
+    // The current implementation checks three places for an executable to use:
+    // 1) Appropriate environment variable (erroring if this is set but not a usable executable)
+    //      example: for cargo, this checks $CARGO environment variable; for rustc, $RUSTC; etc
+    // 2) `<executable_name>`
+    //      example: for cargo, this tries just `cargo`, which will succeed if `cargo` is on the $PATH
+    // 3) `~/.cargo/bin/<executable_name>`
+    //      example: for cargo, this tries ~/.cargo/bin/cargo
+    //      It seems that this is a reasonable place to try for cargo, rustc, and rustup
+    let env_var = executable_name.to_ascii_uppercase();
+    if let Some(path) = env::var_os(&env_var) {
+        return path.into();
+    }
+
+    if lookup_in_path(executable_name) {
+        return executable_name.into();
+    }
+
+    if let Some(mut path) = home::home_dir() {
+        path.push(".cargo");
+        path.push("bin");
+        path.push(executable_name);
+        if path.is_file() {
+            return path;
+        }
+    }
+    executable_name.into()
+}
+
+fn lookup_in_path(exec: &str) -> bool {
+    let paths = env::var_os("PATH").unwrap_or_default();
+    let mut candidates = env::split_paths(&paths).flat_map(|path| {
+        let candidate = path.join(&exec);
+        let with_exe = if env::consts::EXE_EXTENSION == "" {
+            None
+        } else {
+            Some(candidate.with_extension(env::consts::EXE_EXTENSION))
+        };
+        iter::once(candidate).chain(with_exe)
+    });
+    candidates.any(|it| it.is_file())
+}