[lldb] Add early CMake check for 'make' tool #111531
Conversation
Around 400 of LLDB's dotest.py based tests require the make tool to be found in Path. If it's not found, they fail with an obscure error and show up as UNRESOLVED. llvm-lit takes care of MSYS based testing tools like cat, printf, etc., but make is not part of that. Let's catch the situation early and raise an error if LLDB_ENFORCE_STRICT_TEST_REQUIREMENTS was enabled. This error is not fatal: It should fail the build, but not immediately stop the configuration process. There might be other issues further down the line that can be caught in the same buildbot run.
|
@llvm/pr-subscribers-lldb Author: Stefan Gränitz (weliveindetail) ChangesAround 400 of LLDB's dotest.py based tests require the Full diff: https://github.com/llvm/llvm-project/pull/111531.diff 1 Files Affected:
diff --git a/lldb/test/CMakeLists.txt b/lldb/test/CMakeLists.txt
index 5ac474736eb63d..7993be2602e538 100644
--- a/lldb/test/CMakeLists.txt
+++ b/lldb/test/CMakeLists.txt
@@ -29,6 +29,18 @@ if(LLDB_ENFORCE_STRICT_TEST_REQUIREMENTS)
"`LLDB_ENFORCE_STRICT_TEST_REQUIREMENTS=OFF`")
endif()
endforeach()
+
+ # On Windows make is not part of the MSYS tools that llvm-lit takes care of
+ find_program(MAKE_TOOL make)
+ if(MAKE_TOOL)
+ message(STATUS "Found make: ${MAKE_TOOL}")
+ else()
+ message(STATUS "Not found: make")
+ message(SEND_ERROR
+ "LLDB tests require 'make' tool. Please install and add it to Path "
+ "(or otherwise disable strict testing requirements with "
+ "`LLDB_ENFORCE_STRICT_TEST_REQUIREMENTS=OFF`)")
+ endif()
endif()
if(LLDB_BUILT_STANDALONE)
|
There was a problem hiding this comment.
I like the idea of looking for Make at configuration time. However, I think if we go that route, we should pass make as an explicit argument to dotest.py:
- We do for other tools that we find at configuration time (such as
dsymutil). - It eliminates the possibility of
dotest.pyfinding a differentmakethan CMake.
Looking at dotest.py, it already supports --make so hopefully this is trivial to hook up.
|
+1 to what Jonas said. Making |
|
Thanks for the quick feedback. Yes, makes sense. Here we go. |
|
✅ With the latest revision this PR passed the Python code formatter. |
| @@ -272,6 +272,8 @@ def parseOptionsAndInitTestdirs(): | |||
| configuration.make_path = "gmake" | |||
| else: | |||
| configuration.make_path = "make" | |||
| if ' ' in configuration.make_path: | |||
| configuration.make_path = f'"{configuration.make_path}"' | |||
There was a problem hiding this comment.
The default install path on Windows is C:\Program Files (x86)\GnuWin32\bin\make.exe unfortunately.
There was a problem hiding this comment.
That may be fine, but I think this is the wrong place to quote it. I think it should happen somewhere around the place where this variable is concatenated into a string (or passed to whatever it is that requires it to be quoted).
That said, after tracing this variable, I'm a little unsure as to why is this needed. IIUC, this eventually makes its way to the subprocess.check_output call here, which should be able to handle quoting on its own.
Can you explain what the problem was without this?
There was a problem hiding this comment.
I added this because yesterday my tests failed with:
'C:/Program' is not recognized as an internal or external command,
operable program or batch file.
I double-checked again today and this doesn't happen anymore. I am not sure why, maybe I had an inconsistent configuration state. The parameter for dotest.py is --make C:/Program Files (x86)/GnuWin32/bin/make.exe and the command that lldbtest.py runs is starting with C:/Program Files (x86)/GnuWin32/bin/make.exe VPATH=.... I cleared the cached in lldb-test-build.noindex and checked sure the binaries are recreated successfully by the test suite. It works without the quotes.
I am attaching a log file TestBreakpointByLineAndColumn.log with the commands dumped for future reference. I will the quoting and assume it just keeps working.
Many LLDB's dotest.py based tests require the `make` tool. If it's not found in Path, they fail with an obscure error and show up as `UNRESOLVED`. On Windows, llvm-lit takes care of MSYS based testing tools like cat, printf, etc., but `make` is not part of that. Let's catch the situation early and check for it at configuration time. This error isn't fatal: It should fail the build, but not immediately stop the configuration process. There might be other issues further down the line that can be caught in the same buildbot run.
Fixes 0e91323 / #111531 For reasons I can't explain, a clean build works fine for me, and all the bots are working fine. But if I rebuild in some way the make tool becomes None. Looking at the other variables, they had these extra lines so I've added those for make and it seems to solve the problem.
|
Hi, I noticed that our LLDB Windows builders all failed after this change is landed: We are aware our windows bots don't have make tool and we are not planning to add it anyway. Not all lldb tests require |
All the API tests build their inferior with Make and looking at the last successful run on Windows, we have 1206 API tests, 561 shell tests and 261 unit tests. Even if we only consider the ones that are passing, the API tests are still the vast majority. I'm not sure how you come to the conclusion that only "a few tests" require it? |
Fine, let me just disable windows tests on our bots all together. |
|
In fact, I ran into that on Friday myself. I checked @zeroomega Any similar experiences with |
No, I didn't see this issue. Our build passed after I set |
…2342) 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 failed if the path to the `make` tool contains spaces. This patch attempts to fix the functionalities/completion test by adding quotes in the respective Makefile. If it keeps passing on the bots, we can roll out the fix to all affected tests.
…m#112342) In recent PR llvm#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 failed if the path to the `make` tool contains spaces. This patch attempts to fix the functionalities/completion test by adding quotes in the respective Makefile. If it keeps passing on the bots, we can roll out the fix to all affected tests.
Many LLDB's dotest.py based tests require the `make` tool. If it's not found in Path, they fail with an obscure error and show up as `UNRESOLVED`. On Windows, llvm-lit takes care of MSYS based testing tools like cat, printf, etc., but `make` is not part of that. Let's catch the situation early and check for it at configuration time. This error isn't fatal: It should fail the build, but not immediately stop the configuration process. There might be other issues further down the line that can be caught in the same buildbot run.
Fixes 0e91323 / llvm#111531 For reasons I can't explain, a clean build works fine for me, and all the bots are working fine. But if I rebuild in some way the make tool becomes None. Looking at the other variables, they had these extra lines so I've added those for make and it seems to solve the problem.
…m#112342) In recent PR llvm#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 failed if the path to the `make` tool contains spaces. This patch attempts to fix the functionalities/completion test by adding quotes in the respective Makefile. If it keeps passing on the bots, we can roll out the fix to all affected tests.
Many LLDB's dotest.py based tests require the `make` tool. If it's not found in Path, they fail with an obscure error and show up as `UNRESOLVED`. On Windows, llvm-lit takes care of MSYS based testing tools like cat, printf, etc., but `make` is not part of that. Let's catch the situation early and check for it at configuration time. This error isn't fatal: It should fail the build, but not immediately stop the configuration process. There might be other issues further down the line that can be caught in the same buildbot run.
Fixes 0e91323 / llvm#111531 For reasons I can't explain, a clean build works fine for me, and all the bots are working fine. But if I rebuild in some way the make tool becomes None. Looking at the other variables, they had these extra lines so I've added those for make and it seems to solve the problem.
…m#112342) In recent PR llvm#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 failed if the path to the `make` tool contains spaces. This patch attempts to fix the functionalities/completion test by adding quotes in the respective Makefile. If it keeps passing on the bots, we can roll out the fix to all affected tests.
…m#112342) In recent PR llvm#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 failed if the path to the `make` tool contains spaces. This patch attempts to fix the functionalities/completion test by adding quotes in the respective Makefile. If it keeps passing on the bots, we can roll out the fix to all affected tests.
|
In fact, for many developers on Windows, Of course, a better way may be to search in advance, and if not, automatically close |
|
I think it's valuable to get the full config log on buildbots. If this check becomes too much of a burden, I'd rather propose to either turn the error into a warning or revisit the option to guard it by |
Personally, warnings are better than errors for developers |
…m#112342) In recent PR llvm#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 failed if the path to the `make` tool contains spaces. This patch attempts to fix the functionalities/completion test by adding quotes in the respective Makefile. If it keeps passing on the bots, we can roll out the fix to all affected tests.
#111531) Bot maintainers should be aware and it became too much of a burden for developers. In particular on Windows, where make.exe won't be found in Path typically.
|
Yes, for devs it might be too much of a burden and eventually bot maintainers should be aware. If we want the error behavior back, we could make it conditional on |
Fixes 0e91323 / llvm#111531 For reasons I can't explain, a clean build works fine for me, and all the bots are working fine. But if I rebuild in some way the make tool becomes None. Looking at the other variables, they had these extra lines so I've added those for make and it seems to solve the problem.
llvm#111531) Bot maintainers should be aware and it became too much of a burden for developers. In particular on Windows, where make.exe won't be found in Path typically.
Many LLDB's dotest.py based tests require the `make` tool. If it's not found in Path, they fail with an obscure error and show up as `UNRESOLVED`. On Windows, llvm-lit takes care of MSYS based testing tools like cat, printf, etc., but `make` is not part of that. Let's catch the situation early and check for it at configuration time. This error isn't fatal: It should fail the build, but not immediately stop the configuration process. There might be other issues further down the line that can be caught in the same buildbot run.
…m#112342) In recent PR llvm#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 failed if the path to the `make` tool contains spaces. This patch attempts to fix the functionalities/completion test by adding quotes in the respective Makefile. If it keeps passing on the bots, we can roll out the fix to all affected tests.
Many LLDB's dotest.py based tests require the `make` tool. If it's not found in Path, they fail with an obscure error and show up as `UNRESOLVED`. On Windows, llvm-lit takes care of MSYS based testing tools like cat, printf, etc., but `make` is not part of that. Let's catch the situation early and check for it at configuration time. This error isn't fatal: It should fail the build, but not immediately stop the configuration process. There might be other issues further down the line that can be caught in the same buildbot run.
…m#112342) In recent PR llvm#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 failed if the path to the `make` tool contains spaces. This patch attempts to fix the functionalities/completion test by adding quotes in the respective Makefile. If it keeps passing on the bots, we can roll out the fix to all affected tests.
Many LLDB's dotest.py based tests require the `make` tool. If it's not found in Path, they fail with an obscure error and show up as `UNRESOLVED`. On Windows, llvm-lit takes care of MSYS based testing tools like cat, printf, etc., but `make` is not part of that. Let's catch the situation early and check for it at configuration time. This error isn't fatal: It should fail the build, but not immediately stop the configuration process. There might be other issues further down the line that can be caught in the same buildbot run.
…m#112342) In recent PR llvm#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 failed if the path to the `make` tool contains spaces. This patch attempts to fix the functionalities/completion test by adding quotes in the respective Makefile. If it keeps passing on the bots, we can roll out the fix to all affected tests.
llvm#111531) Bot maintainers should be aware and it became too much of a burden for developers. In particular on Windows, where make.exe won't be found in Path typically.
Around 400 of LLDB's dotest.py based tests require the
maketool to be found in Path. If it's not found, they fail with an obscure error and show up asUNRESOLVED. llvm-lit takes care of MSYS based testing tools like cat, printf, etc., but make is not part of that. Let's catch the situation early and raise an error if LLDB_ENFORCE_STRICT_TEST_REQUIREMENTS was enabled. This error is not fatal: It should fail the build, but not immediately stop the configuration process. There might be other issues further down the line that can be caught in the same buildbot run.