Fix runtests.py failing if path to python executable contains a space

[freundTech]

Description

This fixes runtests.py failing if the path to the python executable contains a space.
@PrasanthChettri on gitter mentioned getting the error message 'C:\Program' is not recognized as an internal or external command, operable program or batch file. on windows.

This commit wraps the interpreter path in quotes to prevent the shell from splitting it.

I think this is the best solution for this at the moment. It would also be possible to replace all spaces with escaped spaces (also simple) or change runtests.py from using os.system to using subprocess (more complicated, but would prevent all shell escaping problems by not using a shell at all)

Test Plan

I haven't got a response from @PrasanthChettri on whether this fix is working for him yet, but from my own tests in a windows VM it should work.

I also tested that the script still works on linux.

[freundTech]

Quick force-push due to a typo in the commit message. Sorry about that.

[PrasanthChettri]

@freundTech Hi, I'll check it out and let you know

[PrasanthChettri]

Yeah this runs, awesome 👍
Edit Nope gives me this , dunno if it is normal or not, I ran the tests on this branch

[freundTech]

I can't reproduce this. Except for the daemon and mypy tests all tests run fine for me using this branch and a fresh Windows 10 VM.

[PrasanthChettri]

@freundTech Anybody else with windows can see if this branch works on their system.

[asottile]

this should use shlex.quote

[freundTech]

I looked at shlex.quote, but decided against using it, as it is only designed for Unix shells, not windows (See the big warning that was added to the documentation in python 3.10).

[asottile]

even so, just blindly adding single quotes around it is worse than shlex.quote -- even on windows

[freundTech]

It's not, because shlex.quote doesn't work at all on windows.
shlex.quote, at least it's implementation in the CPython Lib, surrounds the string with single quotes, which windows cmd doesn't support (I just tested it, because I wasn't sure).

On windows cmd "C:\Program Files\Python39\python.exe" works, while 'C:\Program Files\Python39\python.exe' prints The filename, directory name, or volume label syntax is incorrect..

I agree, that surrounding it with double quotes might doesn't prevent all problems, but it does prevent the most common one, which is spaces in paths. Other special characters are much less likely to appear in paths and this isn't security relevant, as it could only be exploited for a self-attack.

If preventing all possible escaping problems is important, then the correct solution would be to use subprocess instead of os.system, as I have already written in the PR description.

[asottile]

actually, some of the code at the top of the file is dead -- here's an improved patch:

diff --git a/runtests.py b/runtests.py
index ea7db85..55cca40 100755
--- a/runtests.py
+++ b/runtests.py
@@ -1,18 +1,9 @@
 #!/usr/bin/env python3
 import subprocess
 from subprocess import Popen
+from shlex import quote
 from os import system
-from sys import argv, exit, platform, executable, version_info
-
-
-# Use the Python provided to execute the script, or fall back to a sane default
-if version_info >= (3, 5, 0):
-    python_name = executable
-else:
-    if platform == 'win32':
-        python_name = 'py -3'
-    else:
-        python_name = 'python3'
+from sys import argv, exit, executable
 
 # Slow test suites
 CMDLINE = 'PythonCmdline'
@@ -55,7 +46,7 @@ MYPYC_OPT_IN = [MYPYC_RUN, MYPYC_RUN_MULTI]
 # time to run.
 cmds = {
     # Self type check
-    'self': python_name + ' -m mypy --config-file mypy_self_check.ini -p mypy',
+    'self': quote(executable) + ' -m mypy --config-file mypy_self_check.ini -p mypy',
     # Lint
     'lint': 'flake8 -j0',
     # Fast test cases only (this is the bulk of the test suite)

[freundTech]

For the dead code part I agree, as mypy currently requires python 3.5 or later.
However I would think that that should get it's own PR. I can also add it to this one if the maintainers would prefer that

[emmatyping]

I think submitting 2 PRs is best if you can @freundTech .

[emmatyping]

or change runtests.py from using os.system to using subprocess

Could you please do this @freundTech ? I think it is the better solution.

[freundTech]

EDIT: Never mind. This was entirely my fault. I forgot to remove double quotes from the input, which are usually removed by the shell.

Original comment: I started working on using subprocess instead, but have run into a weird problem:

pytest seems to break, when not running in a shell. I attached the output below.

Calling subprocess.run with shell=True works, but introduces the exact same problems os.system has. Not using shell=True results in the following (Repeat for all pytest tests. self and lint tests run fine):

error.txt

Also see WIP branch https://github.com/freundTech/mypy/tree/fix-runtests-subprocess.

[hauntsaninja]

Closed in favour of #10121