[lldb] Use packaging module instead of pkg_resources

[JDevlieghere]

Use the packaging [1] module for parsing version numbers, instead of pkg_resources which is distributed with setuptools. I recently switched over to using the latter, knowing it was deprecated (in favor of the packaging module) because it comes with Python out of the box. Newer versions of setuptools have removed pkg_resources so we have to use packaging.

[1] https://pypi.org/project/packaging/

[llvmbot]

@llvm/pr-subscribers-lldb

Author: Jonas Devlieghere (JDevlieghere)

Changes

Use the packaging [1] module for parsing version numbers, instead of pkg_resources which is distributed with setuptools. I recently switched over to using the latter, knowing it was deprecated (in favor of the packaging module) because it comes with Python out of the box. Newer versions of setuptools have removed pkg_resources so we have to use packaging.

[1] https://pypi.org/project/packaging/

---

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

4 Files Affected:

• (modified) lldb/packages/Python/lldbsuite/test/decorators.py (+2-4)
• (modified) lldb/packages/Python/lldbsuite/test/lldbplatformutil.py (+3-4)
• (modified) lldb/test/API/tools/lldb-server/TestAppleSimulatorOSType.py (+2-2)
• (modified) lldb/test/Shell/helper/build.py (+2-2)

diff --git a/lldb/packages/Python/lldbsuite/test/decorators.py b/lldb/packages/Python/lldbsuite/test/decorators.py
index 79cc0a2aeacbe..4b85219ce7c19 100644
--- a/lldb/packages/Python/lldbsuite/test/decorators.py
+++ b/lldb/packages/Python/lldbsuite/test/decorators.py
@@ -1,6 +1,6 @@
 # System modules
 from functools import wraps
-from pkg_resources import packaging
+from packaging.version import parse
 import ctypes
 import locale
 import os
@@ -66,9 +66,7 @@ def fn_neq(x, y):
         "<=": fn_leq,
     }
 
-    return op_lookup[comparison](
-        packaging.version.parse(actual), packaging.version.parse(expected)
-    )
+    return op_lookup[comparison](parse(actual), parse(expected))
 
 
 def _match_decorator_property(expected, actual):
diff --git a/lldb/packages/Python/lldbsuite/test/lldbplatformutil.py b/lldb/packages/Python/lldbsuite/test/lldbplatformutil.py
index 187d16aa1baa6..ecc814b016059 100644
--- a/lldb/packages/Python/lldbsuite/test/lldbplatformutil.py
+++ b/lldb/packages/Python/lldbsuite/test/lldbplatformutil.py
@@ -8,8 +8,7 @@
 import subprocess
 import sys
 import os
-from urllib.parse import urlparse
-from pkg_resources import packaging
+from packaging.version import parse
 
 # LLDB modules
 import lldb
@@ -309,8 +308,8 @@ def expectedCompilerVersion(compiler_version):
         # Assume the compiler version is at or near the top of trunk.
         return operator in [">", ">=", "!", "!=", "not"]
 
-    version = packaging.version.parse(version_str)
-    test_compiler_version = packaging.version.parse(test_compiler_version_str)
+    version = parse(version_str)
+    test_compiler_version = parse(test_compiler_version_str)
 
     if operator == ">":
         return test_compiler_version > version
diff --git a/lldb/test/API/tools/lldb-server/TestAppleSimulatorOSType.py b/lldb/test/API/tools/lldb-server/TestAppleSimulatorOSType.py
index d770447f0771c..6dd838dab168b 100644
--- a/lldb/test/API/tools/lldb-server/TestAppleSimulatorOSType.py
+++ b/lldb/test/API/tools/lldb-server/TestAppleSimulatorOSType.py
@@ -61,9 +61,9 @@ def check_simulator_ostype(self, sdk, platform_name, arch=platform.machine()):
 
         # Older versions of watchOS (<7.0) only support i386
         if platform_name == "watchos":
