[Lldb-commits] [PATCH] D89334: [lldb] Support Python imports relative the to the current file being sourced

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D89334#2344765 <https://reviews.llvm.org/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 (?)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D89334/new/

https://reviews.llvm.org/D89334