Page MenuHomePhabricator

[ScriptInterpreter] Make sure that PYTHONHOME is right.
ClosedPublic

Authored by davide on Mar 22 2019, 2:41 PM.

Diff Detail

Event Timeline

davide created this revision.Mar 22 2019, 2:41 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 22 2019, 2:41 PM
JDevlieghere accepted this revision.Mar 22 2019, 2:54 PM

I think this is okay, although I would prefer if there was a way to deduct the correct python home from the Python library we link against. I guess if that was possible we didn't need to set the PythonHome in the first place. We also can't deduct it from the Python interpreter (PYTHON_EXECUTABLE) because as we've seen in the past, it's possible that the one in your path (e.g. when installed with HomeBrew or from python.org) is different from the system one we link against on Darwin.

TL;DR LGTM

This revision is now accepted and ready to land.Mar 22 2019, 2:54 PM
aprantl added inline comments.Mar 22 2019, 3:07 PM
lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
181

Just to don't break hypothetical users building their own LLDB on ancient versions of macOS let's also change the minor version.

davide updated this revision to Diff 191958.Mar 22 2019, 3:17 PM

Adrian's feedback

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMar 22 2019, 3:18 PM
labath added a subscriber: labath.Mar 25 2019, 5:05 AM

It sounds to me like you could achieve the same thing by generalizing the LLDB_PYTHON_HOME logic in LLDBConfig.cmake. This would have the advantage of centralizing the way we manage python-finding logic (instead of each OS doing it's own thing) and also enable those users, who know what they are doing, to override this logic and point lldb to a different python. (I don't know if there are any such users, but it does not sounds like an impossible scenario).

I think all it would take is to do something like:

  • move LLDB_RELOCATABLE_PYTHON handling outside of if(WINDOWS)
  • have the default value of LLDB_RELOCATABLE_PYTHON be false for darwin
  • possibly tweak the python-finding logic so that it prefers the one in /System/Library/Frameworks/...
ted added a subscriber: ted.Mar 25 2019, 1:17 PM

This doesn't look correct to me - it looks like there are 1 too many #endifs. I think the one at line 179 should be removed - it should have been replaced by the #else that is at line 180.

In D59719#1442181, @ted wrote:

This doesn't look correct to me - it looks like there are 1 too many #endifs. I think the one at line 179 should be removed - it should have been replaced by the #else that is at line 180.

Update your checkout.

It sounds to me like you could achieve the same thing by generalizing the LLDB_PYTHON_HOME logic in LLDBConfig.cmake. This would have the advantage of centralizing the way we manage python-finding logic (instead of each OS doing it's own thing) and also enable those users, who know what they are doing, to override this logic and point lldb to a different python. (I don't know if there are any such users, but it does not sounds like an impossible scenario).

I think all it would take is to do something like:

  • move LLDB_RELOCATABLE_PYTHON handling outside of if(WINDOWS)
  • have the default value of LLDB_RELOCATABLE_PYTHON be false for darwin
  • possibly tweak the python-finding logic so that it prefers the one in /System/Library/Frameworks/...

I'll try to make this more generic. Thanks for the input.

It sounds to me like you could achieve the same thing by generalizing the LLDB_PYTHON_HOME logic in LLDBConfig.cmake. This would have the advantage of centralizing the way we manage python-finding logic (instead of each OS doing it's own thing) and also enable those users, who know what they are doing, to override this logic and point lldb to a different python. (I don't know if there are any such users, but it does not sounds like an impossible scenario).

I think all it would take is to do something like:

  • move LLDB_RELOCATABLE_PYTHON handling outside of if(WINDOWS)
  • have the default value of LLDB_RELOCATABLE_PYTHON be false for darwin
  • possibly tweak the python-finding logic so that it prefers the one in /System/Library/Frameworks/...

Oh, this won't work with xcodebuild, I'm afraid.

It sounds to me like you could achieve the same thing by generalizing the LLDB_PYTHON_HOME logic in LLDBConfig.cmake. This would have the advantage of centralizing the way we manage python-finding logic (instead of each OS doing it's own thing) and also enable those users, who know what they are doing, to override this logic and point lldb to a different python. (I don't know if there are any such users, but it does not sounds like an impossible scenario).

I think all it would take is to do something like:

  • move LLDB_RELOCATABLE_PYTHON handling outside of if(WINDOWS)
  • have the default value of LLDB_RELOCATABLE_PYTHON be false for darwin
  • possibly tweak the python-finding logic so that it prefers the one in /System/Library/Frameworks/...

Oh, this won't work with xcodebuild, I'm afraid.

Why not? The xcode build could hardcode the value for LLDB_PYTHON_HOME, just like you do here now. That could be done either in lldb/Host/Config.h, or by passing the appropriate -D flag to the compiler via the xcode project file. My preference would be to use lldb/Host/Config.h, and to also move the cmake build off of the explicit -D, and put this variable into Config.h.cmake as well.