apply review feedback

Author: untitaker

crates/volta-core/src/error/kind.rs

@@ -429,11 +429,6 @@ pub enum ErrorKind {
         name: String,
     },
 
-    /// Thrown when Volta is unable to find the shimmed command.
-    ShimResolveError {
-        name: String,
-    },
-
     /// Thrown when serializnig a bin config to JSON fails
     StringifyBinConfigError,
 
@@ -1265,13 +1260,6 @@ at {}
 {}"#,
                 name, PERMISSIONS_CTA
             ),
-            ErrorKind::ShimResolveError { name } => write!(
-                f,
-                r#"Could not find shim for "{}"
-
-{}"#,
-                name, PERMISSIONS_CTA,
-            ),
             ErrorKind::StringifyBinConfigError => write!(
                 f,
                 "Could not serialize executable configuration.
@@ -1524,7 +1512,6 @@ impl ErrorKind {
             ErrorKind::SetToolExecutable { .. } => ExitCode::FileSystemError,
             ErrorKind::ShimCreateError { .. } => ExitCode::FileSystemError,
             ErrorKind::ShimRemoveError { .. } => ExitCode::FileSystemError,
-            ErrorKind::ShimResolveError { .. } => ExitCode::FileSystemError,
             ErrorKind::StringifyBinConfigError => ExitCode::UnknownError,
             ErrorKind::StringifyPackageConfigError => ExitCode::UnknownError,
             ErrorKind::StringifyPlatformError => ExitCode::UnknownError,

crates/volta-core/src/shim.rs

@@ -9,7 +9,7 @@ use crate::error::{Context, ErrorKind, Fallible, VoltaError};
 use crate::fs::{read_dir_eager, symlink_file};
 use crate::layout::{volta_home, volta_install};
 use crate::sync::VoltaLock;
-use log::{debug, warn};
+use log::debug;
 
 pub fn regenerate_shims_for_dir(dir: &Path) -> Fallible<()> {
     // Acquire a lock on the Volta directory, if possible, to prevent concurrent changes
@@ -89,23 +89,6 @@ pub fn create(shim_name: &str) -> Fallible<ShimResult> {
     }
 }
 
-pub fn check_reachable(shim_name: &str) -> Fallible<()> {
-    let shim = volta_home()?.shim_file(shim_name);
-    let resolved = which::which(shim_name).map_err(|e| {
-        VoltaError::from_source(
-            e,
-            ErrorKind::ShimResolveError {
-                name: shim_name.to_string(),
-            },
-        )
-    })?;
-    if resolved != shim {
-        warn!("{} is shadowed by another binary of the same name at {}. Please ensure that {} is at the front of your PATH.", shim_name, resolved.display(), shim.display());
-    }
-
-    Ok(())
-}
-
 pub fn delete(shim_name: &str) -> Fallible<ShimResult> {
     let shim = volta_home()?.shim_file(shim_name);
 

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

@@ -1,6 +1,7 @@
 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;
@@ -192,3 +193,26 @@ fn registry_fetch_error(
     let from_url = from_url.as_ref().to_string();
     || ErrorKind::RegistryFetchError { tool, from_url }
 }
+
+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 at the front of your PATH.",
+                shim_name,
+                shim.display()
+            );
+            return;
+        }
+    };
+    if resolved != shim {
+        info!("{} is shadowed by another binary of the same name at {}. Please ensure that {} is at the front of your PATH.", shim_name, resolved.display(), shim.display());
+    }
+}

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

@@ -1,13 +1,12 @@
 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;
 use crate::session::Session;
-use crate::shim;
 use crate::style::{note_prefix, tool_version};
 use crate::sync::VoltaLock;
 use cfg_if::cfg_if;
@@ -208,7 +207,7 @@ impl Tool for Node {
             info_installed(node_version); // includes node and npm version
         }
 
-        shim::check_reachable("node")?;
+        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/npm/mod.rs

@@ -2,13 +2,12 @@ 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;
 use crate::session::Session;
-use crate::shim;
 use crate::style::{success_prefix, tool_version};
 use crate::sync::VoltaLock;
 use log::info;
@@ -65,7 +64,7 @@ impl Tool for Npm {
             .set_active_npm(Some(self.version.clone()))?;
 
         info_installed(self);
-        shim::check_reachable("npm")?;
+        check_shim_reachable("npm");
 
         if let Ok(Some(project)) = session.project_platform() {
             if let Some(npm) = &project.npm {

crates/volta-core/src/tool/package/configure.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(
@@ -37,7 +38,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)?;
-        shim::check_reachable(bin_name)?;
+        check_shim_reachable(bin_name);
 
         BinConfig {
             name: bin_name.clone(),

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

@@ -1,13 +1,12 @@
 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;
 use crate::session::Session;
-use crate::shim;
 use crate::style::tool_version;
 use crate::sync::VoltaLock;
 use semver::Version;
@@ -64,7 +63,7 @@ impl Tool for Yarn {
             .set_active_yarn(Some(self.version.clone()))?;
 
         info_installed(self);
-        shim::check_reachable("yarn")?;
+        check_shim_reachable("yarn");
 
         if let Ok(Some(project)) = session.project_platform() {
             if let Some(yarn) = &project.yarn {

tests/acceptance/support/sandbox.rs

@@ -460,9 +460,12 @@ impl SandboxBuilder {
         self.add_exec_dir_to_path()
     }
 
-    /// Add an arbitrary file to the test project within the sandbox,
-    /// give it executable permissions,
-    /// and add its directory to the *front* of PATH, shadowing any volta binaries.
+    /// 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");