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

[Zachary Turner via lldb-commits]

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/6ca471b5/attachment.html>