[lldb] Support Python imports relative the to the current file being sourced

[JDevlieghere]

Summary

Make it possible to use a relative path in command script import to the location of the file being sourced. This allows the user to put Python scripts next to LLDB command files and importing them without having to specify an absolute path.

rdar://68310384

[JDevlieghere]

Extend test case to show that this works with files sourcing other files.

[kastiglione]

Would you mind adding a couple tests for imports via a path to a python file, ex command script import command.py, maybe even a test that checks nested directories, ex: command script import path/to/command.py?

[kastiglione]

lldb/include/lldb/Interpreter/CommandInterpreter.h

644

if not too large, seems like it could be done in this change.

[JDevlieghere]

Fix edge cases and add more tests

[JDevlieghere]

lldb/include/lldb/Interpreter/CommandInterpreter.h

644

Fixing this requires adding a new variable which is orthogonal to this change. I prefer to keep it separate.

[JDevlieghere]

In D89334#2328372, @kastiglione wrote:

Would you mind adding a couple tests for imports via a path to a python file, ex command script import command.py, maybe even a test that checks nested directories, ex: command script import path/to/command.py?

This was a great suggestion and uncovered some unhandled edge cases. Thanks!

[JDevlieghere]

Remove bogus files

[labath]

The main question on my mind is should we be adding the directory of the python file to the path (which is what I believe is happening now), or if we should add the directory of the command file that is being sourced (and adjust the way we import the file). The main impact of this is how will the imported module "see" itself (what will it's name be), and how it will be able to import other modules.

Imagine the user has the following (reasonable, I think) file structure.

$ROOT/utils/consult_oracle.py
$ROOT/automatic_bug_finder/main.py # uses consult_oracle.py
$ROOT/awesome_backtrace_analyzer/main.py # uses consult_oracle.py
$ROOT/install_super_scripts.lldbinit # calls command script import awesome_backtrace_analyzer/main.py

If "command script import awesome_backtrace_analyzer/main.py" ends up adding $ROOT/awesome_backtrace_analyzer to the path, then this module will have a hard time importing the modules it depends on (it would either have to use weird relative imports, or mess with sys.path itself. If we add just $ROOT then it could simply import utils.consult_oracle.

I think setting the import path to $ROOT would actually make the sys.path manipulation serve some useful purpose (and also reduce the number of sys.path entries we add). If, on the other hand, we are not interested making cross-module imports work "out of the box" (like, we could say that it's the responsibility of individual modules to ensure that), we could also try to import the file without messing with sys.path at all (https://stackoverflow.com/questions/67631/how-to-import-a-module-given-the-full-path gives one way to do that).

lldb/include/lldb/Interpreter/CommandInterpreter.h

554

A comment would be very useful here.

[JDevlieghere]

In D89334#2329667, @labath wrote:

The main question on my mind is should we be adding the directory of the python file to the path (which is what I believe is happening now), or if we should add the directory of the command file that is being sourced (and adjust the way we import the file). The main impact of this is how will the imported module "see" itself (what will it's name be), and how it will be able to import other modules.

Imagine the user has the following (reasonable, I think) file structure.

$ROOT/utils/consult_oracle.py
$ROOT/automatic_bug_finder/main.py # uses consult_oracle.py
$ROOT/awesome_backtrace_analyzer/main.py # uses consult_oracle.py
$ROOT/install_super_scripts.lldbinit # calls command script import awesome_backtrace_analyzer/main.py

If "command script import awesome_backtrace_analyzer/main.py" ends up adding $ROOT/awesome_backtrace_analyzer to the path, then this module will have a hard time importing the modules it depends on (it would either have to use weird relative imports, or mess with sys.path itself. If we add just $ROOT then it could simply import utils.consult_oracle.

I guess then the user should have called command script import awesome_backtrace_analyzer to import the package rather than the main.py inside it. But I get your point. FWIW just adding the $ROOT is how I did the original implementation before adding the tests for the nested directories and .py files that Dave suggested. It would solve this issues but then doesn't support those scenarios. I don't know if it would be less confusing that you can't pass a relative path to a .py file or that you can't import another module as you described.

I think setting the import path to $ROOT would actually make the sys.path manipulation serve some useful purpose (and also reduce the number of sys.path entries we add). If, on the other hand, we are not interested making cross-module imports work "out of the box" (like, we could say that it's the responsibility of individual modules to ensure that), we could also try to import the file without messing with sys.path at all (https://stackoverflow.com/questions/67631/how-to-import-a-module-given-the-full-path gives one way to do that).

I would prefer this approach if it didn't require to name the module ourself. Any heuristic will have the risk of being ambitious as well (which is probably why the API makes you specify the module name).

[labath]

In D89334#2330287, @JDevlieghere wrote:

In D89334#2329667, @labath wrote:

The main question on my mind is should we be adding the directory of the python file to the path (which is what I believe is happening now), or if we should add the directory of the command file that is being sourced (and adjust the way we import the file). The main impact of this is how will the imported module "see" itself (what will it's name be), and how it will be able to import other modules.

Imagine the user has the following (reasonable, I think) file structure.

$ROOT/utils/consult_oracle.py
$ROOT/automatic_bug_finder/main.py # uses consult_oracle.py
$ROOT/awesome_backtrace_analyzer/main.py # uses consult_oracle.py
$ROOT/install_super_scripts.lldbinit # calls command script import awesome_backtrace_analyzer/main.py

If "command script import awesome_backtrace_analyzer/main.py" ends up adding $ROOT/awesome_backtrace_analyzer to the path, then this module will have a hard time importing the modules it depends on (it would either have to use weird relative imports, or mess with sys.path itself. If we add just $ROOT then it could simply import utils.consult_oracle.

I guess then the user should have called command script import awesome_backtrace_analyzer to import the package rather than the main.py inside it. But I get your point. FWIW just adding the $ROOT is how I did the original implementation before adding the tests for the nested directories and .py files that Dave suggested. It would solve this issues but then doesn't support those scenarios. I don't know if it would be less confusing that you can't pass a relative path to a .py file or that you can't import another module as you described.

I don't think the two options are mutually exclusive. I'm pretty sure this is just a limitation of our current importing code, which could be fixed to import awesome_backtrace_analyzer/main.py as awesome_backtrace_analyzer.main like it would be from python.

I think setting the import path to $ROOT would actually make the sys.path manipulation serve some useful purpose (and also reduce the number of sys.path entries we add). If, on the other hand, we are not interested making cross-module imports work "out of the box" (like, we could say that it's the responsibility of individual modules to ensure that), we could also try to import the file without messing with sys.path at all (https://stackoverflow.com/questions/67631/how-to-import-a-module-given-the-full-path gives one way to do that).

I would prefer this approach if it didn't require to name the module ourself. Any heuristic will have the risk of being ambitious as well (which is probably why the API makes you specify the module name).

(I assume you meant ambiguous, not ambitious :P)

Well... yes, if we do the simplest thing of naming the "module" according to the file basename then it will be ambiguous. But I would say that even _that_ is better than what we do now, because it avoids funky interactions between all the sys.path entries that we're adding -- e.g. a random file in the same directory as one of the files user imported becoming visible to python import machinery, and shadowing some other real module. It also gives us the option to do something about that ambiguity -- we could add numerical suffixes to the imported names (and have the import command print the name it used) or whatever...

[JDevlieghere]

In D89334#2331903, @labath wrote:

In D89334#2330287, @JDevlieghere wrote:

In D89334#2329667, @labath wrote:

The main question on my mind is should we be adding the directory of the python file to the path (which is what I believe is happening now), or if we should add the directory of the command file that is being sourced (and adjust the way we import the file). The main impact of this is how will the imported module "see" itself (what will it's name be), and how it will be able to import other modules.

Imagine the user has the following (reasonable, I think) file structure.

$ROOT/utils/consult_oracle.py
$ROOT/automatic_bug_finder/main.py # uses consult_oracle.py
$ROOT/awesome_backtrace_analyzer/main.py # uses consult_oracle.py
$ROOT/install_super_scripts.lldbinit # calls command script import awesome_backtrace_analyzer/main.py

If "command script import awesome_backtrace_analyzer/main.py" ends up adding $ROOT/awesome_backtrace_analyzer to the path, then this module will have a hard time importing the modules it depends on (it would either have to use weird relative imports, or mess with sys.path itself. If we add just $ROOT then it could simply import utils.consult_oracle.

I guess then the user should have called command script import awesome_backtrace_analyzer to import the package rather than the main.py inside it. But I get your point. FWIW just adding the $ROOT is how I did the original implementation before adding the tests for the nested directories and .py files that Dave suggested. It would solve this issues but then doesn't support those scenarios. I don't know if it would be less confusing that you can't pass a relative path to a .py file or that you can't import another module as you described.

I don't think the two options are mutually exclusive. I'm pretty sure this is just a limitation of our current importing code, which could be fixed to import awesome_backtrace_analyzer/main.py as awesome_backtrace_analyzer.main like it would be from python.

I don't think we can do that in the general case without breaking users (see below). I guess we could do it for imports not relative to the current working directory, as this has never worked before. It would then replace the logic described by the comment on line 2793.

I think setting the import path to $ROOT would actually make the sys.path manipulation serve some useful purpose (and also reduce the number of sys.path entries we add). If, on the other hand, we are not interested making cross-module imports work "out of the box" (like, we could say that it's the responsibility of individual modules to ensure that), we could also try to import the file without messing with sys.path at all (https://stackoverflow.com/questions/67631/how-to-import-a-module-given-the-full-path gives one way to do that).

I would prefer this approach if it didn't require to name the module ourself. Any heuristic will have the risk of being ambitious as well (which is probably why the API makes you specify the module name).

(I assume you meant ambiguous, not ambitious :P)

😅

Well... yes, if we do the simplest thing of naming the "module" according to the file basename then it will be ambiguous. But I would say that even _that_ is better than what we do now, because it avoids funky interactions between all the sys.path entries that we're adding -- e.g. a random file in the same directory as one of the files user imported becoming visible to python import machinery, and shadowing some other real module. It also gives us the option to do something about that ambiguity -- we could add numerical suffixes to the imported names (and have the import command print the name it used) or whatever...

From a technical point I agree a 100%, but I just don't see how we can do it without breaking tons of users that are using the module name in command script add:

def __lldb_init_module(debugger, internal_dict):
    debugger.HandleCommand('command script add -f module.function my_function')

I'll hold off on updating the patch until we reached consensus.

[labath]

In D89334#2332452, @JDevlieghere wrote:

In D89334#2331903, @labath wrote:

In D89334#2330287, @JDevlieghere wrote:

I guess then the user should have called command script import awesome_backtrace_analyzer to import the package rather than the main.py inside it. But I get your point. FWIW just adding the $ROOT is how I did the original implementation before adding the tests for the nested directories and .py files that Dave suggested. It would solve this issues but then doesn't support those scenarios. I don't know if it would be less confusing that you can't pass a relative path to a .py file or that you can't import another module as you described.

I don't think the two options are mutually exclusive. I'm pretty sure this is just a limitation of our current importing code, which could be fixed to import awesome_backtrace_analyzer/main.py as awesome_backtrace_analyzer.main like it would be from python.

I don't think we can do that in the general case without breaking users (see below). I guess we could do it for imports not relative to the current working directory, as this has never worked before. It would then replace the logic described by the comment on line 2793.

In general, we cannot impose any kind of natural structure on the path the user gives us -- in an absolute path, the python root could be anywhere. For cwd-relative imports, we could pretend that the cwd is the root, but that seems somewhat unintuitive (and breaks users) (*). Even the usage of the directory of the sourced file as root seems moderately unintuitive to me, though I think it might be a good fit for the motivational use case for this feature.

Speaking of users, if you know any, it might be interesting to ask them about this and see whether they'd be interested in such a thing. I don't write python scripts, so I'm just hypothesizing. (and trying to ensure we don't dig a bigger hole for ourselves than we already have.)

(*) On the flip side, we are already adding . to the path, so this would kind of make sense.

[JDevlieghere]

In D89334#2334881, @labath wrote:

In D89334#2332452, @JDevlieghere wrote:

In D89334#2331903, @labath wrote:

In D89334#2330287, @JDevlieghere wrote:

I guess then the user should have called command script import awesome_backtrace_analyzer to import the package rather than the main.py inside it. But I get your point. FWIW just adding the $ROOT is how I did the original implementation before adding the tests for the nested directories and .py files that Dave suggested. It would solve this issues but then doesn't support those scenarios. I don't know if it would be less confusing that you can't pass a relative path to a .py file or that you can't import another module as you described.

I don't think the two options are mutually exclusive. I'm pretty sure this is just a limitation of our current importing code, which could be fixed to import awesome_backtrace_analyzer/main.py as awesome_backtrace_analyzer.main like it would be from python.

I don't think we can do that in the general case without breaking users (see below). I guess we could do it for imports not relative to the current working directory, as this has never worked before. It would then replace the logic described by the comment on line 2793.

In general, we cannot impose any kind of natural structure on the path the user gives us -- in an absolute path, the python root could be anywhere. For cwd-relative imports, we could pretend that the cwd is the root, but that seems somewhat unintuitive (and breaks users) (*). Even the usage of the directory of the sourced file as root seems moderately unintuitive to me, though I think it might be a good fit for the motivational use case for this feature.

The issue is that we "resolve" cwd-relative paths to absolute paths and treat the result as an absolute path and the "." is only in the system path to make relative imports from inside the module work. We could do the same for the "source root" but that would mean adding yet another path to the system path.

The more I think about it the more I feel like the current approach in this patch is fits best with what we're already doing. If it doesn't work for the user they can always adjust the system path in the top level (imported) module.

[labath]

In D89334#2339018, @JDevlieghere wrote:

In D89334#2334881, @labath wrote:

In D89334#2332452, @JDevlieghere wrote:

I don't think we can do that in the general case without breaking users (see below). I guess we could do it for imports not relative to the current working directory, as this has never worked before. It would then replace the logic described by the comment on line 2793.

In general, we cannot impose any kind of natural structure on the path the user gives us -- in an absolute path, the python root could be anywhere. For cwd-relative imports, we could pretend that the cwd is the root, but that seems somewhat unintuitive (and breaks users) (*). Even the usage of the directory of the sourced file as root seems moderately unintuitive to me, though I think it might be a good fit for the motivational use case for this feature.

The issue is that we "resolve" cwd-relative paths to absolute paths and treat the result as an absolute path and the "." is only in the system path to make relative imports from inside the module work. We could do the same for the "source root" but that would mean adding yet another path to the system path.

I'm afraid you've lost me there. Yes, we turn relative paths into absolute ones, but I don't see how that translates into adding less entries into sys.path. For each module that we import, we add dirname(module_path) to sys.path. If we import two modules from different directories, we will get two sys.path entries, even if the modules were specified as paths relative to the same directory. Canonicalizing the path to that directory will mean less entries.

[JDevlieghere]

In D89334#2341561, @labath wrote:

In D89334#2339018, @JDevlieghere wrote:

In D89334#2334881, @labath wrote:

In D89334#2332452, @JDevlieghere wrote:

I don't think we can do that in the general case without breaking users (see below). I guess we could do it for imports not relative to the current working directory, as this has never worked before. It would then replace the logic described by the comment on line 2793.

In general, we cannot impose any kind of natural structure on the path the user gives us -- in an absolute path, the python root could be anywhere. For cwd-relative imports, we could pretend that the cwd is the root, but that seems somewhat unintuitive (and breaks users) (*). Even the usage of the directory of the sourced file as root seems moderately unintuitive to me, though I think it might be a good fit for the motivational use case for this feature.

The issue is that we "resolve" cwd-relative paths to absolute paths and treat the result as an absolute path and the "." is only in the system path to make relative imports from inside the module work. We could do the same for the "source root" but that would mean adding yet another path to the system path.

I'm afraid you've lost me there. Yes, we turn relative paths into absolute ones, but I don't see how that translates into adding less entries into sys.path. For each module that we import, we add dirname(module_path) to sys.path. If we import two modules from different directories, we will get two sys.path entries, even if the modules were specified as paths relative to the same directory. Canonicalizing the path to that directory will mean less entries.

It doesn't, it translates to more: one for the . and one for dirname(module_path). Which basically translates to n+1 entries because the working directory doesn't change. I was saying we could do the exact same thing for modules relative to the source-dir, but that means 2n entries because for every module we add source_dir and dirname(module_path) and generally both will be different. My point was that it would solve the issue of a relative import that you described last week, but at the cost at adding twice as much entries, which means double the chance of an ambiguous (not ambitious ;-) import. I think that was your main objection to the patch as it is right now and I'm arguing that I think it's the best of the different trade-offs.

[labath]

In D89334#2341889, @JDevlieghere wrote:

In D89334#2341561, @labath wrote:

I'm afraid you've lost me there. Yes, we turn relative paths into absolute ones, but I don't see how that translates into adding less entries into sys.path. For each module that we import, we add dirname(module_path) to sys.path. If we import two modules from different directories, we will get two sys.path entries, even if the modules were specified as paths relative to the same directory. Canonicalizing the path to that directory will mean less entries.

It doesn't, it translates to more: one for the . and one for dirname(module_path). Which basically translates to n+1 entries because the working directory doesn't change. I was saying we could do the exact same thing for modules relative to the source-dir, but that means 2n entries because for every module we add source_dir and dirname(module_path) and generally both will be different. My point was that it would solve the issue of a relative import that you described last week, but at the cost at adding twice as much entries, which means double the chance of an ambiguous (not ambitious ;-) import. I think that was your main objection to the patch as it is right now and I'm arguing that I think it's the best of the different trade-offs.

Why would we be adding both? We've already checked the path and know if the file exists in the cwd or not. What's the point in adding that? My impression was that this patch already avoids adding two paths in this case...

Actually, this guessing of what they user meant to say is one of the things I don't like about this approach. I think it would be better if there was some way (command option or something) to specify where the module should be imported from. The docstring for that option could also explain the rule for how the module is being imported.

[JDevlieghere]

In D89334#2344610, @labath wrote:

In D89334#2341889, @JDevlieghere wrote:

In D89334#2341561, @labath wrote:

I'm afraid you've lost me there. Yes, we turn relative paths into absolute ones, but I don't see how that translates into adding less entries into sys.path. For each module that we import, we add dirname(module_path) to sys.path. If we import two modules from different directories, we will get two sys.path entries, even if the modules were specified as paths relative to the same directory. Canonicalizing the path to that directory will mean less entries.

It doesn't, it translates to more: one for the . and one for dirname(module_path). Which basically translates to n+1 entries because the working directory doesn't change. I was saying we could do the exact same thing for modules relative to the source-dir, but that means 2n entries because for every module we add source_dir and dirname(module_path) and generally both will be different. My point was that it would solve the issue of a relative import that you described last week, but at the cost at adding twice as much entries, which means double the chance of an ambiguous (not ambitious ;-) import. I think that was your main objection to the patch as it is right now and I'm arguing that I think it's the best of the different trade-offs.

Why would we be adding both? We've already checked the path and know if the file exists in the cwd or not. What's the point in adding that? My impression was that this patch already avoids adding two paths in this case...

Actually, this guessing of what they user meant to say is one of the things I don't like about this approach. I think it would be better if there was some way (command option or something) to specify where the module should be imported from. The docstring for that option could also explain the rule for how the module is being imported.

In D89334#2344610, @labath wrote:

In D89334#2341889, @JDevlieghere wrote:

In D89334#2341561, @labath wrote:

I'm afraid you've lost me there. Yes, we turn relative paths into absolute ones, but I don't see how that translates into adding less entries into sys.path. For each module that we import, we add dirname(module_path) to sys.path. If we import two modules from different directories, we will get two sys.path entries, even if the modules were specified as paths relative to the same directory. Canonicalizing the path to that directory will mean less entries.

It doesn't, it translates to more: one for the . and one for dirname(module_path). Which basically translates to n+1 entries because the working directory doesn't change. I was saying we could do the exact same thing for modules relative to the source-dir, but that means 2n entries because for every module we add source_dir and dirname(module_path) and generally both will be different. My point was that it would solve the issue of a relative import that you described last week, but at the cost at adding twice as much entries, which means double the chance of an ambiguous (not ambitious ;-) import. I think that was your main objection to the patch as it is right now and I'm arguing that I think it's the best of the different trade-offs.

Why would we be adding both? We've already checked the path and know if the file exists in the cwd or not. What's the point in adding that? My impression was that this patch already avoids adding two paths in this case...

For paths relative to the CWD we add both already:

1. We add "." to the sys path once: https://github.com/llvm/llvm-project/blob/5d796645d6c8cadeb003715c33e231a8ba05b6de/lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp#L3234
2. We "resolve" (make absolute) the relative path (https://github.com/llvm/llvm-project/blob/5d796645d6c8cadeb003715c33e231a8ba05b6de/lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp#L2747) and if it exists add its dir to the sys path (https://github.com/llvm/llvm-project/blob/5d796645d6c8cadeb003715c33e231a8ba05b6de/lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp#L2785).

For source-relative imports all this was a hypothetical solution to the relative import from Python issue you described.

Actually, this guessing of what they user meant to say is one of the things I don't like about this approach. I think it would be better if there was some way (command option or something) to specify where the module should be imported from. The docstring for that option could also explain the rule for how the module is being imported.

How are we guessing more than before? We're doing the exact same thing as for relative paths, i.e. we resolve them and add the dir's path to the system path (= 2 from above).

[labath]

In D89334#2344765, @JDevlieghere wrote:

For paths relative to the CWD we add both already:

1. We add "." to the sys path once: https://github.com/llvm/llvm-project/blob/5d796645d6c8cadeb003715c33e231a8ba05b6de/lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp#L3234
2. We "resolve" (make absolute) the relative path (https://github.com/llvm/llvm-project/blob/5d796645d6c8cadeb003715c33e231a8ba05b6de/lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp#L2747) and if it exists add its dir to the sys path (https://github.com/llvm/llvm-project/blob/5d796645d6c8cadeb003715c33e231a8ba05b6de/lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp#L2785).

For source-relative imports all this was a hypothetical solution to the relative import from Python issue you described.

I'm afraid we've completely desynchronized by this point. Let's try to reset. The algorithm I'm proposing is:

if (is_relative_to_command_file(path))  {// how to implement that?
  ExtendPathIfNotExists(dirname(command_file));
  import(path);
} else {
  // Same as before
  path = make_absolute(path, cwd);
  ExtendPathIfNotExists(dirname(path));
  import(basename(path));
}

The algorithm that I think this patch implements is:

if (is_relative_to_command_file(path)) // implemented by checking cwd for this file ?
  path = make_absolute(path, dirname(command_file);
else
  path = make_absolute(path, cwd);
ExtendPathIfNotExists(dirname(path));
import(basename(path));

Is that an accurate depiction?

Both algorithms add at most one path entry for each import command. (I'm ignoring the '.' entry which gets added unconditionally.) For cwd-relative imports they behave the same way. The difference is in the command-relative imports. The first algorithm adds at most one path for each command file which executes import commands. The second one can add more -- if the imported scripts are in different directories, then all of those directories will be added to the path.

Actually, this guessing of what they user meant to say is one of the things I don't like about this approach. I think it would be better if there was some way (command option or something) to specify where the module should be imported from. The docstring for that option could also explain the rule for how the module is being imported.

How are we guessing more than before? We're doing the exact same thing as for relative paths, i.e. we resolve them and add the dir's path to the system path (= 2 from above).

But how do we know if the user meant to do a CWD-relative import or a command-relative one? That's an extra level of guessing (uncertainty). What if the file is present at both locations. I think it would be better if there was some way to explicitly say that you're importing a module using this command-relative scheme. And that this might give us an excuse to implement a more pythonic module import scheme (?)

[JDevlieghere]

Briefly discussed this with Pavel on IRC. The latest revision implements what I think you suggested:

• Make the new logic conditional on a new flag (-c).
• Add the "command search dir" to the path, not dirname(path).
• Change the test to import a relative module as baz.hello instead of baz/hello.

[labath]

Let's see how this goes.

lldb/source/Commands/Options.td

709 ↗

(On Diff #300803)

It might be better to make this an error

lldb/test/Shell/ScriptInterpreter/Python/command_relative_import.test

4–9 ↗

(On Diff #300803)

consider using the (new) split-file utility -- It can split single file into multiple chunks and place them in the appropriate folders.

[JDevlieghere]

In D89334#2355870, @labath wrote:

Let's see how this goes.

Thanks for bearing with me :-)

lldb/include/lldb/Interpreter/CommandInterpreter.h

public:
   ///     when no argument is passed.
   /// \param result
   ///     This is used to pass function output and error messages.
   /// \return \b true if the session transcript was successfully written to
   /// disk, \b false otherwise.
   bool SaveTranscript(CommandReturnObject &result,
                       llvm::Optional<std::string> output_file = llvm::None);
 
+  FileSpec GetCurrentSourceDir();
+
 protected:
   friend class Debugger;
 
   // IOHandlerDelegate functions
   void IOHandlerInputComplete(IOHandler &io_handler,
                               std::string &line) override;
 
   ConstString IOHandlerGetControlSequence(char ch) override {
private:
   std::string m_repeat_command; // Stores the command that will be executed for
                                 // an empty command string.
   lldb::IOHandlerSP m_command_io_handler_sp;
   char m_comment_char;
   bool m_batch_command_mode;
   ChildrenTruncatedWarningStatus m_truncation_warning; // Whether we truncated
                                                        // children and whether
                                                        // the user has been told
+
+  // FIXME: Stop using this to control adding to the history and then replace
+  // this with m_command_source_dirs.size().
   uint32_t m_command_source_depth;
+  /// A stack of directory paths. When not empty, the last one is the directory
+  /// of the file that's currently sourced.
+  std::vector<FileSpec> m_command_source_dirs;
   std::vector<uint32_t> m_command_source_flags;
   CommandInterpreterRunResult m_result;
 
   // The exit code the user has requested when calling the 'quit' command.
   // No value means the user hasn't set a custom exit code so far.
   llvm::Optional<int> m_quit_exit_code;
   // If the driver is accepts custom exit codes for the 'quit' command.
   bool m_allow_exit_code = false;
 
   StreamString m_transcript_stream;
 };
 
 } // namespace lldb_private
 
 #endif // LLDB_INTERPRETER_COMMANDINTERPRETER_H

lldb/include/lldb/Interpreter/ScriptInterpreter.h

virtual bool GetLongHelpForCommandObject(StructuredData::GenericSP cmd_obj_sp,
     return false;
   }
 
   virtual bool CheckObjectExists(const char *name) { return false; }
 
   virtual bool
   LoadScriptingModule(const char *filename, bool init_session,
                       lldb_private::Status &error,
-                      StructuredData::ObjectSP *module_sp = nullptr);
+                      StructuredData::ObjectSP *module_sp = nullptr,
+                      FileSpec extra_search_dir = {});
 
   virtual bool IsReservedWord(const char *word) { return false; }
 
   virtual std::unique_ptr<ScriptInterpreterLocker> AcquireInterpreterLock();
 
   const char *GetScriptInterpreterPtyName();
 
   virtual llvm::Expected<unsigned>

lldb/source/Commands/CommandObjectCommands.cpp

 
   bool DoExecute(Args &command, CommandReturnObject &result) override {
     if (command.empty()) {
       result.AppendError("command script import needs one or more arguments");
       result.SetStatus(eReturnStatusFailed);
       return false;
     }
 
+    FileSpec source_dir =
+        GetDebugger().GetCommandInterpreter().GetCurrentSourceDir();
+
     for (auto &entry : command.entries()) {
       Status error;
 
       const bool init_session = true;
       // FIXME: this is necessary because CommandObject::CheckRequirements()
       // assumes that commands won't ever be recursively invoked, but it's
       // actually possible to craft a Python script that does other "command
       // script imports" in __lldb_init_module the real fix is to have
       // recursive commands possible with a CommandInvocation object separate
       // from the CommandObject itself, so that recursive command invocations
       // won't stomp on each other (wrt to execution contents, options, and
       // more)
       m_exe_ctx.Clear();
       if (GetDebugger().GetScriptInterpreter()->LoadScriptingModule(
-              entry.c_str(), init_session, error)) {
+              entry.c_str(), init_session, error, nullptr, source_dir)) {
         result.SetStatus(eReturnStatusSuccessFinishNoResult);
       } else {
         result.AppendErrorWithFormat("module importing failed: %s",
                                      error.AsCString());
         result.SetStatus(eReturnStatusFailed);
       }
     }
 

lldb/source/Interpreter/CommandInterpreter.cpp

       debugger.GetUseColor(), 0, *this, nullptr));
   const bool old_async_execution = debugger.GetAsyncExecution();
 
   // Set synchronous execution if we are not stopping on continue
   if ((flags & eHandleCommandFlagStopOnContinue) == 0)
     debugger.SetAsyncExecution(false);
 
   m_command_source_depth++;
+  m_command_source_dirs.push_back(cmd_file.CopyByRemovingLastPathComponent());
 
   debugger.RunIOHandlerSync(io_handler_sp);
   if (!m_command_source_flags.empty())
     m_command_source_flags.pop_back();
+
+  m_command_source_dirs.pop_back();
   m_command_source_depth--;
+
   result.SetStatus(eReturnStatusSuccessFinishNoResult);
   debugger.SetAsyncExecution(old_async_execution);
 }
 
 bool CommandInterpreter::GetSynchronous() { return m_synchronous_execution; }
 
 void CommandInterpreter::SetSynchronous(bool value) {
   // Asynchronous mode is not supported during reproducer replay.

                      "Bytes written do not match transcript size.");
 
   result.AppendMessageWithFormat("Session's transcripts saved to %s\n",
                                  output_file->c_str());
 
   return true;
 }
 
+FileSpec CommandInterpreter::GetCurrentSourceDir() {
+  if (m_command_source_dirs.empty())
+    return {};
+  return m_command_source_dirs.back();
+}
+
 void CommandInterpreter::GetLLDBCommandsFromIOHandler(
     const char *prompt, IOHandlerDelegate &delegate, void *baton) {
   Debugger &debugger = GetDebugger();
   IOHandlerSP io_handler_sp(
       new IOHandlerEditline(debugger, IOHandler::Type::CommandList,
                             "lldb", // Name of input reader for history
                             llvm::StringRef::withNullAsEmpty(prompt), // Prompt
                             llvm::StringRef(), // Continuation prompt

lldb/source/Interpreter/ScriptInterpreter.cpp

 
 void ScriptInterpreter::CollectDataForWatchpointCommandCallback(
     WatchpointOptions *bp_options, CommandReturnObject &result) {
   result.SetStatus(eReturnStatusFailed);
   result.AppendError(
       "This script interpreter does not support watchpoint callbacks.");
 }
 
-bool ScriptInterpreter::LoadScriptingModule(
-    const char *filename, bool init_session, lldb_private::Status &error,
-    StructuredData::ObjectSP *module_sp) {
+bool ScriptInterpreter::LoadScriptingModule(const char *filename,
+                                            bool init_session,
+                                            lldb_private::Status &error,
+                                            StructuredData::ObjectSP *module_sp,
+                                            FileSpec extra_search_dir) {
   error.SetErrorString(
       "This script interpreter does not support importing modules.");
   return false;
 }
 
 std::string ScriptInterpreter::LanguageToString(lldb::ScriptLanguage language) {
   switch (language) {
   case eScriptLanguageNone:

lldb/source/Plugins/ScriptInterpreter/Lua/ScriptInterpreterLua.h

public:
   ~ScriptInterpreterLua() override;
 
   bool ExecuteOneLine(
       llvm::StringRef command, CommandReturnObject *result,
       const ExecuteScriptOptions &options = ExecuteScriptOptions()) override;
 
   void ExecuteInterpreterLoop() override;
 
-  bool
-  LoadScriptingModule(const char *filename, bool init_session,
+  bool LoadScriptingModule(const char *filename, bool init_session,
                            lldb_private::Status &error,
-                      StructuredData::ObjectSP *module_sp = nullptr) override;
+                           StructuredData::ObjectSP *module_sp = nullptr,
+                           FileSpec extra_search_dir = {}) override;
 
   // Static Functions
   static void Initialize();
 
   static void Terminate();
 
   static lldb::ScriptInterpreterSP CreateInstance(Debugger &debugger);
 

lldb/source/Plugins/ScriptInterpreter/Lua/ScriptInterpreterLua.cpp

if (!m_debugger.GetInputFile().IsValid())
     return;
 
   IOHandlerSP io_handler_sp(new IOHandlerLuaInterpreter(m_debugger, *this));
   m_debugger.RunIOHandlerAsync(io_handler_sp);
 }
 
 bool ScriptInterpreterLua::LoadScriptingModule(
     const char *filename, bool init_session, lldb_private::Status &error,
-    StructuredData::ObjectSP *module_sp) {
+    StructuredData::ObjectSP *module_sp, FileSpec extra_search_dir) {
 
   FileSystem::Instance().Collect(filename);
   if (llvm::Error e = m_lua->LoadModule(filename)) {
     error.SetErrorStringWithFormatv("lua failed to import '{0}': {1}\n",
                                     filename, llvm::toString(std::move(e)));
     return false;
   }
   return true;

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

     str.replace(pos, oldStr.length(), newStr);
     pos += newStr.length();
   }
   return matches;
 }
 
 bool ScriptInterpreterPythonImpl::LoadScriptingModule(
     const char *pathname, bool init_session, lldb_private::Status &error,
-    StructuredData::ObjectSP *module_sp) {
+    StructuredData::ObjectSP *module_sp, FileSpec extra_search_dir) {
   namespace fs = llvm::sys::fs;
+  namespace path = llvm::sys::path;
 
   if (!pathname || !pathname[0]) {
     error.SetErrorString("invalid pathname");
     return false;
   }
 
   lldb::DebuggerSP debugger_sp = m_debugger.shared_from_this();
 
   FileSpec target_file(pathname);
   FileSystem::Instance().Resolve(target_file);
   FileSystem::Instance().Collect(target_file);
   std::string basename(target_file.GetFilename().GetCString());
 
-  StreamString command_stream;
+  fs::file_status st;
+  std::error_code ec = status(target_file.GetPath(), st);
 
   // Before executing Python code, lock the GIL.
   Locker py_lock(this,
                  Locker::AcquireLock |
                      (init_session ? Locker::InitSession : 0) | Locker::NoSTDIN,
                  Locker::FreeAcquiredLock |
                      (init_session ? Locker::TearDownSession : 0));
-  fs::file_status st;
-  std::error_code ec = status(target_file.GetPath(), st);
 
-  if (ec || st.type() == fs::file_type::status_error ||
-      st.type() == fs::file_type::type_unknown ||
-      st.type() == fs::file_type::file_not_found) {
-    // if not a valid file of any sort, check if it might be a filename still
-    // dot can't be used but / and \ can, and if either is found, reject
-    if (strchr(pathname, '\\') || strchr(pathname, '/')) {
-      error.SetErrorString("invalid pathname");
-      return false;
-    }
-    basename = pathname; // not a filename, probably a package of some sort,
-                         // let it go through
-  } else if (is_directory(st) || is_regular_file(st)) {
-    if (target_file.GetDirectory().IsEmpty()) {
-      error.SetErrorString("invalid directory name");
-      return false;
+  auto ExtendSysPath = [this](std::string directory) -> llvm::Error {
+    if (directory.empty()) {
+      return llvm::make_error<llvm::StringError>(
+          "invalid directory name", llvm::inconvertibleErrorCode());
     }
 
-    std::string directory = target_file.GetDirectory().GetCString();
     replace_all(directory, "\\", "\\\\");
     replace_all(directory, "'", "\\'");
 
-    // now make sure that Python has "directory" in the search path
+    // Make sure that Python has "directory" in the search path.
     StreamString command_stream;
     command_stream.Printf("if not (sys.path.__contains__('%s')):\n    "
                           "sys.path.insert(1,'%s');\n\n",
                           directory.c_str(), directory.c_str());
     bool syspath_retval =
         ExecuteMultipleLines(command_stream.GetData(),
                              ScriptInterpreter::ExecuteScriptOptions()
                                  .SetEnableIO(false)
                                  .SetSetLLDBGlobals(false))
             .Success();
     if (!syspath_retval) {
-      error.SetErrorString("Python sys.path handling failed");
-      return false;
+      return llvm::make_error<llvm::StringError>(
+          "Python sys.path handling failed", llvm::inconvertibleErrorCode());
     }
 
-  } else {
-    error.SetErrorString("no known way to import this module specification");
-    return false;
+    return llvm::Error::success();
+  };
+
+  if (ec || st.type() == fs::file_type::status_error ||
+      st.type() == fs::file_type::type_unknown ||
+      st.type() == fs::file_type::file_not_found) {
+    // The path name doesn't exists. It might be a relative import in which
+    // case we need to resolve it using to the extra search directory if
+    // provided. Python doesn't allow having directories in a module name. If
+    // the path name contains a path separator we need to add it to the system
+    // path. For example, if the pathname is `bar/baz` and the search path is
+    // `foo`, we need to add `foo/bar` to the system path and import `baz`.
+    //
+    // If no extra search path is given and the path name contains a path
+    // separator, we know it's invalid and can reject is immediately. If no
+    // path separators are found defer, to Python and let it deal with it.
+    if (extra_search_dir) {
+      FileSpec sys_path = extra_search_dir;
+      ;
+      if (ConstString directory = target_file.GetDirectory()) {
+        llvm::SmallString<128> buffer;
+        extra_search_dir.GetPath(buffer);
+        path::append(buffer, directory.GetCString());
+        sys_path.SetPath(buffer);
+      }
+      if (llvm::Error e = ExtendSysPath(sys_path.GetPath())) {
+        error = std::move(e);
+        return false;
+      }
+    } else if (strchr(pathname, '\\') || strchr(pathname, '/')) {
+      error.SetErrorString("invalid pathname");
+      return false;
+    } else {
+      basename = pathname;
+    }
+  } else if (is_directory(st) || is_regular_file(st)) {
+    if (llvm::Error e =
+            ExtendSysPath(target_file.GetDirectory().GetCString())) {
+      error = std::move(e);
+      return false;
+    }
   }
 
   // Strip .py or .pyc extension
   llvm::StringRef extension = target_file.GetFileNameExtension().GetCString();
   if (!extension.empty()) {
     if (extension == ".py")
       basename.resize(basename.length() - 3);
     else if (extension == ".pyc")
       basename.resize(basename.length() - 4);
   }
 
   // check if the module is already import-ed
+  StreamString command_stream;
   command_stream.Clear();
   command_stream.Printf("sys.modules.__contains__('%s')", basename.c_str());
   bool does_contain = false;
   // this call will succeed if the module was ever imported in any Debugger
   // in the lifetime of the process in which this LLDB framework is living
   bool was_imported_globally =
       (ExecuteOneLineWithReturn(
            command_stream.GetData(),

lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPythonImpl.h

bool RunScriptFormatKeyword(const char *impl_function, Target *target,
                               std::string &output, Status &error) override;
 
   bool RunScriptFormatKeyword(const char *impl_function, StackFrame *frame,
                               std::string &output, Status &error) override;
 
   bool RunScriptFormatKeyword(const char *impl_function, ValueObject *value,
                               std::string &output, Status &error) override;
 
-  bool
-  LoadScriptingModule(const char *filename, bool init_session,
+  bool LoadScriptingModule(const char *filename, bool init_session,
                            lldb_private::Status &error,
-                      StructuredData::ObjectSP *module_sp = nullptr) override;
+                           StructuredData::ObjectSP *module_sp = nullptr,
+                           FileSpec extra_search_dir = {}) override;
 
   bool IsReservedWord(const char *word) override;
 
   std::unique_ptr<ScriptInterpreterLocker> AcquireInterpreterLock() override;
 
   void CollectDataForBreakpointCommandCallback(
       std::vector<BreakpointOptions *> &bp_options_vec,
       CommandReturnObject &result) override;

lldb/test/Shell/ScriptInterpreter/Python/Inputs/hello.in

+command script import baz/hello.py

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

+import lldb
+
+def hello(debugger, command, result, internal_dict):
+    print("Hello, World!")
+
+def __lldb_init_module(debugger, internal_dict):
+    debugger.HandleCommand('command script add -f hello.hello hello')

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

+command script import zip

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/foo/bar/baz
+# RUN: cp %S/Inputs/zip.py %t/foo
+# RUN: cp %S/Inputs/zip.in %t/foo
+# RUN: cp %S/Inputs/hello.py %t/foo/bar/baz
+# RUN: cp %S/Inputs/hello.in %t/foo/bar
+# RUN: echo 'command source %t/foo/bar/hello.in' >> %t/foo/zip.in
+# RUN: %lldb --script-language python -o 'command source %t/foo/zip.in' -o 'zip' -o 'hello' 2>&1 | FileCheck %s
+# CHECK: 95126
+# CHECK: Hello, World!