FDKFeatureCompiler is broken on POSIX

[anthrotype]

This commit which added shell=True to the subprocess invocation of makeotf may have fixed it for windows, but broke it for POSIX:

6d10690

Quoting subprocess docs

On POSIX with shell=True, the shell defaults to /bin/sh... If args is a sequence, the first item specifies the command string, and any additional items will be treated as additional arguments to the shell itself. That is to say, Popen does the equivalent of:

 Popen(['/bin/sh', '-c', args[0], args[1], ...])

So on Unix, fontmake is trying to call makeotf as: /bin/sh -c makeotf -f font.ttf -ff feautures -o output.ttf, which means that the shell is only treating the first argument after -c as the command, the rest is ignored as those are not shell options, but options to the makeotf command...

Interestingly nobody complained in the last 8 months...
Which makes me think not many people rely on this feature.

I would actually like to kill it, or at least remove it from fontmake proper, and maybe readd it later as a new optional ufo2ft feature compiler class.

But in any case, we should implement this properly, and with tests, otherwise it gets unnoticed like now.

Perhaps we can do that after makeotf will return a sensible exit code, instead of always success, leaving us in the position of having to parse its output to determine whether it has failed or not:
adobe-type-tools/afdko#417

Or even better, if ever MakeOTF.py exposed some function that we could import and call directly from fontmake instead of having to create temporary files and spawn a new process.

[anthrotype]

in the meantime, things have changed and now afdko comes installable via pip so it has nice .exe native launchers instead of using the old .cmd batch wrappers.
Therefore this workaround in #365 should no longer be needed.

anybody with a windows machine at hand and with the afdko installed (from pip) could please try this command and let me know if it works now, without adding the shell=True parameter?

import subprocess
subprocess.check_output(["makeotf", "-h"])

[anthrotype]

yes, I tried on my Windows VM without shell=True and it works. Will revert that commit for now.

[chrissimpkins]

Try concatenating the entire command string before you use it as an argument in the call to subprocess.check_output and then use it as a single argument rather than as an argument list as you do with Popen. And definitely consider using a shellquote style approach with the command string if you use this... Don't know about the consequences on Win but this approach works for me on *nix systems.

[anthrotype]

no I don't want to do that. It's not necessary. A list of strings as args is the recommended one.

[anthrotype]

https://docs.python.org/3/library/subprocess.html#frequently-used-arguments

Providing a sequence of arguments is generally preferred, as it allows the module to take care of any required escaping and quoting of arguments (e.g. to permit spaces in file names). If passing a single string, either shell must be True (see below) or else the string must simply name the program to be executed without specifying any arguments.

[chrissimpkins]

Here is an example provided in the current Py3 docs:

subprocess.check_output(
    "ls non_existent_file; exit 0",
    stderr=subprocess.STDOUT,
    shell=True)

[anthrotype]

the use of shell=True is discouraged for security considerations, and is only needed when one needs access to shell-specific features like pipes, wildcards, env variable expansion, etc. (which we don't need in this particular case).

[chrissimpkins]

the use of shell=True is discouraged for security considerations

👍 shlex.quote is available when you must use it.

The Py2 documentation provides an example of list arguments with check_output but only with the executable and a single argument to the executable. It looks like Py3 encourages use of subprocess.run rather than check_output in current version.

Here are the cPython repository tests for subprocess.check_output. They all test with the Python interpreter as executable with -c option followed by a single, complete string of the Python source (and without shell=True). https://github.com/python/cpython/blob/master/Lib/test/test_subprocess.py#L149-L225

Closed so I will quit investigating :)

[miguelsousa]

@anthrotype I've issued a pre-release build that contains the fix for adobe-type-tools/afdko#417, in case you want to give it a try.