Kallithea issues archive

I thought "git bash" was msys which has a nice abstraction of only providing a shell but running programs natively ... but from the description, it sounds more like it is cygwin which introduce a different calling convention?

Can you confirm it works when using "plain" windows command prompt and shell?

Does a change like this make any difference?:

--- a/kallithea/bin/kallithea_cli_front_end.py
+++ b/kallithea/bin/kallithea_cli_front_end.py
@@ -42,7 +42,7 @@ def front_end_build(install_deps, genera

     if install_deps:
         click.echo("Running 'npm install' to install front-end dependencies from package.json")
-        subprocess.check_call(['npm', 'install'], cwd=front_end_dir)
+        subprocess.check_call('npm install', cwd=front_end_dir, shell=True)

     if generate:
         tmp_dir = os.path.join(front_end_dir, 'tmp')

In plain windows command, npm works, though it's because it runs the npm.cmd version and not npm. I tested this out by renaming npm.cmd to npmx.cmd, and then running npm. It can't find that command.

And no, changing ['npm', 'install'] to 'npm install' doesn't work either.

Our use of npm on Windows is probably a bit untested and rough. Thanks for helping out.

Let's be explicit and make sure we agree on the back story.

A bit of googling suggest that it should be downloaded from https://nodejs.org/en/download/ . That should perhaps be documented. (A bit surprising for me that the platform (nodejs) also include a package manager (npm) that runs on/inside the platform.)

In usual Windows style, the installation binary path must be added to PATH (where Unix systems generally install to a /bin directory that already is in PATH).

Looking at https://nodejs.org/dist/v10.15.1/node-v10.15.1-win-x64.zip , I see it contains 'npm.cmd' which is a plain cmd script, "Created by npm", and launches node.exe with the right npm-cli.js. It also contains a plain 'npm' which is a mingw (msys) / cygwin sh script.

Cygwin do evil things, changing semantics of other programs by redefining well-known windows-isms to be more unix-like, thus leaving it as something neither is one or the other. I don't think it is worth it to try to support that.

Msys/mingw are fine. Once programs have been launched, they run fully native.

So you confirm that on plain windows, this works, using npm.cmd: python -c "import subprocess; subprocess.check_call(['npm', '-v'])"

But you found that inside git bash it fails, as Python cannot find npm.cmd? It seems like this must be a cygwin-ish environment. And if using shell=True, it still doesn't use the proper shell for the OS but a special git bash shell? The proper way to support that would be at the Python level. It seems wrong if we have to work around that.

Should we really support git bash, even it it require adding hacks and layering violations that we cannot guarantee actually works? How common and essential is it to use git bash? How do other programs deal with it?

Or is it perhaps an issue of something setting PATHEXT to not include .cmd ?

But ... while I don't like the idea of adding "random" and supposedly unnecessary workarounds we don't understand and can't maintain, I guess it would be ok to be explicit about windows npm actually being npm.cmd ... even though the whole idea of using .cmd was that it should be invisible and irrelevant to us users of it ...

Thoughts? @patrickdepinguin ?

Thanks @kiilerix for the details you found, that was very helpful.

@ewongcc Would it be possible to dump the value of the 'PATHEXT' environment variable from Kallithea?

Could you also try to run 'npm' directly from git bash (not via Kallithea)? Does that work? And what is the value of PATHEXT there?

Depending on the above we could consider asking for help from the Git Bash developers, which seem to be active: https://github.com/git-for-windows/git/issues

Also, I just found this: https://github.com/msys2/MINGW-packages/issues/1039 which claims that the problem is actually in the 'npm' wrapper which should do some 'cygpath' basedir conversion even if not on cygwin. This leads to following patch: https://github.com/msys2/MINGW-packages/pull/1096/commits/241409d8de3fbed0171446acbe272c6bdbb8dc20

Could you try this change manually on the 'npm' script? i.e.:

--- npm 2018-08-02 16:56:34.000000000 +0200
+++ npm.new 2019-02-23 20:35:28.698031593 +0100
@@ -3,9 +3,9 @@

 basedir=`dirname "$0"`

-case `uname` in
-    *CYGWIN*) basedir=`cygpath -w "$basedir"`;;
-esac
+if [ -x "$(command -v cygpath)" ]; then
+  basedir=`cygpath -w "$basedir"`
+fi

 NODE_EXE="$basedir/node.exe"
 if ! [ -x "$NODE_EXE" ]; then

About the problem being in the 'npm' wrapper: That one must be for running directly from bash. It should never be used when a windows program invoke a sub program or a windows shell run 'npm' - then it should look for 'npm.cmd' and other extensions in PATHEXT. The described workaround should thus not be relevant?

