<div dir="ltr">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).<div><br></div><div>pl</div></div><div class="gmail_extra"><br><div class="gmail_quote">On 2 November 2015 at 11:30, Zachary Turner <span dir="ltr"><<a href="mailto:zturner@google.com" target="_blank">zturner@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">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.</div><div class="HOEnZb"><div class="h5"><br><div class="gmail_quote"><div dir="ltr">On Mon, Nov 2, 2015 at 11:28 AM Zachary Turner <<a href="mailto:zturner@google.com" target="_blank">zturner@google.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">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.<div><br></div><div>Then make it a copy of test/dotest.py.  Not real crazy about copying code, but let me know what you think about that.</div></div><br><div class="gmail_quote"><div dir="ltr">On Mon, Nov 2, 2015 at 11:24 AM Zachary Turner <<a href="mailto:zturner@google.com" target="_blank">zturner@google.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">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:<div><br></div><div><div><br></div><div>if __name__ == "__main__":</div><div>    print(__file__ + " is for use as a module only.  It should not be run as a standalone script.")</div><div>    sys.exit(-1)</div></div><div><br></div><div>to <span style="line-height:1.5">packages/Python/lldbsuite/test/dotest/dotest.py</span></div></div><br><div class="gmail_quote"><div dir="ltr">On Mon, Nov 2, 2015 at 11:22 AM Pavel Labath <<a href="mailto:labath@google.com" target="_blank">labath@google.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">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).<div><br><div>Is that correct or I am misunderstanding something?</div></div></div><div dir="ltr"><div><br></div><div>pl</div><div><br></div></div><div class="gmail_extra"><br><div class="gmail_quote">On 2 November 2015 at 11:11, Zachary Turner via lldb-commits <span dir="ltr"><<a href="mailto:lldb-commits@lists.llvm.org" target="_blank">lldb-commits@lists.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">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.<div><br></div><div>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.</div><div><br></div><div>`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.</div><div><br></div><div>Of course, this isn't what you're doing, but it *is* what `dosep` does internally.  `dosep` manually constructs a path to `<span style="line-height:1.5">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.</span></div><div><span style="line-height:1.5"><br></span></div><div><span style="line-height:1.5">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:</span></div><div><span style="line-height:1.5"><br></span></div><div><span style="line-height:1.5">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?</span></div><div><span style="line-height:1.5">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).</span></div><div><span style="line-height:1.5"><br></span></div><div><span style="line-height:1.5">Anyway, I'll get those 2 patches submitted to fix the dosep issue and revert this change, and see what happens.</span></div></div><div><div><br><div class="gmail_quote"><div dir="ltr">On Fri, Oct 30, 2015 at 1:53 PM Jim Ingham <<a href="mailto:jingham@apple.com" target="_blank">jingham@apple.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">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.<br>
<br>
Jim<br>
<br>
> On Oct 30, 2015, at 1:25 PM, Enrico Granata via lldb-commits <<a href="mailto:lldb-commits@lists.llvm.org" target="_blank">lldb-commits@lists.llvm.org</a>> wrote:<br>
><br>
> I think what I was doing is be in lldb/test and do<br>
><br>
> $ ./dotest.py ../packages/python/lldbsuite/functionalities/completion<br>
><br>
>> On Oct 30, 2015, at 12:22 PM, Zachary Turner <<a href="mailto:zturner@google.com" target="_blank">zturner@google.com</a>> wrote:<br>
>><br>
>> 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.<br>
>><br>
>> On Fri, Oct 30, 2015 at 11:58 AM Enrico Granata <<a href="mailto:egranata@apple.com" target="_blank">egranata@apple.com</a>> wrote:<br>
>>> On Oct 29, 2015, at 11:31 PM, Zachary Turner <<a href="mailto:zturner@google.com" target="_blank">zturner@google.com</a>> wrote:<br>
>>><br>
>>> Wow.  That's a weird problem.  Thanks for finding it!<br>
>>><br>
>>> 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).<br>
>>><br>
>><br>
>> I think the problem is exactly that we want lldbtest_config to be *global* state and not package local state.<br>
>> 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.<br>
>><br>
>>> On Oct 30, 2015, at 8:32 AM, Zachary Turner <<a href="mailto:zturner@google.com" target="_blank">zturner@google.com</a>> wrote:<br>
>>><br>
>>> 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)?<br>
>>><br>
>><br>
>> 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.<br>
>><br>
>>> On Thu, Oct 29, 2015 at 6:12 PM Enrico Granata via lldb-commits <<a href="mailto:lldb-commits@lists.llvm.org" target="_blank">lldb-commits@lists.llvm.org</a>> wrote:<br>
>>> Author: enrico<br>
>>> Date: Thu Oct 29 20:09:54 2015<br>
>>> New Revision: 251678<br>
>>><br>
>>> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=251678&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=251678&view=rev</a><br>
>>> Log:<br>
>>> 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<br>
>>><br>
>>> 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<br>
>>><br>
>>> This is a simple hackaround that brings lldbtest_config to global visibility and makes sure the configuration data is correctly shared<br>
>>><br>
>>><br>
>>> Modified:<br>
>>>     lldb/trunk/packages/Python/lldbsuite/test/dotest.py<br>
>>><br>
>>> Modified: lldb/trunk/packages/Python/lldbsuite/test/dotest.py<br>
>>> URL: <a href="http://llvm.org/viewvc/llvm-project/lldb/trunk/packages/Python/lldbsuite/test/dotest.py?rev=251678&r1=251677&r2=251678&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/lldb/trunk/packages/Python/lldbsuite/test/dotest.py?rev=251678&r1=251677&r2=251678&view=diff</a><br>
>>> ==============================================================================<br>
>>> --- lldb/trunk/packages/Python/lldbsuite/test/dotest.py (original)<br>
>>> +++ lldb/trunk/packages/Python/lldbsuite/test/dotest.py Thu Oct 29 20:09:54 2015<br>
>>> @@ -21,6 +21,10 @@ for available options.<br>
>>>  """<br>
>>><br>
>>>  from __future__ import print_function<br>
>>> +# this module needs to have global visibility, otherwise test cases<br>
>>> +# will import it anew in their local namespace, essentially losing access<br>
>>> +# to all the configuration data<br>
>>> +globals()['lldbtest_config'] = __import__('lldbtest_config')<br>
>>><br>
>>>  import use_lldb_suite<br>
>>><br>
>>> @@ -42,7 +46,6 @@ import test_results<br>
>>>  from test_results import EventBuilder<br>
>>>  import inspect<br>
>>>  import unittest2<br>
>>> -import lldbtest_config<br>
>>>  import test_categories<br>
>>><br>
>>>  import six<br>
>>><br>
>>><br>
>>> _______________________________________________<br>
>>> lldb-commits mailing list<br>
>>> <a href="mailto:lldb-commits@lists.llvm.org" target="_blank">lldb-commits@lists.llvm.org</a><br>
>>> <a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits</a><br>
>><br>
>><br>
>> Thanks,<br>
>> - Enrico<br>
>> 📩 egranata@.com ☎️ 27683<br>
>><br>
><br>
><br>
> Thanks,<br>
> - Enrico<br>
> 📩 egranata@.com ☎️ 27683<br>
><br>
> _______________________________________________<br>
> lldb-commits mailing list<br>
> <a href="mailto:lldb-commits@lists.llvm.org" target="_blank">lldb-commits@lists.llvm.org</a><br>
> <a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits</a><br>
<br>
</blockquote></div>
</div></div><br>_______________________________________________<br>
lldb-commits mailing list<br>
<a href="mailto:lldb-commits@lists.llvm.org" target="_blank">lldb-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits</a><br>
<br></blockquote></div><br></div>
</blockquote></div></blockquote></div></blockquote></div>
</div></div></blockquote></div><br></div>