Page MenuHomePhabricator

Allow customized relative PYTHONHOME
ClosedPublic

Authored by hhb on Feb 17 2020, 9:37 AM.

Details

Summary

This change allows a hard coded relative PYTHONHOME setting. So that python can easily be packaged together with lldb.

The change includes:

  1. Extend LLDB_RELOCATABLE_PYTHON to all platforms. It defaults to ON for platforms other than Windows, to keep the behavior compatible.
  2. Allows to customize LLDB_PYTHON_HOME. But still defaults to PYTHON_HOME.
  3. LLDB_PYTHON_HOME can be a path relative to liblldb. If it is relative, we will resolve it before send it to Py_DecodeLocale.

Diff Detail

Event Timeline

hhb created this revision.Feb 17 2020, 9:37 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 17 2020, 9:37 AM
hhb edited the summary of this revision. (Show Details)Feb 17 2020, 9:37 AM
hhb added a reviewer: labath.
labath added a reviewer: ted.Feb 18 2020, 1:14 AM

+ ted, who I believe has a stake in the relocatable python game

Your use case seems reasonable, but I find it hard to understand the meanings of the options in this patch. IIUC, there are now two ways to create an lldb with a "relocatable" python

  1. set LLDB_RELOCATABLE_PYTHON to ON, and set the PYTHONHOME environment variable before launching lldb
  2. set LLDB_RELOCATABLE_PYTHON to OFF, and also set LLDB_PYTHON_HOME to a relative path

It took me like five minutes to understand how setting LLDB_RELOCATABLE_PYTHON=OFF can help you ship python side-by-side with lldb (i.e. make it position indepent/relocatable). I think the main cause of the confusion is that the name LLDB_RELOCATABLE_PYTHON does not express very well what that option does -- it really should be something like LLDB_EMBED_PYTHON_HOME. Can we maybe rename it to something like that? Or maybe even just a single LLDB_PYTHON_HOME variable, with the empty value meaning we use the default python mechanism (PYTHONHOME env var, or the baked-in python default)?

The other thing which bugs me is that the relative python path chosen here will likely only be correct once lldb is installed. Will it be possible to run lldb (and its test suite) configured in this way directly from the build tree? I don't know if there's anything we can or should do about that, but this situation seems less than ideal...

lldb/cmake/modules/LLDBConfig.cmake
67

missing $ in {default_relocatable_python}

155

typo PYTHONHONE

lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
298

Is the remove_dot_dots really necessary? It can produce unexpected results when ..s back up over symlinks...

hhb updated this revision to Diff 245206.Feb 18 2020, 10:37 AM
hhb marked 5 inline comments as done.

Fix comments

hhb updated this revision to Diff 245211.Feb 18 2020, 10:50 AM

Fix comments.

hhb added a comment.Feb 18 2020, 10:51 AM

Can we maybe rename it to something like that?

LLDB_RELOCATABLE_PYTHON is misleading. I'd be happy to rename it. But that may break people who already set this flag.

Is there a standard procedure to do this? Maybe add the new one and remove the old one a year later? Or maybe fail when the old is set. So that at least people know how to fix?

Or maybe even just a single LLDB_PYTHON_HOME variable, with the empty value meaning we use the default python mechanism (PYTHONHOME env var, or the baked-in python default)?

To do that, we have to make LLDB_PYTHON_HOME default to PYTHON_HOME for Windows. And if windows users want to make it "relocatable", they should set LLDB_PYTHON_HOME back to empty. That's also somehow weird...

The other thing which bugs me is that the relative python path chosen here will likely only be correct once lldb is installed. Will it be possible to run lldb (and its test suite) configured in this way directly from the build tree? I don't know if there's anything we can or should do about that, but this situation seems less than ideal...

If someone uses relative python, that mean they should be responsible to put Python at the right place. No matter it is build tree or install tree. Or in other words, the install tree won't work by default either. People still need to put a symlink to python at the right place.

lldb/cmake/modules/LLDBConfig.cmake
67

Good catch. Thanks!

lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
298

I was just trying to make the string shorter. It is not necessary. Removed.

But just curious what is the unexpected result? Say if I have a symlink "/src/python" -> "/usr/local/lib/python3.7", a "/src/python/../" should be resolved to /src, with or without remove_dot_dots?

Well I guess things get more interesting when liblldb is a symlink... That doesn't affect my use case though.