"It should never be used" because it does not have an extension in PATHEXT? (I'm not familiar with Windows development, and don't use it anymore either). That probably sounds reasonable. I fail to really understand what this 'git bash' thing is and how it influences execution of programs started inside of it.

Yes, I would expect everything to work as usual and use the usual PATHEXT when not directly in a bash shell or bash script (or program compiled to work inside this environment).

git bash (and msys and cygwin) are all about making windows look more posix. But still, it is windows, and we have to insist on treating it as "normal" windows - we can't have it as another "platform" that we have to consider.

I apologize for the confusion. Git bash is the mingw64 bash command window that is included in the Git Windows installation.

@patrickdepinguin 1) Regarding PATHEXT, prior to the execution of the npm install command, the PATHEXT is: .COM;.EXE;.BAT;.CMD;.VBS;.VBE;.JS;.JSE;.WSF;.WSH;.MSC

2) Changing the npm script doesn't do anything as it doesn't even find the npm script.

3) git bash uses mingw64.

4) digging deeply into front_end_build(), and following the code, check_call() which calls call() which calls Popen(...).. which finally runs CreateProcess(). Came across this tidbit: https://bugs.python.org/issue8557.

So under windows (if I understand that correctly), explicit inclusion of the extension is required. (Please do correct me if I'm wrong.) Which is probably why nodejs includes *.cmd in the nodejs dir for Windows installation. (still looking for the reason..)

sorry for the mess of a patch..

Also, forgot to mention. Under the Git Bash command prompt, running npm (without the .cmd) works as well as the fact that it doesn't depend on the .cmd existing (as I had renamed the npm.cmd to npmx.cmd).

Yes, for some reason npm added a special bash launcher that can be used under mingw/cygwin instead of native windows. It shouldn't be necessary (as any bash on windows also must obey PATHEXT and be able to run .com / .exe / .cmd files), but it might be convenient in some cases.

@edmundwong can you confirm that on plain windows, this works, using npm.cmd:

python -c "import subprocess; subprocess.check_call(['npm', '-v'])"

?

If so, does it fail when run inside a bash prompt?

That seems to be a minimal case that is close to the root cause, not involving anything Kallithea, and can help us understand what is going on.

Are you perhaps using a special mingw Python when launching from bash? Can you try to explicit use the python.exe used from a cmd prompt? But you found that inside git bash it fails, as Python cannot find npm.cmd? It seems like this must be a cygwin-ish environment. And if using shell=True, it still doesn't use the proper shell for the OS but a special git bash shell? The proper way to support that would be at the Python level. It seems wrong if we have to work around that.

@Mads Killerich

In a windows command prompt (with d:\nodejs in the PATH):

C:\projects\personal>python -c "import subprocess ;
subprocess.check_call(['npm', '-v'])"
Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "C:\Program Files (x86)\Git\python36\lib\subprocess.py", line 286, in check_call
    retcode = call(*popenargs, **kwargs)
  File "C:\Program Files (x86)\Git\python36\lib\subprocess.py", line 267, in call
    with Popen(*popenargs, **kwargs) as p:
  File "C:\Program Files (x86)\Git\python36\lib\subprocess.py", line 709, in __init__
    restore_signals, start_new_session)
  File "C:\Program Files (x86)\Git\python36\lib\subprocess.py", line 997, in _execute_child
    startupinfo)
FileNotFoundError: [WinError 2] The system cannot find the file specified

C:\projects\personal>

If I use 'npm.cmd' instead of 'npm':

C:\projects\personal>python -c "import subprocess ;
subprocess.check_call(['npm.cmd', '-v'])"
6.4.1

in a git bash:

$ python -c "import subprocess; subprocess.check_call(['npm', '-v'])"
Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "C:\Program Files\Git\python27\lib\subprocess.py", line 181, in check_call
    retcode = call(*popenargs, **kwargs)
  File "C:\Program Files\Git\python27\lib\subprocess.py", line 168, in call
    return Popen(*popenargs, **kwargs).wait()
  File "C:\Program Files\Git\python27\lib\subprocess.py", line 390, in __init__
    errread, errwrite)
  File "C:\Program Files\Git\python27\lib\subprocess.py", line 640, in _execute_child
    startupinfo)
WindowsError: [Error 2] The system cannot find the file specified

but using npm.cmd works:

$ python -c "import subprocess; subprocess.check_call(['npm.cmd', '-v'])"
6.4.1

To note though that Python was installed via python.org's msi and installed within the git path (i.e. c:\program files\Git\python27) as the windows git bash installation doesn't include python.

As far as I understand from the above, in a windows command prompt, "npm -v" runs the "npm.cmd -v" command. However, under python's subprocess, it totally ignores the PATHEXT file.

And I think you're right.. this is a Python issue and not Kallithea.
judging from https://bugs.python.org/issue15533 or https://bugs.python.org/issue8557.

