[Lldb-commits] [PATCH] D83840: [lldb][test] Prevent infinite loop while looking for use_lldb_suite_root.py.

Fri Jul 17 06:35:28 PDT 2020

labath added a comment.

In D83840#2156040 <https://reviews.llvm.org/D83840#2156040>, @JDevlieghere wrote:

> In D83840#2155308 <https://reviews.llvm.org/D83840#2155308>, @labath wrote:
>
> > In D83840#2154263 <https://reviews.llvm.org/D83840#2154263>, @shafik wrote:
> >
> > > @labath why do we need two copies of `use_lldb_suite.py`?
> >
> >
> > This script is responsible for setting up an appropriate python import path. Before we can import any code in lldb/third_party/Python/module or lldb/packages/Python, we need to add those paths to `sys.path`. And we cannot completely put this code into a central place because then we wouldn't know how to import that.
> >
> > So, the way get around that is by placing this file into the same folder as the script that needs it. Then, the script can load this file using a relative import, and afterwards, it can import anything it wants. And since we have scripts needing this functionality in multiple places, we have multiple copies of the script.
> >
> > At least that's the current state of the art. It's possible that there are better solutions, but we just don't know about them.
>
>
> We could have CMake configure this at the cost of always having to use the corresponding scripts from the build directory? It seems like only `analyze-project-deps.py` and `host_art_bt.py` are importing lldb right now so it might be worth considering.

The important part is who is importing `use_lldb_suite`, not `lldb`. By the looks of things host_art_bt.py is meant to be included from within lldb, so the path will be correct there.

Not being able to run analyze-project-deps from the source tree would be a minor inconvenience, so I'm not sure if it's worth it. But I am not opposed to the idea either. All that script really needs is the location of the repo root. Since it already has a lot of knowledge about the repo layout, we could just hardcode that too...

I don't know if it's the only script that needs that...

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83840