-            from pkg_resources import packaging
+            from packaging.version import parse
 
-            if packaging.version.parse(vers) < packaging.version.parse("7.0"):
+            if parse(vers) < parse("7.0"):
                 arch = "i386"
 
         triple = "-".join([arch, "apple", platform_name + vers, "simulator"])
diff --git a/lldb/test/Shell/helper/build.py b/lldb/test/Shell/helper/build.py
index d3c25bd944e98..9db2b133483a8 100755
--- a/lldb/test/Shell/helper/build.py
+++ b/lldb/test/Shell/helper/build.py
@@ -519,9 +519,9 @@ def _find_windows_sdk_in_registry_view(self, view):
 
             # Windows SDK version numbers consist of 4 dotted components, so we
             # have to use LooseVersion, as StrictVersion supports 3 or fewer.
-            from pkg_resources import packaging
+            from packaging.version import parse
 
-            sdk_versions.sort(key=lambda x: packaging.version.parse(x), reverse=True)
+            sdk_versions.sort(key=lambda x: parse(x), reverse=True)
             option_value_name = "OptionId.DesktopCPP" + self.msvc_arch_str
             for v in sdk_versions:
                 try:

[rupprecht]

LGTM, but heads up for @mysterymath if this causes issues w/ Fuchsia's build

[mysterymath]

At least at present, this will probably break Fuchsia's build; the Python we ship includes pkg_resources but not packaging. Is the default cpython build from source intended to build and install packaging? Is this something that changed recently?

[JDevlieghere]

At least at present, this will probably break Fuchsia's build; the Python we ship includes pkg_resources but not packaging. Is the default cpython build from source intended to build and install packaging? Is this something that changed recently?

I'm not sure but I don't think so. IIUC the expectation that if folks want to use the packaging module, they have to install it themselves (i.e. with pip). I'm not sure all the bots have this package which is the reason I went with the pkg_resources approach previously, but that no longer works (i.e. Python 3.12 installed via homebrew no longer has it which is why we've seen folks hitting this).

[DavidSpickett]

packaging is already installed on Linaro's bots, so we're ok with this.

[JDevlieghere]

GreenDragon now has packaging and based on David's message, the Linaro bots do as well.

@labath Does your ✅ mean the debian bot has this package installed?
@mysterymath Can you install this package on the Fuchia bots? I assume you must do something similar for pexpect?

[mysterymath]

We don't have pexpect either; instead the tests that require it are disabled (a CMake option was provided for this purpose IIRC). Our build is as hermetic as possible; even if the package were available in the Python on the bots, we wouldn't be able to use it, since we don't make use of the Python on the bots, for the most part. Instead, we build and ship a CPython from source as part of the bot build. This is to make the built toolchain as portable as possible: for the most part, you can drop the toolchain into a directory and run it without any additional setup, regardless of the host.

So far we've avoided providing or requiring any additional Python dependencies, since we have relatively strict requirements for software chain of custody. If this change goes forward, we'd probably need to figure out a way to build packaging from source and to set up a source mirror for it. Our toolchain CI would be broken in the interim.

[mysterymath]

Did a bit of digging around; it looks like at the very least the Arch Linux python package and the Python docker container don't contain the packaging library. It's included in Arch's python-pip package, but having pip as a dependency to test LLDB seems odd.

[labath]

@labath Does your ✅ mean the debian bot has this package installed?

Affirmative.

Did a bit of digging around; it looks like at the very least the Arch Linux python package and the Python docker container don't contain the packaging library. It's included in Arch's python-pip package, but having pip as a dependency to test LLDB seems odd.

FWIW, there appears to be an actual arch package for this https://archlinux.org/packages/extra/any/python-packaging/, so it shouldn't be (I think -- I'm not an Arch user) necessary to install pip.