hhb marked an inline comment as done.Feb 18 2020, 11:00 AM
hhb added inline comments.
lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
298

Kind of understood after searching around. For me remove_dot_dots is the expected behavior. But yea let's remove it to reduce some confusion...

In D74727#1881055, @hhb wrote:

Can we maybe rename it to something like that?

LLDB_RELOCATABLE_PYTHON is misleading. I'd be happy to rename it. But that may break people who already set this flag.

Is there a standard procedure to do this? Maybe add the new one and remove the old one a year later? Or maybe fail when the old is set. So that at least people know how to fix?

We've made breaking changes like that in the past. This might even be a good thing when we're changing behavior.

Or maybe even just a single LLDB_PYTHON_HOME variable, with the empty value meaning we use the default python mechanism (PYTHONHOME env var, or the baked-in python default)?

To do that, we have to make LLDB_PYTHON_HOME default to PYTHON_HOME for Windows. And if windows users want to make it "relocatable", they should set LLDB_PYTHON_HOME back to empty. That's also somehow weird...

Another alternative would be to default to "Auto", like we do with the optional dependencies, in which case it would default to PYTHON_HOME. Empty would always signal a relocatable python.

The other thing which bugs me is that the relative python path chosen here will likely only be correct once lldb is installed. Will it be possible to run lldb (and its test suite) configured in this way directly from the build tree? I don't know if there's anything we can or should do about that, but this situation seems less than ideal...

If someone uses relative python, that mean they should be responsible to put Python at the right place. No matter it is build tree or install tree. Or in other words, the install tree won't work by default either. People still need to put a symlink to python at the right place.

In D74727#1881055, @hhb wrote:

Can we maybe rename it to something like that?

LLDB_RELOCATABLE_PYTHON is misleading. I'd be happy to rename it. But that may break people who already set this flag.

Is there a standard procedure to do this? Maybe add the new one and remove the old one a year later? Or maybe fail when the old is set. So that at least people know how to fix?

We've made breaking changes like that in the past. This might even be a good thing when we're changing behavior.

Yeah, I wouldn't worry about that. When we were doing some dependency refactorings two months ago, we left in some compat code. However, this was only because some buildbots depended on the old options, and the code was removed as soon as those were updated.

But you don't need to worry about that -- I don't see any bot using this option.

Or maybe even just a single LLDB_PYTHON_HOME variable, with the empty value meaning we use the default python mechanism (PYTHONHOME env var, or the baked-in python default)?

To do that, we have to make LLDB_PYTHON_HOME default to PYTHON_HOME for Windows. And if windows users want to make it "relocatable", they should set LLDB_PYTHON_HOME back to empty. That's also somehow weird...

Yes, I can see how that could be confusing. Then let's stick with two options, but rename LLDB_RELOCATABLE_PYTHON to indicate that is really about hardcoding the PYTHONHOME path?

The other thing which bugs me is that the relative python path chosen here will likely only be correct once lldb is installed. Will it be possible to run lldb (and its test suite) configured in this way directly from the build tree? I don't know if there's anything we can or should do about that, but this situation seems less than ideal...

If someone uses relative python, that mean they should be responsible to put Python at the right place. No matter it is build tree or install tree. Or in other words, the install tree won't work by default either. People still need to put a symlink to python at the right place.

Yeah, that makes kind of sense. I think I can go with that...

labath added inline comments.Feb 19 2020, 5:40 AM
lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
298

Yeah, a lot of people think that is the expected behavior, and then get surprised when something different happens. Unfortunately, the surprise does not happen frequently enough to for people to become aware of that...

hhb updated this revision to Diff 245499.Feb 19 2020, 1:26 PM

Rename LLDB_RELOCATABLE_PYTHON to LLDB_EMBED_PYTHON_HOME.

labath accepted this revision.Feb 20 2020, 1:17 AM

This looks good to me.

lldb/cmake/modules/LLDBConfig.cmake
67–69

Maybe this could also be in the if(LLDB_ENABLE_PYTHON) block below?

This revision is now accepted and ready to land.Feb 20 2020, 1:17 AM
hhb updated this revision to Diff 245750.Feb 20 2020, 2:38 PM

Move option() into if

hhb updated this revision to Diff 245770.Feb 20 2020, 5:01 PM

The code should check on LLDB_EMBED_PYTHON_HOME

labath accepted this revision.Feb 21 2020, 4:52 AM
This revision was automatically updated to reflect the committed changes.