fix(installl): make bin entries executable even if not put in `node_modules/.bin` (#25873)

Fixes https://github.com/denoland/deno/issues/25862.

npm only makes bin entries executable if they get linked into `.bin`, as
we did before this PR. So this PR actually deviates from npm, because
it's the only reasonable way to fix this that I can think of.

---

The reason this was broken in moment is the following:

Moment has dependencies on two typescript versions: 1.8 and 3.1

If you have two packages with conflicting bin entries (i.e. two
typescript versions which both have a bin entry `tsc`), in npm it is
non-deterministic and undefined which one will end up in `.bin`.

npm, due to implementation differences, chooses to put typescript 1.8
into the `.bin` directory, and so `node_modules/typescript/bin/tsc` ends
up getting marked executable. We, however, choose typescript 3.2, and so
we end up making `node_modules/typescript3/bin/tsc` executable.

As part of its tests, moment executes `node_modules/typescript/bin/tsc`.
Because we didn't make it executable, this fails.

Since the conflict resolution is undefined in npm, instead of trying to
match it, I think it makes more sense to just make bin entries
executable even if they aren't chosen in the case of a conflict.

1 files changed, 76 insertions, 41 deletions

diff --git a/cli/npm/managed/resolvers/common/bin_entries.rs b/cli/npm/managed/resolvers/common/bin_entries.rs
index 25a020c2b..4524ce832 100644
--- a/cli/npm/managed/resolvers/common/bin_entries.rs
+++ b/cli/npm/managed/resolvers/common/bin_entries.rs
@@ -69,7 +69,11 @@ impl<'a> BinEntries<'a> {
   fn for_each_entry(
     &mut self,
     snapshot: &NpmResolutionSnapshot,
-    mut f: impl FnMut(
+    mut already_seen: impl FnMut(
+      &Path,
+      &str, // bin script
+    ) -> Result<(), AnyError>,
+    mut new: impl FnMut(
       &NpmResolutionPackage,
       &Path,
       &str, // bin name
@@ -90,18 +94,20 @@ impl<'a> BinEntries<'a> {
           deno_npm::registry::NpmPackageVersionBinEntry::String(script) => {
             let name = default_bin_name(package);
             if !seen.insert(name) {
+              already_seen(package_path, script)?;
               // we already set up a bin entry with this name
               continue;
             }
-            f(package, package_path, name, script)?;
+            new(package, package_path, name, script)?;
           }
           deno_npm::registry::NpmPackageVersionBinEntry::Map(entries) => {
             for (name, script) in entries {
               if !seen.insert(name) {
+                already_seen(package_path, script)?;
                 // we already set up a bin entry with this name
                 continue;
               }
-              f(package, package_path, name, script)?;
+              new(package, package_path, name, script)?;
             }
           }
         }
@@ -118,10 +124,14 @@ impl<'a> BinEntries<'a> {
   ) -> Vec<(String, PathBuf)> {
     let mut bins = Vec::new();
     self
-      .for_each_entry(snapshot, |_, package_path, name, script| {
-        bins.push((name.to_string(), package_path.join(script)));
-        Ok(())
-      })
+      .for_each_entry(
+        snapshot,
+        |_, _| Ok(()),
+        |_, package_path, name, script| {
+          bins.push((name.to_string(), package_path.join(script)));
+          Ok(())
+        },
+      )
       .unwrap();
     bins
   }
@@ -139,15 +149,26 @@ impl<'a> BinEntries<'a> {
       )?;
     }
 
-    self.for_each_entry(snapshot, |package, package_path, name, script| {
-      set_up_bin_entry(
-        package,
-        name,
-        script,
-        package_path,
-        bin_node_modules_dir_path,
-      )
-    })?;
+    self.for_each_entry(
+      snapshot,
+      |_package_path, _script| {
+        #[cfg(unix)]
+        {
+          let path = _package_path.join(_script);
+          make_executable_if_exists(&path)?;
+        }
+        Ok(())
+      },
+      |package, package_path, name, script| {
+        set_up_bin_entry(
+          package,
+          name,
+          script,
+          package_path,
+          bin_node_modules_dir_path,
+        )
+      },
+    )?;
 
     Ok(())
   }
@@ -255,44 +276,58 @@ fn set_up_bin_shim(
 }
 
 #[cfg(unix)]
-fn symlink_bin_entry(
-  _package: &NpmResolutionPackage,
-  bin_name: &str,
-  bin_script: &str,
-  package_path: &Path,
-  bin_node_modules_dir_path: &Path,
-) -> Result<(), AnyError> {
+/// Make the file at `path` executable if it exists.
+/// Returns `true` if the file exists, `false` otherwise.
+fn make_executable_if_exists(path: &Path) -> Result<bool, AnyError> {
   use std::io;
-  use std::os::unix::fs::symlink;
-  let link = bin_node_modules_dir_path.join(bin_name);
-  let original = package_path.join(bin_script);
-
   use std::os::unix::fs::PermissionsExt;
-  let mut perms = match std::fs::metadata(&original) {
+  let mut perms = match std::fs::metadata(path) {
     Ok(metadata) => metadata.permissions(),
     Err(err) => {
       if err.kind() == io::ErrorKind::NotFound {
-        log::warn!(
-          "{} Trying to set up '{}' bin for \"{}\", but the entry point \"{}\" doesn't exist.",
-          deno_terminal::colors::yellow("Warning"),
-          bin_name,
-          package_path.display(),
-          original.display()
-        );
-        return Ok(());
+        return Ok(false);
       }
-      return Err(err).with_context(|| {
-        format!("Can't set up '{}' bin at {}", bin_name, original.display())
-      });
+      return Err(err.into());
     }
   };
   if perms.mode() & 0o111 == 0 {
     // if the original file is not executable, make it executable
     perms.set_mode(perms.mode() | 0o111);
-    std::fs::set_permissions(&original, perms).with_context(|| {
-      format!("Setting permissions on '{}'", original.display())
+    std::fs::set_permissions(path, perms).with_context(|| {
+      format!("Setting permissions on '{}'", path.display())
     })?;
   }
+
+  Ok(true)
+}
+
+#[cfg(unix)]
+fn symlink_bin_entry(
+  _package: &NpmResolutionPackage,
+  bin_name: &str,
+  bin_script: &str,
+  package_path: &Path,
+  bin_node_modules_dir_path: &Path,
+) -> Result<(), AnyError> {
+  use std::io;
+  use std::os::unix::fs::symlink;
+  let link = bin_node_modules_dir_path.join(bin_name);
+  let original = package_path.join(bin_script);
+
+  let found = make_executable_if_exists(&original).with_context(|| {
+    format!("Can't set up '{}' bin at {}", bin_name, original.display())
+  })?;
+  if !found {
+    log::warn!(
+      "{} Trying to set up '{}' bin for \"{}\", but the entry point \"{}\" doesn't exist.",
+      deno_terminal::colors::yellow("Warning"),
+      bin_name,
+      package_path.display(),
+      original.display()
+    );
+    return Ok(());
+  }
+
   let original_relative =
     crate::util::path::relative_path(bin_node_modules_dir_path, &original)
       .unwrap_or(original);