Thanks for helping clarifying this.

Now we can completely rule out that it has anything to do with git bash. Do you agree? It would almost make sense to start over from scratch, only including information we now know is relevant ...

But ok, right, it makes sense that pure subprocess doesn't do like the shell does. But then I would expect plain 'npm' to work if using shell=True. (But even if so, I guess we might conclude that it is better to not use shell. Especially if pure subprocess use PATH anyway ... and I guess it does.)

Right now, I tend to prefer the solution of having some kind of "npm command" function or platform dependent "const". It could perhaps also be extra configurable with a .ini setting, but that might come close to giving a chicken-egg problem.

Just adding my experience with this issue here - once I modified all of the subprocess calls to include .cmd on the end, I was able to successfully get the front end to build. Here is what I modified in kallithea_cli_front_end.py

• 45: subprocess.check_call(['npm.cmd', 'install'], cwd=front_end_dir)
• 59: lesscpath = os.path.join(front_end_dir, 'node_modules', '.bin', 'lessc.cmd')
• 96: os.path.join(front_end_dir, 'node_modules', '.bin', 'license-checker.cmd'),

Hmm. Thanks for clarifying that these additional workarounds are needed too ...

But I wonder why Edmund didn't need it ... or didn't report it.

I went into Windows to see things for myself. I made a simple foobar.cmd file with 'echo hello' inside of it, to investigate away from npm.

c:\Users\tdescham>C:\python27\python -c "import subprocess; subprocess.call('foobar')"
Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "C:\python27\lib\subprocess.py", line 172, in call
    return Popen(*popenargs, **kwargs).wait()
  File "C:\python27\lib\subprocess.py", line 394, in __init__
    errread, errwrite)
  File "C:\python27\lib\subprocess.py", line 644, in _execute_child
    startupinfo)
WindowsError: [Error 2] The system cannot find the file specified


c:\Users\tdescham>C:\python27\python -c "import subprocess; subprocess.call('foobar.cmd')"
c:\Users\tdescham>echo hello
hello


c:\Users\tdescham>C:\python27\python -c "import subprocess; subprocess.call('foobar', shell=True)"
c:\Users\tdescham>echo hello
hello


c:\Users\tdescham>C:\python27\python -c "import subprocess; subprocess.call('foobar.cmd', shell=True)"
c:\Users\tdescham>echo hello
hello

So, based on the above, my conclusion is that 'shell=True' does actually fix the problem.

Also confirmed with npm, from a Windows command prompt:

C:\Users\tdescham>C:\python27\python -c "import subprocess; subprocess.call('npm -v', shell=True)"
6.4.1

For real executables, e.g. node.exe, there is no need to do anything special. subprocess.call('node') works fine, without shell=True or explicit extension. The problem is only relevant for scripts like foo.cmd. This is also hinted by comment https://bugs.python.org/issue8557#msg262399 :

CreateProcess executes PE executables and batch files (run via the %ComSpec% interpreter).
It automatically appends the .exe extension when searching for an executable. It does this via the lpExtension parameter of SearchPath [3]. 

Some .com files are PE executables (e.g. chcp.com). Otherwise it's not really usefully to
loop over the PATHEXT extensions unless you're using shell=True, since most are filetypes that CreateProcess doesn't support [4]. 

[4]: If Microsoft's Windows team cared at all about cross-platform
     idioms they'd add shebang support to CreateProcess, which
     would make all scripts, not just batch files, directly
     executable without requiring ShellExecuteEx and registered
     filetypes. ShellExecuteEx doesn't support a lot of useful
     creation flags that are only available by calling
     CreateProcess directly, and it also doesn't check ACLs to
     prevent executing a file. So scripts are second class
     citizens in Windows, which is why Python has to embed 
     scripts in .exe wrappers.

So, we need to know what kind of thing we are executing in order to know how to call it. Or, as suggested in some of the referenced links, do a lookup before calling.

@kiilerix Based on this info, what is your final preference to solve this issue?

@patrickdepinguin I agree, that is what I thought too. Thus, my initial solution should work too. But apparently it didn't? (Or ... Edmund's response could perhaps suggest that he didn't use shell=True?) Did you try my initial patch?

@ewongcc I sent a fix to the mailing list, see https://lists.sfconservancy.org/pipermail/kallithea-general/2019q1/002952.html I was able to reproduce the issue, and that patch fixes it at my end. Could you confirm?

@Mads Killerich I didn't use shell=True as I've always avoided using it. I'll try it with shell=True.

Ok.. I can confirm.. using shell=True does work. My question is "is it ok to do that?" I certainly don't want to introduce any security issues.. or screw something up

Would something like the following work?:

