[PATCH] D29185: Allow llvm's build and test systems to support paths with spaces

[Reid Kleckner via Phabricator via llvm-commits]

rnk added a comment.

Thanks for tackling this!



================
Comment at: docs/GettingStartedVS.rst:60
+in test failures as tests are sometimes written which do not work in this
+scenario.
 
----------------
bd1976llvm wrote:
> gbedwell wrote:
> > I'm not convinced by this message.  LLVM as a whole includes various different projects which new users may come to expect from this document to also work in paths with spaces, but AFAICT this commit only affects the base llvm project itself.  I'm also not entirely comfortable with the idea that it's essentially saying "it may work, unless we break it".  I make no judgement as to whether it's worth making everything work with paths with spaces, but we should probably err on the side of caution and just document that it's not supported if we can't guarantee it.
> This document only refers to building LLLVM not any of the other projects e.g: clang, lld etc...
> CMake works with paths with spaces. This change adds the ability for lit to support them. Therefore, I think the "has support for" is justified. However, I don't think that there are any build bots that are testing this. That's why I wanted a line saying that the tests might have rotted.
Honestly I would remove this paragraph altogether. The best documentation is the kind that doesn't need to exist. It'd be good to add a space to the build directory of some Linux buildbot at some point to ensure that this doesn't regress. I don't think we need a special bot configuration for paths with spaces.


================
Comment at: test/lit.cfg:334
         tool_path = llvm_tools_dir + '/' + tool_name
-    config.substitutions.append((pattern, tool_pipe + tool_path))
+    config.substitutions.append((pattern, tool_pipe + lit.util.escapeSpaces(tool_path)))
 
----------------
MatzeB wrote:
> Escaping spaces is probably a first step, but wouldn't we rather need a (posix?) shell escape mechanism? (maybe python shlex.quote/pipes.quote?)
+1, it should be the opposite of whatever the lit shell does for tokenization, which is roughly to follow bash quoting rules minus variable expansion.


================
Comment at: utils/lit/lit/TestRunner.py:731
+
+    # "%:[STpst]" are escaped paths without colons.
+    if kIsWindows:
----------------
I can't find any tests using these substitutions. Any reason not to remove them? At least don't add %^:s without a clear use case for it.


https://reviews.llvm.org/D29185