diff --git a/cli/task_runner.rs b/cli/task_runner.rs index 7dd42d6c997d8e..dca2a4924eda45 100644 --- a/cli/task_runner.rs +++ b/cli/task_runner.rs @@ -1,6 +1,7 @@ // Copyright 2018-2026 the Deno authors. MIT license. use std::collections::HashMap; +use std::collections::HashSet; use std::ffi::OsStr; use std::ffi::OsString; use std::io::Write; @@ -18,6 +19,7 @@ use deno_task_shell::ShellCommand; use deno_task_shell::ShellCommandContext; use deno_task_shell::ShellPipeReader; use deno_task_shell::ShellPipeWriter; +use node_resolver::BinValue; use tokio::task::JoinHandle; use tokio::task::LocalSet; use tokio_util::sync::CancellationToken; @@ -533,6 +535,20 @@ pub fn resolve_custom_commands( let mut commands = match npm_resolver { CliNpmResolver::Byonm(_) => { // Walk the bin dirs in order (closest first) and merge; closest wins. + // + // NOTE: unlike the managed branch below this deliberately routes *every* + // entry through `deno run`, including ones `read_bin_value` classified as + // `Executable`. A JS bin without a shebang classifies as `Executable`, + // and in BYONM mode running it through deno is the only thing that makes + // it work, so narrowing this would be a regression. The managed branch + // can be stricter because its snapshot packages are already resolved. + // + // The asymmetry is intentional and observable: a workspace member whose + // `bin` points at a JS file with NO shebang works here but, under the + // managed resolver, falls through to `PATH` and fails to exec on unix + // (`Exec format error`) — same as `npm`/`node`, which also produce a + // non-executable shim target in that case. Pinned by the + // `root-noshebang` step of `tests/specs/workspaces/workspace_member_bin`. let mut commands: HashMap> = HashMap::new(); for bin_dir in bin_dirs { for (name, cmd) in @@ -544,19 +560,91 @@ pub fn resolve_custom_commands( commands } CliNpmResolver::Managed(npm_resolver) => { - resolve_managed_npm_commands(node_resolver, npm_resolver)? + let mut commands = + resolve_managed_npm_commands(node_resolver, npm_resolver)?; + // Local workspace members are not part of the npm resolution snapshot, + // so a `bin` they declare is only ever visible as a `node_modules/.bin` + // entry. Merge those in so they're runnable from a task (#36313). + // Prepending the bin dirs to `PATH` isn't enough on its own: on Windows + // the entries are plain ``, `.cmd` and `.ps1` files + // rather than executables. + // + // Only `JsFile` entries are merged. An `Executable` entry is a shell + // script or a native binary; those already resolve through `PATH` (the + // bin dirs are prepended in `prepare_env_vars`) and `deno_task_shell` + // honours their shebang, whereas running one with `deno run --ext=js` + // would fail with a `SyntaxError`. A workspace member's JS bin still + // classifies as `JsFile` on Windows because `windows_shim::generate_sh` + // emits `exec node "$basedir/..." "$@"`, which + // `resolve_execution_path_from_npx_shim` matches. + // + // `seen` holds every name classified in *any* bin dir, whether or not it + // ended up merged. A closer `Executable` entry must still shadow a + // farther `JsFile` of the same name: the closer one is what `PATH` + // resolves to, and a custom command beats `PATH` in `deno_task_shell`, + // so inserting the farther entry would invert closest-first precedence. + // + // KNOWN EXCEPTION — closest-first only holds *among the bin dirs*. + // Entries are merged with `or_insert` semantics (the `contains_key` + // filter), so the first bin dir to provide a name wins over later ones. + // But `commands` was pre-seeded above from the resolution snapshot's + // top-level packages, so a root dependency's bin beats a nearer + // workspace member's — or a nearer dependency's — bin of the same name, + // and it does so outright because a custom command also beats `PATH`. + // npm and pnpm run the *nearer* one. + // + // We accept this rather than fix it here: the snapshot pre-seeding is + // pre-existing behaviour that applies to every managed task, so + // reordering it would reach well beyond workspaces. The fix belongs + // alongside the `TODO(nathanwhit)` on `resolve_managed_npm_commands` + // below, which has to stop flattening top-level package bins into + // unconditional commands before this can be ordered correctly. + let mut seen: HashSet = HashSet::new(); + for bin_dir in bin_dirs { + // Only classify names that aren't already resolved — classifying reads + // the entire file. + let bin_values = node_resolver + .resolve_npm_commands_from_bin_dir_filtered(bin_dir, |name| { + !commands.contains_key(name) && !seen.contains(name) + }); + for (name, bin_value) in bin_values { + seen.insert(name.clone()); + let BinValue::JsFile(path) = bin_value else { + continue; + }; + commands.insert( + name.clone(), + Rc::new(NodeModulesFileRunCommand { + command_name: name, + path, + }) as Rc, + ); + } + } + commands } }; commands.insert("npm".to_string(), Rc::new(NpmCommand)); Ok(commands) } -/// Builds the list of `node_modules/.bin` directories to consult for a task. +/// Builds the list of `node_modules/.bin` directories to consult for a task, +/// ordered closest-first. /// /// For BYONM this walks up the filesystem from `cwd` collecting every -/// `/node_modules/.bin` directory (matching how Node, npm, and pnpm -/// resolve bin commands). For the managed npm resolver there is only ever a -/// single `node_modules/.bin`. +/// `/node_modules/.bin` directory, mirroring the directory *lookup +/// order* Node, npm, and pnpm use. +/// +/// For the managed npm resolver the walk is bounded by the workspace root +/// (the directory holding the root `node_modules`), so a task run with +/// `--cwd ` also sees that member's own `node_modules/.bin` without +/// picking up unrelated directories above the workspace. +/// +/// The returned paths are candidates only: they aren't checked for existence, +/// and the ordering is a property of this list rather than a guarantee about +/// which executable a task ends up running. See `resolve_custom_commands` for +/// how the order is applied, and for the case where the resolution snapshot's +/// top-level packages take precedence over it. pub fn resolve_task_node_modules_bin_dirs( npm_resolver: &CliNpmResolver, cwd: &Path, @@ -566,10 +654,27 @@ pub fn resolve_task_node_modules_bin_dirs( .ancestors() .map(|dir| dir.join("node_modules").join(".bin")) .collect(), - CliNpmResolver::Managed(npm_resolver) => npm_resolver - .root_node_modules_path() - .map(|p| vec![p.join(".bin")]) - .unwrap_or_default(), + CliNpmResolver::Managed(npm_resolver) => { + let Some(root_node_modules_path) = npm_resolver.root_node_modules_path() + else { + return Vec::new(); + }; + let mut bin_dirs = Vec::new(); + // When the cwd is outside the workspace only the root `.bin` applies — + // don't reach into unrelated `node_modules` directories. + if let Some(root_dir) = root_node_modules_path.parent() + && cwd.starts_with(root_dir) + { + bin_dirs.extend( + cwd + .ancestors() + .take_while(|dir| *dir != root_dir) + .map(|dir| dir.join("node_modules").join(".bin")), + ); + } + bin_dirs.push(root_node_modules_path.join(".bin")); + bin_dirs + } } } @@ -599,6 +704,11 @@ fn resolve_managed_npm_commands( let mut result = HashMap::new(); for id in npm_resolver.resolution().top_level_packages() { let package_folder = npm_resolver.resolve_pkg_folder_from_pkg_id(&id)?; + // TODO(nathanwhit): this discards the `BinValue` that + // `resolve_npm_binary_commands_for_package` already computed, so a registry + // package whose bin is a native binary or a shell script is still handed to + // `deno run --ext=js`. See the `.bin` handling in `resolve_custom_commands` + // above for how that distinction should be honoured. let bins = node_resolver.resolve_npm_binary_commands_for_package(&package_folder)?; result.extend(bins.into_iter().map(|(command_name, path)| { diff --git a/libs/node_resolver/resolution.rs b/libs/node_resolver/resolution.rs index 562199a171be46..927888d03ceb77 100644 --- a/libs/node_resolver/resolution.rs +++ b/libs/node_resolver/resolution.rs @@ -1131,6 +1131,22 @@ impl< pub fn resolve_npm_commands_from_bin_dir( &self, bin_dir: &Path, + ) -> BTreeMap { + self.resolve_npm_commands_from_bin_dir_filtered(bin_dir, |_| true) + } + + /// Same as [`Self::resolve_npm_commands_from_bin_dir`], but only classifies + /// entries whose command name passes `filter`. + /// + /// Classifying an entry means reading the whole file off disk + /// ([`read_bin_value`]), and a `node_modules/.bin` can hold hundreds of + /// entries, some of them multi-megabyte bundled CLIs. Callers that already + /// know they will discard some of the names should filter here so that I/O + /// never happens. + pub fn resolve_npm_commands_from_bin_dir_filtered( + &self, + bin_dir: &Path, + filter: impl Fn(&str) -> bool, ) -> BTreeMap { log::debug!("Resolving npm commands in '{}'.", bin_dir.display()); let mut result = BTreeMap::new(); @@ -1141,7 +1157,7 @@ impl< continue; }; if let Some((command, bin_value)) = - self.resolve_bin_dir_entry_command(entry) + self.resolve_bin_dir_entry_command(entry, &filter) { result.insert(command, bin_value); } @@ -1157,10 +1173,16 @@ impl< fn resolve_bin_dir_entry_command( &self, entry: TSys::ReadDirEntry, + filter: &impl Fn(&str) -> bool, ) -> Option<(String, BinValue)> { if entry.path().extension().is_some() { return None; // only look at files without extensions (even on Windows) } + let command_name = entry.file_name().to_string_lossy().into_owned(); + // check the name before touching the file system any further + if !filter(&command_name) { + return None; + } let file_type = entry.file_type().ok()?; let path = if file_type.is_file() { entry.path() @@ -1169,7 +1191,6 @@ impl< } else { return None; }; - let command_name = entry.file_name().to_string_lossy().into_owned(); let bin_value = read_bin_value(&path, &self.sys)?; Some((command_name, bin_value)) } diff --git a/libs/npm_installer/bin_entries.rs b/libs/npm_installer/bin_entries.rs index 90a858ee2808dd..ce7062156d1739 100644 --- a/libs/npm_installer/bin_entries.rs +++ b/libs/npm_installer/bin_entries.rs @@ -39,6 +39,26 @@ fn default_bin_name(package: &NpmResolutionPackage) -> &str { .unwrap_or(package.id.nv.name.as_str()) } +/// The `node_modules/.bin` entry names a package would contribute, after +/// normalization. Mirrors what [`BinEntries::add`] registers. +pub fn bin_names<'a>( + package: &'a NpmResolutionPackage, + extra: &'a NpmPackageExtraInfo, +) -> Vec<&'a str> { + match extra.bin.as_ref() { + Some(deno_npm::registry::NpmPackageVersionBinEntry::String(_)) => { + vec![default_bin_name(package)] + } + Some(deno_npm::registry::NpmPackageVersionBinEntry::Map(entries)) => { + entries + .keys() + .map(|name| normalize_bin_name(name)) + .collect() + } + None => Vec::new(), + } +} + fn normalize_bin_name(bin_name: &str) -> &str { let trimmed = bin_name.trim_end_matches(['/', '\\']); // Leave validation of empty or special path component names to package @@ -86,6 +106,12 @@ impl<'a, TSys: SetupBinEntrySys> BinEntries<'a, TSys> { } } + /// Whether some already-added package contributes a `.bin` entry with this + /// (normalized) name. + pub fn has_bin_name(&self, name: &str) -> bool { + self.seen_names.contains_key(name) + } + /// Add a new bin entry (package with a bin field) pub fn add<'b>( &mut self, @@ -548,6 +574,36 @@ mod test { ) } + /// A snapshot with `package` as the only top-level package. + fn snapshot_with(package: &NpmResolutionPackage) -> NpmResolutionSnapshot { + let req = deno_semver::package::PackageReq::from_str(&format!( + "{}@{}", + package.id.nv.name, package.id.nv.version + )) + .unwrap(); + NpmResolutionSnapshot::new( + SerializedNpmResolutionSnapshot { + root_packages: [(req, package.id.clone())].into_iter().collect(), + packages: vec![ + deno_npm::resolution::SerializedNpmResolutionSnapshotPackage { + id: package.id.clone(), + system: Default::default(), + dist: None, + dependencies: Default::default(), + optional_dependencies: Default::default(), + optional_peer_dependencies: Default::default(), + extra: None, + is_deprecated: false, + has_bin: true, + has_scripts: false, + }, + ], + } + .into_valid() + .unwrap(), + ) + } + fn test_dir(name: &str) -> (PathBuf, impl Drop) { static COUNTER: std::sync::atomic::AtomicU64 = std::sync::atomic::AtomicU64::new(0); @@ -606,6 +662,52 @@ mod test { ); } + /// A workspace member's bins are added after every snapshot package so that + /// a real dependency wins a name collision (see `add_workspace_bin_entries` + /// in local.rs). A collision triggers the depth sort, and because the + /// synthetic workspace package is not in the snapshot it gets the maximum + /// depth and sorts last. The names here are chosen so the depth ordering is + /// what decides: the name tiebreak alone would pick `z-member`. + #[test] + fn snapshot_package_wins_collision_with_workspace_member() { + let sys = sys_traits::impls::RealSys; + let dependency = test_package("a-dep"); + let dep_extra = NpmPackageExtraInfo { + bin: Some(deno_npm::registry::NpmPackageVersionBinEntry::Map( + [("shared-cli".to_string(), "dep.js".to_string())] + .into_iter() + .collect(), + )), + ..Default::default() + }; + // stands in for a workspace member declaring the same bin name + let member = test_package("z-member"); + let member_extra = NpmPackageExtraInfo { + bin: Some(deno_npm::registry::NpmPackageVersionBinEntry::Map( + [("shared-cli".to_string(), "member.js".to_string())] + .into_iter() + .collect(), + )), + ..Default::default() + }; + + let mut entries = BinEntries::new(sys.with_paths_in_errors()); + entries.add( + &dependency, + &dep_extra, + PathBuf::from("/node_modules/a-dep"), + ); + entries.add(&member, &member_extra, PathBuf::from("/workspace/z-member")); + + assert_eq!( + entries.collect_bin_files(&snapshot_with(&dependency)), + vec![( + "shared-cli".to_string(), + PathBuf::from("/node_modules/a-dep/dep.js") + )] + ); + } + #[cfg(unix)] #[test] fn set_up_bin_entry_uses_normalized_bin_name() { diff --git a/libs/npm_installer/hoisted.rs b/libs/npm_installer/hoisted.rs index ab20dfcae8237b..b3ecba1cc6629b 100644 --- a/libs/npm_installer/hoisted.rs +++ b/libs/npm_installer/hoisted.rs @@ -65,7 +65,10 @@ use crate::local::InitializingGuard; use crate::local::LocalNpmInstallSys; use crate::local::LocalNpmPackageInstallerOptions; use crate::local::SyncResolutionWithFsError; +use crate::local::WorkspaceBinPackage; +use crate::local::add_workspace_bin_entries; use crate::local::join_package_name; +use crate::local::resolve_workspace_bin_packages; use crate::package_json::InstallWorkspacePkgDep; use crate::package_json::NpmInstallDepsProvider; use crate::process_state::NpmProcessState; @@ -409,6 +412,10 @@ impl< // 2. Clone all packages from cache into their hoisted positions let workspace_lifecycle_packages = self.resolve_workspace_lifecycle_packages(snapshot)?; + // Declared before `bin_entries` below so it outlives the borrows it hands + // out to it. + let workspace_bin_packages = + resolve_workspace_bin_packages(&self.npm_install_deps_provider); let mut cache_futures = FuturesUnordered::new(); let bin_entries = Rc::new(RefCell::new(BinEntries::new(sys))); let lifecycle_scripts = Rc::new(RefCell::new(LifecycleScripts::new( @@ -553,10 +560,20 @@ impl< // 5. Set up bin entries { - let bin_entries = match Rc::try_unwrap(bin_entries) { + let mut bin_entries = match Rc::try_unwrap(bin_entries) { Ok(bin_entries) => bin_entries.into_inner(), Err(_) => panic!("Should have sole ref to rc."), }; + // Workspace members that declare a `bin` get a root `.bin` entry too, so + // `deno task` and any tooling that looks in `node_modules/.bin` can run + // them (#36313). Added last so snapshot packages win a name collision. + // Only warn about duplicate bin names on the install path — see the + // matching call in `local.rs`. + add_workspace_bin_entries( + &mut bin_entries, + &workspace_bin_packages, + self.clean_on_install, + ); bin_entries.finish( snapshot, &bin_node_modules_dir_path, @@ -607,6 +624,11 @@ impl< .iter() .map(|pkg| (&pkg.nv, pkg.target_dir.as_path())) .collect(); + let workspace_bin_pkgs_by_nv: HashMap<&PackageNv, &WorkspaceBinPackage> = + workspace_bin_packages + .iter() + .map(|pkg| (&pkg.nv, pkg)) + .collect(); for workspace_pkg in self.npm_install_deps_provider.workspace_pkgs() { // The workspace root's `node_modules` is already fully set up above. // (Comparing paths here is unreliable: `root_node_modules_path` is @@ -670,6 +692,7 @@ impl< // hoisted location but points at the member's own alias link. bin_deps.push(crate::local::MemberBinDep { package, + extra: None, read_path: target_path.clone(), link_path: member_node_modules.join(alias.as_str()), }); @@ -680,6 +703,16 @@ impl< let Some(target_dir) = workspace_member_dirs.get(nv) else { continue; }; + // A sibling member that ships executables contributes them to + // this member's `.bin` too (#36313). + if let Some(bin_pkg) = workspace_bin_pkgs_by_nv.get(nv) { + bin_deps.push(crate::local::MemberBinDep { + package: &bin_pkg.package, + extra: Some(&bin_pkg.extra), + read_path: bin_pkg.package_path.clone(), + link_path: member_node_modules.join(alias.as_str()), + }); + } (alias, target_dir.to_path_buf()) } }; @@ -1090,6 +1123,26 @@ fn cleanup_hoisted_packages( remove_unexpected_package(sys, &path, &name_str, &expected_names); } } + + // Wipe the root `.bin` so entries for packages (and workspace members) that + // are no longer part of the install stop resolving. `bin_entries.finish` + // recreates every current entry right after this, and unlike the package + // directories above there's no cheap way to tell which entry belongs to + // which package once it's on disk. The isolated linker does the same in + // `cleanup_unused_packages`. + let bin_dir = root_node_modules_path.join(".bin"); + if let Ok(entries) = sys.fs_read_dir(&bin_dir) { + for entry in entries.flatten() { + let Ok(file_type) = entry.file_type() else { + continue; + }; + if file_type.is_file() { + let _ = sys.fs_remove_file(entry.path()); + } else { + let _ = sys.fs_remove_dir_all(entry.path()); + } + } + } } #[async_trait(?Send)] diff --git a/libs/npm_installer/local.rs b/libs/npm_installer/local.rs index e1cc254ca56471..d808ff9f1a0983 100644 --- a/libs/npm_installer/local.rs +++ b/libs/npm_installer/local.rs @@ -25,6 +25,7 @@ use deno_npm::NpmPackageIdPeerDependencies; use deno_npm::NpmResolutionPackage; use deno_npm::NpmResolutionPackageSystemInfo; use deno_npm::NpmSystemInfo; +use deno_npm::registry::NpmPackageVersionBinEntry; use deno_npm::resolution::NpmResolutionSnapshot; use deno_npm_cache::NpmCache; use deno_npm_cache::NpmCacheHttpClient; @@ -244,12 +245,20 @@ impl< deno_local_registry_dir.join(".setup-cache.bin"), ); + // Declared before `bin_entries` below so it outlives the borrows it hands + // out to it. + let workspace_bin_packages = + resolve_workspace_bin_packages(&self.npm_install_deps_provider); + // 1. Check if packages changed and clean up if needed if self.clean_on_install { let root_folder_names = root_package_folder_names(snapshot, &self.npm_install_deps_provider); - let packages_hash = - calculate_packages_hash(&package_partitions, &root_folder_names); + let packages_hash = calculate_packages_hash( + &package_partitions, + &root_folder_names, + &workspace_bin_packages, + ); if setup_cache.packages_changed(packages_hash) { cleanup_unused_packages( sys.as_ref(), @@ -916,10 +925,21 @@ impl< // 9. Set up `node_modules/.bin` entries for packages that need it. { - let bin_entries = match Rc::try_unwrap(bin_entries) { + let mut bin_entries = match Rc::try_unwrap(bin_entries) { Ok(bin_entries) => bin_entries.into_inner(), Err(_) => panic!("Should have sole ref to rc."), }; + // Workspace members that declare a `bin` get a root `.bin` entry too, so + // `deno task` and any tooling that looks in `node_modules/.bin` can run + // them (#36313). Added last so snapshot packages win a name collision. + // The collision warning is only worth emitting when the user actually + // asked to install; `deno run`/`deno task` re-link too and would + // otherwise re-print it ahead of every command's output. + add_workspace_bin_entries( + &mut bin_entries, + &workspace_bin_packages, + self.clean_on_install, + ); bin_entries.finish( snapshot, &bin_node_modules_dir_path, @@ -976,6 +996,11 @@ impl< .iter() .map(|pkg| (&pkg.nv, pkg.target_dir.as_path())) .collect(); + let workspace_bin_pkgs_by_nv: HashMap<&PackageNv, &WorkspaceBinPackage> = + workspace_bin_packages + .iter() + .map(|pkg| (&pkg.nv, pkg)) + .collect(); for workspace_pkg in self.npm_install_deps_provider.workspace_pkgs() { // The workspace root's `node_modules` is already fully set up above. // (Comparing paths here is unreliable: `root_node_modules_path` is @@ -1032,6 +1057,7 @@ impl< // its bins (the alias link exists either way). bin_deps.push(MemberBinDep { package, + extra: None, read_path: local_registry_package_path.clone(), link_path: member_node_modules.join(alias.as_str()), }); @@ -1042,6 +1068,16 @@ impl< let Some(target_dir) = workspace_member_dirs.get(nv) else { continue; }; + // A sibling member that ships executables contributes them to + // this member's `.bin` too (#36313). + if let Some(bin_pkg) = workspace_bin_pkgs_by_nv.get(nv) { + bin_deps.push(MemberBinDep { + package: &bin_pkg.package, + extra: Some(&bin_pkg.extra), + read_path: bin_pkg.package_path.clone(), + link_path: member_node_modules.join(alias.as_str()), + }); + } (alias, target_dir.to_path_buf(), nv.to_string()) } }; @@ -1835,11 +1871,195 @@ pub(crate) fn remove_stale_member_symlinks( } } +/// A non-root workspace member that declares a `bin` in its package.json. +/// +/// Workspace members are not npm packages in the resolution snapshot, so a +/// synthetic [`NpmResolutionPackage`] is built for them here. That lets their +/// executables go through the same [`BinEntries`] machinery as registry +/// packages (bin-name normalization, collision handling, unix symlinks and +/// windows shims) instead of duplicating any of it. See +/// https://github.com/denoland/deno/issues/36313. +pub(crate) struct WorkspaceBinPackage { + pub nv: PackageNv, + pub package: NpmResolutionPackage, + pub extra: NpmPackageExtraInfo, + /// The member's own directory (where its `bin` scripts live). + pub package_path: PathBuf, +} + +/// Builds a [`WorkspaceBinPackage`] for every non-root workspace member that +/// declares a `bin`. The workspace root is skipped because npm never links a +/// package's own executables into its `node_modules/.bin`. +pub(crate) fn resolve_workspace_bin_packages( + npm_install_deps_provider: &NpmInstallDepsProvider, +) -> Vec { + npm_install_deps_provider + .workspace_pkgs() + .iter() + .filter(|pkg| !pkg.is_root) + .filter_map(|pkg| { + let bin = pkg.bin.clone()?; + let extra = NpmPackageExtraInfo { + bin: Some(bin), + ..Default::default() + }; + Some(WorkspaceBinPackage { + nv: pkg.nv.clone(), + package: NpmResolutionPackage { + id: NpmPackageId { + nv: pkg.nv.clone(), + peer_dependencies: NpmPackageIdPeerDependencies::from([]), + }, + copy_index: 0, + system: NpmResolutionPackageSystemInfo::default(), + dist: None, + dependencies: Default::default(), + optional_dependencies: Default::default(), + optional_peer_dependencies: Default::default(), + extra: Some(extra.clone()), + is_deprecated: false, + has_bin: true, + // This stand-in exists only to link the member's executables; its + // lifecycle scripts are handled by the workspace lifecycle packages. + has_scripts: false, + }, + extra, + package_path: pkg.target_dir.clone(), + }) + }) + .collect() +} + +/// Adds the workspace members' executables to the root `node_modules/.bin` +/// entries. +/// +/// Precedence rules for a `.bin` name declared more than once: +/// +/// * **snapshot package vs. workspace member** — the snapshot package wins. +/// These are added *after* every snapshot package, `BinEntries` keeps the +/// first entry it sees for a given name, and when a collision forces a depth +/// sort the synthetic workspace packages aren't in the snapshot so they get +/// depth `u64::MAX` and sort last. Silently replacing a real dependency's +/// executable with a workspace member's would be surprising, and it matches +/// npm, which links the dependency. The member's `bin` then silently doesn't +/// appear, so [`warn_on_workspace_bin_name_collisions`] reports this case +/// too, even when only one member declares the name. +/// * **workspace member vs. workspace member** — both get depth `u64::MAX`, so +/// the sort falls back to `sort_by_depth`'s `nv` tiebreak, which is +/// *descending*; the greatest `@` therefore wins. That's +/// arbitrary, so [`warn_on_workspace_bin_name_collisions`] warns about it. +/// npm hard-errors here, but a warning keeps an otherwise fine workspace +/// installable. +/// +/// `warn_on_collisions` should only be set on the install path. `node_modules` +/// is re-linked by `deno run`/`deno task` too, and npm reports this kind of +/// problem once, at install time, rather than ahead of every command. The +/// trade-off of that gating is that a user who never re-runs a clean install — +/// they only ever `deno task` against an already-linked `node_modules` — won't +/// be shown the warning at all; we take it over reprinting the same warning +/// ahead of every single command. +pub(crate) fn add_workspace_bin_entries<'a, TSys: SetupBinEntrySys>( + bin_entries: &mut BinEntries<'a, TSys>, + workspace_bin_packages: &'a [WorkspaceBinPackage], + warn_on_collisions: bool, +) { + if warn_on_collisions { + // Must run before the members are added below so `has_bin_name` still only + // reports names claimed by snapshot packages. + warn_on_workspace_bin_name_collisions(workspace_bin_packages, |name| { + bin_entries.has_bin_name(name) + }); + } + for pkg in workspace_bin_packages { + // Point at the member's real directory rather than its root + // `node_modules/` symlink so the generated shim resolves even before + // that symlink is created (it's created after the root `.bin` is set up). + // + // NOTE: this means `package_path` is a directory inside the user's own + // source tree, so on unix `BinEntries` will `chmod +x` the member's `bin` + // script in place (see `make_executable_if_exists` in `bin_entries.rs`) — + // a git-tracked `packages/foo/cli.js` can flip 100644 -> 100755 and show + // up in `git status` after an install. npm's `bin-links` does exactly the + // same thing, so this is parity rather than a deno-specific quirk. It also + // fires for a member that *loses* a name collision, because the + // `already_seen` branch chmods too. + bin_entries.add(&pkg.package, &pkg.extra, pkg.package_path.clone()); + } +} + +/// Warns when a workspace member's `bin` name won't end up in the root +/// `node_modules/.bin` pointing at that member — either because another member +/// declares the same name (and which one wins is essentially arbitrary), or +/// because a dependency already claims it. See [`add_workspace_bin_entries`] +/// for the precedence. +fn warn_on_workspace_bin_name_collisions( + workspace_bin_packages: &[WorkspaceBinPackage], + is_claimed_by_dependency: impl Fn(&str) -> bool, +) { + for message in workspace_bin_name_collision_warnings( + workspace_bin_packages, + is_claimed_by_dependency, + ) { + log::warn!("{} {}", deno_terminal::colors::yellow("Warning"), message); + } +} + +/// Builds the warning messages for [`warn_on_workspace_bin_name_collisions`]. +/// +/// `is_claimed_by_dependency` reports whether a snapshot package already +/// contributes that name. Those always win, so in that case the member isn't +/// linked and the message must not claim otherwise — including when only a +/// single member declares the name, which is the likelier way to hit this. +fn workspace_bin_name_collision_warnings( + workspace_bin_packages: &[WorkspaceBinPackage], + is_claimed_by_dependency: impl Fn(&str) -> bool, +) -> Vec { + let mut members_by_bin_name: BTreeMap<&str, BTreeSet<&PackageNv>> = + BTreeMap::new(); + for pkg in workspace_bin_packages { + for name in crate::bin_entries::bin_names(&pkg.package, &pkg.extra) { + members_by_bin_name.entry(name).or_default().insert(&pkg.nv); + } + } + let mut messages = Vec::new(); + for (bin_name, members) in members_by_bin_name { + let claimed_by_dependency = is_claimed_by_dependency(bin_name); + // A single member that wins its name outright is the normal case. + if members.len() < 2 && !claimed_by_dependency { + continue; + } + let member_names = members + .iter() + .map(|nv| nv.name.as_str()) + .collect::>() + .join(", "); + messages.push(match (claimed_by_dependency, members.len()) { + (true, 1) => format!( + "Workspace member \"{member_names}\" declares a \"{bin_name}\" bin, but it will not be linked into node_modules/.bin because a dependency already provides it." + ), + (true, _) => format!( + "Multiple workspace members declare a \"{bin_name}\" bin: {member_names}. None of them will be linked into node_modules/.bin because a dependency already provides it." + ), + // `BTreeSet` is sorted ascending and the greatest `nv` wins the entry. + (false, _) => format!( + "Multiple workspace members declare a \"{bin_name}\" bin: {member_names}. Only \"{}\" will be linked into node_modules/.bin.", + members.last().unwrap().name, + ), + }); + } + messages +} + /// A workspace member's direct dependency that may contribute executables to /// the member's `node_modules/.bin`. pub(crate) struct MemberBinDep<'a> { /// The resolved npm package, used for its `bin` metadata. pub package: &'a NpmResolutionPackage, + /// Already-known extra info for the package. Workspace members aren't in the + /// snapshot and their `bin` comes straight from the package.json the deps + /// provider read, so there's nothing to look up. `None` means read it from + /// `read_path`. + pub extra: Option<&'a NpmPackageExtraInfo>, /// Where the package's `package.json` is read from: its real location in the /// layout (the `.deno` store path for the isolated linker, or the hoisted /// package directory for the hoisted linker). @@ -1863,8 +2083,9 @@ pub(crate) struct MemberBinDep<'a> { /// shims are plain files (``, `.cmd`, `.ps1`) rather than /// symlinks. /// -/// Sibling workspace members are not npm packages and so are not included in -/// `bin_deps`; their executables are not linked into a member's `.bin` yet. +/// Sibling workspace members that declare a `bin` are included in `bin_deps` +/// too (via a synthetic [`WorkspaceBinPackage`]), so a member can invoke a +/// sibling's executable the same way it invokes a registry dependency's. pub(crate) async fn setup_member_bin_entries<'a, TSys: LocalNpmInstallSys>( sys: SysWithPathsInErrors<'a, TSys>, snapshot: &'a NpmResolutionSnapshot, @@ -1895,17 +2116,22 @@ pub(crate) async fn setup_member_bin_entries<'a, TSys: LocalNpmInstallSys>( if !dep.package.has_bin { continue; } - // Cached from the root setup that ran earlier, so this is a map lookup - // rather than a disk read in the common case. - let extra = extra_info_provider - .get_package_extra_info( - &dep.package.id.nv, - &dep.read_path, - ExpectedExtraInfo::from_package(dep.package), - ) - .await - .map_err(SyncResolutionWithFsError::Other)?; - bin_entries.add(dep.package, &extra, dep.link_path.clone()); + match dep.extra { + Some(extra) => bin_entries.add(dep.package, extra, dep.link_path.clone()), + None => { + // Cached from the root setup that ran earlier, so this is a map lookup + // rather than a disk read in the common case. + let extra = extra_info_provider + .get_package_extra_info( + &dep.package.id.nv, + &dep.read_path, + ExpectedExtraInfo::from_package(dep.package), + ) + .await + .map_err(SyncResolutionWithFsError::Other)?; + bin_entries.add(dep.package, &extra, dep.link_path.clone()); + } + } } // Ignore setup failures here: every package linked into a member is also // linked at the root, whose `.bin` setup already reports a missing entrypoint @@ -2032,6 +2258,7 @@ pub(crate) fn join_package_name( fn calculate_packages_hash( package_partitions: &deno_npm::resolution::NpmPackagesPartitioned, root_folder_names: &BTreeSet, + workspace_bin_packages: &[WorkspaceBinPackage], ) -> u64 { use std::hash::Hash; use std::hash::Hasher; @@ -2056,9 +2283,40 @@ fn calculate_packages_hash( folder_name.hash(&mut hasher); } + // and hash the workspace members' executables, which also land in the root + // `node_modules/.bin` but aren't part of the resolution snapshot. Without + // this, renaming a member's bin, dropping its `bin` field, or removing the + // member entirely would never trip the cleanup and the old entry would + // linger (as a dangling symlink, in the last case). + for name in workspace_bin_hash_names(workspace_bin_packages) { + name.hash(&mut hasher); + } + hasher.finish() } +/// The `@/` pairs contributed by the workspace +/// members, sorted so the hash doesn't depend on iteration order. +fn workspace_bin_hash_names( + workspace_bin_packages: &[WorkspaceBinPackage], +) -> BTreeSet { + let mut names = BTreeSet::new(); + for pkg in workspace_bin_packages { + match pkg.extra.bin.as_ref() { + Some(NpmPackageVersionBinEntry::String(script)) => { + names.insert(format!("{}/{}", pkg.nv, script)); + } + Some(NpmPackageVersionBinEntry::Map(entries)) => { + for (name, script) in entries { + names.insert(format!("{}/{}={}", pkg.nv, name, script)); + } + } + None => {} + } + } + names +} + /// Calculates the set of package folder names that are expected to have a /// symlink at the root of the node_modules directory (resolved package.json /// and import map dependencies plus the snapshot's top level packages). @@ -2353,6 +2611,7 @@ mod test { copy_packages: Vec::new(), }, &root_folder_names, + &[], ); let reversed_hash = calculate_packages_hash( &deno_npm::resolution::NpmPackagesPartitioned { @@ -2360,10 +2619,174 @@ mod test { copy_packages: Vec::new(), }, &root_folder_names, + &[], ); assert_eq!(hash, reversed_hash); } + #[test] + fn test_calculate_packages_hash_includes_workspace_bins() { + fn workspace_bin_pkg( + nv: &str, + bin: NpmPackageVersionBinEntry, + ) -> WorkspaceBinPackage { + let nv = PackageNv::from_str(nv).unwrap(); + let extra = NpmPackageExtraInfo { + bin: Some(bin), + ..Default::default() + }; + WorkspaceBinPackage { + nv: nv.clone(), + package: NpmResolutionPackage { + id: NpmPackageId { + nv, + peer_dependencies: NpmPackageIdPeerDependencies::from([]), + }, + copy_index: 0, + system: Default::default(), + dist: None, + dependencies: Default::default(), + optional_dependencies: Default::default(), + optional_peer_dependencies: Default::default(), + extra: Some(extra.clone()), + is_deprecated: false, + has_bin: true, + has_scripts: false, + }, + extra, + package_path: PathBuf::from("/workspace/packages/member"), + } + } + fn hash(workspace_bin_packages: &[WorkspaceBinPackage]) -> u64 { + calculate_packages_hash( + &deno_npm::resolution::NpmPackagesPartitioned { + packages: Vec::new(), + copy_packages: Vec::new(), + }, + &BTreeSet::new(), + workspace_bin_packages, + ) + } + + let map = |name: &str, script: &str| { + NpmPackageVersionBinEntry::Map(HashMap::from([( + name.to_string(), + script.to_string(), + )])) + }; + + let base = vec![workspace_bin_pkg("member@1.0.0", map("tool", "./cli.js"))]; + // renaming the bin must change the hash so the stale `.bin/tool` is pruned + let renamed = + vec![workspace_bin_pkg("member@1.0.0", map("tool2", "./cli.js"))]; + // so must dropping the `bin` field entirely, or removing the member + let removed: Vec = Vec::new(); + // ...and pointing the same bin name at a different script + let repointed = + vec![workspace_bin_pkg("member@1.0.0", map("tool", "./other.js"))]; + // the string form participates too + let string_form = vec![workspace_bin_pkg( + "member@1.0.0", + NpmPackageVersionBinEntry::String("./cli.js".to_string()), + )]; + + let base_hash = hash(&base); + assert_ne!(base_hash, hash(&renamed)); + assert_ne!(base_hash, hash(&removed)); + assert_ne!(base_hash, hash(&repointed)); + assert_ne!(base_hash, hash(&string_form)); + // and it stays stable for an unchanged set + assert_eq!(base_hash, hash(&base)); + } + + #[test] + fn test_workspace_bin_name_collision_warnings() { + fn workspace_bin_pkg(nv: &str, bin_name: &str) -> WorkspaceBinPackage { + let nv = PackageNv::from_str(nv).unwrap(); + let extra = NpmPackageExtraInfo { + bin: Some(NpmPackageVersionBinEntry::Map(HashMap::from([( + bin_name.to_string(), + "./cli.js".to_string(), + )]))), + ..Default::default() + }; + WorkspaceBinPackage { + nv: nv.clone(), + package: NpmResolutionPackage { + id: NpmPackageId { + nv, + peer_dependencies: NpmPackageIdPeerDependencies::from([]), + }, + copy_index: 0, + system: Default::default(), + dist: None, + dependencies: Default::default(), + optional_dependencies: Default::default(), + optional_peer_dependencies: Default::default(), + extra: Some(extra.clone()), + is_deprecated: false, + has_bin: true, + has_scripts: false, + }, + extra, + package_path: PathBuf::from("/workspace/packages/member"), + } + } + + let one_member = vec![workspace_bin_pkg("member-a@1.0.0", "tool")]; + let two_members = vec![ + workspace_bin_pkg("member-a@1.0.0", "tool"), + workspace_bin_pkg("member-b@1.0.0", "tool"), + ]; + + // a single member that wins its name outright is the normal case + assert!( + workspace_bin_name_collision_warnings(&one_member, |_| false).is_empty() + ); + + // ...but a single member whose name a dependency already claims must still + // warn, and the message must not claim the member gets linked + let warnings = + workspace_bin_name_collision_warnings(&one_member, |name| name == "tool"); + assert_eq!( + warnings, + vec![ + "Workspace member \"member-a\" declares a \"tool\" bin, but it will not be linked into node_modules/.bin because a dependency already provides it." + .to_string() + ] + ); + // an unrelated dependency name doesn't warn + let unrelated = + workspace_bin_name_collision_warnings(&one_member, |name| { + name == "other" + }); + assert!(unrelated.is_empty()); + + // member vs. member is unchanged: the greatest `nv` wins + let warnings = + workspace_bin_name_collision_warnings(&two_members, |_| false); + assert_eq!( + warnings, + vec![ + "Multiple workspace members declare a \"tool\" bin: member-a, member-b. Only \"member-b\" will be linked into node_modules/.bin." + .to_string() + ] + ); + + // ...and when a dependency claims it, neither member is named as a winner + let warnings = + workspace_bin_name_collision_warnings(&two_members, |name| { + name == "tool" + }); + assert_eq!( + warnings, + vec![ + "Multiple workspace members declare a \"tool\" bin: member-a, member-b. None of them will be linked into node_modules/.bin because a dependency already provides it." + .to_string() + ] + ); + } + #[test] fn test_symlink_package_dir_replaces_existing_link() { let temp_dir = TempDir::new(); diff --git a/libs/npm_installer/package_json.rs b/libs/npm_installer/package_json.rs index 6e92a76eeb5d12..59c32ed739efa7 100644 --- a/libs/npm_installer/package_json.rs +++ b/libs/npm_installer/package_json.rs @@ -4,6 +4,7 @@ use std::path::PathBuf; use std::sync::Arc; use deno_config::workspace::Workspace; +use deno_npm::registry::NpmPackageVersionBinEntry; use deno_package_json::PackageJsonDepValue; use deno_package_json::PackageJsonDepValueParseError; use deno_package_json::PackageJsonDepWorkspaceReq; @@ -62,6 +63,11 @@ pub struct InstallWorkspacePkg { /// is set up separately, so the per-member linking must skip it. pub is_root: bool, pub scripts: std::collections::HashMap, + /// The member's package.json `bin`, in the same shape npm uses. Workspace + /// members are not part of the npm resolution snapshot, so this is the only + /// place their executables are known and it's what the installers use to + /// create their `node_modules/.bin` entries (#36313). + pub bin: Option, pub deps: Vec, } @@ -153,6 +159,35 @@ fn package_json_to_lifecycle_nv( PackageNv { name, version } } +/// Parses a package.json `bin` into the same representation the npm registry +/// uses. `deno_package_json` only keeps the raw json value, so the two shapes +/// npm supports are handled here: a single string (the executable is named +/// after the package) and a map of name to script. Anything else is ignored +/// rather than erroring, matching how npm tolerates a malformed `bin`. +fn package_json_to_bin_entry( + pkg_json: &deno_package_json::PackageJson, +) -> Option { + match pkg_json.bin.as_ref()? { + serde_json::Value::String(script) => { + Some(NpmPackageVersionBinEntry::String(script.clone())) + } + serde_json::Value::Object(obj) => { + let map = obj + .iter() + .filter_map(|(name, script)| { + Some((name.clone(), script.as_str()?.to_string())) + }) + .collect::>(); + if map.is_empty() { + None + } else { + Some(NpmPackageVersionBinEntry::Map(map)) + } + } + _ => None, + } +} + impl NpmInstallDepsProvider { pub fn empty() -> Self { Self::default() @@ -424,6 +459,7 @@ impl NpmInstallDepsProvider { .collect() }) .unwrap_or_default(), + bin: package_json_to_bin_entry(pkg_json), deps: workspace_pkg_deps, }); @@ -509,3 +545,52 @@ impl NpmInstallDepsProvider { &self.workspace_member_version_errors } } + +#[cfg(test)] +mod test { + use super::*; + + fn bin_entry(json: &str) -> Option { + let pkg_json = deno_package_json::PackageJson::load_from_string( + PathBuf::from("/workspace/member/package.json"), + json, + ) + .unwrap(); + package_json_to_bin_entry(&pkg_json) + } + + #[test] + fn parses_string_bin() { + assert_eq!( + bin_entry(r#"{ "name": "local-cli", "bin": "./cli.js" }"#), + Some(NpmPackageVersionBinEntry::String("./cli.js".to_string())) + ); + } + + #[test] + fn parses_map_bin() { + assert_eq!( + bin_entry( + r#"{ "name": "local-cli", "bin": { "local-cli": "./cli.js" } }"# + ), + Some(NpmPackageVersionBinEntry::Map( + [("local-cli".to_string(), "./cli.js".to_string())] + .into_iter() + .collect() + )) + ); + } + + #[test] + fn ignores_missing_or_malformed_bin() { + assert_eq!(bin_entry(r#"{ "name": "local-cli" }"#), None); + // an empty map has nothing to link + assert_eq!(bin_entry(r#"{ "name": "local-cli", "bin": {} }"#), None); + // non-string values within the map are skipped, like npm tolerates + assert_eq!( + bin_entry(r#"{ "name": "local-cli", "bin": { "local-cli": 1 } }"#), + None + ); + assert_eq!(bin_entry(r#"{ "name": "local-cli", "bin": [] }"#), None); + } +} diff --git a/tests/specs/workspaces/workspace_member_bin/__test__.jsonc b/tests/specs/workspaces/workspace_member_bin/__test__.jsonc new file mode 100644 index 00000000000000..7be8e18320643d --- /dev/null +++ b/tests/specs/workspaces/workspace_member_bin/__test__.jsonc @@ -0,0 +1,66 @@ +// Tests that a local workspace member which declares a `bin` in its +// package.json is linked into `node_modules/.bin` — both the workspace root's +// and that of a sibling member which depends on it — and that `deno task` can +// invoke it. Only external npm dependencies used to get `.bin` entries, so a +// task like `"cli": "local-cli"` failed with "command not found". +// +// This covers the default isolated (`.deno`) linker; the hoisted linker is +// covered by the sibling `workspace_member_bin_hoisted` spec. No network +// access is required — the workspace contains only local members. +// +// https://github.com/denoland/deno/issues/36313 +{ + "tempDir": true, + "steps": [ + { + "args": "install", + "output": "[WILDCARD]" + }, + { + "args": "run --allow-read check_bin.ts", + "output": "ok\n" + }, + { + // "Hello from deno" (rather than "node") asserts the task runner resolves + // the member's JS bin to a `deno run` command instead of leaving it to a + // `PATH` lookup, which would execute the `#!/usr/bin/env node` shebang + // with whatever Node happens to be installed — or fail outright. + "args": "task --cwd apps/web cli", + "output": "member_task.out" + }, + { + "args": "task root-cli", + "output": "root_task.out" + }, + { + // A member whose bin is a POSIX shell script rather than JavaScript must + // still run: it resolves through `PATH` (the root `.bin` is prepended) + // and `deno_task_shell` honours the shebang. Handing it to + // `deno run --ext=js` instead would fail with a `SyntaxError`. + "if": "unix", + "args": "task root-sh", + "output": "root_sh_task.out" + }, + { + // A member whose bin is JavaScript with NO shebang classifies as + // `Executable` rather than `JsFile`, so under the managed resolver it is + // left to `PATH` and the kernel refuses to exec it. This is *not* a + // regression — before #36313 such a member got no `.bin` entry at all — + // and `npm`/`node` behave the same way on unix. It differs from BYONM, + // which deliberately routes every `.bin` entry through `deno run`; see + // the NOTE in `resolve_custom_commands`. Locked in so the asymmetry + // can't change silently. + // + // Linux only, because *how* the exec fails is platform specific: on + // macOS the spawn falls back to running the file with `/bin/sh` (the + // `ENOEXEC` behaviour of `execvp`), so instead of "Exec format error" + // the JS source comes back as a pile of shell syntax errors. What is + // being asserted here — that the entry is not routed through `deno run` + // — is the same on either platform, so testing it on one is enough. + "if": "linux", + "args": "task root-noshebang", + "output": "root_noshebang_task.out", + "exitCode": 1 + } + ] +} diff --git a/tests/specs/workspaces/workspace_member_bin/apps/web/package.json b/tests/specs/workspaces/workspace_member_bin/apps/web/package.json new file mode 100644 index 00000000000000..647b10ac22b4d6 --- /dev/null +++ b/tests/specs/workspaces/workspace_member_bin/apps/web/package.json @@ -0,0 +1,12 @@ +{ + "name": "web", + "version": "1.0.0", + "private": true, + "type": "module", + "scripts": { + "cli": "local-cli" + }, + "dependencies": { + "local-cli": "workspace:*" + } +} diff --git a/tests/specs/workspaces/workspace_member_bin/check_bin.ts b/tests/specs/workspaces/workspace_member_bin/check_bin.ts new file mode 100644 index 00000000000000..d9bec54a0ca317 --- /dev/null +++ b/tests/specs/workspaces/workspace_member_bin/check_bin.ts @@ -0,0 +1,77 @@ +// Verifies that a local workspace member which declares a `bin` in its +// package.json gets a `node_modules/.bin` entry, both in the workspace root +// and in a sibling member that depends on it. Previously only external npm +// dependencies got these, so `deno task` could not invoke a local member's +// executable. https://github.com/denoland/deno/issues/36313 + +/** The files a `node_modules/.bin` entry points at. On unix the entry is a + * symlink into the member, so the entry path itself resolves. On Windows it is + * a generated shim script that embeds `$basedir`-relative paths (one for the + * interpreter, one for the script), so pull those out of the text. */ +function binTargets(binPath: string): string[] { + if (Deno.build.os !== "windows") { + // touch the entry so a missing one reports as NotFound here + Deno.lstatSync(binPath); + return [binPath]; + } + const dir = binPath.slice(0, binPath.lastIndexOf("/")); + const text = Deno.readTextFileSync(binPath); + return [...text.matchAll(/\$basedir\/([^"]+)/g)].map((m) => `${dir}/${m[1]}`); +} + +/** Asserts that the `.bin` entry actually resolves to `expectedFile`. Merely + * existing isn't enough: a shim built from the wrong package path would look + * identical on disk. */ +function assertBinTarget(binPath: string, expectedFile: string) { + let expected: string; + try { + expected = Deno.realPathSync(expectedFile); + } catch { + throw new Error(`expected bin script ${expectedFile} to exist`); + } + let candidates: string[]; + try { + candidates = binTargets(binPath); + } catch (err) { + if (err instanceof Deno.errors.NotFound) { + throw new Error(`expected ${binPath} to exist`); + } + throw err; + } + const resolved = candidates.map((path) => { + try { + return Deno.realPathSync(path); + } catch { + return null; + } + }); + if (!resolved.includes(expected)) { + throw new Error( + `expected ${binPath} to point at ${expected}, but it resolved to ${ + JSON.stringify(resolved) + }`, + ); + } +} + +// (a) The root `node_modules/.bin` has the member's executable, so tooling +// (and a root task) can run it. +assertBinTarget("node_modules/.bin/local-cli", "packages/local-cli/cli.js"); + +// (b) The depending member's own `node_modules/.bin` has it too, mirroring how +// npm and pnpm lay out workspaces. +assertBinTarget( + "apps/web/node_modules/.bin/local-cli", + "packages/local-cli/cli.js", +); + +// (c) A member whose bin is not a JavaScript file gets an entry all the same — +// it just has to keep resolving through `PATH` instead of being handed to +// `deno run`. +assertBinTarget("node_modules/.bin/shtool", "packages/shtool/tool.sh"); + +// (d) Likewise for a JS bin with no shebang. It's linked, it just can't be +// exec'd through `PATH` on unix — see the `root-noshebang` task step. +assertBinTarget("node_modules/.bin/noshebang", "packages/noshebang/cli.js"); + +console.log("ok"); diff --git a/tests/specs/workspaces/workspace_member_bin/deno.json b/tests/specs/workspaces/workspace_member_bin/deno.json new file mode 100644 index 00000000000000..6550194b204919 --- /dev/null +++ b/tests/specs/workspaces/workspace_member_bin/deno.json @@ -0,0 +1,14 @@ +{ + "nodeModulesDir": "auto", + "workspace": [ + "./packages/local-cli", + "./packages/shtool", + "./packages/noshebang", + "./apps/web" + ], + "tasks": { + "root-cli": "local-cli", + "root-sh": "shtool hi", + "root-noshebang": "noshebang" + } +} diff --git a/tests/specs/workspaces/workspace_member_bin/member_task.out b/tests/specs/workspaces/workspace_member_bin/member_task.out new file mode 100644 index 00000000000000..d7e895418a0add --- /dev/null +++ b/tests/specs/workspaces/workspace_member_bin/member_task.out @@ -0,0 +1,2 @@ +Task cli local-cli +Hello from deno diff --git a/tests/specs/workspaces/workspace_member_bin/packages/local-cli/cli.js b/tests/specs/workspaces/workspace_member_bin/packages/local-cli/cli.js new file mode 100644 index 00000000000000..ffefac84caa93a --- /dev/null +++ b/tests/specs/workspaces/workspace_member_bin/packages/local-cli/cli.js @@ -0,0 +1,5 @@ +#!/usr/bin/env node +// `deno task` runs this itself (`deno run --ext=js`) rather than handing it to +// whichever `node` happens to be on `PATH`, so a workspace member's executable +// works without Node installed. Print the runtime to lock that in. +console.log("Hello from", typeof Deno === "undefined" ? "node" : "deno"); diff --git a/tests/specs/workspaces/workspace_member_bin/packages/local-cli/package.json b/tests/specs/workspaces/workspace_member_bin/packages/local-cli/package.json new file mode 100644 index 00000000000000..0d25e6cbc6d7f0 --- /dev/null +++ b/tests/specs/workspaces/workspace_member_bin/packages/local-cli/package.json @@ -0,0 +1,8 @@ +{ + "name": "local-cli", + "version": "1.0.0", + "type": "module", + "bin": { + "local-cli": "./cli.js" + } +} diff --git a/tests/specs/workspaces/workspace_member_bin/packages/noshebang/cli.js b/tests/specs/workspaces/workspace_member_bin/packages/noshebang/cli.js new file mode 100644 index 00000000000000..2d157220205c38 --- /dev/null +++ b/tests/specs/workspaces/workspace_member_bin/packages/noshebang/cli.js @@ -0,0 +1,2 @@ +// Deliberately no `#!` line — see the `noshebang` step in `__test__.jsonc`. +console.log("noshebang ran"); diff --git a/tests/specs/workspaces/workspace_member_bin/packages/noshebang/package.json b/tests/specs/workspaces/workspace_member_bin/packages/noshebang/package.json new file mode 100644 index 00000000000000..52bb22e3b55f0e --- /dev/null +++ b/tests/specs/workspaces/workspace_member_bin/packages/noshebang/package.json @@ -0,0 +1,8 @@ +{ + "name": "noshebang", + "version": "1.0.0", + "type": "module", + "bin": { + "noshebang": "./cli.js" + } +} diff --git a/tests/specs/workspaces/workspace_member_bin/packages/shtool/package.json b/tests/specs/workspaces/workspace_member_bin/packages/shtool/package.json new file mode 100644 index 00000000000000..04998bee01e1c2 --- /dev/null +++ b/tests/specs/workspaces/workspace_member_bin/packages/shtool/package.json @@ -0,0 +1,7 @@ +{ + "name": "shtool", + "version": "1.0.0", + "bin": { + "shtool": "./tool.sh" + } +} diff --git a/tests/specs/workspaces/workspace_member_bin/packages/shtool/tool.sh b/tests/specs/workspaces/workspace_member_bin/packages/shtool/tool.sh new file mode 100755 index 00000000000000..9d62b5e514fba1 --- /dev/null +++ b/tests/specs/workspaces/workspace_member_bin/packages/shtool/tool.sh @@ -0,0 +1,2 @@ +#!/bin/sh +echo "shtool ran: $*" diff --git a/tests/specs/workspaces/workspace_member_bin/root_noshebang_task.out b/tests/specs/workspaces/workspace_member_bin/root_noshebang_task.out new file mode 100644 index 00000000000000..29c6951d25398c --- /dev/null +++ b/tests/specs/workspaces/workspace_member_bin/root_noshebang_task.out @@ -0,0 +1,2 @@ +Task root-noshebang noshebang +[WILDCARD]Exec format error[WILDCARD] \ No newline at end of file diff --git a/tests/specs/workspaces/workspace_member_bin/root_sh_task.out b/tests/specs/workspaces/workspace_member_bin/root_sh_task.out new file mode 100644 index 00000000000000..03a5c6580378c3 --- /dev/null +++ b/tests/specs/workspaces/workspace_member_bin/root_sh_task.out @@ -0,0 +1,2 @@ +Task root-sh shtool hi +shtool ran: hi diff --git a/tests/specs/workspaces/workspace_member_bin/root_task.out b/tests/specs/workspaces/workspace_member_bin/root_task.out new file mode 100644 index 00000000000000..30adccc4ea0b90 --- /dev/null +++ b/tests/specs/workspaces/workspace_member_bin/root_task.out @@ -0,0 +1,2 @@ +Task root-cli local-cli +Hello from deno diff --git a/tests/specs/workspaces/workspace_member_bin_collision/__test__.jsonc b/tests/specs/workspaces/workspace_member_bin_collision/__test__.jsonc new file mode 100644 index 00000000000000..3f3ebb10fed9c8 --- /dev/null +++ b/tests/specs/workspaces/workspace_member_bin_collision/__test__.jsonc @@ -0,0 +1,29 @@ +// A registry dependency (`@denotest/one-bin`) and a workspace member both +// declare a `thing-bin` executable. The dependency wins the root +// `node_modules/.bin` entry: workspace members' bins are added after every +// snapshot package, and the synthetic packages built for them aren't in the +// resolution snapshot so they sort last when the collision forces a depth +// sort. Only a synthetic unit test covered this before. +// +// Two members declare it, which also exercises the three-way case: the +// duplicate-bin warning must not claim one of them "will be linked" when the +// dependency is what actually wins. +// +// https://github.com/denoland/deno/issues/36313 +{ + "tempDir": true, + "steps": [ + { + "args": "install", + "output": "install.out" + }, + { + "args": "run --allow-read check_bin.ts", + "output": "ok\n" + }, + { + "args": "task run-bin", + "output": "task.out" + } + ] +} diff --git a/tests/specs/workspaces/workspace_member_bin_collision/check_bin.ts b/tests/specs/workspaces/workspace_member_bin_collision/check_bin.ts new file mode 100644 index 00000000000000..f978fb093f9e79 --- /dev/null +++ b/tests/specs/workspaces/workspace_member_bin_collision/check_bin.ts @@ -0,0 +1,40 @@ +// A registry dependency and a workspace member both declare a `thing-bin` +// executable. The dependency must win: silently shadowing a real dependency's +// executable with a local member's would be surprising, and npm links the +// dependency too. + +function binTargets(binPath: string): string[] { + if (Deno.build.os !== "windows") { + Deno.lstatSync(binPath); + return [binPath]; + } + const dir = binPath.slice(0, binPath.lastIndexOf("/")); + const text = Deno.readTextFileSync(binPath); + return [...text.matchAll(/\$basedir\/([^"]+)/g)].map((m) => `${dir}/${m[1]}`); +} + +const resolved = binTargets("node_modules/.bin/thing-bin").map((path) => { + try { + return Deno.realPathSync(path); + } catch { + return null; + } +}); + +for (const dir of ["thing", "thing-alt"]) { + const member = Deno.realPathSync(`packages/${dir}/cli.js`); + if (resolved.includes(member)) { + throw new Error( + `node_modules/.bin/thing-bin should point at the @denotest/one-bin ` + + `dependency, not the workspace member (${member})`, + ); + } +} +if (!resolved.some((path) => path?.replaceAll("\\", "/").includes("one-bin"))) { + throw new Error( + `expected node_modules/.bin/thing-bin to point into @denotest/one-bin, ` + + `got ${JSON.stringify(resolved)}`, + ); +} + +console.log("ok"); diff --git a/tests/specs/workspaces/workspace_member_bin_collision/deno.json b/tests/specs/workspaces/workspace_member_bin_collision/deno.json new file mode 100644 index 00000000000000..edbc8fc63ae42c --- /dev/null +++ b/tests/specs/workspaces/workspace_member_bin_collision/deno.json @@ -0,0 +1,10 @@ +{ + "nodeModulesDir": "auto", + "workspace": [ + "./packages/thing", + "./packages/thing-alt" + ], + "tasks": { + "run-bin": "thing-bin" + } +} diff --git a/tests/specs/workspaces/workspace_member_bin_collision/install.out b/tests/specs/workspaces/workspace_member_bin_collision/install.out new file mode 100644 index 00000000000000..0b291f9e8486fe --- /dev/null +++ b/tests/specs/workspaces/workspace_member_bin_collision/install.out @@ -0,0 +1 @@ +[WILDCARD]Warning Multiple workspace members declare a "thing-bin" bin: thing, thing-alt. None of them will be linked into node_modules/.bin because a dependency already provides it.[WILDCARD] \ No newline at end of file diff --git a/tests/specs/workspaces/workspace_member_bin_collision/package.json b/tests/specs/workspaces/workspace_member_bin_collision/package.json new file mode 100644 index 00000000000000..8c9ebd3bbdb7fb --- /dev/null +++ b/tests/specs/workspaces/workspace_member_bin_collision/package.json @@ -0,0 +1,7 @@ +{ + "name": "root", + "version": "1.0.0", + "dependencies": { + "@denotest/one-bin": "1.0.0" + } +} diff --git a/tests/specs/workspaces/workspace_member_bin_collision/packages/thing-alt/cli.js b/tests/specs/workspaces/workspace_member_bin_collision/packages/thing-alt/cli.js new file mode 100644 index 00000000000000..a465d976bf0511 --- /dev/null +++ b/tests/specs/workspaces/workspace_member_bin_collision/packages/thing-alt/cli.js @@ -0,0 +1,2 @@ +#!/usr/bin/env node +console.log("workspace member won the collision"); diff --git a/tests/specs/workspaces/workspace_member_bin_collision/packages/thing-alt/package.json b/tests/specs/workspaces/workspace_member_bin_collision/packages/thing-alt/package.json new file mode 100644 index 00000000000000..dd6d015c5eaa0b --- /dev/null +++ b/tests/specs/workspaces/workspace_member_bin_collision/packages/thing-alt/package.json @@ -0,0 +1,8 @@ +{ + "name": "thing-alt", + "version": "1.0.0", + "type": "module", + "bin": { + "thing-bin": "./cli.js" + } +} diff --git a/tests/specs/workspaces/workspace_member_bin_collision/packages/thing/cli.js b/tests/specs/workspaces/workspace_member_bin_collision/packages/thing/cli.js new file mode 100644 index 00000000000000..a465d976bf0511 --- /dev/null +++ b/tests/specs/workspaces/workspace_member_bin_collision/packages/thing/cli.js @@ -0,0 +1,2 @@ +#!/usr/bin/env node +console.log("workspace member won the collision"); diff --git a/tests/specs/workspaces/workspace_member_bin_collision/packages/thing/package.json b/tests/specs/workspaces/workspace_member_bin_collision/packages/thing/package.json new file mode 100644 index 00000000000000..2fc0cc30fef1f0 --- /dev/null +++ b/tests/specs/workspaces/workspace_member_bin_collision/packages/thing/package.json @@ -0,0 +1,8 @@ +{ + "name": "thing", + "version": "1.0.0", + "type": "module", + "bin": { + "thing-bin": "./cli.js" + } +} diff --git a/tests/specs/workspaces/workspace_member_bin_collision/task.out b/tests/specs/workspaces/workspace_member_bin_collision/task.out new file mode 100644 index 00000000000000..dc1349884ec507 --- /dev/null +++ b/tests/specs/workspaces/workspace_member_bin_collision/task.out @@ -0,0 +1,2 @@ +Task run-bin thing-bin +thing-bin diff --git a/tests/specs/workspaces/workspace_member_bin_dep_claim/__test__.jsonc b/tests/specs/workspaces/workspace_member_bin_dep_claim/__test__.jsonc new file mode 100644 index 00000000000000..771394d6ceee8b --- /dev/null +++ b/tests/specs/workspaces/workspace_member_bin_dep_claim/__test__.jsonc @@ -0,0 +1,20 @@ +// A single workspace member declares a `thing-bin` executable that a registry +// dependency (`@denotest/one-bin`) already provides. The dependency wins the +// root `node_modules/.bin` entry, so the member's `bin` silently doesn't +// appear — the warning has to fire even though no *other* member competes for +// the name, which is the likeliest way to hit this. +// +// https://github.com/denoland/deno/issues/36313 +{ + "tempDir": true, + "steps": [ + { + "args": "install", + "output": "install.out" + }, + { + "args": "task run-bin", + "output": "task.out" + } + ] +} diff --git a/tests/specs/workspaces/workspace_member_bin_dep_claim/deno.json b/tests/specs/workspaces/workspace_member_bin_dep_claim/deno.json new file mode 100644 index 00000000000000..88e72acfe5c94b --- /dev/null +++ b/tests/specs/workspaces/workspace_member_bin_dep_claim/deno.json @@ -0,0 +1,9 @@ +{ + "nodeModulesDir": "auto", + "workspace": [ + "./packages/thing" + ], + "tasks": { + "run-bin": "thing-bin" + } +} diff --git a/tests/specs/workspaces/workspace_member_bin_dep_claim/install.out b/tests/specs/workspaces/workspace_member_bin_dep_claim/install.out new file mode 100644 index 00000000000000..4c1cd0fd63f992 --- /dev/null +++ b/tests/specs/workspaces/workspace_member_bin_dep_claim/install.out @@ -0,0 +1 @@ +[WILDCARD]Warning Workspace member "thing" declares a "thing-bin" bin, but it will not be linked into node_modules/.bin because a dependency already provides it.[WILDCARD] \ No newline at end of file diff --git a/tests/specs/workspaces/workspace_member_bin_dep_claim/package.json b/tests/specs/workspaces/workspace_member_bin_dep_claim/package.json new file mode 100644 index 00000000000000..8c9ebd3bbdb7fb --- /dev/null +++ b/tests/specs/workspaces/workspace_member_bin_dep_claim/package.json @@ -0,0 +1,7 @@ +{ + "name": "root", + "version": "1.0.0", + "dependencies": { + "@denotest/one-bin": "1.0.0" + } +} diff --git a/tests/specs/workspaces/workspace_member_bin_dep_claim/packages/thing/cli.js b/tests/specs/workspaces/workspace_member_bin_dep_claim/packages/thing/cli.js new file mode 100644 index 00000000000000..a465d976bf0511 --- /dev/null +++ b/tests/specs/workspaces/workspace_member_bin_dep_claim/packages/thing/cli.js @@ -0,0 +1,2 @@ +#!/usr/bin/env node +console.log("workspace member won the collision"); diff --git a/tests/specs/workspaces/workspace_member_bin_dep_claim/packages/thing/package.json b/tests/specs/workspaces/workspace_member_bin_dep_claim/packages/thing/package.json new file mode 100644 index 00000000000000..2fc0cc30fef1f0 --- /dev/null +++ b/tests/specs/workspaces/workspace_member_bin_dep_claim/packages/thing/package.json @@ -0,0 +1,8 @@ +{ + "name": "thing", + "version": "1.0.0", + "type": "module", + "bin": { + "thing-bin": "./cli.js" + } +} diff --git a/tests/specs/workspaces/workspace_member_bin_dep_claim/task.out b/tests/specs/workspaces/workspace_member_bin_dep_claim/task.out new file mode 100644 index 00000000000000..dc1349884ec507 --- /dev/null +++ b/tests/specs/workspaces/workspace_member_bin_dep_claim/task.out @@ -0,0 +1,2 @@ +Task run-bin thing-bin +thing-bin diff --git a/tests/specs/workspaces/workspace_member_bin_duplicate/__test__.jsonc b/tests/specs/workspaces/workspace_member_bin_duplicate/__test__.jsonc new file mode 100644 index 00000000000000..4c2d422161195e --- /dev/null +++ b/tests/specs/workspaces/workspace_member_bin_duplicate/__test__.jsonc @@ -0,0 +1,25 @@ +// Two workspace members declaring the same `bin` name. Only one of them can +// own the root `node_modules/.bin` entry and which one is essentially +// arbitrary (the greatest `@` wins the depth sort's tiebreak), +// so `deno install` warns instead of resolving it silently. npm hard-errors +// here; a warning keeps an otherwise fine workspace installable. +// +// No network access is required — the workspace contains only local members. +// +// https://github.com/denoland/deno/issues/36313 +{ + "tempDir": true, + "steps": [ + { + "args": "install", + "output": "[WILDCARD]Warning Multiple workspace members declare a \"dup\" bin: dup-a, dup-b. Only \"dup-b\" will be linked into node_modules/.bin.[WILDCARD]" + }, + { + // `deno task` re-links `node_modules` too, but the warning is gated on + // the install path — the absence of a leading `[WILDCARD]` here is what + // asserts it isn't re-printed ahead of every task's output. + "args": "task dup", + "output": "task.out" + } + ] +} diff --git a/tests/specs/workspaces/workspace_member_bin_duplicate/deno.json b/tests/specs/workspaces/workspace_member_bin_duplicate/deno.json new file mode 100644 index 00000000000000..b7a486c44b4bde --- /dev/null +++ b/tests/specs/workspaces/workspace_member_bin_duplicate/deno.json @@ -0,0 +1,10 @@ +{ + "nodeModulesDir": "auto", + "workspace": [ + "./packages/dup-a", + "./packages/dup-b" + ], + "tasks": { + "dup": "dup" + } +} diff --git a/tests/specs/workspaces/workspace_member_bin_duplicate/packages/dup-a/cli.js b/tests/specs/workspaces/workspace_member_bin_duplicate/packages/dup-a/cli.js new file mode 100644 index 00000000000000..d7510c1091f571 --- /dev/null +++ b/tests/specs/workspaces/workspace_member_bin_duplicate/packages/dup-a/cli.js @@ -0,0 +1,2 @@ +#!/usr/bin/env node +console.log("dup-a"); diff --git a/tests/specs/workspaces/workspace_member_bin_duplicate/packages/dup-a/package.json b/tests/specs/workspaces/workspace_member_bin_duplicate/packages/dup-a/package.json new file mode 100644 index 00000000000000..977e8894e68cf5 --- /dev/null +++ b/tests/specs/workspaces/workspace_member_bin_duplicate/packages/dup-a/package.json @@ -0,0 +1,8 @@ +{ + "name": "dup-a", + "version": "1.0.0", + "type": "module", + "bin": { + "dup": "./cli.js" + } +} diff --git a/tests/specs/workspaces/workspace_member_bin_duplicate/packages/dup-b/cli.js b/tests/specs/workspaces/workspace_member_bin_duplicate/packages/dup-b/cli.js new file mode 100644 index 00000000000000..9ac1758a982147 --- /dev/null +++ b/tests/specs/workspaces/workspace_member_bin_duplicate/packages/dup-b/cli.js @@ -0,0 +1,2 @@ +#!/usr/bin/env node +console.log("dup-b"); diff --git a/tests/specs/workspaces/workspace_member_bin_duplicate/packages/dup-b/package.json b/tests/specs/workspaces/workspace_member_bin_duplicate/packages/dup-b/package.json new file mode 100644 index 00000000000000..daa3073e937123 --- /dev/null +++ b/tests/specs/workspaces/workspace_member_bin_duplicate/packages/dup-b/package.json @@ -0,0 +1,8 @@ +{ + "name": "dup-b", + "version": "1.0.0", + "type": "module", + "bin": { + "dup": "./cli.js" + } +} diff --git a/tests/specs/workspaces/workspace_member_bin_duplicate/task.out b/tests/specs/workspaces/workspace_member_bin_duplicate/task.out new file mode 100644 index 00000000000000..f9bc7f6793fc44 --- /dev/null +++ b/tests/specs/workspaces/workspace_member_bin_duplicate/task.out @@ -0,0 +1,2 @@ +Task dup dup +dup-b diff --git a/tests/specs/workspaces/workspace_member_bin_hoisted/__test__.jsonc b/tests/specs/workspaces/workspace_member_bin_hoisted/__test__.jsonc new file mode 100644 index 00000000000000..1b06d99b6355aa --- /dev/null +++ b/tests/specs/workspaces/workspace_member_bin_hoisted/__test__.jsonc @@ -0,0 +1,41 @@ +// The hoisted-linker counterpart of the `workspace_member_bin` spec: a local +// workspace member that declares a `bin` in its package.json must be linked +// into `node_modules/.bin` (the workspace root's and the depending member's) +// and be invocable from `deno task`. The linker is selected through the +// deno.json `nodeModulesLinker` option so every nested `deno` invocation uses +// it too. No network access is required — the workspace contains only local +// members. +// +// The `shtool` member (a `#!/bin/sh` bin) is here so `check_bin.ts` can assert +// the hoisted linker links non-JavaScript executables too, but it is NOT run as +// a task: `nodeModulesDir: "manual"` means `deno task` resolves through the +// BYONM npm resolver, which routes *every* `.bin` entry through `deno run`. The +// isolated `workspace_member_bin` spec covers running a shell-script bin from a +// task. +// +// https://github.com/denoland/deno/issues/36313 +{ + "tempDir": true, + "steps": [ + { + "args": "install", + "output": "[WILDCARD]" + }, + { + "args": "run --allow-read check_bin.ts", + "output": "ok\n" + }, + { + // "Hello from deno" (rather than "node") asserts the task runner resolves + // the member's JS bin to a `deno run` command instead of leaving it to a + // `PATH` lookup, which would execute the `#!/usr/bin/env node` shebang + // with whatever Node happens to be installed — or fail outright. + "args": "task --cwd apps/web cli", + "output": "member_task.out" + }, + { + "args": "task root-cli", + "output": "root_task.out" + } + ] +} diff --git a/tests/specs/workspaces/workspace_member_bin_hoisted/apps/web/package.json b/tests/specs/workspaces/workspace_member_bin_hoisted/apps/web/package.json new file mode 100644 index 00000000000000..647b10ac22b4d6 --- /dev/null +++ b/tests/specs/workspaces/workspace_member_bin_hoisted/apps/web/package.json @@ -0,0 +1,12 @@ +{ + "name": "web", + "version": "1.0.0", + "private": true, + "type": "module", + "scripts": { + "cli": "local-cli" + }, + "dependencies": { + "local-cli": "workspace:*" + } +} diff --git a/tests/specs/workspaces/workspace_member_bin_hoisted/check_bin.ts b/tests/specs/workspaces/workspace_member_bin_hoisted/check_bin.ts new file mode 100644 index 00000000000000..90e1f21fb73df9 --- /dev/null +++ b/tests/specs/workspaces/workspace_member_bin_hoisted/check_bin.ts @@ -0,0 +1,73 @@ +// Verifies that a local workspace member which declares a `bin` in its +// package.json gets a `node_modules/.bin` entry, both in the workspace root +// and in a sibling member that depends on it. Previously only external npm +// dependencies got these, so `deno task` could not invoke a local member's +// executable. https://github.com/denoland/deno/issues/36313 + +/** The files a `node_modules/.bin` entry points at. On unix the entry is a + * symlink into the member, so the entry path itself resolves. On Windows it is + * a generated shim script that embeds `$basedir`-relative paths (one for the + * interpreter, one for the script), so pull those out of the text. */ +function binTargets(binPath: string): string[] { + if (Deno.build.os !== "windows") { + // touch the entry so a missing one reports as NotFound here + Deno.lstatSync(binPath); + return [binPath]; + } + const dir = binPath.slice(0, binPath.lastIndexOf("/")); + const text = Deno.readTextFileSync(binPath); + return [...text.matchAll(/\$basedir\/([^"]+)/g)].map((m) => `${dir}/${m[1]}`); +} + +/** Asserts that the `.bin` entry actually resolves to `expectedFile`. Merely + * existing isn't enough: a shim built from the wrong package path would look + * identical on disk. */ +function assertBinTarget(binPath: string, expectedFile: string) { + let expected: string; + try { + expected = Deno.realPathSync(expectedFile); + } catch { + throw new Error(`expected bin script ${expectedFile} to exist`); + } + let candidates: string[]; + try { + candidates = binTargets(binPath); + } catch (err) { + if (err instanceof Deno.errors.NotFound) { + throw new Error(`expected ${binPath} to exist`); + } + throw err; + } + const resolved = candidates.map((path) => { + try { + return Deno.realPathSync(path); + } catch { + return null; + } + }); + if (!resolved.includes(expected)) { + throw new Error( + `expected ${binPath} to point at ${expected}, but it resolved to ${ + JSON.stringify(resolved) + }`, + ); + } +} + +// (a) The root `node_modules/.bin` has the member's executable, so tooling +// (and a root task) can run it. +assertBinTarget("node_modules/.bin/local-cli", "packages/local-cli/cli.js"); + +// (b) The depending member's own `node_modules/.bin` has it too, mirroring how +// npm and pnpm lay out workspaces. +assertBinTarget( + "apps/web/node_modules/.bin/local-cli", + "packages/local-cli/cli.js", +); + +// (c) A member whose bin is not a JavaScript file gets an entry all the same — +// it just has to keep resolving through `PATH` instead of being handed to +// `deno run`. +assertBinTarget("node_modules/.bin/shtool", "packages/shtool/tool.sh"); + +console.log("ok"); diff --git a/tests/specs/workspaces/workspace_member_bin_hoisted/deno.json b/tests/specs/workspaces/workspace_member_bin_hoisted/deno.json new file mode 100644 index 00000000000000..b94f004cd4b2f5 --- /dev/null +++ b/tests/specs/workspaces/workspace_member_bin_hoisted/deno.json @@ -0,0 +1,13 @@ +{ + // the hoisted linker requires the "manual" node_modules dir + "nodeModulesDir": "manual", + "nodeModulesLinker": "hoisted", + "workspace": [ + "./packages/local-cli", + "./packages/shtool", + "./apps/web" + ], + "tasks": { + "root-cli": "local-cli" + } +} diff --git a/tests/specs/workspaces/workspace_member_bin_hoisted/member_task.out b/tests/specs/workspaces/workspace_member_bin_hoisted/member_task.out new file mode 100644 index 00000000000000..d7e895418a0add --- /dev/null +++ b/tests/specs/workspaces/workspace_member_bin_hoisted/member_task.out @@ -0,0 +1,2 @@ +Task cli local-cli +Hello from deno diff --git a/tests/specs/workspaces/workspace_member_bin_hoisted/packages/local-cli/cli.js b/tests/specs/workspaces/workspace_member_bin_hoisted/packages/local-cli/cli.js new file mode 100644 index 00000000000000..ffefac84caa93a --- /dev/null +++ b/tests/specs/workspaces/workspace_member_bin_hoisted/packages/local-cli/cli.js @@ -0,0 +1,5 @@ +#!/usr/bin/env node +// `deno task` runs this itself (`deno run --ext=js`) rather than handing it to +// whichever `node` happens to be on `PATH`, so a workspace member's executable +// works without Node installed. Print the runtime to lock that in. +console.log("Hello from", typeof Deno === "undefined" ? "node" : "deno"); diff --git a/tests/specs/workspaces/workspace_member_bin_hoisted/packages/local-cli/package.json b/tests/specs/workspaces/workspace_member_bin_hoisted/packages/local-cli/package.json new file mode 100644 index 00000000000000..0d25e6cbc6d7f0 --- /dev/null +++ b/tests/specs/workspaces/workspace_member_bin_hoisted/packages/local-cli/package.json @@ -0,0 +1,8 @@ +{ + "name": "local-cli", + "version": "1.0.0", + "type": "module", + "bin": { + "local-cli": "./cli.js" + } +} diff --git a/tests/specs/workspaces/workspace_member_bin_hoisted/packages/shtool/package.json b/tests/specs/workspaces/workspace_member_bin_hoisted/packages/shtool/package.json new file mode 100644 index 00000000000000..04998bee01e1c2 --- /dev/null +++ b/tests/specs/workspaces/workspace_member_bin_hoisted/packages/shtool/package.json @@ -0,0 +1,7 @@ +{ + "name": "shtool", + "version": "1.0.0", + "bin": { + "shtool": "./tool.sh" + } +} diff --git a/tests/specs/workspaces/workspace_member_bin_hoisted/packages/shtool/tool.sh b/tests/specs/workspaces/workspace_member_bin_hoisted/packages/shtool/tool.sh new file mode 100755 index 00000000000000..9d62b5e514fba1 --- /dev/null +++ b/tests/specs/workspaces/workspace_member_bin_hoisted/packages/shtool/tool.sh @@ -0,0 +1,2 @@ +#!/bin/sh +echo "shtool ran: $*" diff --git a/tests/specs/workspaces/workspace_member_bin_hoisted/root_task.out b/tests/specs/workspaces/workspace_member_bin_hoisted/root_task.out new file mode 100644 index 00000000000000..30adccc4ea0b90 --- /dev/null +++ b/tests/specs/workspaces/workspace_member_bin_hoisted/root_task.out @@ -0,0 +1,2 @@ +Task root-cli local-cli +Hello from deno diff --git a/tests/specs/workspaces/workspace_member_bin_prune/__test__.jsonc b/tests/specs/workspaces/workspace_member_bin_prune/__test__.jsonc new file mode 100644 index 00000000000000..9de8177dce3794 --- /dev/null +++ b/tests/specs/workspaces/workspace_member_bin_prune/__test__.jsonc @@ -0,0 +1,45 @@ +// Re-running `deno install` after a workspace member renames its `bin`, drops +// its `bin`, or leaves the workspace entirely must prune the stale root +// `node_modules/.bin` entry. Change detection used to hash only the npm +// resolution snapshot, so none of these ever tripped the cleanup and the old +// entry survived — as a dangling symlink, in the last case. +// +// This covers the default isolated (`.deno`) linker; the hoisted linker is +// covered by the sibling `workspace_member_bin_prune_hoisted` spec. No network +// access is required — the workspace contains only local members. +// +// https://github.com/denoland/deno/issues/36313 +{ + "tempDir": true, + "steps": [ + { "args": "install", "output": "[WILDCARD]" }, + { + "args": "run --allow-read check.ts toolname:exists othername:exists", + "output": "ok\n" + }, + { + "args": "run --allow-read --allow-write edit.ts rename-bin", + "output": "edited\n" + }, + { "args": "install", "output": "[WILDCARD]" }, + { + "args": "run --allow-read check.ts renamed:exists toolname:missing othername:exists", + "output": "ok\n" + }, + { + "args": "run --allow-read --allow-write edit.ts remove-bin", + "output": "edited\n" + }, + { "args": "install", "output": "[WILDCARD]" }, + { + "args": "run --allow-read check.ts renamed:missing othername:exists", + "output": "ok\n" + }, + { "args": "run -A edit.ts remove-member", "output": "edited\n" }, + { "args": "install", "output": "[WILDCARD]" }, + { + "args": "run --allow-read check.ts othername:missing", + "output": "ok\n" + } + ] +} diff --git a/tests/specs/workspaces/workspace_member_bin_prune/check.ts b/tests/specs/workspaces/workspace_member_bin_prune/check.ts new file mode 100644 index 00000000000000..706313cbed51e0 --- /dev/null +++ b/tests/specs/workspaces/workspace_member_bin_prune/check.ts @@ -0,0 +1,46 @@ +// Asserts the presence/absence of root `node_modules/.bin` entries. +// +// Usage: check.ts :exists :missing ... +// +// `missing` uses `lstat`, not `stat`, so a dangling symlink left behind by a +// deleted workspace member counts as present (which is exactly the state that +// went unnoticed before the workspace members' bins were folded into the +// install's change-detection hash). + +for (const arg of Deno.args) { + const [name, expectation] = arg.split(":"); + const path = `node_modules/.bin/${name}`; + let exists = true; + try { + Deno.lstatSync(path); + } catch (err) { + if (!(err instanceof Deno.errors.NotFound)) { + throw err; + } + exists = false; + } + switch (expectation) { + case "exists": + if (!exists) { + throw new Error(`expected ${path} to exist`); + } + break; + case "missing": + if (exists) { + let target: string; + try { + target = Deno.readLinkSync(path); + } catch { + target = ""; + } + throw new Error( + `expected ${path} to have been pruned (target: ${target})`, + ); + } + break; + default: + throw new Error(`unknown expectation: ${arg}`); + } +} + +console.log("ok"); diff --git a/tests/specs/workspaces/workspace_member_bin_prune/deno.json b/tests/specs/workspaces/workspace_member_bin_prune/deno.json new file mode 100644 index 00000000000000..234428c8ff6cda --- /dev/null +++ b/tests/specs/workspaces/workspace_member_bin_prune/deno.json @@ -0,0 +1,7 @@ +{ + "nodeModulesDir": "auto", + "workspace": [ + "./packages/tool", + "./packages/other" + ] +} diff --git a/tests/specs/workspaces/workspace_member_bin_prune/edit.ts b/tests/specs/workspaces/workspace_member_bin_prune/edit.ts new file mode 100644 index 00000000000000..bab75e38136782 --- /dev/null +++ b/tests/specs/workspaces/workspace_member_bin_prune/edit.ts @@ -0,0 +1,37 @@ +// Mutates the workspace between `deno install` runs so the spec can assert +// that the previous state's `node_modules/.bin` entries are pruned. + +function editJson(path: string, edit: (json: any) => void) { + const json = JSON.parse(Deno.readTextFileSync(path)); + edit(json); + Deno.writeTextFileSync(path, `${JSON.stringify(json, null, 2)}\n`); +} + +switch (Deno.args[0]) { + case "rename-bin": + // `tool` renames its executable: `.bin/toolname` must not survive. + editJson("packages/tool/package.json", (json) => { + json.bin = { renamed: "./cli.js" }; + }); + break; + case "remove-bin": + // `tool` drops its `bin` field entirely. + editJson("packages/tool/package.json", (json) => { + delete json.bin; + }); + break; + case "remove-member": + // `other` leaves the workspace: its entry would otherwise be left behind + // as a dangling symlink. + editJson("deno.json", (json) => { + json.workspace = json.workspace.filter((m: string) => + m !== "./packages/other" + ); + }); + Deno.removeSync("packages/other", { recursive: true }); + break; + default: + throw new Error(`unknown edit: ${Deno.args[0]}`); +} + +console.log("edited"); diff --git a/tests/specs/workspaces/workspace_member_bin_prune/packages/other/cli.js b/tests/specs/workspaces/workspace_member_bin_prune/packages/other/cli.js new file mode 100644 index 00000000000000..8202d675577a9a --- /dev/null +++ b/tests/specs/workspaces/workspace_member_bin_prune/packages/other/cli.js @@ -0,0 +1,2 @@ +#!/usr/bin/env node +console.log("other"); diff --git a/tests/specs/workspaces/workspace_member_bin_prune/packages/other/package.json b/tests/specs/workspaces/workspace_member_bin_prune/packages/other/package.json new file mode 100644 index 00000000000000..21a8f5c77209aa --- /dev/null +++ b/tests/specs/workspaces/workspace_member_bin_prune/packages/other/package.json @@ -0,0 +1,8 @@ +{ + "name": "other", + "version": "1.0.0", + "type": "module", + "bin": { + "othername": "./cli.js" + } +} diff --git a/tests/specs/workspaces/workspace_member_bin_prune/packages/tool/cli.js b/tests/specs/workspaces/workspace_member_bin_prune/packages/tool/cli.js new file mode 100644 index 00000000000000..c30d62173c5771 --- /dev/null +++ b/tests/specs/workspaces/workspace_member_bin_prune/packages/tool/cli.js @@ -0,0 +1,2 @@ +#!/usr/bin/env node +console.log("tool"); diff --git a/tests/specs/workspaces/workspace_member_bin_prune/packages/tool/package.json b/tests/specs/workspaces/workspace_member_bin_prune/packages/tool/package.json new file mode 100644 index 00000000000000..bb842141dc70e9 --- /dev/null +++ b/tests/specs/workspaces/workspace_member_bin_prune/packages/tool/package.json @@ -0,0 +1,8 @@ +{ + "name": "tool", + "version": "1.0.0", + "type": "module", + "bin": { + "toolname": "./cli.js" + } +} diff --git a/tests/specs/workspaces/workspace_member_bin_prune_hoisted/__test__.jsonc b/tests/specs/workspaces/workspace_member_bin_prune_hoisted/__test__.jsonc new file mode 100644 index 00000000000000..6b261622598286 --- /dev/null +++ b/tests/specs/workspaces/workspace_member_bin_prune_hoisted/__test__.jsonc @@ -0,0 +1,41 @@ +// The hoisted-linker counterpart of the `workspace_member_bin_prune` spec: +// re-running `deno install` after a workspace member renames its `bin`, drops +// its `bin`, or leaves the workspace entirely must prune the stale root +// `node_modules/.bin` entry. The hoisted cleanup skipped every dot-prefixed +// directory, so it never touched `.bin` at all. +// +// https://github.com/denoland/deno/issues/36313 +{ + "tempDir": true, + "steps": [ + { "args": "install", "output": "[WILDCARD]" }, + { + "args": "run --allow-read check.ts toolname:exists othername:exists", + "output": "ok\n" + }, + { + "args": "run --allow-read --allow-write edit.ts rename-bin", + "output": "edited\n" + }, + { "args": "install", "output": "[WILDCARD]" }, + { + "args": "run --allow-read check.ts renamed:exists toolname:missing othername:exists", + "output": "ok\n" + }, + { + "args": "run --allow-read --allow-write edit.ts remove-bin", + "output": "edited\n" + }, + { "args": "install", "output": "[WILDCARD]" }, + { + "args": "run --allow-read check.ts renamed:missing othername:exists", + "output": "ok\n" + }, + { "args": "run -A edit.ts remove-member", "output": "edited\n" }, + { "args": "install", "output": "[WILDCARD]" }, + { + "args": "run --allow-read check.ts othername:missing", + "output": "ok\n" + } + ] +} diff --git a/tests/specs/workspaces/workspace_member_bin_prune_hoisted/check.ts b/tests/specs/workspaces/workspace_member_bin_prune_hoisted/check.ts new file mode 100644 index 00000000000000..706313cbed51e0 --- /dev/null +++ b/tests/specs/workspaces/workspace_member_bin_prune_hoisted/check.ts @@ -0,0 +1,46 @@ +// Asserts the presence/absence of root `node_modules/.bin` entries. +// +// Usage: check.ts :exists :missing ... +// +// `missing` uses `lstat`, not `stat`, so a dangling symlink left behind by a +// deleted workspace member counts as present (which is exactly the state that +// went unnoticed before the workspace members' bins were folded into the +// install's change-detection hash). + +for (const arg of Deno.args) { + const [name, expectation] = arg.split(":"); + const path = `node_modules/.bin/${name}`; + let exists = true; + try { + Deno.lstatSync(path); + } catch (err) { + if (!(err instanceof Deno.errors.NotFound)) { + throw err; + } + exists = false; + } + switch (expectation) { + case "exists": + if (!exists) { + throw new Error(`expected ${path} to exist`); + } + break; + case "missing": + if (exists) { + let target: string; + try { + target = Deno.readLinkSync(path); + } catch { + target = ""; + } + throw new Error( + `expected ${path} to have been pruned (target: ${target})`, + ); + } + break; + default: + throw new Error(`unknown expectation: ${arg}`); + } +} + +console.log("ok"); diff --git a/tests/specs/workspaces/workspace_member_bin_prune_hoisted/deno.json b/tests/specs/workspaces/workspace_member_bin_prune_hoisted/deno.json new file mode 100644 index 00000000000000..344cab1948ce8e --- /dev/null +++ b/tests/specs/workspaces/workspace_member_bin_prune_hoisted/deno.json @@ -0,0 +1,8 @@ +{ + "nodeModulesDir": "manual", + "nodeModulesLinker": "hoisted", + "workspace": [ + "./packages/tool", + "./packages/other" + ] +} diff --git a/tests/specs/workspaces/workspace_member_bin_prune_hoisted/edit.ts b/tests/specs/workspaces/workspace_member_bin_prune_hoisted/edit.ts new file mode 100644 index 00000000000000..bab75e38136782 --- /dev/null +++ b/tests/specs/workspaces/workspace_member_bin_prune_hoisted/edit.ts @@ -0,0 +1,37 @@ +// Mutates the workspace between `deno install` runs so the spec can assert +// that the previous state's `node_modules/.bin` entries are pruned. + +function editJson(path: string, edit: (json: any) => void) { + const json = JSON.parse(Deno.readTextFileSync(path)); + edit(json); + Deno.writeTextFileSync(path, `${JSON.stringify(json, null, 2)}\n`); +} + +switch (Deno.args[0]) { + case "rename-bin": + // `tool` renames its executable: `.bin/toolname` must not survive. + editJson("packages/tool/package.json", (json) => { + json.bin = { renamed: "./cli.js" }; + }); + break; + case "remove-bin": + // `tool` drops its `bin` field entirely. + editJson("packages/tool/package.json", (json) => { + delete json.bin; + }); + break; + case "remove-member": + // `other` leaves the workspace: its entry would otherwise be left behind + // as a dangling symlink. + editJson("deno.json", (json) => { + json.workspace = json.workspace.filter((m: string) => + m !== "./packages/other" + ); + }); + Deno.removeSync("packages/other", { recursive: true }); + break; + default: + throw new Error(`unknown edit: ${Deno.args[0]}`); +} + +console.log("edited"); diff --git a/tests/specs/workspaces/workspace_member_bin_prune_hoisted/packages/other/cli.js b/tests/specs/workspaces/workspace_member_bin_prune_hoisted/packages/other/cli.js new file mode 100644 index 00000000000000..8202d675577a9a --- /dev/null +++ b/tests/specs/workspaces/workspace_member_bin_prune_hoisted/packages/other/cli.js @@ -0,0 +1,2 @@ +#!/usr/bin/env node +console.log("other"); diff --git a/tests/specs/workspaces/workspace_member_bin_prune_hoisted/packages/other/package.json b/tests/specs/workspaces/workspace_member_bin_prune_hoisted/packages/other/package.json new file mode 100644 index 00000000000000..21a8f5c77209aa --- /dev/null +++ b/tests/specs/workspaces/workspace_member_bin_prune_hoisted/packages/other/package.json @@ -0,0 +1,8 @@ +{ + "name": "other", + "version": "1.0.0", + "type": "module", + "bin": { + "othername": "./cli.js" + } +} diff --git a/tests/specs/workspaces/workspace_member_bin_prune_hoisted/packages/tool/cli.js b/tests/specs/workspaces/workspace_member_bin_prune_hoisted/packages/tool/cli.js new file mode 100644 index 00000000000000..c30d62173c5771 --- /dev/null +++ b/tests/specs/workspaces/workspace_member_bin_prune_hoisted/packages/tool/cli.js @@ -0,0 +1,2 @@ +#!/usr/bin/env node +console.log("tool"); diff --git a/tests/specs/workspaces/workspace_member_bin_prune_hoisted/packages/tool/package.json b/tests/specs/workspaces/workspace_member_bin_prune_hoisted/packages/tool/package.json new file mode 100644 index 00000000000000..bb842141dc70e9 --- /dev/null +++ b/tests/specs/workspaces/workspace_member_bin_prune_hoisted/packages/tool/package.json @@ -0,0 +1,8 @@ +{ + "name": "tool", + "version": "1.0.0", + "type": "module", + "bin": { + "toolname": "./cli.js" + } +} diff --git a/tests/specs/workspaces/workspace_member_bin_shadow/__test__.jsonc b/tests/specs/workspaces/workspace_member_bin_shadow/__test__.jsonc new file mode 100644 index 00000000000000..1bc998d6520e73 --- /dev/null +++ b/tests/specs/workspaces/workspace_member_bin_shadow/__test__.jsonc @@ -0,0 +1,37 @@ +// Closest-first precedence between `node_modules/.bin` directories when the +// closest entry is NOT JavaScript. +// +// Two workspace members declare a `foo` bin: `a-sh` points at a `#!/bin/sh` +// script and `z-js` at a JS file. `z-js` wins the root `.bin`, while +// `apps/web` — which only depends on `a-sh` — gets the shell script in its own +// `.bin`. A task run from `apps/web` must therefore run the shell script, the +// same thing a plain `PATH` lookup with those bin dirs prepended would find. +// +// The managed task runner only merges `JsFile` entries as custom commands and +// leaves executables to `PATH`; skipping the closer `a-sh` entry without +// recording its *name* let the farther `z-js` entry become a custom command, +// and custom commands beat `PATH` in `deno_task_shell` — inverting closest +// first. https://github.com/denoland/deno/issues/36313 +{ + "tempDir": true, + // The shell-script bin has no meaning on Windows. + "if": "unix", + "steps": [ + { + "args": "install", + "output": "[WILDCARD]" + }, + { + "args": "run --allow-read check_bin.ts", + "output": "ok\n" + }, + { + "args": "task --cwd apps/web foo", + "output": "member_task.out" + }, + { + "args": "task root-foo", + "output": "root_task.out" + } + ] +} diff --git a/tests/specs/workspaces/workspace_member_bin_shadow/apps/web/package.json b/tests/specs/workspaces/workspace_member_bin_shadow/apps/web/package.json new file mode 100644 index 00000000000000..7eb8a1082877a9 --- /dev/null +++ b/tests/specs/workspaces/workspace_member_bin_shadow/apps/web/package.json @@ -0,0 +1,12 @@ +{ + "name": "web", + "version": "1.0.0", + "private": true, + "type": "module", + "scripts": { + "foo": "foo" + }, + "dependencies": { + "a-sh": "workspace:*" + } +} diff --git a/tests/specs/workspaces/workspace_member_bin_shadow/check_bin.ts b/tests/specs/workspaces/workspace_member_bin_shadow/check_bin.ts new file mode 100644 index 00000000000000..9022036d5d8a71 --- /dev/null +++ b/tests/specs/workspaces/workspace_member_bin_shadow/check_bin.ts @@ -0,0 +1,26 @@ +// Sanity check for the fixture the task steps rely on: the two `foo` entries +// must actually point at different members, otherwise the precedence +// assertions below would pass for the wrong reason. + +function assertBinTarget(binPath: string, expectedFile: string) { + const expected = Deno.realPathSync(expectedFile); + let resolved: string; + try { + resolved = Deno.realPathSync(binPath); + } catch { + throw new Error(`expected ${binPath} to exist`); + } + if (resolved !== expected) { + throw new Error( + `expected ${binPath} to point at ${expected}, but got ${resolved}`, + ); + } +} + +// `z-js` wins the root entry: both members sort to depth `u64::MAX` and the +// tiebreak is a descending `@` compare. +assertBinTarget("node_modules/.bin/foo", "packages/z-js/cli.js"); +// `apps/web` only depends on `a-sh`, so its own `.bin` has the shell script. +assertBinTarget("apps/web/node_modules/.bin/foo", "packages/a-sh/tool.sh"); + +console.log("ok"); diff --git a/tests/specs/workspaces/workspace_member_bin_shadow/deno.json b/tests/specs/workspaces/workspace_member_bin_shadow/deno.json new file mode 100644 index 00000000000000..2f09d4ba37c94a --- /dev/null +++ b/tests/specs/workspaces/workspace_member_bin_shadow/deno.json @@ -0,0 +1,11 @@ +{ + "nodeModulesDir": "auto", + "workspace": [ + "./packages/a-sh", + "./packages/z-js", + "./apps/web" + ], + "tasks": { + "root-foo": "foo" + } +} diff --git a/tests/specs/workspaces/workspace_member_bin_shadow/member_task.out b/tests/specs/workspaces/workspace_member_bin_shadow/member_task.out new file mode 100644 index 00000000000000..64885b0b9b5b2e --- /dev/null +++ b/tests/specs/workspaces/workspace_member_bin_shadow/member_task.out @@ -0,0 +1,2 @@ +Task foo foo +SH foo ran diff --git a/tests/specs/workspaces/workspace_member_bin_shadow/packages/a-sh/package.json b/tests/specs/workspaces/workspace_member_bin_shadow/packages/a-sh/package.json new file mode 100644 index 00000000000000..6fd60b59806adf --- /dev/null +++ b/tests/specs/workspaces/workspace_member_bin_shadow/packages/a-sh/package.json @@ -0,0 +1,7 @@ +{ + "name": "a-sh", + "version": "1.0.0", + "bin": { + "foo": "./tool.sh" + } +} diff --git a/tests/specs/workspaces/workspace_member_bin_shadow/packages/a-sh/tool.sh b/tests/specs/workspaces/workspace_member_bin_shadow/packages/a-sh/tool.sh new file mode 100755 index 00000000000000..1c7e7cc8191182 --- /dev/null +++ b/tests/specs/workspaces/workspace_member_bin_shadow/packages/a-sh/tool.sh @@ -0,0 +1,2 @@ +#!/bin/sh +echo "SH foo ran" diff --git a/tests/specs/workspaces/workspace_member_bin_shadow/packages/z-js/cli.js b/tests/specs/workspaces/workspace_member_bin_shadow/packages/z-js/cli.js new file mode 100644 index 00000000000000..e25aec2dc2ef99 --- /dev/null +++ b/tests/specs/workspaces/workspace_member_bin_shadow/packages/z-js/cli.js @@ -0,0 +1,2 @@ +#!/usr/bin/env node +console.log("JS foo ran"); diff --git a/tests/specs/workspaces/workspace_member_bin_shadow/packages/z-js/package.json b/tests/specs/workspaces/workspace_member_bin_shadow/packages/z-js/package.json new file mode 100644 index 00000000000000..10acb149cc6c51 --- /dev/null +++ b/tests/specs/workspaces/workspace_member_bin_shadow/packages/z-js/package.json @@ -0,0 +1,8 @@ +{ + "name": "z-js", + "version": "1.0.0", + "type": "module", + "bin": { + "foo": "./cli.js" + } +} diff --git a/tests/specs/workspaces/workspace_member_bin_shadow/root_task.out b/tests/specs/workspaces/workspace_member_bin_shadow/root_task.out new file mode 100644 index 00000000000000..9ecb48b9c2c245 --- /dev/null +++ b/tests/specs/workspaces/workspace_member_bin_shadow/root_task.out @@ -0,0 +1,2 @@ +Task root-foo foo +JS foo ran diff --git a/tests/specs/workspaces/workspace_member_bin_string/__test__.jsonc b/tests/specs/workspaces/workspace_member_bin_string/__test__.jsonc new file mode 100644 index 00000000000000..a2fdc194761b84 --- /dev/null +++ b/tests/specs/workspaces/workspace_member_bin_string/__test__.jsonc @@ -0,0 +1,29 @@ +// End-to-end coverage of the *string* form of a workspace member's `bin` +// (`"bin": "./cli.js"` rather than a map). The entry name comes from the +// package name, with the scope stripped for a scoped package, so +// `@scope/tool` must be linked as `node_modules/.bin/tool`. +// +// No network access is required — the workspace contains only local members. +// +// https://github.com/denoland/deno/issues/36313 +{ + "tempDir": true, + "steps": [ + { + "args": "install", + "output": "[WILDCARD]" + }, + { + "args": "run --allow-read check_bin.ts", + "output": "ok\n" + }, + { + "args": "task scoped", + "output": "scoped_task.out" + }, + { + "args": "task plain", + "output": "plain_task.out" + } + ] +} diff --git a/tests/specs/workspaces/workspace_member_bin_string/check_bin.ts b/tests/specs/workspaces/workspace_member_bin_string/check_bin.ts new file mode 100644 index 00000000000000..d185f05ba35f49 --- /dev/null +++ b/tests/specs/workspaces/workspace_member_bin_string/check_bin.ts @@ -0,0 +1,64 @@ +// Verifies the string form of a workspace member's `bin` +// (`"bin": "./cli.js"`), which takes its entry name from the package name — +// and for a scoped package that means the name *without* the scope. + +/** See `workspace_member_bin/check_bin.ts` — on Windows the entry is a shim + * file that embeds `$basedir`-relative paths rather than a symlink. */ +function binTargets(binPath: string): string[] { + if (Deno.build.os !== "windows") { + Deno.lstatSync(binPath); + return [binPath]; + } + const dir = binPath.slice(0, binPath.lastIndexOf("/")); + const text = Deno.readTextFileSync(binPath); + return [...text.matchAll(/\$basedir\/([^"]+)/g)].map((m) => `${dir}/${m[1]}`); +} + +function assertBinTarget(binPath: string, expectedFile: string) { + const expected = Deno.realPathSync(expectedFile); + let candidates: string[]; + try { + candidates = binTargets(binPath); + } catch (err) { + if (err instanceof Deno.errors.NotFound) { + throw new Error(`expected ${binPath} to exist`); + } + throw err; + } + const resolved = candidates.map((path) => { + try { + return Deno.realPathSync(path); + } catch { + return null; + } + }); + if (!resolved.includes(expected)) { + throw new Error( + `expected ${binPath} to point at ${expected}, but it resolved to ${ + JSON.stringify(resolved) + }`, + ); + } +} + +function assertMissing(path: string) { + try { + Deno.lstatSync(path); + } catch (err) { + if (err instanceof Deno.errors.NotFound) { + return; + } + throw err; + } + throw new Error(`expected ${path} not to exist`); +} + +// `@scope/tool` -> `.bin/tool`: the scope is dropped, and no `@scope` +// directory is created inside `.bin`. +assertBinTarget("node_modules/.bin/tool", "packages/scoped-tool/cli.js"); +assertMissing("node_modules/.bin/@scope"); + +// an unscoped package keeps its full name +assertBinTarget("node_modules/.bin/plain-tool", "packages/plain-tool/main.js"); + +console.log("ok"); diff --git a/tests/specs/workspaces/workspace_member_bin_string/deno.json b/tests/specs/workspaces/workspace_member_bin_string/deno.json new file mode 100644 index 00000000000000..3238fd32833959 --- /dev/null +++ b/tests/specs/workspaces/workspace_member_bin_string/deno.json @@ -0,0 +1,11 @@ +{ + "nodeModulesDir": "auto", + "workspace": [ + "./packages/scoped-tool", + "./packages/plain-tool" + ], + "tasks": { + "scoped": "tool", + "plain": "plain-tool" + } +} diff --git a/tests/specs/workspaces/workspace_member_bin_string/packages/plain-tool/main.js b/tests/specs/workspaces/workspace_member_bin_string/packages/plain-tool/main.js new file mode 100644 index 00000000000000..f2b79a3a1ba66d --- /dev/null +++ b/tests/specs/workspaces/workspace_member_bin_string/packages/plain-tool/main.js @@ -0,0 +1,2 @@ +#!/usr/bin/env node +console.log("plain tool"); diff --git a/tests/specs/workspaces/workspace_member_bin_string/packages/plain-tool/package.json b/tests/specs/workspaces/workspace_member_bin_string/packages/plain-tool/package.json new file mode 100644 index 00000000000000..e1b4fa213e95de --- /dev/null +++ b/tests/specs/workspaces/workspace_member_bin_string/packages/plain-tool/package.json @@ -0,0 +1,6 @@ +{ + "name": "plain-tool", + "version": "1.0.0", + "type": "module", + "bin": "./main.js" +} diff --git a/tests/specs/workspaces/workspace_member_bin_string/packages/scoped-tool/cli.js b/tests/specs/workspaces/workspace_member_bin_string/packages/scoped-tool/cli.js new file mode 100644 index 00000000000000..53a2f3859f5449 --- /dev/null +++ b/tests/specs/workspaces/workspace_member_bin_string/packages/scoped-tool/cli.js @@ -0,0 +1,2 @@ +#!/usr/bin/env node +console.log("scoped tool"); diff --git a/tests/specs/workspaces/workspace_member_bin_string/packages/scoped-tool/package.json b/tests/specs/workspaces/workspace_member_bin_string/packages/scoped-tool/package.json new file mode 100644 index 00000000000000..5f197855bf24d7 --- /dev/null +++ b/tests/specs/workspaces/workspace_member_bin_string/packages/scoped-tool/package.json @@ -0,0 +1,6 @@ +{ + "name": "@scope/tool", + "version": "1.0.0", + "type": "module", + "bin": "./cli.js" +} diff --git a/tests/specs/workspaces/workspace_member_bin_string/plain_task.out b/tests/specs/workspaces/workspace_member_bin_string/plain_task.out new file mode 100644 index 00000000000000..ee0ef6b6f684bf --- /dev/null +++ b/tests/specs/workspaces/workspace_member_bin_string/plain_task.out @@ -0,0 +1,2 @@ +Task plain plain-tool +plain tool diff --git a/tests/specs/workspaces/workspace_member_bin_string/scoped_task.out b/tests/specs/workspaces/workspace_member_bin_string/scoped_task.out new file mode 100644 index 00000000000000..b5324291a65797 --- /dev/null +++ b/tests/specs/workspaces/workspace_member_bin_string/scoped_task.out @@ -0,0 +1,2 @@ +Task scoped tool +scoped tool