Skip to content

fix(install): link a local workspace member's bin into node_modules/.bin - #36330

Open
nathanwhitbot wants to merge 5 commits into
denoland:mainfrom
nathanwhitbot:orcha/impl-23edbf5e
Open

fix(install): link a local workspace member's bin into node_modules/.bin#36330
nathanwhitbot wants to merge 5 commits into
denoland:mainfrom
nathanwhitbot:orcha/impl-23edbf5e

Conversation

@nathanwhitbot

Copy link
Copy Markdown
Contributor

Closes #36313.

A local workspace member declaring bin in its package.json was symlinked into node_modules/<name>, but never got a node_modules/.bin entry — so deno task --cwd apps/web cli failed with local-cli: command not found. External npm dependencies with bins worked fine.

Root cause

Workspace members are not part of the npm resolution snapshot. Every code path that creates a .bin entry is keyed on &NpmResolutionPackage + NpmPackageExtraInfo (libs/npm_installer/bin_entries.rs), and deno task's managed-resolver command lookup (resolve_managed_npm_commands) only walks snapshot.top_level_packages(). Members were invisible to both. The doc comment on local.rs::setup_member_bin_entries stated the gap outright.

Fix

InstallerInstallWorkspacePkg now carries a parsed bin. deno_package_json::PackageJson only exposes bin as a raw serde_json::Value, so the String and Map shapes are parsed in package_json_to_bin_entry. resolve_workspace_bin_packages builds a synthetic NpmResolutionPackage per non-root member — the same precedent as the existing resolve_workspace_lifecycle_packages — and add_workspace_bin_entries pushes them into the same BinEntries as snapshot packages just before finish(). They are added last, so a real dependency wins any name collision. Wired through both linkers: isolated/pnpm-style (local.rs) and hoisted (hoisted.rs).

Task runner — the managed branch of resolve_task_node_modules_bin_dirs now walks cwd ancestors up to the root node_modules parent, and resolve_custom_commands merges those .bin entries. Prepending to PATH alone is not sufficient: on Windows the shims are plain <name> / .cmd / .ps1 files, and a #!/usr/bin/env node shebang would otherwise exec real node rather than deno.

Only entries classified as BinValue::JsFile are merged into custom commands; Executable entries (shell scripts, native binaries) are left to PATH, where deno_task_shell honours the shebang. Routing those through deno run --ext=js would SyntaxError. Names of skipped Executable entries are still recorded so a farther bin dir can't re-classify the same name as JsFile and win, which would invert closest-first precedence.

Pruningcalculate_packages_hash previously hashed only snapshot ids and remote packages, so member bins never tripped cleanup_unused_packages, leaving stale/dangling root .bin entries when a member's bin changed. The hoisted linker had no hash gate at all and cleanup_hoisted_packages skips dot-prefixed dirs, so it wipes the root .bin explicitly.

Notes for reviewers

Chmod side effect. The root .bin entry points at the member's real source directory, not the node_modules/<name> symlink (that symlink is created later in the install). So make_executable_if_exists chmods the user's git-tracked script +x (100644 → 100755), which will show up as a mode change in git status. npm's bin-links does the same thing, so this matches ecosystem behaviour, but it's worth being aware of.

Known precedence exception, documented not fixed. Closest-first holds among the bin dirs, but the command map is pre-seeded from the snapshot's top-level packages, so a root dependency's bin beats a nearer workspace member's bin of the same name — the opposite of npm/pnpm. This is pre-existing behaviour affecting every managed task, and reordering it reaches well beyond workspaces, so it's documented in resolve_custom_commands with a pointer to the TODO(nathanwhit) on resolve_managed_npm_commands where the real fix belongs.

BYONM/managed asymmetry. The BYONM branch deliberately routes every entry through deno run, including Executable ones, because a JS bin with no shebang classifies as Executable and running it through deno is the only thing that makes it work there. Under the managed resolver such a bin falls through to PATH and fails to exec on unix — 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.

Collision warning is gated on clean_on_install so it isn't reprinted ahead of every deno task re-link; the cost is that a user who only ever tasks against an already-linked node_modules won't see it.

Tests

50 unit tests in deno_npm_installer, plus 9 spec tests under tests/specs/workspaces/: the issue's exact repro on both linkers (workspace_member_bin, _hoisted), the string form of bin and scoped member names (_string), member-vs-member and dependency-claimed collisions (_collision, _duplicate, _dep_claim), stale-entry pruning on both linkers (_prune, _prune_hoisted), and shell-script/no-shebang shadowing (_shadow). specs::task (128) stays green.

A local workspace member that declares a `bin` in its package.json was
symlinked into `node_modules/<name>`, but no `node_modules/.bin` entry
was ever created for it, so `deno task` could not invoke it
("local-cli: command not found"). External npm dependencies with bins
worked fine.

The root cause is that workspace members are not part of the npm
resolution snapshot: everything that creates `.bin` entries is keyed on
an `NpmResolutionPackage`, and `deno task`'s managed-resolver command
lookup only walks `snapshot.top_level_packages()`.

- `NpmInstallDepsProvider` now carries each member's `bin`.
  `deno_package_json` only exposes it as a raw json value, so the two
  shapes npm supports (string and map) are parsed into
  `NpmPackageVersionBinEntry`.
- Both installers build a synthetic `NpmResolutionPackage` per non-root
  member that declares a `bin` (same trick as the existing workspace
  lifecycle packages) and feed it through `BinEntries`, so bin-name
  normalization, collision handling, unix symlinks and windows shims are
  all reused. They are added after every snapshot package so a real
  dependency still wins a name collision, matching npm.
- A member's own `node_modules/.bin` now also gets the executables of
  the sibling members it depends on.
- `deno task` with the managed resolver now consults the cwd's ancestor
  `node_modules/.bin` directories up to the workspace root (closest
  first) and merges the commands it finds there with `or_insert`, so
  snapshot-derived commands keep precedence. PATH alone isn't enough
  because the windows shims are plain files.

Closes denoland#36313

(cherry picked from commit 676f8d74727e39363f4e9545f7931cde27ed7587)
(cherry picked from commit 199bb1ddada5088cc11cbd8898f657584117fec8)
(cherry picked from commit f13f3a06d3253066ae5efc3bd633035130f62d41)
…eno run`

Follow-up to "link a workspace member's `bin` into node_modules/.bin"
(denoland#36313), addressing review findings on that change.

1. BLOCKING regression. Merging every `node_modules/.bin` entry into the
   managed task resolver's custom commands routed them all through
   `NodeModulesFileRunCommand` (`deno run --ext=js -A <path>`), because
   `resolve_npm_commands_from_bin_dir` discarded the
   `BinValue::Executable` vs `JsFile` classification. Any non-JS entry —
   a POSIX shell script or a native binary — that previously resolved
   through `PATH` now died with a JS `SyntaxError`. The Managed branch
   now merges only `JsFile` entries and leaves `Executable` ones to
   `PATH`, where `deno_task_shell` honours their shebang. A 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. The BYONM
   branch is deliberately unchanged: a JS bin with no shebang also
   classifies as `Executable` there and BYONM relies on running it
   through deno.

2. Stale root `.bin` entries were never pruned. `calculate_packages_hash`
   hashed only snapshot package ids and `remote_pkgs`, so renaming a
   member's bin, dropping its `bin` field, or removing the member never
   tripped `cleanup_unused_packages` — the old entry survived, as a
   dangling symlink in the last case. The workspace members' bins are
   now folded into the hash. The hoisted linker had no hash gate at all
   and `cleanup_hoisted_packages` skipped every dot-prefixed directory,
   so it never touched `.bin`; it now wipes the root `.bin`, which
   `bin_entries.finish` repopulates immediately afterwards.

3. Two workspace members declaring the same bin name resolved silently
   and arbitrarily (the greatest `<name>@<version>` wins the depth
   sort's tiebreak). `deno install` now warns, naming both members and
   the winner, and the precedence rules are documented on
   `add_workspace_bin_entries`. npm hard-errors here; a warning keeps an
   otherwise fine workspace installable.

4. Performance. `resolve_custom_commands` runs once per task invocation
   and classifying a bin entry reads the whole file, which a large root
   `.bin` makes expensive (and `deno task --recursive` pays it per
   member). New `NodeResolver::resolve_npm_commands_from_bin_dir_filtered`
   lets the caller filter on the entry *name* first, so names already
   resolved from the snapshot are never read off disk.

5. Documented the chmod side effect: because `add_workspace_bin_entries`
   passes the member's real source directory as `package_path`,
   `make_executable_if_exists` chmods the user's git-tracked bin script
   +x on every install (100644 -> 100755). npm's bin-links does the
   same, so this is parity, but it is a new side effect on files inside
   the user's repo.

Tests: the two existing specs gained a `#!/bin/sh` member (the case
broken by 1), symlink/shim *target* assertions rather than bare
existence checks, and a "Hello from deno" assertion that genuinely fails
without the task-runner merge — without it a bare bin name is only a
`PATH` lookup, so the `#!/usr/bin/env node` shebang runs under whatever
Node is installed, or fails when none is. New specs cover the `bin`
string form including a scoped name, the snapshot-vs-member collision
precedence, the member-vs-member duplicate warning, and stale-entry
pruning for both linkers.

