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

Investigate why `spawnSync` without `shell: true` results in ENOENT for npm/yarn in some windows systems #16119

Closed
filipesilva opened this issue Nov 8, 2019 · 5 comments

Comments

@ngbot ngbot bot modified the milestone: Backlog Nov 8, 2019
dgp1130 added a commit to dgp1130/angular-cli that referenced this issue Nov 8, 2019
…ted in a directory with spaces. (angular#16073)"

This reverts commit 217eb29.

Reverting as this is breaking some Windows/CI builds. See angular#16119.
@alan-agius4

This comment has been minimized.

Copy link
Collaborator

@alan-agius4 alan-agius4 commented Nov 11, 2019

From: https://nodejs.org/dist/latest-v10.x/docs/api/child_process.html#child_process_spawning_bat_and_cmd_files_on_windows

The importance of the distinction between child_process.exec() and child_process.execFile() can vary based on platform. On Unix-type operating systems (Unix, Linux, macOS) child_process.execFile() can be more efficient because it does not spawn a shell by default. On Windows, however, .bat and .cmd files are not executable on their own without a terminal, and therefore cannot be launched using child_process.execFile(). When running on Windows, .bat and .cmd files can be invoked using child_process.spawn() with the shell option set, with child_process.exec(), or by spawning cmd.exe and passing the .bat or .cmd file as an argument (which is what the shell option and child_process.exec() do). In any case, if the script filename contains spaces it needs to be quoted.

In order not to require shell in Windows. We'd need to change the spawn command to the below when running on windows

    const { status, stderr, stdout, error } = child_process_1.spawnSync('cmd.exe', ['/c', packageManager, ...installArgs, ...extraArgs], {
        stdio: 'pipe',
        encoding: 'utf8',
        cwd,
    });

That being said, I don't think we should do anything (ie: leave shell: true) as based on the docs, this is what shell option does.

When running on Windows, .bat and .cmd files can be invoked using child_process.spawn() with the shell option set, with child_process.exec(), or by spawning cmd.exe and passing the .bat or .cmd file as an argument (which is what the shell option and child_process.exec() do).

@alan-agius4

This comment has been minimized.

Copy link
Collaborator

@alan-agius4 alan-agius4 commented Nov 11, 2019

Note: in the same there is also another spawnSync which uses shell

const { status, error } = spawnSync('node', argv, {
stdio: 'inherit',
shell: true,
env: {
...process.env,
NG_DISABLE_VERSION_CHECK: 'true',
NG_CLI_ANALYTICS: 'false',
},
});

@alan-agius4 alan-agius4 self-assigned this Nov 11, 2019
@filipesilva

This comment has been minimized.

Copy link
Member Author

@filipesilva filipesilva commented Nov 11, 2019

SGTM, great work getting to the bottom of this!

@clydin

This comment has been minimized.

Copy link
Member

@clydin clydin commented Nov 11, 2019

npm is a batch file on windows?

@filipesilva

This comment has been minimized.

Copy link
Member Author

@filipesilva filipesilva commented Nov 11, 2019

Looks like it:

PS C:\Users\kamik> get-command npm

CommandType     Name                                               Version    Source
-----------     ----                                               -------    ------
Application     npm.cmd                                            0.0.0.0    C:\Program Files\nodejs\npm.cmd


PS C:\Users\kamik> get-command yarn

CommandType     Name                                               Version    Source
-----------     ----                                               -------    ------
Application     yarn.cmd                                           0.0.0.0    C:\Program Files (x86)\Yarn\bin\yarn.cmd
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.