The Windows shell path is hard-coded to `C:\Windows\SysWOW64`, ignoring `%SystemRoot%` and naming the 32-bit shell
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
- Domain
- cli, operating-systems
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
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
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
More from objectionary/eoc
-
bug good-title
Difficulty 2/5 1-3 hours Newbie friendliness 85/100
objectionary/eoc#1360 ·
-
bug good-title
Difficulty 2/5 1-3 hours Newbie friendliness 78/100
objectionary/eoc#1359 ·
-
bug good-title
Difficulty 2/5 1-3 hours Newbie friendliness 76/100
objectionary/eoc#1358 ·
-
Difficulty 1/5 Under an hour Newbie friendliness 90/100
objectionary/eoc#1357 ·
-
Difficulty 2/5 1-3 hours Newbie friendliness 72/100
objectionary/eoc#1356 ·
All issues in objectionary/eoc
Similar issues
-
Difficulty 2/5 1-3 hours Newbie friendliness 88/100
HarperFast/skills#96 ·
-
[Block] Latest Posts [Type] Bug
Difficulty 2/5 1-3 hours Newbie friendliness 76/100
-
Difficulty 2/5 1-3 hours Newbie friendliness 78/100
Automattic/studio#4908 ·
-
Difficulty 2/5 1-3 hours Newbie friendliness 74/100
-
Difficulty 2/5 1-3 hours Newbie friendliness 86/100
sugarlabs/musicblocks#8847 ·