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.

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| {
+      .for_each_entry(
-        bins.push((name.to_string(), package_path.join(script)));
+        snapshot,
-        Ok(())
+        |_, _| 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| {
+    self.for_each_entry(
-      set_up_bin_entry(
+      snapshot,
-        package,
+      |_package_path, _script| {
-        name,
+        #[cfg(unix)]
-        script,
+        {
-        package_path,
+          let path = _package_path.join(_script);
-        bin_node_modules_dir_path,
+          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(())
   }
@@ -254,6 +275,32 @@ fn set_up_bin_shim(
   Ok(())
 }
 
+#[cfg(unix)]
+/// 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::PermissionsExt;
+  let mut perms = match std::fs::metadata(path) {
+    Ok(metadata) => metadata.permissions(),
+    Err(err) => {
+      if err.kind() == io::ErrorKind::NotFound {
+        return Ok(false);
+      }
+      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(path, perms).with_context(|| {
+      format!("Setting permissions on '{}'", path.display())
+    })?;
+  }
+
+  Ok(true)
+}
+
 #[cfg(unix)]
 fn symlink_bin_entry(
   _package: &NpmResolutionPackage,
@@ -267,32 +314,20 @@ fn symlink_bin_entry(
   let link = bin_node_modules_dir_path.join(bin_name);
   let original = package_path.join(bin_script);
 
-  use std::os::unix::fs::PermissionsExt;
+  let found = make_executable_if_exists(&original).with_context(|| {
-  let mut perms = match std::fs::metadata(&original) {
+    format!("Can't set up '{}' bin at {}", bin_name, original.display())
-    Ok(metadata) => metadata.permissions(),
+  })?;
-    Err(err) => {
+  if !found {
-      if err.kind() == io::ErrorKind::NotFound {
+    log::warn!(
-        log::warn!(
+      "{} Trying to set up '{}' bin for \"{}\", but the entry point \"{}\" doesn't exist.",
-          "{} Trying to set up '{}' bin for \"{}\", but the entry point \"{}\" doesn't exist.",
+      deno_terminal::colors::yellow("Warning"),
-          deno_terminal::colors::yellow("Warning"),
+      bin_name,
-          bin_name,
+      package_path.display(),
-          package_path.display(),
+      original.display()
-          original.display()
+    );
-        );
+    return Ok(());
-        return Ok(());
-      }
-      return Err(err).with_context(|| {
-        format!("Can't set up '{}' bin at {}", bin_name, original.display())
-      });
-    }
-  };
-  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())
-    })?;
   }
+
   let original_relative =
     crate::util::path::relative_path(bin_node_modules_dir_path, &original)
       .unwrap_or(original);

tests/registry/npm/@denotest/bin/1.0.0/cli-cjs.js

@@ -1,3 +1,5 @@
+#!/usr/bin/env -S node
+
 const process = require("process");
 
 for (const arg of process.argv.slice(2)) {

tests/specs/npm/bin_entries_prefer_closer/__test__.jsonc

@@ -18,6 +18,13 @@
     {
       "args": "task run-no-ext",
       "output": "Task run-no-ext cli-no-ext hello world\n@denotest/bin 0.7.0\n"
+    },
+    {
+      // even though we didn't put it in .bin, make sure the bin entry is marked executable
+      "if": "unix",
+      "commandName": "node_modules/.deno/@denotest+bin@1.0.0/node_modules/@denotest/bin/cli-cjs.js",
+      "args": "hello",
+      "output": "hello\n"
     }
   ]
 }