Skip to content

[Python3] Use correct Python version for LLDB-related tests #33181

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 4 commits into from
Jul 30, 2020

Conversation

tbkka
Copy link
Contributor

@tbkka tbkka commented Jul 29, 2020

LLDB builds a Python module in the form of a C shared library. This is used in some Swift tests.

Since Python module format changed between Python 2 and Python 3, these tests must be run with the same Python version that was used when LLDB built it.

Of course, in a system with multiple Python versions installed, this may or may not be the same version used by the rest of the Swift build.

This adds some logic to lit.cfg to figure out the version of Python used by LLDB and expose that as %{lldb-python} for use in tests that need to access the LLDB module.

tbkka added 2 commits July 28, 2020 18:00
The linux-fatal-backtrace script needs to run with an LLDB
module loaded into Python.  This in turn requires that the
test be run with the exact same Python version as was used
by LLDB (not just the same Python module directory).  This
can get confused on systems with multiple versions of Python
installed.

This replaces `lldb-python-path` (the Python module directory path)
with `lldb-python` (which is the correct Python version run with
the LLDB path).  This should ensure that this test is always
run with the same Python version and module that LLDB used.
@tbkka
Copy link
Contributor Author

tbkka commented Jul 29, 2020

@swift-ci Please test Linux

@tbkka
Copy link
Contributor Author

tbkka commented Jul 29, 2020

Since the only affected test runs only on Linux, let's fully test there first and make sure everything works before running a smoke test on macOS.

@tbkka tbkka requested review from compnerd and gmilos July 29, 2020 16:30
Copy link
Member

@compnerd compnerd left a comment

Choose a reason for hiding this comment

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

Thanks, this definitely will allow the tests to pass. However, we should also simultaneously look at migrating the lldb build to python3. Either way, we should do this to make the system less fragile.

@tbkka
Copy link
Contributor Author

tbkka commented Jul 29, 2020

@swift-ci Please test Linux

@swift-ci
Copy link
Contributor

Build failed
Swift Test Linux Platform
Git Sha - 689e974

Previous code required a decimal point before it would
recognize a path component as a Python version identifier,
so it would accept `python2.7` but not `python3`.
@tbkka
Copy link
Contributor Author

tbkka commented Jul 29, 2020

@swift-ci Please smoke test macOS

@rintaro
Copy link
Member

rintaro commented Jul 30, 2020

@swift-ci Please smoke test Linux

@tbkka tbkka merged commit 1e55916 into swiftlang:master Jul 30, 2020
@JDevlieghere
Copy link
Contributor

While this is fine as a ad-hoc solution, really what we want here is to figure out the Python LLDB was build against from the LLDB build because it's the one-and-only source of truth. I can envision many scenarios where this won't work, the simplest one where a user passes a -DPYTHON_HOME CMake flag to LLDB to a Python that's not in path.

@tbkka
Copy link
Contributor Author

tbkka commented Jul 31, 2020

If the _lldb module is intended to be used by user Python scripts, then there needs to be a mechanism outside of the build itself for doing this. Currently, the lldb executable has the -P option to provide the path to the built module. There should be another option to provide a path to a suitable python executable that can run that module. (I suppose LLDB itself could serve as that executable, since it has the appropriate Python version embedded in it. 🤔)

@JDevlieghere
Copy link
Contributor

JDevlieghere commented Jul 31, 2020

If the _lldb module is intended to be used by user Python scripts, then there needs to be a mechanism outside of the build itself for doing this.

I disagree, I don't think that's our responsibility. Nothing prevents you from running Python from inside the embedded script interpreter, maybe we should modify the test to do that instead? FWIW this constraint is not limited to LLDB but applies to anyone that builds a Python module that itself links against Python.

Also, the potential problem I described above is inherent to the unfortunate swift <- lldb <- swift test dependency chain. If this were an LLDB test this wouldn't be an issue because the build would be aware of which Python was used. This is not really different from making sure LLVM and Swift are using the same Python interpreter. But because of the dependency we can't really tell you. I guess another solution would be for us to copy the used Python interpreter into a well known location (say next to the lldb binary) and have the Swift test use that.

@JDevlieghere
Copy link
Contributor

My preferred solution would be to move to @compnerd unified build!

@JDevlieghere
Copy link
Contributor

Implemented this in #33235

drexin pushed a commit to drexin/swift that referenced this pull request Aug 12, 2020
…g#33181)

* Use the correct Python executable to match LLDB

The linux-fatal-backtrace script needs to run with an LLDB
module loaded into Python.  This in turn requires that the
test be run with the exact same Python version as was used
by LLDB (not just the same Python module directory).  This
can get confused on systems with multiple versions of Python
installed.

This replaces `lldb-python-path` (the Python module directory path)
with `lldb-python` (which is the correct Python version run with
the LLDB path).  This should ensure that this test is always
run with the same Python version and module that LLDB used.

* Use consistent braces for the lldb-python substitution

* Use os.path utilities to dissect paths

* Allow `python3` as a path identifier

Previous code required a decimal point before it would
recognize a path component as a Python version identifier,
so it would accept `python2.7` but not `python3`.
@tbkka tbkka deleted the tbkka/py3m branch October 16, 2020 00:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants