Skip to content

fix(package): ship apply_patches and its patches, invoke via bash - #14565

Draft
Planeshifter wants to merge 2 commits into
developfrom
philipp/ci-fix-windows-npm-postinstall-2026-08-23
Draft

Planeshifter wants to merge 2 commits into
developfrom
philipp/ci-fix-windows-npm-postinstall-2026-08-23

Conversation

@Planeshifter

Copy link
Copy Markdown
Member

Description

What is the purpose of this pull request?

This pull request:

  • fixes npm install failing on Windows for the published @stdlib/stdlib package. windows_test_npm_install (job Node.js v16) has failed on every scheduled run against develop for roughly the past month: run 32564185436. Symptom: npm ERR! 'tools' is not recognized as an internal or external command, operable program or batch file. during the postinstall lifecycle script (@stdlib/stdlib@0.4.1 postinstall: tools/scripts/apply_patches), causing the install to fail outright.
  • two compounding bugs. First, .npmignore excluded /etc/ and /tools/ in their entirety, so tools/scripts/apply_patches (the script package.json declares as postinstall) and the one patch it applies (etc/patches/unified-message-control+1.0.4.patch, for a transitive dev-only lint dependency) were never included in the published npm tarball. npm install therefore always tried to run a lifecycle script that does not exist in the installed package, on every platform. Windows surfaces this most visibly because npm spawns lifecycle scripts via cmd.exe there, and cmd.exe cannot interpret a missing/bash-shebang script as a command. Second, even once the script is shipped, it needs an explicit interpreter on Windows, since cmd.exe does not understand #!/usr/bin/env bash shebangs.
  • fixes this in two commits. First, .npmignore selectively un-ignores etc/patches/ and tools/scripts/apply_patches, using the same negation technique already applied to /docs/* + !/docs/types/* (npm's packer, like git, cannot un-ignore a file inside a directory that's ignored wholesale; the parent must be un-ignored via /dir/* + !/dir/subpath). postinstall becomes bash tools/scripts/apply_patches, since bash resolves reliably on PATH in the CI runner (MSYS2-based) but cmd.exe needs it named explicitly.
  • adds a same-day follow-up: postinstall becomes bash tools/scripts/apply_patches || exit 0. A default Git for Windows install only adds Git\cmd to PATH, not Git\bin/Git\usr\bin where bash.exe lives, so bash is not guaranteed to resolve for every Windows contributor running npm install locally outside CI. The patch this script applies only targets a transitive dev-only lint dependency, never installed by consumers of the published package, and the script already treats each individual patch application as best-effort (git apply ... || true internally). The invocation now follows the same philosophy: a missing bash no longer fails the install.

Validation:

  • npm pack --dry-run confirms both tools/scripts/apply_patches and etc/patches/unified-message-control+1.0.4.patch are now included in the published tarball (43 total files, +2 from before), and nothing else under etc//tools/ leaks in.
  • Packed a real tarball, extracted it into a directory with no .git (matching how make test-npm-install installs @stdlib/stdlib@latest from the registry into a bare scratch directory), and ran the postinstall command directly: exits 0 and patches apply when bash and a git checkout are both present; exits 0 and no-ops gracefully when either is absent (tested by removing bash from PATH entirely).
  • Reviewed by three independent automated passes (correctness, regression scope, style/conventions) across two revision cycles. The first correctness pass caught that a bash-only fix wouldn't work without the .npmignore change, since the script wouldn't exist in the package at all. The second correctness pass caught that bash isn't guaranteed on PATH for every Windows contributor outside CI, leading to the || exit 0 follow-up. All three reviewers approved the final revision with no blocking findings.

Related Issues

Does this pull request have any related issues?

This pull request has no related issues.

Questions

Any questions for reviewers of this pull request?

No.

Other

Any other information relevant to this pull request? This may include screenshots, references, and/or implementation notes.

Three items worth a maintainer's attention, none blocking:

  • The patch under etc/patches/ is applied via git rev-parse --show-toplevel to locate the repo root. In an npm-installed package (no .git present, e.g. the registry-install CI job, or a real end-user install), this resolves to nothing or to the consumer's unrelated repo root, so the patch silently never actually applies outside of a stdlib source checkout. This is pre-existing behavior, not introduced here, and it's harmless since the target dependency (unified-message-control) is dev-only and never installed by consumers, but the shipped patch is effectively dead weight for every non-source-checkout install. Worth a follow-up issue if patch application for consumers is ever intended to matter.
  • linux_test_install and macos_test_npm_install show the same "failure on every scheduled run for the past month" pattern in the Actions history. Their job logs could not be retrieved via available tooling to confirm the exact failure signature, but the root cause here (missing postinstall script in the published tarball) applies identically to any platform's npm install, so they likely share it. Flagging for a maintainer to confirm once this lands and a new version publishes.
  • This fix alone will not turn windows_test_npm_install (or the Linux/macOS install jobs) green immediately upon merge: make test-npm-install installs @stdlib/stdlib@latest from the npm registry, not from this branch or develop. The jobs will stay red until the next npm release is published with this fix included.

Checklist

Please ensure the following tasks are completed before submitting this pull request.

AI Assistance

When authoring the changes proposed in this PR, did you use any kind of AI assistance?

  • Yes
  • No

If you answered "yes" above, how did you use AI assistance?

  • Code generation (e.g., when writing an implementation or fixing a bug)
  • Test/benchmark generation
  • Documentation (including examples)
  • Research and understanding

Disclosure

If you answered "yes" to using AI assistance, please provide a short disclosure indicating how you used AI assistance. This helps reviewers determine how much scrutiny to apply when reviewing your contribution. Example disclosures: "This PR was written primarily by Claude Code." or "I consulted ChatGPT to understand the codebase, but the proposed changes were fully authored manually by myself.".

This PR was written primarily by Claude Code as part of an automated CI-failure triage and fix routine, based on live GitHub Actions job log analysis. The fix was reviewed by three independent automated review passes across two revision cycles before being proposed here.


@stdlib-js/reviewers


Generated by Claude Code

claude added 2 commits August 23, 2026 14:18
The job `Node.js v16`/`v18`/`v20` (windows-latest) on workflow
`windows_test_npm_install` has failed on every scheduled run against
develop for the past month with `npm ERR! 'tools' is not recognized
as an internal or external command`.

Root cause: `.npmignore` excluded `/etc/` and `/tools/` in their
entirety, so the `postinstall` script (`tools/scripts/apply_patches`)
declared in package.json, along with the patches it applies under
`etc/patches/`, were never included in the published npm tarball.
`npm install` therefore always tried to execute a lifecycle script
that does not exist in the installed package, regardless of platform;
on Windows this surfaces as the "not recognized" error above because
cmd.exe (npm's default script-shell there) cannot interpret the
missing path as a command at all.

This commit selectively un-ignores `etc/patches/` and
`tools/scripts/apply_patches`, using the same negation technique
`.npmignore` already applies to `/docs/*` + `!/docs/types/*`, so the
postinstall script and its patches are shipped. It also prefixes the
`postinstall` command with `bash`, since npm spawns lifecycle scripts
via cmd.exe on Windows regardless of the invoking shell, and cmd.exe
cannot execute a bash-shebang script directly; `bash` resolves on
PATH via the Git-for-Windows/MSYS2 toolchain already required by the
windows_test_npm_install runner. Behavior on Linux/macOS is
unchanged, since the script already required bash and takes no
positional arguments.

Validated by `npm pack`, extracting the resulting tarball into a
directory with no `.git` (matching how `make test-npm-install`
installs `@stdlib/stdlib@latest` from the registry), and confirming
`bash tools/scripts/apply_patches` now runs and exits 0 from within
it.

Ref: https://github.com/stdlib-js/stdlib/actions/runs/32564185436
Follow-up to 8a5fe1a. On Windows, a default Git for Windows install
only adds `Git\cmd` to PATH (not `Git\bin`/`Git\usr\bin`, where
`bash.exe` lives), so `bash` is not guaranteed to resolve for every
Windows contributor running `npm install` outside of CI (CI's
windows_test_npm_install runner uses an MSYS2 shell, which does put
`bash` on PATH reliably).

The patch this script applies only targets a transitive dev-only
lint dependency (`unified-message-control`), not a runtime
dependency, and the script itself already treats every patch
application as best-effort (`git apply ... || true`). This commit
extends that same best-effort philosophy to the invocation itself:
if `bash` cannot be found at all, `npm install` now succeeds anyway
instead of failing with `'bash' is not recognized as an internal or
external command`.

Validated by simulating both the happy path and a PATH with no
`bash` present via `sh -c "bash tools/scripts/apply_patches || exit
0"`; both exit 0.

Ref: https://github.com/stdlib-js/stdlib/actions/runs/32564185436

This branch has not been deployed

No deployments
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.

2 participants