[PATCH 2 of 8] cli: fix 'front-end-build' on Windows (Issue #332)

[Mads Kiilerich]

On 3/18/19 11:09 PM, Thomas De Schampheleire wrote:
> # HG changeset patch
> # User Thomas De Schampheleire <thomas.de_schampheleire at nokia.com>
> # Date 1552944330 -3600
> #      Mon Mar 18 22:25:30 2019 +0100
> # Node ID 04bfb03e74e911d719bb0917c4b486b158779262
> # Parent  23d84aa503fe9ef7c49c81f7bafe89a8e0073e77
> cli: fix 'front-end-build' on Windows (Issue #332)
>
> On Windows, the command 'npm' is actually 'npm.cmd', a script and not a PE
> executable. 'subprocess' will not resolve 'npm' into 'npm.cmd', while it
> would resolve e.g. 'git' into 'git.exe', as the latter _is_ a PE
> executable.
>
> One solution is to change all references to the problematic scripts by
> adding the '.cmd' extension explicitly, but this would not be compatible
> with UNIX systems and thus require special handling.
>
> By using 'shell=True', we can have one solution supporting both Windows and
> UNIX.


I agree this *should* work. It is slightly annoying that it can't be 
combined with using the arg list that is superior (especially on unix) 
but we have to use a command line snippet, but it kind of makes sense.

But I would be great if Edmund from 
https://bitbucket.org/conservancy/kallithea/issues/332/ could confirm 
the fix.


> 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
> @@ -42,7 +42,8 @@ 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)
> +        # shell=True to support Windows, as the command is actually a script


Perhaps clarify that it is a .cmd script.

And perhaps also mention (in the commit message, not repeated here) that 
there on windows also might be a mingw/msys/cygwin "sh" script without 
extension ... but that will never be picked up by the standard windows 
shell.


> +        subprocess.check_call('npm install', shell=True, cwd=front_end_dir)
>   
>       if generate:
>           tmp_dir = os.path.join(front_end_dir, 'tmp')
> @@ -59,9 +60,11 @@ def front_end_build(install_deps, genera
>           lesscpath = 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)
> +        # shell=True to support Windows, as the command is actually a script
> +        subprocess.check_call(
> +                '%s --source-map --source-map-less-inline %s %s'
> +                % (lesscpath, lesspath, csspath),


Now we get the problem of having to shell quote (absolute) paths. Or 
perhaps we can claim that we only support shell-safe paths. But that 
should perhaps be documented. And checked.


> +                shell=True, cwd=front_end_dir)
>   
>           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'))
> @@ -90,13 +93,13 @@ def front_end_build(install_deps, genera
>           shutil.copytree(os.path.join(front_end_dir, 'node_modules', 'codemirror'), os.path.join(public_dir, 'codemirror'))
>   
>           click.echo("Generating LICENSES.txt")
> +        license_checker_path = os.path.join(front_end_dir, 'node_modules', '.bin', 'license-checker')
>           check_licensing_json_path = os.path.join(tmp_dir, 'licensing.json')
>           licensing_txt_path = os.path.join(public_dir, 'LICENSES.txt')
> -        subprocess.check_call([
> -            os.path.join(front_end_dir, 'node_modules', '.bin', 'license-checker'),
> -            '--json',
> -            '--out', check_licensing_json_path,
> -            ], cwd=front_end_dir)
> +        # shell=True to support Windows, as the command is actually a script
> +        subprocess.check_call('%s --json --out %s'
> +                % (license_checker_path, check_licensing_json_path),
> +                shell=True, cwd=front_end_dir)
>           with open(check_licensing_json_path) as jsonfile:
>               rows = json.loads(jsonfile.read())
>               with open(licensing_txt_path, 'w') as out:


/Mads