Skip to content

fix(sh shim): support running Windows binaries from wsl#56

Merged
zkochan merged 1 commit into
pnpm:mainfrom
nadalaba:pnpm/fix-wsl
Jun 14, 2026
Merged

fix(sh shim): support running Windows binaries from wsl#56
zkochan merged 1 commit into
pnpm:mainfrom
nadalaba:pnpm/fix-wsl

Conversation

@nadalaba

@nadalaba nadalaba commented Jun 10, 2026

Copy link
Copy Markdown

add the same changes as npm#178, but when the prog does not end with the Windows .exe extension keep the passed parameter in its linux-form path, because on this fork, prog can be a posix binary.

Summary by CodeRabbit

Release Notes

  • Bug Fixes
    • Enhanced Windows and WSL environment path handling in generated shell shims for improved compatibility with Cygwin, MSYS, and WSL2 scenarios.
    • Updated shell shim generation to correctly resolve and execute targets in Windows environments.

@coderabbitai

coderabbitai Bot commented Jun 10, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: aefdec3c-4da0-4214-9d07-498762a1216f

📥 Commits

Reviewing files that changed from the base of the PR and between bb8df42 and e8560a8.

📒 Files selected for processing (3)
  • src/index.ts
  • test/e2e.test.js.snapshot
  • test/test.js.snapshot
📜 Recent review details
🔇 Additional comments (11)
src/index.ts (5)

445-446: LGTM!


459-470: LGTM!


545-555: LGTM!


557-561: LGTM!


519-526: WSL detection covers only WSL2 (potential WSL1 gap)

  • In src/index.ts (lines 519-526), basedir_win is converted via wslpath -w only in the case \uname -a`branchWSL2)`; WSL1 won’t match this pattern and will skip the conversion.
  • Current in-repo evidence only shows *WSL2* handling (no WSL1/Microsoft/*WSL* fallback), so whether WSL1 support is intentionally excluded isn’t clear—either add WSL1 coverage/tests or document that the shim is WSL2-only.
test/e2e.test.js.snapshot (2)

1-27: LGTM!


29-55: LGTM!

test/test.js.snapshot (4)

1-34: LGTM!


137-170: LGTM!


652-685: LGTM!

Also applies to: 972-998, 1021-1047


1077-1110: LGTM!


📝 Walkthrough

Walkthrough

cmd-shim now enhances POSIX shell shim generation to detect Windows and WSL environments via uname -a, compute a Windows-compatible base directory path using cygpath or wslpath, and route executable invocations to use these converted paths instead of the raw POSIX paths.

Changes

Windows and WSL path handling for POSIX shell shims

Layer / File(s) Summary
Core implementation: Windows path computation and conditional execution
src/index.ts
generateShShim derives Windows-form quoted target paths and introduces shTarget_win for Windows execution routing. The embedded shell script template now sets basedir_win using cygpath/wslpath based on uname -a output, and the exec logic probes for .exe variants and conditionally selects between Windows and non-Windows target variables.
Snapshot updates for all shim types
test/e2e.test.js.snapshot, test/test.js.snapshot
Generated POSIX shell shim snapshots reflect the new basedir_win computation and uname -a platform detection applied consistently across bat.shim, env.shim variants (with different args, NODE_PATH, and PATH handling), sh.shim variants, exe.shim, and from.env.s.shim, each now using the converted basedir_win in exec invocations for Windows/WSL compatibility.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

  • pnpm/cmd-shim#55: Both PRs update generateShShim's generated shim content for Windows/MSYS scenarios and touch the same cmd.exe invocation paths and switch handling logic in src/index.ts.

Suggested reviewers

  • zkochan

Poem

🐰 A shim that learns to speak two tongues — 🪟
Windows paths and Cygwin too,
basedir_win converts with care,
WSL paths dance in the air,
One script now serves everywhere! ✨

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'fix(sh shim): support running Windows binaries from wsl' directly and specifically describes the main change: enabling sh shims to run Windows binaries from WSL by adding Windows path handling logic.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@qodo-free-for-open-source-projects

Copy link
Copy Markdown

PR Summary by Qodo

Fix sh shim generation to run Windows binaries correctly under WSL
🐞 Bug fix 🧪 Tests 🕐 20-40 Minutes

Grey Divider

Walkthroughs

Description
• Detect WSL2 in generated shims and compute a Windows-style basedir for .exe execution.
• Keep POSIX paths for non-.exe programs to support posix binaries in this fork.
• Update shim snapshots to reflect new WSL/Cygwin path handling and exec fallbacks.
Diagram
graph TD
A["generateShShim()"] --> B["Generated .sh shim"] --> C{"Detect environment"}
C -->|"CYGWIN/MINGW/MSYS"| D["cygpath -> basedir_win"]
C -->|"WSL2"| E["wslpath -> basedir_win"]
C -->|"Other"| F["basedir_win=basedir"]
B --> G{"Program is .exe?"}
G -->|"Yes"| H["Exec uses basedir_win + Win paths"]
G -->|"No"| I["Exec uses basedir + POSIX paths"]
subgraph Legend
  direction LR
  _p["Process"] ~~~ _d{"Decision"}
end
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Detect WSL via env vars (/proc/version, WSL_INTEROP)
  • ➕ More robust than matching uname -a output (which can vary by distro/kernel).
  • ➕ Allows distinguishing WSL1 vs WSL2 if behavior needs to diverge later.
  • ➖ Adds additional probing logic and platform-specific file reads.
  • ➖ Would diverge further from upstream cmd-shim behavior being mirrored.
2. Always normalize basedir to Windows path when wslpath exists
  • ➕ Simpler branching: one path representation for all executions under WSL.
  • ➕ Avoids mismatched path styles when downstream tools parse argv.
  • ➖ Breaks this fork’s requirement where prog may be a POSIX binary and should keep Linux-form paths.
  • ➖ Could regress non-Windows targets invoked from WSL.

Recommendation: Keep the PR’s approach: compute both basedir and basedir_win, then only use Windows-path arguments when executing .exe programs (including custom node exec paths). This preserves upstream parity for Windows targets under WSL while maintaining correct POSIX semantics for this fork’s posix-binary use cases.

Grey Divider

File Changes

Bug fix (1)
index.ts Add WSL2-aware path conversion and .exe-specific argument selection in sh shims +50/-19

Add WSL2-aware path conversion and .exe-specific argument selection in sh shims

• Extends 'generateShShim' to compute a Windows-style 'basedir_win' (via 'wslpath' on WSL2 and 'cygpath' on Cygwin-like environments). Updates exec selection to prefer '.exe' variants and to pass Windows-path targets only when the resolved program is a Windows executable, keeping POSIX paths for non-.exe programs.

src/index.ts


Tests (2)
e2e.test.js.snapshot Refresh e2e snapshot for new basedir_win/WSL2 shim header +17/-7

Refresh e2e snapshot for new basedir_win/WSL2 shim header

• Updates the expected generated shim content to include 'basedir_win' initialization and the new 'uname -a' switching logic (including the WSL2 'wslpath' branch).

test/e2e.test.js.snapshot


test.js.snapshot Update unit snapshots for .exe fallback exec paths and basedir_win usage +327/-129

Update unit snapshots for .exe fallback exec paths and basedir_win usage

• Refreshes multiple shim snapshots to reflect the new header (basedir normalization + basedir_win) and the updated exec fallback ordering that prefers '.exe' where appropriate. Also updates node/cmd/sh shebang scenarios to use 'basedir_win' for Windows-target script paths when executing '.exe' binaries.

test/test.js.snapshot


Grey Divider

Qodo Logo

@zkochan zkochan merged commit f660f8f into pnpm:main Jun 14, 2026
5 of 6 checks passed
zkochan added a commit to pnpm/pnpm that referenced this pull request Jun 15, 2026
- Port the shell shim behavior from pnpm/cmd-shim#56 to pacquet.
- Generate `basedir_win` with Cygwin/MSYS/WSL2 handling and use it only when invoking `.exe` runtime branches.
- Preserve POSIX target paths for non-`.exe` runtime branches and add the `.cmd`/`.bat` `/C` runtime fallback.
- Gate MSYS-specific cmd switch escaping behind an `$msys` runtime flag, so MSYS gets `//C` while WSL2 and other shells keep `/C`.
- Bump `@zkochan/cmd-shim` to 9.0.6.
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