[libcxx-commits] [PATCH] D99242: [libcxx] [test] Quote env variables that are set with a shell "export" in ssh.py

[Martin Storsjö via Phabricator via libcxx-commits]

mstorsjo added inline comments.


================
Comment at: libcxx/utils/ssh.py:111
         if args.env:
-            remoteCommands.append('export {}'.format(' '.join(args.env)))
+            remoteCommands.append('export {}'.format(pipes.quote(' '.join(args.env))))
         remoteCommands.append(subprocess.list2cmdline(commandLine))
----------------
Quuxplusone wrote:
> `pipes.quote` is gone and/or undocumented in Python 3; the Internet tells me to use `shlex.quote` instead.
> 
> I'm skeptical of "shell escaping" cleanups because it's essentially impossible to do right, and so any //given// commit is likely to make matters more complicated without making them any more correct. I'd like the commit message to contain a concrete example of something that didn't work before, that does work now.
> (I'm guessing this has to do with a `PATH` containing names-with-spaces; but I want the commit message to tell me explicitly.)
Yes, but the shebang only specifies `python`, so depending on distribution, this still can be executed by python2, deprecated or not - and `shlex.quote` is python3 only. Don't remember what the overall progress of python 2->3 transition/forcing scripts to python3 in llvm, but I didn't want to get sidetracked with that right now.

I can clarify the message regarding what chars are quoted/escaped. Spaces is an obvious one, but the one I tripped on actually is semicolons. (The semicolons aren't right on their own either - they come from trying to set a windows `PATH` while crosstesting from linux via ssh. No path from the build host make sense on the remote system anyway, regardless of how they're separated, but the cross testing setup works well for statically linked setups anyway.) Tl;dr, unneeded env vars can contain semicolons, this makes sure ssh.py doesn't trip up on them, even if they aren't entirely correct.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99242/new/

https://reviews.llvm.org/D99242