BTW, is your buildbot not just using "ninja check-lldb"? So far I haven't been able to reproduce any problems by using ninja check-lldb, but if I know what command line is being used maybe I can reproduce a failure and diagnose it.
On Mon, Nov 2, 2015 at 11:51 AM Zachary Turner <ztur...@google.com> wrote: > The other part of the patch should be in now (r251822). Can you make sure > the bot is passing by running the top-level script now? If not let me know. > > On Mon, Nov 2, 2015 at 11:50 AM Pavel Labath <lab...@google.com> wrote: > >> It doesn't matter to me which dotest to run, I just want to know what is >> the "right way". Due to enrico's patch, I couldn't run one dotest, so I >> switched to another one. Now, with your patch, the other way is also >> broken. Could you also go through with the other part of your change >> (reverting enrico's commit), as we are out of ways to run the test suite? >> (I have now switched back the builtbots to using test/dotest.py). >> >> pl >> >> On 2 November 2015 at 11:30, Zachary Turner <ztur...@google.com> wrote: >> >>> Sorry for the email spam. I still think the buildbot should run the >>> version in lldb/test though. `test` should be a top-level folder, it makes >>> it easy for people who clone the repo to instantly look and figure out how >>> our test suite works. So I think that's the flow that the buildbot should >>> test. Developer convenience should come second. >>> >>> On Mon, Nov 2, 2015 at 11:28 AM Zachary Turner <ztur...@google.com> >>> wrote: >>> >>>> If you want to have a way to run the test suite without typing a long >>>> relative path to go into the packages tree, then one thing that might work >>>> is to make a new file packages/Python/dotest.py. >>>> >>>> Then make it a copy of test/dotest.py. Not real crazy about copying >>>> code, but let me know what you think about that. >>>> >>>> On Mon, Nov 2, 2015 at 11:24 AM Zachary Turner <ztur...@google.com> >>>> wrote: >>>> >>>>> Don't change your buildbot to not use it. That's an error. >>>>> packages/Python/lldbsuite/test/dotest *must* be imported, as it depends >>>>> on >>>>> its __init__.py being run. If your buildbot doesn't use it anymore, then >>>>> I >>>>> think the patch I just submitted (r251819) will break your buildbot, >>>>> because I added this code: >>>>> >>>>> >>>>> if __name__ == "__main__": >>>>> print(__file__ + " is for use as a module only. It should not be >>>>> run as a standalone script.") >>>>> sys.exit(-1) >>>>> >>>>> to packages/Python/lldbsuite/test/dotest/dotest.py >>>>> >>>>> On Mon, Nov 2, 2015 at 11:22 AM Pavel Labath <lab...@google.com> >>>>> wrote: >>>>> >>>>>> 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