(cherry picked from commit 415c1f526c3102af672c50a298b2b664af354ff1)
(cherry picked from commit b6247cfe41705fb41c50161c85b25461963459fe)
Follow-up fixes to the workspace-member `bin` support.

`resolve_custom_commands`' managed branch only merges `.bin` entries that
classify as `BinValue::JsFile` and leaves executables to `PATH`, but an
`Executable` entry was skipped without recording its *name*. A farther bin
directory could then classify the same name as `JsFile` and register it as a
custom command — and custom commands beat `PATH` in `deno_task_shell` — so the
farther entry won outright. Track every classified name in a `HashSet` so the
closest entry always shadows farther ones, whatever its kind.

`warn_on_workspace_bin_name_collisions` only looked at the workspace members,
so when a dependency also declared the name it claimed `Only "<member>" will be
linked into node_modules/.bin` while the dependency was what actually got
linked. It now takes the already-registered bin names into account (new
`BinEntries::has_bin_name`) and says none of the members are linked in that
case.

The warning was also emitted from `add_workspace_bin_entries`, which runs on
every re-link, so `deno run`/`deno task` re-printed it ahead of their own
output. Gate it on `clean_on_install`, matching npm's report-once-at-install
behavior.

Specs: new `workspace_member_bin_shadow` locks in closest-first precedence when
the closest entry is a shell script; `workspace_member_bin_collision` gains a
second member for the three-way case; `workspace_member_bin_duplicate`'s
`task.out` no longer needs a leading `[WILDCARD]`, which is what proves the
gating; `workspace_member_bin` pins the (unchanged, intentional) behavior of a
member JS bin with no shebang.

(cherry picked from commit 82b0fec61331c0ad570dd431022a4dba0e487572)
…e member

Final review round on the workspace-member `bin` linking change.

- Document the one place closest-first `node_modules/.bin` precedence does
  not hold: `resolve_custom_commands`' managed branch pre-seeds its command
  map from the resolution snapshot's top-level packages, so a root
  dependency's bin beats a nearer workspace member's bin of the same name
  even though npm and pnpm would run the nearer one. This is pre-existing
  behaviour for snapshot packages and reordering it would affect every
  managed task, so it is documented rather than changed; the eventual fix
  belongs alongside the existing TODO on `resolve_managed_npm_commands`.
- Stop `resolve_task_node_modules_bin_dirs`' doc comment from promising more
  than it delivers: the paths it returns are candidates that are never
  checked for existence, and the closest-first ordering describes the list
  rather than which executable a task ends up running.
- Warn when a *single* workspace member declares a bin name that a
  dependency already claims. `warn_on_workspace_bin_name_collisions`
  returned early for fewer than two members, so the member's bin silently
  did not appear — the likeliest way to hit this in practice. The message
  building moved into a pure `workspace_bin_name_collision_warnings` so it
  can be unit tested; member-vs-member behaviour is unchanged.
- Document the `clean_on_install` gating trade-off on
  `add_workspace_bin_entries`: gating the warning to install time keeps it
  from reprinting ahead of every `deno task`, at the cost of a user who
  never re-runs a clean install not seeing it.

Adds `local::test::test_workspace_bin_name_collision_warnings` and the
`workspaces::workspace_member_bin_dep_claim` spec for the new warning.
The `root-noshebang` step of `workspace_member_bin` asserted the exec
failure text of a `.bin` entry left to `PATH`. On macOS the spawn falls
back to `/bin/sh` (execvp's ENOEXEC behaviour) instead of failing with
"Exec format error", so the step failed on both macOS runners. What the
step asserts — that the entry is not routed through `deno run` — holds
either way, so run it on linux only.
@nathanwhitbot

Copy link
Copy Markdown
Contributor Author

Pushed a test-only fix for the macOS spec failure (specs::workspaces::workspace_member_bin).

The root-noshebang step asserted the text of the exec failure for a .bin entry left to PATH. On Linux that's Exec format error; on macOS the spawn falls back to running the file with /bin/sh (execvp's ENOEXEC behaviour), so the JS source comes back as a pile of shell syntax errors instead — hence the failure on both macOS runners. The thing the step actually asserts (a no-shebang JS bin is not routed through deno run under the managed resolver) holds identically on both, so the step is now "if": "linux" with a comment explaining the divergence. No product code changed; all 9 workspace_member_bin* specs pass locally.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Local workspace package bin entries are not linked or available in tasks

2 participants