[BUG] The generated .cmd script for CLI packages has faulty error handling

[staudenmayerc]

What / Why

npm version: 6.13.4 (node 12.14.1, 64 bit)

From what I understand, when you (globally) install a package via npm that also has a CLI component, wrapper scripts will get generated, in my example this was the "yarn" package (also verified the bug with other packages), which lead to the generation of a "yarn" shell script and a "yarn.cmd" batch file for Windows.

When I now use the CLI command via yarn somescript (which calls yarn.cmd because I'm on Windows), and the execution of said script fails, the errorlevel will be set to 1 correctly (this can be verified with echo %errorlevel%), but somehow is ignored when using &&.

For example, yarn somescript && dir will still execute dir even when yarn somescript failed.

After some experimentation, I found out that restructuring the generated yarn.cmd file solves the problem:

@ECHO off
GOTO Start

:find_dp0
SET dp0=%~dp0
EXIT /b

:Start

SETLOCAL
CALL :find_dp0

IF EXIST "%dp0%\node.exe" (
  SET "_prog=%dp0%\node.exe"
) ELSE (
  SET "_prog=node"
  SET PATHEXT=%PATHEXT:;.JS;=;%
)

"%_prog%"  "%dp0%\node_modules\yarn\bin\yarn.js" %*

With this, the errorcode is honored as expected. The only two changes necessary were moving find_dp0 to the top and jumping over it, and to leave out EXITLOCAL, which should not be necessary, since the documentation states that exiting the script will have the same effect. This allows the actual call to %_prog% to be the last line in the script, therefore obviating the need to set the errorcode manually.

How

Steps to Reproduce

On Windows:

C:\> npm install -g typescript
C:\> tsc --invalidflagname && dir

Expected Behavior

dir should not be run in the example

edit: Also read this which explains why %errorlevel% is different from the observed && behavior, but it doesn't change the fact that the .cmd doesn't set the errorlevel correctly

[isaacs]

Interesting! Can you send a PR to https://github.com/npm/cmd-shim with this restructured output?

[staudenmayerc]

@isaacs Done: npm/cmd-shim#46

[rivy]

This error originates from a CMD shell bug. exit EXITCODE and exit /B EXITCODE do NOT return EXITCODE as the process exit code unless the BAT/CMD file is executed with a call BAT_FILE command. The ERRORLEVEL is set and returned to the parent shell correctly, but the process exit code is not. There's more detail, some references, and an alternate solution in my commit for pl2bat, which is a similar command wrapper for perl.

This solution works because of another countervailing quirk which does pass the ERRORLEVEL out as the process exit code if and only if the command executing the target executable is the last command in the last parsable unit of the BAT/CMD file. And, this solution is better because it avoids another bug (not-resetting the window title between commands) caused by my initial fix for pl2bat.

A couple of other notes...

• @echo off needs to be within a @setLocal (which should be the first executable line), otherwise you are changing it for the calling process as well which can lead to unwanted output or loss of output for scripts calling other scripts.
• To avoid environment pollution for the target executable, use an endLocal before calling it. To use your current environment for just the call, use a single parse unit (eg, use endLocal & "%_prog%" "%dp0%\node_modules\yarn\bin\yarn.js" %* as the last line).

[rivy]

I'm curious as to why :find_dp0 is needed. I'm unable to find a reference that explains the need.

If it's not needed, this construction should work:

@setLocal
@echo off
set dp0=%~dp0
set "dp0=%dp0:~0,-1%" &rem clip the trailing path separator

IF EXIST "%dp0%\node.exe" (
  SET "_prog=%dp0%\node.exe"
) ELSE (
  SET "_prog=node"
  SET PATHEXT=%PATHEXT:;.JS;=;%
)

endLocal & "%_prog%" "%dp0%\node_modules\yarn\bin\yarn.js" %*

[rivy]

I was able to find an answer to the question of why :find_dp0 on stackoverflow.
I also wanted to suppress the annoying "Terminate batch job (Y/N)?" when interrupting my development watch processes.

So, now, I'm using this construction (hexo as an example):

@setLocal
@echo off
goto :_START_

:set_real_dp0
@rem:: ref: "https://stackoverflow.com/questions/19781569/cmd-failure-of-d0-when-call-quotes-the-name-of-the-batch-file"
@rem:: ref: "https://stackoverflow.com/questions/12141482/what-is-the-reason-for-batch-file-path-referenced-with-dp0-sometimes-changes-o/26851883#26851883"
@rem:: ref: "https://www.dostips.com/forum/viewtopic.php?f=3&t=5057"
set dp0=%~dp0
set "dp0=%dp0:~0,-1%" &@rem:: clip trailing path separator
goto :EOF

:_START_
call :set_real_dp0

IF EXIST "%dp0%\node.exe" (
  SET "_prog=%dp0%\node.exe"
) ELSE (
  SET "_prog=node"
  SET PATHEXT=%PATHEXT:;.JS;=;%
)

endLocal & goto #_undefined_# 2>NUL || title %COMSPEC% & "%_prog%" "%dp0%\node_modules\hexo-cli\bin\hexo" %*