[lldb] Add $HOME to Python's sys.path

[JDevlieghere]

Summary

Add support for importing scripts relative the current home directory.

rdar://68310384

[kastiglione]

Could this have undesirable side effects? I wouldn't expect command script import to be searching my home dir. Second question: is there value in requiring the explicit use of ~, for ex: command script import ~/path.

[labath]

What exactly is the use case for this?

Having a place where the user can place lldb python scripts and have them be automatically accessible seems like a nice feature, but I am not convinced that $HOME is the right place for it. I definitely wouldn't want lldb scripts to litter my home directory, and if I started putting them in a subfolder, their import paths would get weird (particularly if they want to import one another).

I think it'd be better if this was some subdirectory of $HOME. Since we already have $HOME/.lldb, maybe we could put this there (or some subdirectory thereof)?

In D89295#2326782, @kastiglione wrote:

Could this have undesirable side effects? I wouldn't expect command script import to be searching my home dir. Second question: is there value in requiring the explicit use of ~, for ex: command script import ~/path.

That actually seems to work (if you add .py to the end).

[teemperor]

In D89295#2327367, @labath wrote:

What exactly is the use case for this?

Xcode running a project-specific lldbinit file but users can't specify a command script import relative/path/script.py as the CWD for the lldb-rpc-server is /. This makes it impossible to check in custom LLDB Python scripts to an Xcode project that are loaded for all developers (as one can only specify the full path to the Python script).

I'm not sure if this patch is actually solving the problem. Now all devs need to have the project in the same path relative to their home directory.

I think we could solve this by having Xcode set the CWD to the project's source dir or we add the directories of .lldbinit files to Python's sys.path.

[labath]

Interesting. I think that implementing a project-specific script repository would be indeed best done in xcode, since that's the thing which knows what a "project" is.

I'm not sure if this needs any special kind of support from lldb (it sounds like xcode could just modify the import path itself), but if we wanted to add some explicit form of lldb support, I think it should take form of a setting, whose value is automatically added to the import path.

[kastiglione]

In D89295#2327367, @labath wrote:

That actually seems to work (if you add .py to the end).

Yes that was my (poorly explained) point, I use this form regularly. Is it not better to require an explicit ~ instead of allowing for $HOME as an implicit location.

[kastiglione]

For lldbinit files, and any file that gets command source'd, I think it would be useful if they could perform command script import some/path/to/command.py, where some is resolved relative to the dirname of the lldb file. For example, given an lldbinit file at my/project/scripts/project.lldb, it could load a python at my/project/scripts/commands/my.py by running command script import commands/my.py.

[teemperor]

In D89295#2327751, @kastiglione wrote:

For lldbinit files, and any file that gets command source'd, I think it would be useful if they could perform command script import some/path/to/command.py, where some is resolved relative to the dirname of the lldb file. For example, given an lldbinit file at my/project/scripts/project.lldb, it could load a python at my/project/scripts/commands/my.py by running command script import commands/my.py.

+1, I think that would be useful. Not sure about the right way to implement this though (and whether we should make this implicit a relative path vs for example some kind of placeholder 'variable' or something like that).

[kastiglione]

(and whether we should make this implicit a relative path vs for example some kind of placeholder 'variable' or something like that).

I'm not sure either, but if we do it could probably be as a ./ as prefix?

[JDevlieghere]

I misunderstood what was being asked for. In LLDB there are two places we look for an .lldbinit file, in the home directory and in the current working directory. We already add . to the system path so I don't think it's that unreasonable to do the same for $HOME to be able to use imports relative to the .lldbinit file in both cases.

In Xcode there's an option to specify a custom "LLDB Init File" which is really nothing more than a HandleCommandsFromFile. So what they're asking is to be able to use any relative import when sourcing a file.

[JDevlieghere]

PS: when I created this patch I used $HOME in the title, I didn't expect that to get expanded?!

[kastiglione]

I don't think it's that unreasonable to do the same for $HOME to be able to use imports relative to the .lldbinit file in both cases.

In theory I agree, but in practice I think it's unnecessary because ~ can be used (and possibly should be explicit), and because $HOME is a weird place to put lldb python scripts, as @labath stated.

Should we create some kind of $PATH variable for lldb to use for loading scripts? People could set this in their ~/.lldbinit? This still wouldn't resolve the original bug report, but it may be a nice general addition.

[JDevlieghere]

In D89295#2327809, @kastiglione wrote:

I don't think it's that unreasonable to do the same for $HOME to be able to use imports relative to the .lldbinit file in both cases.

In theory I agree, but in practice I think it's unnecessary because ~ can be used (and possibly should be explicit), and because $HOME is a weird place to put lldb python scripts, as @labath stated.

Other than as a way to the test this change I never advocated putting Python scripts in your home directory. And playing the Devil's advocate: I think the same argument could be made for using ./ as for ~. If it were up to me I still think there's some value in making the two consistent, but I like I said I misunderstood the request so I don't feel strongly enough to fight for it.

