JDevlieghere added a comment. In D89334#2331903 <https://reviews.llvm.org/D89334#2331903>, @labath wrote:
> In D89334#2330287 <https://reviews.llvm.org/D89334#2330287>, @JDevlieghere > wrote: > >> In D89334#2329667 <https://reviews.llvm.org/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. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D89334/new/ https://reviews.llvm.org/D89334 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