That said, using a package designed (AIUI) for python versions for parsing versions of gcc/clang does strike me as somewhat... unusual, even if it does work, so maybe there is case to be made for writing something custom (I'm not sure if we really need anything more elaborate than tuple([int(part) for part in version.split(".")]))

[JDevlieghere]

That said, using a package designed (AIUI) for python versions for parsing versions of gcc/clang does strike me as somewhat... unusual, even if it does work, so maybe there is case to be made for writing something custom (I'm not sure if we really need anything more elaborate than tuple([int(part) for part in version.split(".")]))

Agreed. The first time I took a stab at this, that was the first thing I tried, but I quickly discovered that we had at least one tool (I think it was Python itself?) that contained alphabetical characters (something like 1.2.3rc or 1.2.3.dev) and I didn't feel like dealing with all the possible edge cases. We have other packages the test suite relies on (pexpect, psutil, etc) so it seemed reasonable, but if folks feel strongly about it we can maintain our own "version parsing".

[JDevlieghere]

FWIW, there appears to be an actual arch package for this https://archlinux.org/packages/extra/any/python-packaging/, so it shouldn't be (I think -- I'm not an Arch user) necessary to install pip.

Homebrew on macOS works similarly, it has a python-packaging package that you install with brew install python-packaging.

[mysterymath]

We have other packages the test suite relies on (pexpect, psutil, etc) so it seemed reasonable, but if folks feel strongly about it we can maintain our own "version parsing".

Notably the other packages are optional; the lion's share of the test suite can be run without them. Cursorily it looks like this wouldn't be the case here? Admittedly, Fuchsia is the reason at least one of those callouts has been added; we've struggled reigning in LLVM/LLDB's deps due to the manual nature of our build scripts, difficulty keeping things hermetic, and bureaucratic hassles involved in taking on new deps. But it might be a good idea to have a broader discussion about what the mandatory dep story is/should be for parts of LLVM, rather than doing this piecemeal on patches like this.

EDIT:
I asked around afterwards about the idea of checking in wheels to our CI, installing them just to run the LLDB test suite, then uninstalling them before shipping the Python. That does seem to tick all the boxes, so we may have a solution if and when the LLDB test suite unavoidably needs certain Python deps.

[labath]

That said, using a package designed (AIUI) for python versions for parsing versions of gcc/clang does strike me as somewhat... unusual, even if it does work, so maybe there is case to be made for writing something custom (I'm not sure if we really need anything more elaborate than tuple([int(part) for part in version.split(".")]))

Agreed. The first time I took a stab at this, that was the first thing I tried, but I quickly discovered that we had at least one tool (I think it was Python itself?) that contained alphabetical characters (something like 1.2.3rc or 1.2.3.dev) and I didn't feel like dealing with all the possible edge cases.

Ah yes, I guess one of the versions we are parsing is the version of python itself..

We have other packages the test suite relies on (pexpect, psutil, etc) so it seemed reasonable, but if folks feel strongly about it we can maintain our own "version parsing".

While I don't think we should be adding deps willy-nilly, I don't think this one is more problematic than others in that they are all available through common package management systems. OTOH, it has two things going against it:

• it's a hard dep breaking the ability to run any test (whereas e.g. pexpect would only break pexpect-based tests)
• it should be relatively easy to replace

This does not constitute an endorsement of one direction of the other, I'm just thinking out loud.

[JDevlieghere]

Thanks for checking @mysterymath. Based on your response it sounds like we have a path forward for the Fuchsia bots. Please let me know when it's safe to land this. I'll add packaging to requirements.txt that was added in #94220.

[mysterymath]

@JDevlieghere , we just landed a patch that adds packaging for Fuchsia. Should be good to go ahead.

[aganea]

This is breaking the github CI as seen here: https://buildkite.com/llvm-project/github-pull-requests/builds/73339#01902659-9fbc-44d2-9a5b-783f5af41b78. Should we add the "packaging" module to https://github.com/llvm/llvm-project/blob/main/lldb/test/requirements.txt ?