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
