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

> In D89334#2339018 <https://reviews.llvm.org/D89334#2339018>, @JDevlieghere wrote:
>
>> In D89334#2334881 <https://reviews.llvm.org/D89334#2334881>, @labath wrote:
>>
>>> In D89334#2332452 <https://reviews.llvm.org/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 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 both will generally 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.


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

https://reviews.llvm.org/D89334