[lldb][test][NFC] Add option to exclude third_party packages #83191
Conversation
The goal here is to remove the third_party/Python/module tree, which LLDB tests only use to `import pexpect`. This package is available on `pip`, and I believe should not be hard to obtain. However, in case it isn't easily available, deleting the tree right now could cause disruption. This introduces a `LLDB_TEST_USE_VENDOR_PACKAGES` cmake param that can be enabled, and the tests will continue loading that tree. By default, it is disabled, and an eager cmake check runs that makes sure `pexpect` is available before waiting for the test to fail in an obscure way. Later, this option will go away, and when it does, we can delete the tree too. Ideally this is not disruptive, and we can remove it in a week or two.
|
@llvm/pr-subscribers-lldb Author: Jordan Rupprecht (rupprecht) ChangesThe goal here is to remove the third_party/Python/module tree, which LLDB tests only use to However, in case it isn't easily available, deleting the tree right now could cause disruption. This introduces a Later, this option will go away, and when it does, we can delete the tree too. Ideally this is not disruptive, and we can remove it in a week or two. Full diff: https://github.com/llvm/llvm-project/pull/83191.diff 7 Files Affected:
diff --git a/lldb/cmake/modules/LLDBConfig.cmake b/lldb/cmake/modules/LLDBConfig.cmake
index a758261073ac57..5d62213c3f5838 100644
--- a/lldb/cmake/modules/LLDBConfig.cmake
+++ b/lldb/cmake/modules/LLDBConfig.cmake
@@ -67,6 +67,8 @@ option(LLDB_SKIP_STRIP "Whether to skip stripping of binaries when installing ll
option(LLDB_SKIP_DSYM "Whether to skip generating a dSYM when installing lldb." OFF)
option(LLDB_ENFORCE_STRICT_TEST_REQUIREMENTS
"Fail to configure if certain requirements are not met for testing." OFF)
+option(LLDB_TEST_USE_VENDOR_PACKAGES
+ "Use packages from lldb/third_party/Python/module instead of system deps." OFF)
set(LLDB_GLOBAL_INIT_DIRECTORY "" CACHE STRING
"Path to the global lldbinit directory. Relative paths are resolved relative to the
diff --git a/lldb/test/API/lit.cfg.py b/lldb/test/API/lit.cfg.py
index 12675edc0fd3b9..f9497b632fc504 100644
--- a/lldb/test/API/lit.cfg.py
+++ b/lldb/test/API/lit.cfg.py
@@ -309,3 +309,6 @@ def delete_module_cache(path):
# Propagate XDG_CACHE_HOME
if "XDG_CACHE_HOME" in os.environ:
config.environment["XDG_CACHE_HOME"] = os.environ["XDG_CACHE_HOME"]
+
+if is_configured("use_vendor_packages"):
+ config.environment["LLDB_TEST_USE_VENDOR_PACKAGES"] = "1"
diff --git a/lldb/test/API/lit.site.cfg.py.in b/lldb/test/API/lit.site.cfg.py.in
index 053331dc4881f7..c2602acd2ef85a 100644
--- a/lldb/test/API/lit.site.cfg.py.in
+++ b/lldb/test/API/lit.site.cfg.py.in
@@ -38,6 +38,7 @@ config.libcxx_include_target_dir = "@LIBCXX_GENERATED_INCLUDE_TARGET_DIR@"
# The API tests use their own module caches.
config.lldb_module_cache = os.path.join("@LLDB_TEST_MODULE_CACHE_LLDB@", "lldb-api")
config.clang_module_cache = os.path.join("@LLDB_TEST_MODULE_CACHE_CLANG@", "lldb-api")
+config.use_vendor_packages = @LLDB_TEST_USE_VENDOR_PACKAGES@
# Plugins
lldb_build_intel_pt = '@LLDB_BUILD_INTEL_PT@'
diff --git a/lldb/test/CMakeLists.txt b/lldb/test/CMakeLists.txt
index 1aa8843b6a2e78..d8cbb24b6c9b81 100644
--- a/lldb/test/CMakeLists.txt
+++ b/lldb/test/CMakeLists.txt
@@ -26,6 +26,20 @@ if(LLDB_ENFORCE_STRICT_TEST_REQUIREMENTS)
endforeach()
endif()
+# The "pexpect" package should come from the system environment, not from the
+# LLDB tree. However, we delay the deletion of it from the tree in case
+# users/buildbots don't have the package yet and need some time to install it.
+if (NOT LLDB_TEST_USE_VENDOR_PACKAGES)
+ lldb_find_python_module(pexpect)
+ if (NOT PY_pexpect_FOUND)
+ message(FATAL_ERROR
+ "Python module 'pexpect' not found. Please install it via pip or via "
+ "your operating system's package manager. For a temporary workaround, "
+ "use a version from the LLDB tree with "
+ "`LLDB_TEST_USE_VENDOR_PACKAGES=ON`")
+ endif()
+endif()
+
if(LLDB_BUILT_STANDALONE)
# In order to run check-lldb-* we need the correct map_config directives in
# llvm-lit. Because this is a standalone build, LLVM doesn't know about LLDB,
@@ -240,7 +254,8 @@ llvm_canonicalize_cmake_booleans(
LLDB_HAS_LIBCXX
LLDB_TOOL_LLDB_SERVER_BUILD
LLDB_USE_SYSTEM_DEBUGSERVER
- LLDB_IS_64_BITS)
+ LLDB_IS_64_BITS
+ LLDB_TEST_USE_VENDOR_PACKAGES)
# Configure the individual test suites.
add_subdirectory(API)
diff --git a/lldb/use_lldb_suite_root.py b/lldb/use_lldb_suite_root.py
index fd42f63a3c7f30..b8f8acf5dd94a4 100644
--- a/lldb/use_lldb_suite_root.py
+++ b/lldb/use_lldb_suite_root.py
@@ -21,5 +21,7 @@ def add_lldbsuite_packages_dir(lldb_root):
lldb_root = os.path.dirname(inspect.getfile(inspect.currentframe()))
-add_third_party_module_dirs(lldb_root)
+# Use environment variables to avoid plumbing flags, lit configs, etc.
+if os.getenv("LLDB_TEST_USE_VENDOR_PACKAGES"):
+ add_third_party_module_dirs(lldb_root)
add_lldbsuite_packages_dir(lldb_root)
diff --git a/lldb/utils/lldb-dotest/CMakeLists.txt b/lldb/utils/lldb-dotest/CMakeLists.txt
index 09f41dbce421ec..2ba40f009cc92a 100644
--- a/lldb/utils/lldb-dotest/CMakeLists.txt
+++ b/lldb/utils/lldb-dotest/CMakeLists.txt
@@ -10,6 +10,7 @@ set(LLDB_LIBS_DIR "${LLVM_LIBRARY_OUTPUT_INTDIR}")
llvm_canonicalize_cmake_booleans(
LLDB_BUILD_INTEL_PT
LLDB_HAS_LIBCXX
+ LLDB_TEST_USE_VENDOR_PACKAGES
)
if ("libcxx" IN_LIST LLVM_ENABLE_RUNTIMES)
diff --git a/lldb/utils/lldb-dotest/lldb-dotest.in b/lldb/utils/lldb-dotest/lldb-dotest.in
index 5cd49d253b9937..9291f59b41982d 100755
--- a/lldb/utils/lldb-dotest/lldb-dotest.in
+++ b/lldb/utils/lldb-dotest/lldb-dotest.in
@@ -1,4 +1,5 @@
#!@Python3_EXECUTABLE@
+import os
import subprocess
import sys
@@ -17,8 +18,12 @@ has_libcxx = @LLDB_HAS_LIBCXX@
libcxx_libs_dir = "@LIBCXX_LIBRARY_DIR@"
libcxx_include_dir = "@LIBCXX_GENERATED_INCLUDE_DIR@"
libcxx_include_target_dir = "@LIBCXX_GENERATED_INCLUDE_TARGET_DIR@"
+use_vendor_packages = @LLDB_TEST_USE_VENDOR_PACKAGES@
if __name__ == '__main__':
+ if use_vendor_packages:
+ os.putenv("LLDB_TEST_USE_VENDOR_PACKAGES", "1")
+
wrapper_args = sys.argv[1:]
dotest_args = []
# split on an empty string will produce [''] and if you
|
There was a problem hiding this comment.
LGTM
@JDevlieghere @adrian-prantl we should consider adding this to the list of required python modules on our bots too and enforce it with LLDB_ENFORCE_STRICT_TEST_REQUIREMENTS? Probably not in this PR, but after we can get our bots configured correctly.
I get the intent but doing it this way is actually going to be more of an issue. As changes to our bot config have to go through zorg, a master restart, then they finally hit the builders, this takes up to a week sometimes. With this new option defaulting to OFF that means potentially a week of broken builds. I would suggest making this option default |
|
Though maybe you intend this to break builds, that way obscure builders that we don't know about will still hear about this? I can understand that angle, in which case do we want to make sure all known public builders have pexpect installed before this commit goes in? I will sort out Linaro's bots this week. |
|
Also are there known compatible/incompatible versions of pexpect? Is |
Yes, but I agree with your suggestion to land this with a default of
Thanks!
btw, I see many tests w/ comments like this: |
rupprecht
changed the title
This executed Alex' idea [1] of adding pexpect to the list of "strict test requirements" as we're planning to stop vendoring it. This will ensure all the bots have the package before we toggle the default. [1] #83191
Added in 7933009. Let's see if this blows up on our bots :-) EDIT: Of course it did. I had some trouble accessing the nodes so I reverted the patch. All GreenDragon nodes should all have pexpect now. I'll wait until tomorrow to try again in case I still missed something. |
|
The way the cmake is written it seems that Py_pexpect_FOUND is set to false the first time you run cmake. Then you install pexpect and lldb_find_python_module bails early because Py_pexpect_FOUND is still set to false. If you remove the CMake cache it'll then find pexpect as, well, expected. It should perhaps unset the variable first, I'll see if I can implement that. |
|
And a data point, AArch64 and Arm Linux are fine with pexpect-4.9.0. Still working on getting it installed on the bot containers. |
|
FYI @labath as the x86 bot owner. |
|
This executed Alex' idea [1] of adding pexpect to the list of "strict test requirements" as we're planning to stop vendoring it. This will ensure all the bots have the package before we toggle the default. [1] #83191
|
Arm and AArch64 bots now have pexpect 4.9.0 installed. For Linaro's bots at least, we would be ok with you flipping |
|
@DavidSpickett If you haven't already, might I suggest looking into a CMake setting I implemented last year? |
|
Done: ec8df55 / llvm/llvm-zorg@e08c692 Won't be on the bots for a few days but I checked them all and we've got everything installed. |
I checked w/ them that lldb-x86_64-debian has python3-pexpect (specifically 4.8) installed.
Thanks! Cmake is still pretty foreign to me. It didn't occur to me that the result of finding a python library would be cached :) but I guess it makes sense from a build system perspective to not want to reconfigure everything all the time. Since it sounds like all the bots should have |
The goal here is to remove the third_party/Python/module tree, which LLDB tests only use to
import pexpect. This package is available onpip, and I believe should not be hard to obtain.However, in case it isn't easily available, deleting the tree right now could cause disruption. This introduces a
LLDB_TEST_USE_VENDOR_PACKAGEScmake param that can be enabled, and the tests will continue loading that tree. By default, it is enabled, meaning there's really no change here. A followup change will disable it by default once all known build bots are updated to include this package. When disabled, an eager cmake check runs that makes surepexpectis available before waiting for the test to fail in an obscure way.Later, this option will go away, and when it does, we can delete the tree too. Ideally this is not disruptive, and we can remove it in a week or two.