Should we create some kind of $PATH variable for lldb to use for loading scripts? People could set this in their ~/.lldbinit? This still wouldn't resolve the original bug report, but it may be a nice general addition.

They could already do what AddToSysPath does by hand if they really wanted to. I think this is more about improving the UX.

script sys.path.append("$SRCROOT")

[labath]

In D89295#2327767, @teemperor wrote:

In D89295#2327751, @kastiglione wrote:

For lldbinit files, and any file that gets command source'd, I think it would be useful if they could perform command script import some/path/to/command.py, where some is resolved relative to the dirname of the lldb file. For example, given an lldbinit file at my/project/scripts/project.lldb, it could load a python at my/project/scripts/commands/my.py by running command script import commands/my.py.

+1, I think that would be useful. Not sure about the right way to implement this though (and whether we should make this implicit a relative path vs for example some kind of placeholder 'variable' or something like that).

That would be kind of useful, but the thing which worries me about that is the sys.path packing it will cause. I don't think that sys.path modification should be taken lightly, as every new entry there creates an opportunity for creating ambiguous imports (being able to import one file through several paths). And a lot of "interesting" things can happen when a python module gets imported through multiple paths. I think it's bad enough that every "command script import" adds a new sys.path entry. I wouldn't want "command source" to do the same...

[JDevlieghere]

In D89295#2327905, @labath wrote:

In D89295#2327767, @teemperor wrote:

In D89295#2327751, @kastiglione wrote:

For lldbinit files, and any file that gets command source'd, I think it would be useful if they could perform command script import some/path/to/command.py, where some is resolved relative to the dirname of the lldb file. For example, given an lldbinit file at my/project/scripts/project.lldb, it could load a python at my/project/scripts/commands/my.py by running command script import commands/my.py.

+1, I think that would be useful. Not sure about the right way to implement this though (and whether we should make this implicit a relative path vs for example some kind of placeholder 'variable' or something like that).

That would be kind of useful, but the thing which worries me about that is the sys.path packing it will cause. I don't think that sys.path modification should be taken lightly, as every new entry there creates an opportunity for creating ambiguous imports (being able to import one file through several paths). And a lot of "interesting" things can happen when a python module gets imported through multiple paths. I think it's bad enough that every "command script import" adds a new sys.path entry. I wouldn't want "command source" to do the same...

The way I started implementing it is that the CommandInterpreter keeps a stack of FileSpecs and when you do command script import it asks the current interpreter for the current one and passes it along. So rather than always adding a new sys.path entry when sourcing a file, we now insert two sys.path entries when doing a command script import when sourcing and still one otherwise. Does that sound like a reasonable compromise?

[JDevlieghere]

Abandoning in favor of https://reviews.llvm.org/D89334

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

void ScriptInterpreterPythonImpl::InitializePrivate() {
   // RAII-based initialization which correctly handles multiple-initialization,
   // version- specific differences among Python 2 and Python 3, and saving and
   // restoring various other pieces of state that can get mucked with during
   // initialization.
   InitializePythonRAII initialize_guard;
 
   LLDBSwigPyInit();
 
-  // Update the path python uses to search for modules to include the current
-  // directory.
-
+  // We need to call this before AddToSysPath as it uses the sys module.
   PyRun_SimpleString("import sys");
+
+  // Update the path python uses to search for modules to include the current
+  // directory and the home directory.
   AddToSysPath(AddLocation::End, ".");
+  FileSpec home_dir_spec;
+  llvm::SmallString<128> home_dir;
+  if (FileSystem::Instance().GetHomeDirectory(home_dir))
+    AddToSysPath(AddLocation::End, std::string(home_dir));
 
   // Don't denormalize paths when calling file_spec.GetPath().  On platforms
   // that use a backslash as the path separator, this will result in executing
   // python code containing paths with unescaped backslashes.  But Python also
   // accepts forward slashes, so to make life easier we just use that.
   if (FileSpec file_spec = GetPythonDir())
     AddToSysPath(AddLocation::Beginning, file_spec.GetPath(false));
   if (FileSpec file_spec = HostInfo::GetShlibDir())

lldb/test/Shell/ScriptInterpreter/Python/Inputs/zip.py

+import lldb
+
+def zip(debugger, command, result, internal_dict):
+    print("95126")
+
+def __lldb_init_module(debugger, internal_dict):
+    debugger.HandleCommand('command script add -f zip.zip zip')

lldb/test/Shell/ScriptInterpreter/Python/relative_import.test

+# REQUIRES: python
+
+# RUN: rm -rf %t && mkdir -p %t
+# RUN: cp %S/Inputs/zip.py %t
+# RUN: env HOME=%t %lldb --script-language python -o 'command script import zip' -o 'zip' 2>&1 | FileCheck %s
+# CHECK: 95126