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

[ben via Phabricator via llvm-commits]

bd1976llvm added a comment.

In https://reviews.llvm.org/D29185#657844, @MatzeB wrote:

> I trust that this patch fixes the issues we have today and that bugpoint issue really should be fixed.
>
> On the other hand I am not sure we will be able to keep this working consistently in a project with hundreds of comitters. So maybe we should just make our life easier and declare this an unsupported configuration. I would propose to add this to our toplevel CMakeLists.txt:
>
>   if(CMAKE_SOURCE_DIR MATCHES " " OR CMAKE_BINARY_DIR MATCHES " ")
>     message(WARNING "Spaces in the path to the source or build directory are"
>     " supported on a best-effort basis. We recommend to use paths without spaces.")
>   endif()
>


Really like this - it also means that I can remove the text in the docs/GettingStartedVS.rst file.

In https://reviews.llvm.org/D29185#657870, @MatzeB wrote:

> Would it make sense to split this into
>
> - cmake changes (look good to me)
> - test changes (they all look good to me)
> - lit changes (I have to find more time to read/think about those)
> - bash script changes (they look good to me)


Ok - I will create a new patch with everything but the lit changes and I will add your proposed CMake changes to this new patch. Won't happen until tomorrow though.



================
Comment at: docs/GettingStartedVS.rst:60
+in test failures as tests are sometimes written which do not work in this
+scenario.
 
----------------
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.


https://reviews.llvm.org/D29185