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

[Thomas De Schampheleire]

El mar., 19 mar. 2019 a las 2:48, Mads Kiilerich
(<mads at kiilerich.com>) escribió:
>
> 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.

Ok, I pinged him via Bitbucket.
Note that I was able to reproduce the problem, and verified that this
patch fixes it.

>
>
> > 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.

I think quoting is the better approach. Basically adding double quotes
around each %s. I tested on Linux and Windows, seems to work fine.

>
>
> > +                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
>