As an alternative, since this is breaking your buildbot, I can check in the lion's share of the work (which should fix about 95% of the fixups), and you can do any remaining ones that pop up on the linux buildbot after I'm gone. Do you prefer that?
On Mon, Nov 2, 2015 at 5:40 PM Zachary Turner <ztur...@google.com> wrote: > Yes, but it's a VERY large mechanical code change. For example, > everywhere we were doign something like this: > > import lldbutil > > from one of the tests, we have to rewrite this as: > > import lldbsuite.test.lldbutil as lldbutil > > And we have to catch all the different ways of importing, like: > > from lldbutil import x > import lldb, lldbutil > > and there are about 20 or 30 modules other than lldbutil I have to fix up > as well. > > I'm in the process of doing this right now, and I've almost got all the > instances found and fixed. But I have to leave in about 15 minutes. If I > don't get it in tonight, it will be in tomorrow morning first thing. > > On Mon, Nov 2, 2015 at 5:37 PM Pavel Labath <lab...@google.com> wrote: > >> Good news Zachary, I'm glad we finally got to the bottom of this. >> >> I've tested your proposed changes locally, and on our buildbot (which, >> for whatever reason, did not like Enrico's workaround). Could we get >> this in as soon as possible? >> >> cheers, >> pl >> >> >> On 2 November 2015 at 16:43, Zachary Turner <ztur...@google.com> wrote: >> > +Enrico Granata >> > >> > >> > On Mon, Nov 2, 2015 at 4:42 PM Zachary Turner <ztur...@google.com> >> wrote: >> >> >> >> Ok, I think I figured out the root of all the problems. >> >> >> >> First, in setupSysPath we have this: >> >> >> >> sys.path.insert(0, scriptPath) >> >> >> >> This basically adds `lldb/packages/Python/lldbsuite/test` to sys.path. >> >> >> >> Then, in the individual tests we write this: >> >> >> >> from lldbtest import * >> >> >> >> It finds lldbtest at the above path, and imports it. But this is *not* >> >> the same copy of lldbtest that has already been imported, because that >> copy >> >> was imported as a member of the lldbsuite.test package. So now we >> have this >> >> module in sys.modules twice. Once as lldbtest, found by looking in >> >> lldbsuite/test since it was in sys.path. And once as >> >> lldbsuite.test.lldbtest, found because lldbsuite was in sys.path, using >> >> subpackage resolution. >> >> >> >> I think the fix for all of these problems is to *remove* scriptPath >> from >> >> sys.path. We shouldn't be mucking with sys.path for stuff in our own >> test >> >> suite. We should resolve the references explicitly with subpackage >> >> references. For example, even if I don't write __package__ = >> >> "lldbsuite.test" in TestMultithreaded.py, I can still fix the problem >> with >> >> the below patch: >> >> >> >> - from lldbtest import * >> >> + from lldbsuite.test.lldbtest import * >> >> >> >> because now we're referencing the module that has already been >> imported. >> >> This is the "correct" way to do it, so if we tighten up sys.path, >> nobody >> >> should be able to make this mistake again. >> >> >> >> On Mon, Nov 2, 2015 at 4:32 PM Zachary Turner <ztur...@google.com> >> wrote: >> >>> >> >>> I think what is happening is that when we go through unittest2, the >> >>> package link is being broken. >> >>> >> >>> Inside dotest.py : __package__ = lldbsuite.test >> >>> Inside unittest2.loadTestsFromName : __package__ = unittest2 >> >>> Inside TestMultithreaded.py : __package__ = None >> >>> >> >>> Restoring the link by writing >> >>> >> >>> __package__ = "lldbsuite.test" >> >>> >> >>> at the top of TestMultithreaded.py seems to fix the problem, although >> >>> that really bothersome to have to do that in every single file. I'm >> still >> >>> trying to figure out the proper fix. >> >>> >> >>> On Mon, Nov 2, 2015 at 3:41 PM Pavel Labath via lldb-commits >> >>> <lldb-commits@lists.llvm.org> wrote: >> >>>> >> >>>> Author: labath >> >>>> Date: Mon Nov 2 17:39:09 2015 >> >>>> New Revision: 251862 >> >>>> >> >>>> URL: http://llvm.org/viewvc/llvm-project?rev=251862&view=rev >> >>>> Log: >> >>>> Revert "Remove the __import__ hack of lldbtest_config." >> >>>> >> >>>> The hack still seems to be necessary. Putting it back in until we >> figure >> >>>> out why. >> >>>> >> >>>> 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=251862&r1=251861&r2=251862&view=diff >> >>>> >> >>>> >> ============================================================================== >> >>>> --- lldb/trunk/packages/Python/lldbsuite/test/dotest.py (original) >> >>>> +++ lldb/trunk/packages/Python/lldbsuite/test/dotest.py Mon Nov 2 >> >>>> 17:39:09 2015 >> >>>> @@ -19,11 +19,14 @@ 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 >> >>>> -import lldbsuite >> >>>> >> >>>> -import lldbtest_config >> >>>> +import lldbsuite >> >>>> >> >>>> import atexit >> >>>> import commands >> >>>> >> >>>> >> >>>> _______________________________________________ >> >>>> 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
