Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: use spawn's shell option to follow npm and fix batch script shells #43

Closed
wants to merge 2 commits into from

Conversation

gluxon
Copy link
Member

@gluxon gluxon commented Apr 15, 2024

Problem

A PR earlier today (#42) attempted to fix spawn EINVAL errors on Windows, but accidentally changed how arguments were passed to the custom shell. Thankfully a test in the pnpm repo caught this and failed.

● pnpm run with custom shell

    expect(received).toStrictEqual(expected) // deep equality
 
    - Expected  - 1
    + Received  + 2
 
      Array [
        "-c",
    -   "foo bar",
    +   "foo",
    +   "bar",
      ]

Changes

Instead of option 3, let's go with option 2.

I thought through a few options on how to fix this:

  1. Re-fork the @npm/run-script package and use that in pnpm.
  2. Match how @npm/run-script uses spawn and pass { shell: scriptShell } as an option.
  3. Set { shell: true } on Windows when scriptShell is a batch file.

Testing

I used pnpm patch to edit @pnpm/npm-lifecycle to trigger a CI job in the pnpm repo. Running on my fork to make sure the fix above works.

https://github.com/gluxon/pnpm/actions/runs/8692862152

@gluxon
Copy link
Member Author

gluxon commented Apr 15, 2024

Hm. This doesn't work either.

2024-04-15T16:49:15.3193085Z PASS test/exec.e2e.ts (24.974 s)
2024-04-15T16:57:16.2263573Z  LOGGING RETRY ERRORS  pnpm run with custom shell
2024-04-15T16:57:16.2313711Z  RETRY 1 
2024-04-15T16:57:16.2314144Z 
2024-04-15T16:57:16.2314333Z     spawn EINVAL
2024-04-15T16:57:16.2314618Z 
2024-04-15T16:57:16.2316456Z       at spawn (../../node_modules/.pnpm/@pnpm+npm-lifecycle@3.0.2_patch_hash=yeclh3jd7vzubunvrx77npkhwe_typanion@3.14.0/node_modules/@pnpm/npm-lifecycle/lib/spawn.js:36:15)
2024-04-15T16:57:16.2319796Z       at runCmd_ (../../node_modules/.pnpm/@pnpm+npm-lifecycle@3.0.2_patch_hash=yeclh3jd7vzubunvrx77npkhwe_typanion@3.14.0/node_modules/@pnpm/npm-lifecycle/index.js:277:7)

https://productionresultssa16.blob.core.windows.net/actions-results/56c7068b-4db9-44aa-8200-2f4942f92f6a/workflow-job-run-383f6702-5211-583b-2357-5ea270effcaa/logs/job/job-logs.txt?rsct=text%2Fplain&se=2024-04-15T17%3A12%3A30Z&sig=ggu0OFjzychPA4bMJke2KzqRsl6fCLrYLTXuWtiLaEE%3D&sp=r&spr=https&sr=b&st=2024-04-15T17%3A02%3A25Z&sv=2021-12-02

@gluxon gluxon closed this Apr 15, 2024
@gluxon gluxon deleted the use-shell-for-script-shell branch April 15, 2024 17:18
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.

1 participant