[libcxx] [test] Quote the python executable in the executor #68208
Conversation
mstorsjo
added
the
libc++
|
@llvm/pr-subscribers-libcxx ChangesThis reapplies the change from On Windows, the Python interpreter is by default installed in a path like "C:\Program Files\Python38\python.exe", which requires quoting when included in a concatenated command string (as opposed to a command list, where each argument is a separate element). This doesn't show up in the CI environments as they use Python installed in a different directory, like C:\Python. Full diff: https://github.com/llvm/llvm-project/pull/68208.diff 1 Files Affected:
diff --git a/libcxx/utils/libcxx/test/params.py b/libcxx/utils/libcxx/test/params.py
index d4e4b722347d623..16dcc837108c550 100644
--- a/libcxx/utils/libcxx/test/params.py
+++ b/libcxx/utils/libcxx/test/params.py
@@ -320,7 +320,7 @@ def getStdFlag(cfg, std):
Parameter(
name="executor",
type=str,
- default=f"{sys.executable} {Path(__file__).resolve().parent.parent.parent / 'run.py'}",
+ default=f"\"{sys.executable}\" {Path(__file__).resolve().parent.parent.parent / 'run.py'}",
help="Custom executor to use instead of the configured default.",
actions=lambda executor: [AddSubstitution("%{executor}", executor)],
)
|
libcxx/utils/libcxx/test/params.py
Outdated
| @@ -320,7 +320,7 @@ def getStdFlag(cfg, std): | |||
| Parameter( | |||
| name="executor", | |||
| type=str, | |||
| default=f"{sys.executable} {Path(__file__).resolve().parent.parent.parent / 'run.py'}", | |||
| default=f"\"{sys.executable}\" {Path(__file__).resolve().parent.parent.parent / 'run.py'}", | |||
There was a problem hiding this comment.
Could also use shutil.quote(sys.executable) to avoid unnecessary quoting?
There was a problem hiding this comment.
Yes, that’d probably work too - I’ll try that out.
I guess it could; I guess most people would avoid checking out the source in such a path with spaces, but the default install location of Python usually is in a place with spaces. I’ll try adding that as well. |
|
I updated the branch so that it uses a proper quote function, and quote both the script and the python executable. The proper python cross platform quoting for shell commands is |
libcxx/utils/libcxx/test/params.py
Outdated
| @@ -320,7 +325,7 @@ def getStdFlag(cfg, std): | |||
| Parameter( | |||
| name="executor", | |||
| type=str, | |||
| default=f"\"{sys.executable}\" {Path(__file__).resolve().parent.parent.parent / 'run.py'}", | |||
| default=f"{quote(sys.executable)} {quote(str(Path(__file__).resolve().parent.parent.parent / 'run.py'))}", | |||
There was a problem hiding this comment.
| default=f"{quote(sys.executable)} {quote(str(Path(__file__).resolve().parent.parent.parent / 'run.py'))}", | |
| default=default=subprocess.list2cmdline(sys.executable,str(Path(file).resolve().parent.parent.parent / 'run.py')), |
Wouldn't this be easier?
There was a problem hiding this comment.
Thanks, indeed it would! (There’s duplicate default= in there though.) I’ll give it a shot later tonight.
There was a problem hiding this comment.
Hmm actually it looks like list2cmdline is a private implementation detail: https://github.com/python/cpython/blob/6e97a9647ae028facb392d12fc24973503693bd6/Lib/subprocess.py#L66 so should probably use shlex.quote() instead.
There was a problem hiding this comment.
Oh, good catch. I saw some discussions about whether this function should be made public (as they can't practically change it anyway, since lots of people seem to use it) - but for now I guess it'd be best to not use it.
If we're not using that one, I'm on the fence about whether we should go for shlex.quote(), always wrapping double quotes (like we did in cmake before), or maybe a custom function that wraps quotes around it if there are spaces in the string. shlex.quote() isn't generally a great fit, as it is documented to not be cross platform - it specifically does quoting for unix paths/shells. For Windows paths, it always quotes them as they contain backslashes. (Always quoting them isn't an issue in itself per se, though. And the fact that shlex.quote() uses single quotes instead of double quotes, is ok as long as we're using the lit internal shell instead of an actual Windows shell.)
There was a problem hiding this comment.
Switched to shlex.quote now. Not ideal for Windows paths, but should work as long as we're using the lit internal shell.
This reapplies the change from c218c80, which was lost in the refactoring in 78d649a. On Windows, the Python interpreter is by default installed in a path like "C:\Program Files\Python38\python.exe", which requires quoting when included in a concatenated command string (as opposed to a command list, where each argument is a separate element). This doesn't show up in the CI environments as they use Python installed in a different directory, like C:\Python.
Don't use shlex.quote(), that's documented to only be relevant for Unix (and it uses single quotes which aren't normally tolerated by windows shells - even if the lit internal shell does handle them). Additionally, shlex.quote() always quotes paths that include backslashes, even if those don't need quoting on Windows.
This isn't ideal on Windows, but subprocess.list2cmdline is private. As long as we're using the lit internal shell, the shlex quoting should work fine.
mstorsjo
force-pushed
the
libcxx-quote-executor
branch
from
2c087a9 to
8bae7f4
Compare
This reapplies the change from
c218c80, which was lost in the refactoring in 78d649a.
On Windows, the Python interpreter is by default installed in a path like "C:\Program Files\Python38\python.exe", which requires quoting when included in a concatenated command string (as opposed to a command list, where each argument is a separate element).
This doesn't show up in the CI environments as they use Python installed in a different directory, like C:\Python.