[Lldb-commits] [lldb] r251862 - Revert "Remove the __import__ hack of lldbtest_config."

[Zachary Turner via lldb-commits]

Patch is in the pipeline right now, but just in case it gets borked along
the way, I'm attaching it to this email.  (I really need to leave since my
bus is heading out).



On Mon, Nov 2, 2015 at 5:43 PM Zachary Turner <zturner at google.com> wrote:

> 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 <zturner at 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 <labath at 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 <zturner at google.com> wrote:
>>> > +Enrico Granata
>>> >
>>> >
>>> > On Mon, Nov 2, 2015 at 4:42 PM Zachary Turner <zturner at 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 <zturner at 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 at 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 at lists.llvm.org
>>> >>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
>>>
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20151103/090a6279/attachment-0001.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Tighten-up-sys.path-and-use-absolute-imports-everywh.patch
Type: application/octet-stream
Size: 309498 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20151103/090a6279/attachment-0001.obj>