Biopython: problem with parsing command line options in MuscleCommandline(clwstrict=True)

[wdecoster]

Setup

I am reporting a problem with Biopython 1.76, Python 3.7.7 and operating system CentOS7

>>> import sys; print(sys.version)
3.7.7 (default, May  7 2020, 21:25:33) 
[GCC 7.3.0]
>>> import platform; print(platform.python_implementation()); print(platform.platform())
CPython
Linux-3.10.0-1062.18.1.el7.x86_64-x86_64-with-centos-7.8.2003-Core
>>> import Bio; print(Bio.__version__)
1.76

My code which doesn't work:

subprocess.Popen((str(MuscleCommandline(clwstrict=True)),
                              stdin=subprocess.PIPE,
                              stdout=subprocess.PIPE,
                              stderr=subprocess.PIPE,
                              universal_newlines=True)

Expected behaviour

Executes muscle (opens it to read from stdin to feed fasta files afterwards). Muscle is installed and in $PATH.

Actual behaviour

Returns:
FileNotFoundError: [Errno 2] No such file or directory: 'muscle -clwstrict': 'muscle -clwstrict'

However, what does work is without clwstrict=True.
I could fix this problem with the shlex module to parse command line options before passing those to subprocess:

````
subprocess.Popen(shlex.split(str(MuscleCommandline(clwstrict=True))),
stdin=subprocess.PIPE,
stdout=subprocess.PIPE,
stderr=subprocess.PIPE,
universal_newlines=True)

(*Please fill this in, and provide any exception message in full*)

### Steps to reproduce

#### Errors:

import subprocess
subprocess.Popen((str(MuscleCommandline(clwstrict=True)),
stdin=subprocess.PIPE,
stdout=subprocess.PIPE,
stderr=subprocess.PIPE,
universal_newlines=True)

#### Works:

subprocess.Popen(shlex.split(str(MuscleCommandline(clwstrict=True))),
stdin=subprocess.PIPE,
stdout=subprocess.PIPE,
stderr=subprocess.PIPE,
universal_newlines=True)
```

[peterjc]

There is an example very much like this in our tutorial, "MUSCLE using stdout" under http://biopython.org/DIST/docs/tutorial/Tutorial.html which does this:

>>> import subprocess
>>> from Bio.Align.Applications import MuscleCommandline
>>> muscle_cline = MuscleCommandline(input="opuntia.fasta")
>>> child = subprocess.Popen(str(muscle_cline),
...                          stdout=subprocess.PIPE,
...                          stderr=subprocess.PIPE,
...                          universal_newlines=True,
...                          shell=(sys.platform!="win32"))
>>> from Bio import AlignIO
>>> align = AlignIO.read(child.stdout, "fasta")
>>> print(align)
SingleLetterAlphabet() alignment with 7 rows and 906 columns
TATACATTAAAGGAGGGGGATGCGGATAAATGGAAAGGCGAAAG...AGA gi|6273289|gb|AF191663.1|AF191663
TATACATTAAAGGAGGGGGATGCGGATAAATGGAAAGGCGAAAG...AGA gi|6273291|gb|AF191665.1|AF191665
TATACATTAAAGGAGGGGGATGCGGATAAATGGAAAGGCGAAAG...AGA gi|6273290|gb|AF191664.1|AF191664
TATACATTAAAGAAGGGGGATGCGGATAAATGGAAAGGCGAAAG...AGA gi|6273287|gb|AF191661.1|AF191661
TATACATAAAAGAAGGGGGATGCGGATAAATGGAAAGGCGAAAG...AGA gi|6273286|gb|AF191660.1|AF191660
TATACATTAAAGAAGGGGGATGCGGATAAATGGAAAGGCGAAAG...AGA gi|6273285|gb|AF191659.1|AF191659
TATACATTAAAGAAGGGGGATGCGGATAAATGGAAAGGCGAAAG...AGA gi|6273284|gb|AF191658.1|AF191658

The key difference looks to be using shell=True on Linux?

Note my goal in the example was writing something which would work cross platform - if you can restrict to Linux only, avoiding shell=True is considered best practice from a security point of view - not likely to be an issue unless your script takes user input which might appear in the command line via the filenames etc.

If you want to avoid shell=True from memory you need to give a list of strings, not a single string - which your example does via shlex.split.

[peterjc]

P.S. Cross reference #2877 - you might be interested in that, but equally this is a good example that writing cross platform code with subprocess can be tricky.

[wdecoster]

I'm on Linux, and that's the only OS that I intend to support. I use the example from http://biopython.org/DIST/docs/tutorial/Tutorial.html#htoc85, but that example doesn't work for me without the shlex part.
The muscle -clwstrict command needs to get split in ['muscle', '-clwstrict']

[peterjc]

There does seem to be a problem with the Tutorial examples (not just missing import sys), it used to work... there is a small chance there was a Python 2 vs 3 issue in our updates.

[alvyjudy]

subprocess requires the commands to be provided in a list of strings (which might be why shlex fixes the issue).

I infer that shell=True works because subprocess passes the responsibility of interpreting the command to the system rather than its own implementation, judging from this line of the doc

Unlike some other popen functions, this implementation will never implicitly call a system shell. This means that all characters, including shell metacharacters, can safely be passed to child processes. If the shell is invoked explicitly, via shell=True, it is the application’s responsibility to ensure that all whitespace and metacharacters are quoted appropriately to avoid shell injection

With that said, I suspect that the same issue would arise with other command line parser if they are passed to subprocess via str() . We can either change the tutorial to include shlex or add a method to AbstractCommandLine that returns a list of strings (or maybe there is already one? i'm quite new here:)

I'll be happy to submit a PR if that's desired (after figuring out which route to go of course)

[peterjc]

I took the cross platform view that a single command line string was the common interface (and therefore the code uses shell=True as needed). Keeping that approach and working out what hopefully small change are needed to the code or the examples seems easier (but see also final paragraph).

At the time this and the Tutorial was written I could test it on Windows XP, macOS and Linux - but now I don't have easy access to Windows anymore.

Using lists of strings (pre-broken up like sys.argv) is perfect for Linux and macOS because the OS does this anyway to command line strings before calling the tool and giving it argv. The trouble we ran into is on Windows the command is given the string, and has to break it up itself - and they don't all do it the same way as Unix style platforms. i.e. If we built a list-of-strings based command to give to subprocess, IIRC it didn't always work as expected on Windows. This way round subprocess has a function which turns the list-of-strings into a single command line string, which is fraught with special cases - not just spaces in filenames.

i.e. We might have to keep the single-string-command route for Windows, while adding a list-of-strings alternative for Linux and macOS? I think that would work OK.

All in all it was (and is) a pain, which is part of the reason why in #2877 I am suggesting dropping our command line wrappers completely. It is something not specific to bioinformatics, and perhaps better left to a generic third party library or the standard library. That still means a bunch of work in the short term updating all our documentation and the examples.

[alvyjudy]

I actually prefer dropping the commandline wrapper. It doesn't necessarily make things easier since now it creates multiple interfaces that one has to learn (the wrapper, subprocess and the CLI itself, and how they all chain together). The documentation would be much cleaner if it teaches the CLI and how to use it with subprocess.

[peterjc]

So for #2877 would you suggest using subprocess directly? And if so, building the command as a single long string, or a list of strings?

[alvyjudy]

I might need to look into the windows environment myself first since I’m not that familiar with that environment :(