Merge pull request #1292 from untitaker/sanity-check-shims

Check volta shims for reachability during tool install

Author: chriskrycho

Cargo.lock

@@ -52,6 +52,7 @@ chain-map = "0.1.0"
 indexmap = "2.2.6"
 retry = "2"
 fs2 = "0.4.3"
+which = "4.2.5"
 
 [target.'cfg(windows)'.dependencies]
 winreg = "0.52.0"

crates/volta-core/Cargo.toml

@@ -2,11 +2,13 @@ use std::env;
 use std::fmt::{self, Display};
 
 use crate::error::{ErrorKind, Fallible};
+use crate::layout::volta_home;
 use crate::session::Session;
 use crate::style::{note_prefix, success_prefix, tool_version};
 use crate::sync::VoltaLock;
 use crate::version::VersionSpec;
 use crate::VOLTA_FEATURE_PNPM;
+use cfg_if::cfg_if;
 use log::{debug, info};
 
 pub mod node;
@@ -226,3 +228,36 @@ fn registry_fetch_error(
     let from_url = from_url.as_ref().to_string();
     || ErrorKind::RegistryFetchError { tool, from_url }
 }
+
+cfg_if!(
+    if #[cfg(windows)] {
+        const PATH_VAR_NAME: &str = "Path";
+    } else {
+        const PATH_VAR_NAME: &str = "PATH";
+    }
+);
+
+pub fn check_shim_reachable(shim_name: &str) {
+    let home = match volta_home() {
+        Ok(home) => home,
+        Err(_) => return,
+    };
+
+    let shim = home.shim_file(shim_name);
+    let resolved = match which::which(shim_name) {
+        Ok(resolved) => resolved,
+        Err(_) => {
+            info!(
+                "{} cannot find command {}. Please ensure that {} is available on your {}.",
+                note_prefix(),
+                shim_name,
+                home.shim_dir().display(),
+                PATH_VAR_NAME,
+            );
+            return;
+        }
+    };
+    if resolved != shim {
+        info!("{} {} is shadowed by another binary of the same name at {}. To ensure your commands work as expected, please move {} to the start of your {}.", note_prefix(), shim_name, resolved.display(), home.shim_dir().display(), PATH_VAR_NAME);
+    }
+}

crates/volta-core/src/tool/mod.rs

@@ -1,8 +1,8 @@
 use std::fmt::{self, Display};
 
 use super::{
-    check_fetched, debug_already_fetched, info_fetched, info_installed, info_pinned,
-    info_project_version, FetchStatus, Tool,
+    check_fetched, check_shim_reachable, debug_already_fetched, info_fetched, info_installed,
+    info_pinned, info_project_version, FetchStatus, Tool,
 };
 use crate::error::{ErrorKind, Fallible};
 use crate::inventory::node_available;
@@ -243,6 +243,8 @@ impl Tool for Node {
             info_installed(node_version); // includes node and npm version
         }
 
+        check_shim_reachable("node");
+
         if let Ok(Some(project)) = session.project_platform() {
             info_project_version(tool_version("node", &project.node));
         }

crates/volta-core/src/tool/node/mod.rs

@@ -2,8 +2,8 @@ use std::fmt::{self, Display};
 
 use super::node::load_default_npm_version;
 use super::{
-    check_fetched, debug_already_fetched, info_fetched, info_installed, info_pinned,
-    info_project_version, FetchStatus, Tool,
+    check_fetched, check_shim_reachable, debug_already_fetched, info_fetched, info_installed,
+    info_pinned, info_project_version, FetchStatus, Tool,
 };
 use crate::error::{Context, ErrorKind, Fallible};
 use crate::inventory::npm_available;
@@ -64,6 +64,7 @@ impl Tool for Npm {
             .set_active_npm(Some(self.version.clone()))?;
 
         info_installed(self);
+        check_shim_reachable("npm");
 
         if let Ok(Some(project)) = session.project_platform() {
             if let Some(npm) = &project.npm {

crates/volta-core/src/tool/npm/mod.rs

@@ -6,6 +6,7 @@ use crate::error::{ErrorKind, Fallible};
 use crate::layout::volta_home;
 use crate::platform::{Image, PlatformSpec};
 use crate::shim;
+use crate::tool::check_shim_reachable;
 
 /// Read the manifest for the package being installed
 pub(super) fn parse_manifest(
@@ -38,6 +39,7 @@ pub(super) fn write_config_and_shims(
     // Generate the shims and bin configs for each bin provided by the package
     for bin_name in &manifest.bin {
         shim::create(bin_name)?;
+        check_shim_reachable(bin_name);
 
         BinConfig {
             name: bin_name.clone(),

crates/volta-core/src/tool/package/configure.rs

@@ -1,8 +1,8 @@
 use std::fmt::{self, Display};
 
 use super::{
-    check_fetched, debug_already_fetched, info_fetched, info_installed, info_pinned,
-    info_project_version, FetchStatus, Tool,
+    check_fetched, check_shim_reachable, debug_already_fetched, info_fetched, info_installed,
+    info_pinned, info_project_version, FetchStatus, Tool,
 };
 use crate::error::{ErrorKind, Fallible};
 use crate::inventory::yarn_available;
@@ -63,6 +63,7 @@ impl Tool for Yarn {
             .set_active_yarn(Some(self.version.clone()))?;
 
         info_installed(self);
+        check_shim_reachable("yarn");
 
         if let Ok(Some(project)) = session.project_platform() {
             if let Some(yarn) = &project.yarn {

crates/volta-core/src/tool/yarn/mod.rs

@@ -500,6 +500,23 @@ impl SandboxBuilder {
         self.add_exec_dir_to_path()
     }
 
+    /// Prepend executable directory to the beginning of the PATH (chainable)
+    ///
+    /// This is useful to test binaries shadowing volta shims.
+    ///
+    /// Cannot be used in combination with `add_exec_dir_to_path`, and will panic if called twice.
+    /// No particular reason except it's likely a programming error.
+    pub fn prepend_exec_dir_to_path(mut self) -> Self {
+        if self.has_exec_path {
+            panic!("need to call prepend_exec_dir_to_path before anything else");
+        }
+
+        let exec_path = self.root().join("exec");
+        self.path_dirs.insert(0, exec_path);
+        self.has_exec_path = true;
+        self
+    }
+
     /// Set a package config file for the sandbox (chainable)
     pub fn package_config(mut self, name: &str, contents: &str) -> Self {
         let package_cfg_file = package_config_file(name);

tests/acceptance/support/sandbox.rs

@@ -383,3 +383,26 @@ fn install_yarn_3_without_node_errors() {
             )
     );
 }
+
+#[test]
+fn install_node_with_shadowed_binary() {
+    #[cfg(windows)]
+    const SCRIPT_FILENAME: &str = "node.bat";
+    #[cfg(not(windows))]
+    const SCRIPT_FILENAME: &str = "node";
+
+    let s = sandbox()
+        .node_available_versions(NODE_VERSION_INFO)
+        .distro_mocks::<NodeFixture>(&NODE_VERSION_FIXTURES)
+        .env("VOLTA_LOGLEVEL", "info")
+        .prepend_exec_dir_to_path()
+        .executable_file(SCRIPT_FILENAME, "echo hello world")
+        .build();
+
+    assert_that!(
+        s.volta("install node"),
+        execs()
+            .with_status(ExitCode::Success as i32)
+            .with_stdout_contains("[..]is shadowed by another binary of the same name at [..]")
+    );
+}