BTW, is lldb/test/dotest.py here to stay? I thought it was there just to avoid breaking anybody who runs dotest directly (instead of ninja check-lldb), and therefore we will remove it once everybody gets a chance to migrate (I have already changed our buildbots to not use it).
Is that correct or I am misunderstanding something? pl On 2 November 2015 at 11:11, Zachary Turner via lldb-commits < lldb-commits@lists.llvm.org> wrote: > I looked into this some more, and unfortunately I can't reproduce it. > That being said, I'm not convinced this patch fixes anything. The reason > is that when you import something, whether it be through the `__import__` > function or the `import` statement, the module itself is cached in > `sys.modules`. Whether the *name* for that module is introduced globally > (as is done in this patch) or locally (as is done when you use an `import` > statement) is irrelevant. Because the next time someone else imports it, > it will still find the same instance of the module in `sys.modules` and > just create a new name. It won't import it anew. > > If this patch does actually fix something, I think it must be a > coincidence. That said, I do have an idea as to what the problem might > be. Or at the very least, I know of a problem that would definitely lead > to strange behavior. > > `lldbsuite` is now a package, and it relies on the code in its > `__init__.py` being run. If you run > `packages/python/lldbsuite/test/dotest.py` manually, then `__init__.py` > doesn't get run, and `lldbExec` doesn't get initialized properly. > > Of course, this isn't what you're doing, but it *is* what `dosep` does > internally. `dosep` manually constructs a path to > `packages/python/lldbsuite/test/dotest.py` > and execs it. I have a patch that fixes this and makes `dosep` exec > `lldb/test/dotest.py` instead, which will then lead to the package being > imported, and `__init__.py` being run, and everything being initialized > properly. > > I'm going to commit that patch by itself, and then I will submit a > followup patch that reverts the change from this patch (since I can't > reproduce this problem, I can't check whether or not my patch fixes it). > So if my revert breaks you again, feel free to revert the revert. Although > if there's any way you can investigate a bit to understand what's going on > a little bit more, but I would be very grateful. In particular, I wonder > about the following things: > > 1) When lldbExec is not initialized properly, is this in the same process > instance that you ran from the command line, or is it in the > multiprocessing fork? > 2) If you add code to `lldbsuite/__init__.py` to print the process id and > the value of `lldb_root`, and then add code in `dotest.py` to print the > process id and the value of lldbExec, what does the output look like? > (Each line should be printed up to twice, due to the multiprocessing fork). > > Anyway, I'll get those 2 patches submitted to fix the dosep issue and > revert this change, and see what happens. > > On Fri, Oct 30, 2015 at 1:53 PM Jim Ingham <jing...@apple.com> wrote: > >> Note, the other important step was that you had to have an lldb installed >> in /usr/bin/lldb that FAILED this test. If you have a more recent lldb >> there, the test will succeed, and you won't notice you aren't testing your >> newly built sources. >> >> Jim >> >> > On Oct 30, 2015, at 1:25 PM, Enrico Granata via lldb-commits < >> lldb-commits@lists.llvm.org> wrote: >> > >> > I think what I was doing is be in lldb/test and do >> > >> > $ ./dotest.py ../packages/python/lldbsuite/functionalities/completion >> > >> >> On Oct 30, 2015, at 12:22 PM, Zachary Turner <ztur...@google.com> >> wrote: >> >> >> >> Can you give me a command line which will reproduce the original >> problem? Because I ran through the entire test suite and nothing failed, >> so I want to make sure we're doing the same thing. I'm still a little >> confused about how this happens, but I plan to look into it when I'm back >> on Monday and see if I can understand it better to identify a better fix. >> >> >> >> On Fri, Oct 30, 2015 at 11:58 AM Enrico Granata <egran...@apple.com> >> wrote: >> >>> On Oct 29, 2015, at 11:31 PM, Zachary Turner <ztur...@google.com> >> wrote: >> >>> >> >>> Wow. That's a weird problem. Thanks for finding it! >> >>> >> >>> Would it work if we move the definition of the `lldbtest_config` >> class into lldbsuite/test/__init__.py? This way the configuration should >> be part of the global package state of the lldbsuite.test package, which >> all the tests are already members of the same package, so they wouldn't >> even need to import anything (I think). >> >>> >> >> >> >> I think the problem is exactly that we want lldbtest_config to be >> *global* state and not package local state. >> >> Honestly, I think if we are not content with the fix as it stands, the >> right way would be to change the way unittest2 imports test cases as to use >> the package-level global scope instead of the global global state as it is >> now. >> >> >> >>> On Oct 30, 2015, at 8:32 AM, Zachary Turner <ztur...@google.com> >> wrote: >> >>> >> >>> I'm also still a little confused why this worked before my patch. >> How is unittest2 importing the individual tests in a way that behaves >> differently when dotest is a package (now) versus a standalone script >> (before)? >> >>> >> >> >> >> That is a good question. One to which “because Python” is the only >> answer I can think of. I suspect scripts live at the global scope anyway, >> so we were just getting lucky with those imports making it through >> correctly. >> >> >> >>> On Thu, Oct 29, 2015 at 6:12 PM Enrico Granata via lldb-commits < >> lldb-commits@lists.llvm.org> wrote: >> >>> Author: enrico >> >>> Date: Thu Oct 29 20:09:54 2015 >> >>> New Revision: 251678 >> >>> >> >>> URL: http://llvm.org/viewvc/llvm-project?rev=251678&view=rev >> >>> Log: >> >>> Some test cases that need the lldbExec path were failing because >> lldbExec was turning out to be None even though it was being validly set by >> dotest.py >> >>> >> >>> It turns out that lldbtest_config was being imported locally to >> "lldbsuite.test" instead of globally, so when the test cases got >> individually brought by a global import via __import__ by unittest2, they >> did not see the lldbtest_config import, and ended up importing a new >> separate copy of it, with lldbExec unset >> >>> >> >>> This is a simple hackaround that brings lldbtest_config to global >> visibility and makes sure the configuration data is correctly shared >> >>> >> >>> >> >>> Modified: >> >>> lldb/trunk/packages/Python/lldbsuite/test/dotest.py >> >>> >> >>> Modified: lldb/trunk/packages/Python/lldbsuite/test/dotest.py >> >>> URL: >> http://llvm.org/viewvc/llvm-project/lldb/trunk/packages/Python/lldbsuite/test/dotest.py?rev=251678&r1=251677&r2=251678&view=diff >> >>> >> ============================================================================== >> >>> --- lldb/trunk/packages/Python/lldbsuite/test/dotest.py (original) >> >>> +++ lldb/trunk/packages/Python/lldbsuite/test/dotest.py Thu Oct 29 >> 20:09:54 2015 >> >>> @@ -21,6 +21,10 @@ for available options. >> >>> """ >> >>> >> >>> from __future__ import print_function >> >>> +# this module needs to have global visibility, otherwise test cases >> >>> +# will import it anew in their local namespace, essentially losing >> access >> >>> +# to all the configuration data >> >>> +globals()['lldbtest_config'] = __import__('lldbtest_config') >> >>> >> >>> import use_lldb_suite >> >>> >> >>> @@ -42,7 +46,6 @@ import test_results >> >>> from test_results import EventBuilder >> >>> import inspect >> >>> import unittest2 >> >>> -import lldbtest_config >> >>> import test_categories >> >>> >> >>> import six >> >>> >> >>> >> >>> _______________________________________________ >> >>> lldb-commits mailing list >> >>> lldb-commits@lists.llvm.org >> >>> http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits >> >> >> >> >> >> Thanks, >> >> - Enrico >> >> 📩 egranata@.com ☎️ 27683 >> >> >> > >> > >> > Thanks, >> > - Enrico >> > 📩 egranata@.com ☎️ 27683 >> > >> > _______________________________________________ >> > lldb-commits mailing list >> > lldb-commits@lists.llvm.org >> > http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits >> >> > _______________________________________________ > lldb-commits mailing list > lldb-commits@lists.llvm.org > http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits > >
_______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
