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

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added a comment.

In D89334#2344610 <https://reviews.llvm.org/D89334#2344610>, @labath wrote:

> In D89334#2341889 <https://reviews.llvm.org/D89334#2341889>, @JDevlieghere wrote:
>
>> In D89334#2341561 <https://reviews.llvm.org/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 <https://reviews.llvm.org/D89334#2344610>, @labath wrote:

> In D89334#2341889 <https://reviews.llvm.org/D89334#2341889>, @JDevlieghere wrote:
>
>> In D89334#2341561 <https://reviews.llvm.org/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).


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

https://reviews.llvm.org/D89334