Skip to content

[lldb] Replace the usage of module imp with module importlib #70443

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Oct 31, 2023

Conversation

tuliom
Copy link
Contributor

@tuliom tuliom commented Oct 27, 2023

imp got removed in Python 3.12 [1] and the community recommends using importlib in newer Python versions.

[1] https://docs.python.org/3.12/whatsnew/3.12.html#imp

imp got removed in Python 3.12 [1] and the community recommends using
importlib in newer Python versions.

[1] https://docs.python.org/3.12/whatsnew/3.12.html#imp
@tuliom tuliom requested a review from JDevlieghere as a code owner October 27, 2023 11:57
@llvmbot llvmbot added the lldb label Oct 27, 2023
@llvmbot
Copy link
Member

llvmbot commented Oct 27, 2023

@llvm/pr-subscribers-lldb

Author: Tulio Magno Quites Machado Filho (tuliom)

Changes

imp got removed in Python 3.12 [1] and the community recommends using importlib in newer Python versions.

[1] https://docs.python.org/3.12/whatsnew/3.12.html#imp


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

2 Files Affected:

  • (modified) lldb/scripts/use_lldb_suite.py (+22-8)
  • (modified) lldb/test/API/use_lldb_suite.py (+21-8)
diff --git a/lldb/scripts/use_lldb_suite.py b/lldb/scripts/use_lldb_suite.py
index 6388d87b181ce03..4cedfa532cf972d 100644
--- a/lldb/scripts/use_lldb_suite.py
+++ b/lldb/scripts/use_lldb_suite.py
@@ -17,11 +17,25 @@ def find_lldb_root():
 
 
 lldb_root = find_lldb_root()
-import imp
-
-fp, pathname, desc = imp.find_module("use_lldb_suite_root", [lldb_root])
-try:
-    imp.load_module("use_lldb_suite_root", fp, pathname, desc)
-finally:
-    if fp:
-        fp.close()
+
+# Module imp got removed in Python 3.12.
+if (
+    sys.version_info.major == 3 and sys.version_info.minor >= 12
+) or sys.version_info.major > 3:
+    import importlib.machinery
+    import importlib.util
+
+    path = os.path.join(lldb_root, "use_lldb_suite_root.py")
+    loader = importlib.machinery.SourceFileLoader("use_lldb_suite_root", path)
+    spec = importlib.util.spec_from_loader("use_lldb_suite_root", loader=loader)
+    module = importlib.util.module_from_spec(spec)
+    loader.exec_module(module)
+else:
+    import imp
+
+    fp, pathname, desc = imp.find_module("use_lldb_suite_root", [lldb_root])
+    try:
+        imp.load_module("use_lldb_suite_root", fp, pathname, desc)
+    finally:
+        if fp:
+            fp.close()
diff --git a/lldb/test/API/use_lldb_suite.py b/lldb/test/API/use_lldb_suite.py
index e237dd4b8a5607c..c9332d9921b4eb3 100644
--- a/lldb/test/API/use_lldb_suite.py
+++ b/lldb/test/API/use_lldb_suite.py
@@ -20,11 +20,24 @@ def find_lldb_root():
 
 lldb_root = find_lldb_root()
 
-import imp
-
-fp, pathname, desc = imp.find_module("use_lldb_suite_root", [lldb_root])
-try:
-    imp.load_module("use_lldb_suite_root", fp, pathname, desc)
-finally:
-    if fp:
-        fp.close()
+# Module imp got removed in Python 3.12.
+if (
+    sys.version_info.major == 3 and sys.version_info.minor >= 12
+) or sys.version_info.major > 3:
+    import importlib.machinery
+    import importlib.util
+
+    path = os.path.join(lldb_root, "use_lldb_suite_root.py")
+    loader = importlib.machinery.SourceFileLoader("use_lldb_suite_root", path)
+    spec = importlib.util.spec_from_loader("use_lldb_suite_root", loader=loader)
+    module = importlib.util.module_from_spec(spec)
+    loader.exec_module(module)
+else:
+    import imp
+
+    fp, pathname, desc = imp.find_module("use_lldb_suite_root", [lldb_root])
+    try:
+        imp.load_module("use_lldb_suite_root", fp, pathname, desc)
+    finally:
+        if fp:
+            fp.close()

@DavidSpickett
Copy link
Collaborator

Do you know when importlib was added? I wonder if the overlap is enough to not need a fallback. We appear to advertise a 3.6 or 3.7 minimum in our documentation, and llvm wants >=3.6 (https://llvm.org/docs/GettingStarted.html#software).

@JDevlieghere will know for sure.

@tuliom
Copy link
Contributor Author

tuliom commented Oct 27, 2023

This document mentions 3.1.
Many functions were added after 3.6, but none of them are being explicitly used in this patch.
I'm afraid the easiest way to guarantee is by testing with Python 3.6.

@tuliom
Copy link
Contributor Author

tuliom commented Oct 27, 2023

I believe it's also important to mention this PR does not solve issue #70453.
But it helps to solve ModuleNotFoundError: No module named 'imp' kind of issues.

Copy link
Member

@bulbazord bulbazord left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

importlib has been around since Python 3.1, maybe we shouldn't have the conditional logic to use imp instead? I'm pretty sure the minimum supported version is 3.6 since that's what LLVM requires.

@JDevlieghere
Copy link
Member

+1, if you can verify that Python 3.6 supports the importlib approach we should drop the fallback.

@tuliom
Copy link
Contributor Author

tuliom commented Oct 30, 2023

Do you know what is the reason for requiring such an old Python version?
I'm asking because it's been hard to find a distro that provides both Python 3.6 (released in 2016) and cmake 3.20 (released in 2021).

For the record, Ubuntu 18.04 does have Python 3.6, but it has cmake 3.10.

@bulbazord
Copy link
Member

Do you know what is the reason for requiring such an old Python version? I'm asking because it's been hard to find a distro that provides both Python 3.6 (released in 2016) and cmake 3.20 (released in 2021).

For the record, Ubuntu 18.04 does have Python 3.6, but it has cmake 3.10.

The minimum supported version of Python is 3.6 because of the requirements of the lit testing tool that llvm uses. It's written completely in python from what I remember. Bumping the minimum required python version would require a discussion on the LLVM Discourse. I would personally like to see the minimum version bumped up, but I don't have time to drive that right now.

The CMake portion is a little easier to manage -- You can build a newer version of CMake on Ubuntu 18.04 (that's what we do downstream for the Swift project).

@tuliom
Copy link
Contributor Author

tuliom commented Oct 31, 2023

I confirmed the new code works with Python 3.6 and removed the conditional.

Copy link
Member

@bulbazord bulbazord left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for taking the time to make sure it works with Python 3.6! I'm a big fan of this kind of modernization/cleanup work. 😄

@tuliom tuliom merged commit 2260ebf into llvm:main Oct 31, 2023
medismailben pushed a commit to medismailben/llvm-project that referenced this pull request Nov 6, 2023
)

imp got removed in Python 3.12 [1] and the community recommends using
importlib in newer Python versions.

[1] https://docs.python.org/3.12/whatsnew/3.12.html#imp

(cherry picked from commit 2260ebf)
JDevlieghere pushed a commit to swiftlang/llvm-project that referenced this pull request Apr 17, 2024
)

imp got removed in Python 3.12 [1] and the community recommends using
importlib in newer Python versions.

[1] https://docs.python.org/3.12/whatsnew/3.12.html#imp

(cherry picked from commit 2260ebf)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants