Page MenuHomePhabricator

[dotest] Simplify logic to find the Python path
ClosedPublic

Authored by JDevlieghere on Oct 5 2020, 10:50 AM.

Details

Summary

Simplify the logic of parsing the lldb -P output to find the python path. This removes the special handling for the LLDB.framework and instead of pattern matching known errors focus on finding a directory path that contains an __init__.py.

Diff Detail

Event Timeline

JDevlieghere requested review of this revision.Oct 5 2020, 10:50 AM
JDevlieghere created this revision.
rupprecht accepted this revision.Oct 5 2020, 4:30 PM
rupprecht added inline comments.
lldb/packages/Python/lldbsuite/test/dotest.py
548–549

Is DEVNULL necessary? I think stderr=None might work just as well.

FWIW, subprocess.DEVNULL exists as of python 3.3, but I guess we still have python2 users.

This revision is now accepted and ready to land.Oct 5 2020, 4:30 PM
JDevlieghere marked an inline comment as done.Oct 5 2020, 4:34 PM
JDevlieghere added inline comments.
lldb/packages/Python/lldbsuite/test/dotest.py
548–549

Neat, None is the default argument so I'll just omit it.

This revision was automatically updated to reflect the committed changes.
JDevlieghere marked an inline comment as done.
Herald added a project: Restricted Project. · View Herald TranscriptOct 5 2020, 7:04 PM

About -P, the man page for lldb and the driver's Options.td say it:

Prints out the path to the lldb.py file for this version of lldb.

Should it do just that? If so this can be simplified further.

About -P, the man page for lldb and the driver's Options.td say it:

Prints out the path to the lldb.py file for this version of lldb.

Should it do just that? If so this can be simplified further.

It can print the path, or it can print <COULD NOT FIND PATH> if e.g. lldb wasn't built with python support or has some non-standard python setup. So I think this is basically as simple as the parsing can get.

labath added a comment.Oct 6 2020, 8:40 AM

About -P, the man page for lldb and the driver's Options.td say it:

Prints out the path to the lldb.py file for this version of lldb.

Should it do just that? If so this can be simplified further.

It can print the path, or it can print <COULD NOT FIND PATH> if e.g. lldb wasn't built with python support or has some non-standard python setup. So I think this is basically as simple as the parsing can get.

One way this could be simplified further is to ditch -P and pass down the appropriate value from cmake/lit.

About -P, the man page for lldb and the driver's Options.td say it:

Prints out the path to the lldb.py file for this version of lldb.

Should it do just that? If so this can be simplified further.

It can print the path, or it can print <COULD NOT FIND PATH> if e.g. lldb wasn't built with python support or has some non-standard python setup. So I think this is basically as simple as the parsing can get.

One way this could be simplified further is to ditch -P and pass down the appropriate value from cmake/lit.

LLDB in Xcode supports both Python 2 and 3 and we use a default com.apple.dt.lldb DefaultPythonVersion to override the version. By using -P here we can run the test suite with both versions by just changing the value of the default without having to reconfigure CMake. I'm not saying we should not do this because our donwstream fork, just that it would complicate things for us.

One way this could be simplified further is to ditch -P and pass down the appropriate value from cmake/lit.

LLDB in Xcode supports both Python 2 and 3 and we use a default com.apple.dt.lldb DefaultPythonVersion to override the version. By using -P here we can run the test suite with both versions by just changing the value of the default without having to reconfigure CMake. I'm not saying we should not do this because our donwstream fork, just that it would complicate things for us.

Yea, I'm not volunteering to do that (though I might one day), but... doesn't the python version that lldb uses and the python version running dotest needs to match? I'd expect that you'd still need to change /something/ so that it runs the different version of the python interpreter...

One way this could be simplified further is to ditch -P and pass down the appropriate value from cmake/lit.

LLDB in Xcode supports both Python 2 and 3 and we use a default com.apple.dt.lldb DefaultPythonVersion to override the version. By using -P here we can run the test suite with both versions by just changing the value of the default without having to reconfigure CMake. I'm not saying we should not do this because our donwstream fork, just that it would complicate things for us.

Yea, I'm not volunteering to do that (though I might one day), but... doesn't the python version that lldb uses and the python version running dotest needs to match? I'd expect that you'd still need to change /something/ so that it runs the different version of the python interpreter...

Yep, we have a little snippet that checks the default in lit.cfg.py that swaps out the configured Python interpreter with the system one matching the default when set, similar to our little hack for running Python under ASan: https://github.com/llvm/llvm-project/blob/a3adcba645eec31b42ad0a1f727975c5c9c236f0/lldb/test/API/lit.cfg.py#L117