The Windows shell path is hard-coded to `C:\Windows\SysWOW64`, ignoring `%SystemRoot%` and naming the 32-bit shell

Open Beginner friendly
#1,260 1 comment 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Assessment

Difficulty
2/5
Estimated time
1-3 hours
Newbie friendliness
78/100
Issue type
Bug
Clarity
Clearly specified
Activity status
Active
Tech stack
javascript, powershell

Research direction

Start in src/mvnw.js around line 119 and inspect how the Windows shell is passed to spawn. Run an eoc command that shells out to Maven on Windows, then verify the shell resolution no longer depends on C:\Windows or SysWOW64 and still launches the Maven toolchain successfully.

Written by the indexing model from the issue text.

Description

bug

What happens

src/mvnw.js hard-codes an absolute path to PowerShell:

function shell() {
  if (process.platform === 'win32') {
    return 'C:\Windows\SysWOW64\WindowsPowerShell\v1.0\powershell.exe';
  }
}

Two things are wrong with that string, and neither shows up on a default x64 install, which is presumably why it has survived.

It assumes Windows lives on C: in a directory named Windows. Windows can be installed elsewhere, and the location is what %SystemRoot% is for. When it is not C:\Windows, spawn fails with ENOENT naming a path the user never configured, and every eoc command that shells out to Maven stops there.

It also names SysWOW64, which is the 32-bit subsystem directory. The native shell on a 64-bit install is under System32; both exist on this machine:

C:\Windows\SysWOW64\WindowsPowerShell\v1.0\powershell.exe   423424 bytes
C:\Windows\System32\WindowsPowerShell\v1.0\powershell.exe   454656 bytes

So on x64 the code runs the 32-bit shell to launch a 64-bit toolchain, for no stated reason. On a 32-bit Windows there is no SysWOW64 at all and the path does not resolve.

Root cause

The intent is only to name a shell for spawn, at line 119. Nothing about the invocation needs a specific bitness or a specific location, so the absolute path is doing more work than the code asked of it.

Suggested fix

Let Windows resolve it, the way every other tool does:

function shell() {
  if (process.platform === 'win32') {
    return process.env.ComSpec || 'powershell.exe';
  }
}

A bare powershell.exe is found through PATH, which is correct on every install and on both bitnesses. If the location must stay explicit, build it from the environment rather than from a literal:

path.join(process.env.SystemRoot || 'C:\Windows', 'System32', 'WindowsPowerShell', 'v1.0', 'powershell.exe')

Worth deciding at the same time whether the shell is needed at all. The Maven call has no pipe, no redirection and no glob, and dropping the shell option entirely would remove this question along with the argument-quoting one filed separately.

Dominant language
JavaScript
Stars
36
Forks
64
Avg merge
1d 14h
Merged PRs (30d)
17

Contributor guide

No contributing guide indexed for this repository

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

More from objectionary/eoc

All issues in objectionary/eoc

Similar issues

More JavaScript issues

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.