diff --git a/kallithea/bin/kallithea_cli_front_end.py b/kallithea/bin/kallithea_cli_front_end.py
--- a/kallithea/bin/kallithea_cli_front_end.py
+++ b/kallithea/bin/kallithea_cli_front_end.py
@@ -21,19 +21,21 @@ import subprocess
 import json
 import sys

 import kallithea


 def make_win_cmd(in_name):
     retval = in_name
+    ret_use_shell = False
     if sys.platform.startswith('win'):
         retval = '%s.cmd' % in_name
-    return retval
+        ret_use_shell = True
+    return retval, ret_use_shell


 @cli_base.register_command()
 @click.option('--install-deps/--no-install-deps', default=True,
         help='Skip installation of dependencies, via "npm".')
 @click.option('--generate/--no-generate', default=True,
         help='Skip generation of front-end files.')
 def front_end_build(install_deps, generate):
@@ -44,41 +46,41 @@ def front_end_build(install_deps, genera
     covers Python dependencies.

     The installation of front-end dependencies happens via the tool 'npm' which
     is expected to be installed already.
     """
     front_end_dir = os.path.abspath(os.path.join(kallithea.__file__, '..', 'front-end'))
     public_dir = os.path.abspath(os.path.join(kallithea.__file__, '..', 'public'))

-    npm_exe = make_win_cmd('npm')
+    npm_exe, use_shell = make_win_cmd('npm')

     if install_deps:
         click.echo("Running 'npm install' to install front-end dependencies from package.json")
-        subprocess.check_call([npm_exe, 'install'], cwd=front_end_dir)
+        subprocess.check_call([npm_exe, 'install'], cwd=front_end_dir, shell=use_shell)

     if generate:
         tmp_dir = os.path.join(front_end_dir, 'tmp')
         if not os.path.isdir(tmp_dir):
             os.mkdir(tmp_dir)

         click.echo("Building CSS styling based on Bootstrap")
         with open(os.path.join(tmp_dir, 'pygments.css'), 'w') as f:
             subprocess.check_call(['pygmentize',
                     '-S', 'default',
                     '-f', 'html',
                     '-a', '.code-highlight'],
                     stdout=f)
-        lesscpath = make_win_cmd(
+        lesscpath, use_shell = make_win_cmd(
             os.path.join(front_end_dir, 'node_modules', '.bin', 'lessc'))
         lesspath = os.path.join(front_end_dir, 'main.less')
         csspath = os.path.join(public_dir, 'css', 'style.css')
         subprocess.check_call([lesscpath, '--source-map',
                 '--source-map-less-inline', lesspath, csspath],
-                cwd=front_end_dir)
+                cwd=front_end_dir, shell=use_shell)

         click.echo("Preparing Bootstrap JS")
         shutil.copy(os.path.join(front_end_dir, 'node_modules', 'bootstrap', 'dist', 'js', 'bootstrap.js'), os.path.join(public_dir, 'js', 'bootstrap.js'))

         click.echo("Preparing jQuery JS with Flot, Caret and Atwho")
         shutil.copy(os.path.join(front_end_dir, 'node_modules', 'jquery', 'dist', 'jquery.min.js'), os.path.join(public_dir, 'js', 'jquery.min.js'))
         shutil.copy(os.path.join(front_end_dir, 'node_modules', 'jquery.flot', 'jquery.flot.js'), os.path.join(public_dir, 'js', 'jquery.flot.js'))
         shutil.copy(os.path.join(front_end_dir, 'node_modules', 'jquery.flot', 'jquery.flot.selection.js'), os.path.join(public_dir, 'js', 'jquery.flot.selection.js'))

The rationale for the ret_use_shell is to return to the calling code the fact that it is being run on a Windows system and should use shell=True. If it isn't a Windows system, set shell as false.

Ok. We could have saved a lot of time and confusion if my question had been answered ... or if it had been more clear that it wasn't an answered.

shell=True is evidently the right way to make plain "npm" work on Windows. It is annoying it doesn't work with argv on posix, and shell=True thus force us to use a command line snippet and thus have concerns about quoting. That is reliability and "not entirely right" concerns, but I don't see any serious security concerns.

The question is if we want to hardcode the assumption that these commands are .cmd on windows, or if we just want to be supportive of the idea that they might be .cmd . I think I prefer to just be supportive.

But how about keep using an argv, but use shell=kallithea.is_windows? Posix will keep using the native argv handling without shell and quoting, and windows subprocess will use its internal and secure list2cmdline. That will also be self-documenting that while testing might show that shell isn't needed on posix, it is needed on windows, and annotate will point to the more detailed discussion in the commit message. @patrickdepinguin right now I think that seems like the best solution ;-)

Ok that seems reasonable. I wasn't aware you can pass an array with shell=True.

I pushed a fix as 5867f45810da396080fc7018164bd3dac24b7f03 . Thanks for reporting!