[lldb] Don't rely on ScriptInterpreterPythonImpl::Initialize in the unit tests #82096
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…nittests
The unit tests only test the Python objects and don't actually use
anything from the LLDB module. That means that all the additional
complexity in ScriptInterpreterPythonImpl::Initialize is overkill.
By doing the initialization by hand, we avoid the annoying
ModuleNotFoundError.
Traceback (most recent call last):
File "<string>", line 1, in <module>
ModuleNotFoundError: No module named 'lldb'
The error is the result of us stubbing out the SWIG (specifically
`PyInit__lldb`) because we cannot link against libLLDB from the unit
tests.
The downside of doing the initialization manually is that we do lose a
bit of test coverage. For example, issue llvm#70453 also manifested itself
in the unit tests.
JDevlieghere
force-pushed
the
PythonTestSuiteInit
branch
from
5e3abb8 to
7a05913
Compare
JDevlieghere
changed the title
JDevlieghere
requested review from
bulbazord,
felipepiovezan,
medismailben and
jimingham
|
@llvm/pr-subscribers-lldb Author: Jonas Devlieghere (JDevlieghere) ChangesThe unit tests only test the Python objects and don't actually use By doing the initialization by hand, we avoid the annoying Traceback (most recent call last): The error is the result of us stubbing out the SWIG (specifically The downside of doing the initialization manually is that we do lose a Full diff: https://github.com/llvm/llvm-project/pull/82096.diff 1 Files Affected:
diff --git a/lldb/unittests/ScriptInterpreter/Python/PythonTestSuite.cpp b/lldb/unittests/ScriptInterpreter/Python/PythonTestSuite.cpp
index 5f0cc4c23db7b2..23162436d42c94 100644
--- a/lldb/unittests/ScriptInterpreter/Python/PythonTestSuite.cpp
+++ b/lldb/unittests/ScriptInterpreter/Python/PythonTestSuite.cpp
@@ -9,43 +9,27 @@
#include "gtest/gtest.h"
#include "Plugins/ScriptInterpreter/Python/SWIGPythonBridge.h"
-#include "Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.h"
-#include "Plugins/ScriptInterpreter/Python/ScriptInterpreterPythonImpl.h"
#include "Plugins/ScriptInterpreter/Python/lldb-python.h"
-#include "lldb/Host/FileSystem.h"
-#include "lldb/Host/HostInfo.h"
-
#include "PythonTestSuite.h"
#include <optional>
-using namespace lldb_private;
-class TestScriptInterpreterPython : public ScriptInterpreterPythonImpl {
-public:
- using ScriptInterpreterPythonImpl::Initialize;
-};
-
void PythonTestSuite::SetUp() {
- FileSystem::Initialize();
- HostInfoBase::Initialize();
- // ScriptInterpreterPython::Initialize() depends on HostInfo being
- // initializedso it can compute the python directory etc.
- TestScriptInterpreterPython::Initialize();
-
// Although we don't care about concurrency for the purposes of running
// this test suite, Python requires the GIL to be locked even for
// deallocating memory, which can happen when you call Py_DECREF or
// Py_INCREF. So acquire the GIL for the entire duration of this
// test suite.
+ Py_InitializeEx(0);
m_gil_state = PyGILState_Ensure();
+ PyRun_SimpleString("import sys");
}
void PythonTestSuite::TearDown() {
PyGILState_Release(m_gil_state);
- TestScriptInterpreterPython::Terminate();
- HostInfoBase::Terminate();
- FileSystem::Terminate();
+ // We could call Py_FinalizeEx here, but initializing and finalizing Python is
+ // pretty slow, so just keep Python initialized across tests.
}
// The following functions are the Pythonic implementations of the required
@@ -219,7 +203,7 @@ bool lldb_private::python::SWIGBridge::LLDBSwigPythonCallCommandObject(
}
bool lldb_private::python::SWIGBridge::LLDBSwigPythonCallParsedCommandObject(
- PyObject *implementor, lldb::DebuggerSP debugger,
+ PyObject *implementor, lldb::DebuggerSP debugger,
StructuredDataImpl &args_impl,
lldb_private::CommandReturnObject &cmd_retobj,
lldb::ExecutionContextRefSP exe_ctx_ref_sp) {
|
I think this is an acceptable tradeoff. The unit tests are for testing LLDB's python internals, not for testing LLDB's python interface and integration. |
bulbazord
approved these changes
Feb 18, 2024
medismailben
approved these changes
Feb 18, 2024
|
This change has caused a failing test on Linux: https://lab.llvm.org/buildbot/#/builders/68/builds/69157 That's not the first build, because the bot was red for ages beforehand and I'm not going to check them all. I've bisected it on arm64 instead to confirm it and get a better traceback: Seems like this Initialize had some side effects beyond Python itself? |
Fixed w/ 6757913 |
|
Thanks! Fixed the x86 bot and I confirmed it locally. |
|
Thank you! |
JDevlieghere
added a commit
to swiftlang/llvm-project
that referenced
this pull request
Apr 17, 2024
…nit tests (llvm#82096) The unit tests only test the Python objects and don't actually use anything from the LLDB module. That means that all the additional complexity in ScriptInterpreterPythonImpl::Initialize is overkill. By doing the initialization by hand, we avoid the annoying ModuleNotFoundError. Traceback (most recent call last): File "<string>", line 1, in <module> ModuleNotFoundError: No module named 'lldb' The error is the result of us stubbing out the SWIG (specifically `PyInit__lldb`) because we cannot link against libLLDB from the unit tests. The downside of doing the initialization manually is that we do lose a bit of test coverage. For example, issue llvm#70453 also manifested itself in the unit tests. (cherry picked from commit 5c96e71)
JDevlieghere
added a commit
to swiftlang/llvm-project
that referenced
this pull request
May 24, 2024
…nit tests (llvm#82096) The unit tests only test the Python objects and don't actually use anything from the LLDB module. That means that all the additional complexity in ScriptInterpreterPythonImpl::Initialize is overkill. By doing the initialization by hand, we avoid the annoying ModuleNotFoundError. Traceback (most recent call last): File "<string>", line 1, in <module> ModuleNotFoundError: No module named 'lldb' The error is the result of us stubbing out the SWIG (specifically `PyInit__lldb`) because we cannot link against libLLDB from the unit tests. The downside of doing the initialization manually is that we do lose a bit of test coverage. For example, issue llvm#70453 also manifested itself in the unit tests. (cherry picked from commit 5c96e71)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The unit tests only test the Python objects and don't actually use
anything from the LLDB module. That means that all the additional
complexity in ScriptInterpreterPythonImpl::Initialize is overkill.
By doing the initialization by hand, we avoid the annoying
ModuleNotFoundError.
Traceback (most recent call last):
File "", line 1, in
ModuleNotFoundError: No module named 'lldb'
The error is the result of us stubbing out the SWIG (specifically
PyInit__lldb) because we cannot link against libLLDB from the unittests.
The downside of doing the initialization manually is that we do lose a
bit of test coverage. For example, issue #70453 also manifested itself
in the unit tests.