This is an archive of the discontinued LLVM Phabricator instance.

[lldb] Unifying lldb python path
ClosedPublic

Authored by hhb on Oct 3 2019, 9:48 PM.

Details

Summary

This is based on mgorny's D67890.

There are 3 places where python site-package path is calculated independently:

  1. finishSwigPythonLLDB.py where files are written to site-packages.
  1. lldb/scripts/CMakeLists.txt where site-packages are installed.
  1. ScriptInterpreterPython.cpp where site-packages are added to PYTHONPATH.

This change creates the path once and use it everywhere. So that they will not go out of sync.

Also it provides a chance for cross compile users to specify the right path for site-packages.

Event Timeline

hhb created this revision.Oct 3 2019, 9:48 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 3 2019, 9:48 PM
hhb updated this revision to Diff 223135.Oct 3 2019, 9:51 PM

Fix description..

hhb retitled this revision from [lldb] Remove unused variables. to [lldb] Unifying lldb python path.Oct 3 2019, 9:51 PM
hhb edited the summary of this revision. (Show Details)
hhb updated this revision to Diff 223346.Oct 4 2019, 5:35 PM

Rebase

hhb edited the summary of this revision. (Show Details)Oct 4 2019, 5:37 PM
hhb edited the summary of this revision. (Show Details)
mgorny added inline comments.Oct 4 2019, 11:46 PM
lldb/CMakeLists.txt
42

I still like my (False, False, '') version better than having to recalculate path afterwards.

hhb marked an inline comment as done.Oct 5 2019, 9:47 AM
hhb added inline comments.
lldb/CMakeLists.txt
42

I don't have a strong opinion here. Let's see what labath@ think.

That been said, I did this because some distribution modified get_python_lib() to return differently based on prefix. One example is Debian/Ubuntu, where 'dist-packages' will be used if prefix is '', '/usr' or '/usr/local'.

In reality, that only makes difference when CMAKE_INSTALL_PREFIX is set. But I guess it doesn't really matter whether we use 'dist-packages' or 'site-packages' that time, as long as it is consistent everywhere.

hhb marked an inline comment as not done.Oct 5 2019, 9:58 AM
hhb added inline comments.
lldb/CMakeLists.txt
42

Considering DESTINT, maybe empty string is better...

By the way, what's the first parameter plat_specific? In all platforms I have, it doesn't make any difference...

mgorny added inline comments.Oct 5 2019, 10:27 AM
lldb/CMakeLists.txt
42

Technically, it's for arch-dependent vs arch-independent modules, i.e. should be True for .so extensions and False for .py modules.

Judging by the documentation, it's only used to switch between sys.base_prefix (i.e. --prefix given to build Python) and sys.base_exec_prefix (--exec-prefix). However, I'm not aware of any platform where two different prefixes are used for Python.

When a prefix is given as third argument, its value is ignored. So True vs False shouldn't really matter here, hence I've left it at the default (False).

labath accepted this revision.Oct 7 2019, 4:31 AM

Thanks for the cleanup. This looks fine to me. I'll leave it up to you to figure out the best way to compute the relative python path...

lldb/CMakeLists.txt
42

All else being equal, I would prefer the version which does not recompute the relative path. However, if there is a meaningful functional difference between the two versions, then we can stick to this one, if it is more correct. As for whether the difference is "meaningful" and which version is more "correct", you guys are probably more qualified to answer that than I am...

lldb/scripts/finishSwigWrapperClasses.py
196

Given that the arg is marked as mandatory here, is there a need for the check in get_framework_python_dir? Maybe that could be an assert ?

This revision is now accepted and ready to land.Oct 7 2019, 4:31 AM
hhb marked 3 inline comments as done and an inline comment as not done.Oct 7 2019, 12:47 PM
hhb added inline comments.
lldb/CMakeLists.txt
42

I'll switch to the no recompute version in this patch. While they have some difference, I'm not aware of any platform where that matters. So I'll go with the easier way.

Also it is weird to build differently based on install prefix. Specially the prefix can be overwritten by DESTDIR afterwards..

lldb/scripts/finishSwigWrapperClasses.py
196

Other mandatory parameters do not even have an assert... Remove the check in get_framework_python_dir.

hhb updated this revision to Diff 223638.Oct 7 2019, 12:47 PM
hhb marked an inline comment as done.

Fix comments

mgorny accepted this revision.Oct 7 2019, 12:50 PM

Cool work. I presume you've tested it. I can test it tomorrow if you need me to. However, I can do that after the commit.

hhb added a comment.Oct 7 2019, 12:53 PM

Cool work. I presume you've tested it. I can test it tomorrow if you need me to. However, I can do that after the commit.

I'm doing a final round of testing on linux/darwin/windows(mingw). I'll commit after that.

hhb updated this revision to Diff 223689.Oct 7 2019, 4:10 PM

Converts python output path to cmake format.

This looks like a bug on windows exists before this change...

hhb updated this revision to Diff 223701.Oct 7 2019, 5:09 PM

Reverts the change related to python dir for windows.

FileSpec should always contain normalized path. I.e. using '/' even in windows.

hhb closed this revision.Oct 7 2019, 5:36 PM

This is merged as 61f471a and 0016b45. Closing...

Harbormaster completed remote builds in B39120: Diff 223689.
Harbormaster completed remote builds in B39129: Diff 223701.
lldb/scripts/finishSwigWrapperClasses.py