[lldb] Support tests with nested make invocations on Windows 1/2

[weliveindetail]

In recent PR #111531 for Windows support, we enabled tests that require the make tool. On Windows default install directories likely contain spaces, in this case e.g. C:\Program Files (x86)\GnuWin32\bin\make.exe. It's typically handled well by CMake, so that today invocations from dotest.py don't cause issues. However, we also have nested invocations from a number of Makefiles themselves. These still fail if the path to the make tool contains spaces.

[llvmbot]

@llvm/pr-subscribers-lldb

Author: Stefan Gränitz (weliveindetail)

Changes

In recent PR #111531 for Windows support, we enabled tests that require the make tool. On Windows default install directories likely contain spaces, in this case e.g. C:\Program Files (x86)\GnuWin32\bin\make.exe. It's typically handled well by CMake, so that today invocations from dotest.py don't cause issues. However, we also have nested invocations from a number of Makefiles themselves. These still fail if the path to the make tool contains spaces.

---

Full diff: https://github.com/llvm/llvm-project/pull/112342.diff

1 Files Affected:

• (modified) lldb/test/API/functionalities/completion/Makefile (+1-1)

diff --git a/lldb/test/API/functionalities/completion/Makefile b/lldb/test/API/functionalities/completion/Makefile
index f46742243bd96d..e457693f62ca44 100644
--- a/lldb/test/API/functionalities/completion/Makefile
+++ b/lldb/test/API/functionalities/completion/Makefile
@@ -4,7 +4,7 @@ USE_LIBDL := 1
 a.out: lib_shared
 
 lib_shared:
-	$(MAKE) -f $(MAKEFILE_RULES) \
+	"$(MAKE)" -f $(MAKEFILE_RULES) \
 		DYLIB_ONLY=YES DYLIB_CXX_SOURCES=shared.cpp DYLIB_NAME=shared
 
 include Makefile.rules

[DavidSpickett]

Seems ok to me.

I checked our bot and make is in a path without spaces, that's why we didn't see this problem there.

[weliveindetail]

Thanks for the quick feedback! Yes, the buildbot in https://lab.llvm.org/buildbot/#/builders/141 isn't affected.

Debug output (in swiftlang lldb) is:

Reading makefiles...
Updating goal targets....
 File `all' does not exist.
   File `a.out' does not exist.
     File `lib_shared' does not exist.
    Must remake target `lib_shared'.
make.exe: Entering directory `S:/b/5/lldb-test-build.noindex/functionalities/completion/TestCompletion.test_target_modules_search_paths_insert'
C:/Program Files (x86)/GnuWin32/bin/make.exe -f S:\SourceCache\llvm-project\lldb\packages\Python\lldbsuite\test\make/Makefile.rules \
                DYLIB_ONLY=YES DYLIB_CXX_SOURCES=shared.cpp DYLIB_NAME=shared
'C:/Program' is not recognized as an internal or external command,
operable program or batch file.
make.exe: *** [lib_shared] Error 1

Some more context:

• This a follow-up from our previous discussion here: [lldb] Add early CMake check for 'make' tool #111531 (comment)
• Failures caused by this issue aren't obvious, because tests show up as UNRESOLVED (building the inferior seems to be considered a precondition)

[weliveindetail]

If this approach is accepted, I'd like to roll it out to 32 more affected tests (upstream). I listed them here: https://gist.github.com/weliveindetail/9a3cfcfc698f8814f5924ead7ba68bf3

I only tested this on Windows so far, but I expect it be fine on all other platforms. Should we merge only this single instance and see if the bots play well before I roll it out? Or can I add all the others as well immediately?

What do you think?

[DavidSpickett]

Being cautious costs nothing so I'd land one then come back with the rest if it works out.

[DavidSpickett]

The alternatives:

• Some automatic symlinking of the make to a path without spaces. Which is A: surprising and B: doubly surprising if the build directory is already in a path with spaces and we break out of it to make a symlink somewhere.
  • Probably what the admin of a machine should be dealing with, it's not for us to do.
• Some template folks can include in the makefile that quotes this for them.
  • Adding "" but with more steps to create more problems.
• Have dotest escape the spaces somehow, is that even possible? C:\Program/ Files ?
  • Though then we have files that work via dotest but not with make itself, which will annoy a few maintainers eventually :)

[weliveindetail]

Yes, agree I think that both symlinking and Makefile templating make things worse, because they get even more complicated.

We shortly discussed the dotest escape approach in #111531 (comment), but back then I couldn't reproduce the issue. Would be fine for me, but it seems less obvious why this is necessary for make and not for the other paths that we deal with in dotest.

[DavidSpickett]

See what @labath says since they commented there too.

[labath]

LGTM.

For better or worse, I can reproduce the same behavior on linux when I put make in a path with spaces.

@slydiman has recently proposed (and I pushed back) overriding some variables (including $(MAKE)) to include the quotes. I like this better (despite some drawbacks) as we're not overriding a predefined variable, and is in line with the recommendation in https://stackoverflow.com/questions/23330243/gnu-makefile-how-to-and-when-to-quote-strings

[weliveindetail]

Great, thanks. I will give this a try and if functionalities/completion keeps passing on the bots, I roll it out to the other Makefiles.