From 13c53d9727e0e529d04fd8b7709cb84b723fb0d8 Mon Sep 17 00:00:00 2001 From: Nathan Whitaker <17734409+nathanwhit@users.noreply.github.com> Date: Thu, 26 Sep 2024 09:36:25 -0700 Subject: [PATCH] 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. --- .../managed/resolvers/common/bin_entries.rs | 117 ++++++++++++------ .../npm/@denotest/bin/1.0.0/cli-cjs.js | 2 + .../bin_entries_prefer_closer/__test__.jsonc | 7 ++ 3 files changed, 85 insertions(+), 41 deletions(-) diff --git a/cli/npm/managed/resolvers/common/bin_entries.rs b/cli/npm/managed/resolvers/common/bin_entries.rs index 25a020c2bc..4524ce8326 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(()) } @@ -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 { + 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 mut perms = match std::fs::metadata(&original) { - 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 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 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); diff --git a/tests/registry/npm/@denotest/bin/1.0.0/cli-cjs.js b/tests/registry/npm/@denotest/bin/1.0.0/cli-cjs.js index 7b6ba27241..671e0c4a5a 100644 --- a/tests/registry/npm/@denotest/bin/1.0.0/cli-cjs.js +++ b/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)) { diff --git a/tests/specs/npm/bin_entries_prefer_closer/__test__.jsonc b/tests/specs/npm/bin_entries_prefer_closer/__test__.jsonc index 92d43e7617..4e9c682df3 100644 --- a/tests/specs/npm/bin_entries_prefer_closer/__test__.jsonc +++ b/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" } ] }