Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gh-114272: fix Windows test_asyncio/test_subprocess when sys.executable contains unescaped spaces #128160

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

gh-114272: fix Windows test_asyncio/test_subprocess when sys.executable contains unescaped spaces #128160

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
68 changes: 41 additions & 27 deletions Lib/test/test_asyncio/test_subprocess.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,12 +25,26 @@
if support.check_sanitizer(address=True):
raise unittest.SkipTest("Exposes ASAN flakiness in GitHub CI")


def get_quoted_sys_executable():
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you refactor the function and use shorter names for the attributes? it's a test file so it doesn't matter what we use. Or use an LRU-cache instead. OTOH, you can setup the test cases so that they have a test attribute (namely cls.sysexec = ... in setUpClass and you'd use as self.sysexec instead of sys.executable in the tests)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi picnixz.
I think caching is overkill. I'd do it for my own code but not for opensource to keep it simple. I'll remove it.

if hasattr(get_quoted_sys_executable, '_cached_get_quoted_sys_executable'):
return get_quoted_sys_executable._cached_get_quoted_sys_executable

sysExecutable = sys.executable
if not ((sysExecutable.startswith('"') and sysExecutable.endswith('"')) or
(sysExecutable.startswith("'") and sysExecutable.endswith("'"))):
if ' ' in (sysExecutable if sys.platform == 'win32' else sysExecutable.replace('\\ ', '')):
Comment on lines +34 to +36
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't we use some smarter escape mechanism? for instance, something in os.path or shlex.quote perhaps?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

os.path does not add the quotes and shlex.quote quotes a quoted string ad-nauseam.
i think we should merge this have less failed tests, and somebody later (maybe me) should fix shlex.quote

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

*fewer failed test
I do not know if other libraries should benefit from this . I just run python -m unittest for the first time, and the only failures are fixed by this. There are some skips, but that's also for another time.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @zooba for how we should proceed here.

sysExecutable = f'"{sysExecutable}"'

get_quoted_sys_executable._cached_get_quoted_sys_executable = sysExecutable
return sysExecutable

# Program blocking
PROGRAM_BLOCKED = [sys.executable, '-c', 'import time; time.sleep(3600)']
PROGRAM_BLOCKED = [get_quoted_sys_executable(), '-c', 'import time; time.sleep(3600)']

# Program copying input to output
PROGRAM_CAT = [
sys.executable, '-c',
get_quoted_sys_executable(), '-c',
';'.join(('import sys',
'data = sys.stdin.buffer.read()',
'sys.stdout.buffer.write(data)'))]
Expand Down Expand Up @@ -209,7 +223,7 @@ def test_kill(self):

def test_kill_issue43884(self):
if sys.platform == 'win32':
blocking_shell_command = f'"{sys.executable}" -c "import time; time.sleep(2)"'
blocking_shell_command = f'"{get_quoted_sys_executable()}" -c "import time; time.sleep(2)"'
else:
blocking_shell_command = 'sleep 1; sleep 1'
creationflags = 0
Expand Down Expand Up @@ -255,7 +269,7 @@ def test_send_signal(self):
old_handler = signal.signal(signal.SIGHUP, signal.SIG_DFL)
try:
code = 'import time; print("sleeping", flush=True); time.sleep(3600)'
args = [sys.executable, '-c', code]
args = [get_quoted_sys_executable(), '-c', code]
proc = self.loop.run_until_complete(
asyncio.create_subprocess_exec(
*args,
Expand Down Expand Up @@ -304,7 +318,7 @@ def test_stdin_broken_pipe(self):
# the program ends before the stdin can be fed
proc = self.loop.run_until_complete(
asyncio.create_subprocess_exec(
sys.executable, '-c', code,
get_quoted_sys_executable(), '-c', code,
stdin=subprocess.PIPE,
**kwargs
)
Expand All @@ -330,7 +344,7 @@ def test_communicate_ignore_broken_pipe(self):
# the program ends before the stdin can be fed
proc = self.loop.run_until_complete(
asyncio.create_subprocess_exec(
sys.executable, '-c', 'pass',
get_quoted_sys_executable(), '-c', 'pass',
stdin=subprocess.PIPE,
)
)
Expand Down Expand Up @@ -362,7 +376,7 @@ async def connect_read_pipe_mock(*args, **kw):
self.loop.connect_read_pipe = connect_read_pipe_mock

proc = await asyncio.create_subprocess_exec(
sys.executable, '-c', code,
get_quoted_sys_executable(), '-c', code,
stdin=asyncio.subprocess.PIPE,
stdout=asyncio.subprocess.PIPE,
limit=limit,
Expand Down Expand Up @@ -390,7 +404,7 @@ def test_stdin_not_inheritable(self):
async def len_message(message):
code = 'import sys; data = sys.stdin.read(); print(len(data))'
proc = await asyncio.create_subprocess_exec(
sys.executable, '-c', code,
get_quoted_sys_executable(), '-c', code,
stdin=asyncio.subprocess.PIPE,
stdout=asyncio.subprocess.PIPE,
stderr=asyncio.subprocess.PIPE,
Expand All @@ -409,7 +423,7 @@ def test_empty_input(self):
async def empty_input():
code = 'import sys; data = sys.stdin.read(); print(len(data))'
proc = await asyncio.create_subprocess_exec(
sys.executable, '-c', code,
get_quoted_sys_executable(), '-c', code,
stdin=asyncio.subprocess.PIPE,
stdout=asyncio.subprocess.PIPE,
stderr=asyncio.subprocess.PIPE,
Expand All @@ -428,7 +442,7 @@ def test_devnull_input(self):
async def empty_input():
code = 'import sys; data = sys.stdin.read(); print(len(data))'
proc = await asyncio.create_subprocess_exec(
sys.executable, '-c', code,
get_quoted_sys_executable(), '-c', code,
stdin=asyncio.subprocess.DEVNULL,
stdout=asyncio.subprocess.PIPE,
stderr=asyncio.subprocess.PIPE,
Expand All @@ -447,7 +461,7 @@ def test_devnull_output(self):
async def empty_output():
code = 'import sys; data = sys.stdin.read(); print(len(data))'
proc = await asyncio.create_subprocess_exec(
sys.executable, '-c', code,
get_quoted_sys_executable(), '-c', code,
stdin=asyncio.subprocess.PIPE,
stdout=asyncio.subprocess.DEVNULL,
stderr=asyncio.subprocess.PIPE,
Expand All @@ -466,7 +480,7 @@ def test_devnull_error(self):
async def empty_error():
code = 'import sys; data = sys.stdin.read(); print(len(data))'
proc = await asyncio.create_subprocess_exec(
sys.executable, '-c', code,
get_quoted_sys_executable(), '-c', code,
stdin=asyncio.subprocess.PIPE,
stdout=asyncio.subprocess.PIPE,
stderr=asyncio.subprocess.DEVNULL,
Expand All @@ -487,7 +501,7 @@ def test_devstdin_input(self):
async def devstdin_input(message):
code = 'file = open("/dev/stdin"); data = file.read(); print(len(data))'
proc = await asyncio.create_subprocess_exec(
sys.executable, '-c', code,
get_quoted_sys_executable(), '-c', code,
stdin=asyncio.subprocess.PIPE,
stdout=asyncio.subprocess.PIPE,
stderr=asyncio.subprocess.PIPE,
Expand Down Expand Up @@ -643,7 +657,7 @@ async def _test_popen_error(self, stdin):
with warnings.catch_warnings(record=True) as warns:
with self.assertRaises(exc):
await asyncio.create_subprocess_exec(
sys.executable,
get_quoted_sys_executable(),
'-c',
'pass',
stdin=stdin
Expand Down Expand Up @@ -671,7 +685,7 @@ async def execute():
'sys.exit(1)'])

process = await asyncio.create_subprocess_exec(
sys.executable, '-c', code,
get_quoted_sys_executable(), '-c', code,
stdout=asyncio.subprocess.PIPE,
)

Expand All @@ -687,15 +701,15 @@ async def execute():
def test_create_subprocess_exec_text_mode_fails(self):
async def execute():
with self.assertRaises(ValueError):
await subprocess.create_subprocess_exec(sys.executable,
await subprocess.create_subprocess_exec(get_quoted_sys_executable(),
text=True)

with self.assertRaises(ValueError):
await subprocess.create_subprocess_exec(sys.executable,
await subprocess.create_subprocess_exec(get_quoted_sys_executable(),
encoding="utf-8")

with self.assertRaises(ValueError):
await subprocess.create_subprocess_exec(sys.executable,
await subprocess.create_subprocess_exec(get_quoted_sys_executable(),
errors="strict")

self.loop.run_until_complete(execute())
Expand All @@ -704,26 +718,26 @@ def test_create_subprocess_shell_text_mode_fails(self):

async def execute():
with self.assertRaises(ValueError):
await subprocess.create_subprocess_shell(sys.executable,
await subprocess.create_subprocess_shell(get_quoted_sys_executable(),
text=True)

with self.assertRaises(ValueError):
await subprocess.create_subprocess_shell(sys.executable,
await subprocess.create_subprocess_shell(get_quoted_sys_executable(),
encoding="utf-8")

with self.assertRaises(ValueError):
await subprocess.create_subprocess_shell(sys.executable,
await subprocess.create_subprocess_shell(get_quoted_sys_executable(),
errors="strict")

self.loop.run_until_complete(execute())

def test_create_subprocess_exec_with_path(self):
async def execute():
p = await subprocess.create_subprocess_exec(
os_helper.FakePath(sys.executable), '-c', 'pass')
os_helper.FakePath(get_quoted_sys_executable()), '-c', 'pass')
await p.wait()
p = await subprocess.create_subprocess_exec(
sys.executable, '-c', 'pass', os_helper.FakePath('.'))
get_quoted_sys_executable(), '-c', 'pass', os_helper.FakePath('.'))
await p.wait()

self.assertIsNone(self.loop.run_until_complete(execute()))
Expand All @@ -739,7 +753,7 @@ async def check_stdout_output(self, coro, output):

def test_create_subprocess_env_shell(self) -> None:
async def main() -> None:
executable = sys.executable
executable = get_quoted_sys_executable()
if sys.platform == "win32":
executable = f'"{executable}"'
cmd = f'''{executable} -c "import os, sys; sys.stdout.write(os.getenv('FOO'))"'''
Expand All @@ -754,7 +768,7 @@ async def main() -> None:

def test_create_subprocess_env_exec(self) -> None:
async def main() -> None:
cmd = [sys.executable, "-c",
cmd = [get_quoted_sys_executable(), "-c",
"import os, sys; sys.stdout.write(os.getenv('FOO'))"]
env = os.environ.copy()
env["FOO"] = "baz"
Expand Down Expand Up @@ -825,7 +839,7 @@ async def main() -> None:
exit_future = asyncio.Future()
code = 'import sys; sys.stdout.write("stdout"); sys.stderr.write("stderr")'
transport, _ = await loop.subprocess_exec(lambda: MyProtocol(exit_future),
sys.executable, '-c', code, stdin=None)
get_quoted_sys_executable(), '-c', code, stdin=None)
await exit_future
transport.close()

Expand Down Expand Up @@ -858,7 +872,7 @@ async def get_command_stdout(cmd, *args):

async def main():
outputs = [f'foo{i}' for i in range(10)]
res = await asyncio.gather(*[get_command_stdout(sys.executable, '-c',
res = await asyncio.gather(*[get_command_stdout(get_quoted_sys_executable(), '-c',
f'print({out!r})') for out in outputs])
self.assertEqual(res, outputs)

Expand Down
Loading