tools/pyboard: Run exec: command as a string.

[aykevl]

The Python documentation recommends to pass the command as a string when using Popen(..., shell=True).
https://docs.python.org/3.5/library/subprocess.html#subprocess.Popen

The shell argument (which defaults to False) specifies whether to use the shell as the program to execute. If shell is True, it is recommended to pass args as a string rather than as a sequence.

I noticed this problem because I tried running tests with exec:cmd... and I passed some arguments (which somehow didn't end up in the argv of the called process without this change).

[dpgeorge]

I noticed this problem because I tried running tests with exec:cmd... and I passed some arguments (which somehow didn't end up in the argv of the called process without this change).

Can you provide a reproducible case that gave this issue? It would be good to understand what the issue is/was and why this is the right fix. This is a subtle change that may break other things.

[aykevl]

The way I can reproduce it is with this command, which runs the binary for the PCA10028 in my emulator. The first argument is the firmware binary as compiled with make BOARD=pca10028 in ports/nrf.

./run-tests --device 'exec:../../emculator/emculator ../ports/nrf/build-pca10028/firmware.bin' --target nrf

I once tried printing all arguments it receives and it receives none without this PR, thus complaining about a missing argument. I also tried passing the unix micropython binary to run-tests but it causes a syntax error, probably because it doesn't understand raw mode.

Are there other places where exec: is used? I couldn't find any in the source tree.

[aykevl]

Here is a better way to reproduce the error (aborted because it hangs):

$ cat repro.py 
#!/usr/bin/env python

import sys
sys.stderr.write('%r' % (sys.argv))

$ ./run-tests --device 'exec:./repro.py foo bar' --target nrf
['./repro.py']^CTraceback (most recent call last):
  File "./run-tests", line 584, in <module>
    main()
  File "./run-tests", line 538, in main
    pyb.enter_raw_repl()
  File "/home/ayke/src/micropython/tests/pyboard.py", line 295, in enter_raw_repl
    data = self.read_until(1, b'raw REPL; CTRL-B to exit\r\n>')
  File "/home/ayke/src/micropython/tests/pyboard.py", line 265, in read_until
    data = self.serial.read(min_num_bytes)
  File "/home/ayke/src/micropython/tests/pyboard.py", line 178, in read
    data += self.subp.stdout.read(size - len(data))
KeyboardInterrupt

You can see repro.py only gets a single argument, not all three as expected.

This is the relevant line in the Python source: https://github.com/python/cpython/blob/3.5/Lib/subprocess.py#L1176

So the executed command (in the repro above) will be something like ['sh', '-c', './repro.py', 'foo', 'bar'], which is incorrect as all parameters must be passed in the -c argument to be parsed by sh: ['sh', '-c', './repro.py foo bar'].

[pfalcon]

Are there other places where exec: is used? I couldn't find any in the source tree.

exec: and execpty: support was introduced to allow to communicate with QEMU, as prompted by Zephyr port: 647e72c . In the end, it was found that Zephyr's QEMU works "better" using execpty:, and that's what can be grepped in the codebase. The change introduced by this patch looks right, especially if backed by that quotation from the docs. But please be ready to test such changes against a well-known kind of tool (like QEMU), not just your own.

[dpgeorge]

Thanks @aykevl for the additional info. I could reproduce the problem.

So the executed command (in the repro above) will be something like ['sh', '-c', './repro.py', 'foo', 'bar'], which is incorrect as all parameters must be passed in the -c argument to be parsed by sh: ['sh', '-c', './repro.py foo bar'].

Yes, that's the crux of the issue, that when passed as a list, the additional arguments (foo and bar above) are passed to the shell, not to the executing program.

I tested also with the zephyr port, changing pyboard.py to use shell=True for the ProcessPtyToTerminal class. With shell enabled there, cmd must also be passed as a string for it to work.

Merged in 0d7a088 with some additional comments in the commit message.