[lldb][test][NFC] Add option to exclude third_party packages #83191

rupprecht · 2024-02-27T21:29:12Z

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 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 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.

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.

llvmbot · 2024-02-27T21:29:33Z

@llvm/pr-subscribers-lldb

Author: Jordan Rupprecht (rupprecht)

Changes

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.

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

7 Files Affected:

(modified) lldb/cmake/modules/LLDBConfig.cmake (+2)
(modified) lldb/test/API/lit.cfg.py (+3)
(modified) lldb/test/API/lit.site.cfg.py.in (+1)
(modified) lldb/test/CMakeLists.txt (+16-1)
(modified) lldb/use_lldb_suite_root.py (+3-1)
(modified) lldb/utils/lldb-dotest/CMakeLists.txt (+1)
(modified) lldb/utils/lldb-dotest/lldb-dotest.in (+5)

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

bulbazord

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.

DavidSpickett · 2024-02-28T09:03:39Z

This introduces a LLDB_TEST_USE_VENDOR_PACKAGES cmake param that can be enabled, and the tests will continue loading that tree.

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 ON, and builders can turn it OFF once they've installed the right packages. Then finally remove the option altogether. It'll take a longer time but we won't have a period of broken builds during it.

DavidSpickett · 2024-02-28T09:07:53Z

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.

DavidSpickett · 2024-02-28T09:10:45Z

Also are there known compatible/incompatible versions of pexpect? Is pip install pexpect going to install something with significant API differences to the vendored copy?

rupprecht · 2024-02-28T13:18:08Z

Though maybe you intend this to break builds, that way obscure builders that we don't know about will still hear about this?

Yes, but I agree with your suggestion to land this with a default of ON, which makes this change NFC. I can then switch it to OFF once known buildbots are updated. I'll update the PR later today.

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.

Thanks!

Also are there known compatible/incompatible versions of pexpect? Is pip install pexpect going to install something with significant API differences to the vendored copy?

Our vendored version is at 4.6, with two local patches for timeouts:
- a8419b1
- 97c6ef4
For my local machine (where tests are passing :) ), python3-pexpect is 4.8. Looks like that was released in 2020, and probably the version most people have.
The page https://pypi.org/project/pexpect/ lists trunk as version 4.9.0. We have some internal builds using that version which also work.
https://pexpect.readthedocs.io/en/stable/history.html doesn't seem to list anything significant recently

btw, I see many tests w/ comments like this: @skipIfWindows # llvm.org/pr22274: need a pexpect replacement for windows. As of pexpect-4.0, there is (experimental) windows support. So if you're interested in Windows support, it'd be a good idea to see if those tests work now.

DavidSpickett

You need to update the PR description but otherwise LGTM.

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

JDevlieghere · 2024-02-29T05:21:38Z

@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.

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.

DavidSpickett · 2024-02-29T11:29:15Z

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.

DavidSpickett · 2024-02-29T11:58:52Z

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.

DavidSpickett · 2024-02-29T12:45:14Z

FYI @labath as the x86 bot owner.

DavidSpickett · 2024-02-29T14:22:53Z

It should perhaps unset the variable first, I'll see if I can implement that.

ec95379

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

DavidSpickett · 2024-02-29T16:15:07Z

Arm and AArch64 bots now have pexpect 4.9.0 installed.

For Linaro's bots at least, we would be ok with you flipping LLDB_TEST_USE_VENDOR_PACKAGES to OFF as the next step. This saves us getting a change through zorg and should "just work" now.

bulbazord · 2024-02-29T18:33:38Z

@DavidSpickett If you haven't already, might I suggest looking into a CMake setting I implemented last year? LLDB_ENFORCE_STRICT_TEST_REQUIREMENTS -- this will cause your CMake configure step to fail if your bots are missing python packages so that you know your bots are running in a reasonable state. :)

DavidSpickett · 2024-03-01T09:56:14Z

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.

rupprecht · 2024-03-04T17:21:27Z

FYI @labath as the x86 bot owner.

I checked w/ them that lldb-x86_64-debian has python3-pexpect (specifically 4.8) installed.

It should perhaps unset the variable first, I'll see if I can implement that.

ec95379

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 pexpect now, I think I can flip the default later today and be ready to revert if there's unexpected breakage.

) 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 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 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.

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] llvm#83191

) 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 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 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.

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] llvm#83191

rupprecht added the lldb label Feb 27, 2024

rupprecht requested review from DavidSpickett and bulbazord February 27, 2024 21:29

rupprecht requested a review from JDevlieghere as a code owner February 27, 2024 21:29

bulbazord approved these changes Feb 27, 2024

View reviewed changes

Change default LLDB_TEST_USE_VENDOR_PACKAGES from off to on

226ff91

rupprecht changed the title ~~[lldb][test] Exclude third_party packages by default~~ [lldb][test][NFC] Add option to exclude third_party packages Feb 28, 2024

DavidSpickett approved these changes Feb 28, 2024

View reviewed changes

rupprecht merged commit 249cf35 into llvm:main Feb 28, 2024
4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[lldb][test][NFC] Add option to exclude third_party packages #83191

[lldb][test][NFC] Add option to exclude third_party packages #83191

rupprecht commented Feb 27, 2024 •

edited

Loading

llvmbot commented Feb 27, 2024

bulbazord left a comment

DavidSpickett commented Feb 28, 2024

DavidSpickett commented Feb 28, 2024 •

edited

Loading

DavidSpickett commented Feb 28, 2024

rupprecht commented Feb 28, 2024

DavidSpickett left a comment

JDevlieghere commented Feb 29, 2024 •

edited

Loading

DavidSpickett commented Feb 29, 2024 •

edited

Loading

DavidSpickett commented Feb 29, 2024

DavidSpickett commented Feb 29, 2024

DavidSpickett commented Feb 29, 2024

DavidSpickett commented Feb 29, 2024

bulbazord commented Feb 29, 2024

DavidSpickett commented Mar 1, 2024 •

edited

Loading

rupprecht commented Mar 4, 2024

[lldb][test][NFC] Add option to exclude third_party packages #83191

[lldb][test][NFC] Add option to exclude third_party packages #83191

Conversation

rupprecht commented Feb 27, 2024 • edited Loading

llvmbot commented Feb 27, 2024

bulbazord left a comment

Choose a reason for hiding this comment

DavidSpickett commented Feb 28, 2024

DavidSpickett commented Feb 28, 2024 • edited Loading

DavidSpickett commented Feb 28, 2024

rupprecht commented Feb 28, 2024

DavidSpickett left a comment

Choose a reason for hiding this comment

JDevlieghere commented Feb 29, 2024 • edited Loading

DavidSpickett commented Feb 29, 2024 • edited Loading

DavidSpickett commented Feb 29, 2024

DavidSpickett commented Feb 29, 2024

DavidSpickett commented Feb 29, 2024

DavidSpickett commented Feb 29, 2024

bulbazord commented Feb 29, 2024

DavidSpickett commented Mar 1, 2024 • edited Loading

rupprecht commented Mar 4, 2024

rupprecht commented Feb 27, 2024 •

edited

Loading

DavidSpickett commented Feb 28, 2024 •

edited

Loading

JDevlieghere commented Feb 29, 2024 •

edited

Loading

DavidSpickett commented Feb 29, 2024 •

edited

Loading

DavidSpickett commented Mar 1, 2024 •

edited

Loading