Running tests with Python 3.2.1

[GoogleCodeExporter]

The attached patch fixes iwyu_test_util.py to run with Python 3.2.1. It also 
addresses a couple of Windows-specific issues.

Whilst I do't expect this to be committed (I expect you might be targetting an 
older version of Python?) it might be useful for anyone having problems running 
the tests. If it's useful I can update the Wiki with a link to this issue.

The fixes are:

lines 42-43) Windows needs to normalise dir separators and look for '.exe'
line 71) Windows can't set close_fds=True and redirect stdout/stderr
line 72) With Python 3, the process output is treated as a bytes and needs to 
be decoded as utf-8 before it can be treated as a string
lines 205-206, 216) Fix a warning about leaking file handles (Python3? Related 
to close_fds change above?)
line 331) With Python 3, generator.next() becomes next(generator)
lines 365, 367) With Python 3, 'print x' becomes 'print(x)'
line 381) assert_ is deprecated and should be replaced with assertTrue

Original issue reported on code.google.com by paul.hol...@gmail.com on 16 Jul 2011 at 2:10

Attachments:

• iwyu_test_util.patch

[GoogleCodeExporter]

Nice!  I don't even have python3 installed, so haven't thought to test on it.

In general, I'm happy to make any changes to support python3, that continue to 
work on python2.  The only one I'm not sure about is the replacement of 
x.next() with next(x).

Do you have a python2 interpreter handy to test on?  If next(x) works there, 
feel free to apply all of these fixes.  If not, let's figure out a way to get 
that to work on both python2 and python3, that's not too ugly.

According to 
http://diveintopython3.org/porting-code-to-python-3-with-2to3.html#next, 
python3 still supports the next method(), so the change to next(generator) 
isn't actually necessary.  Is that no longer true? 

Original comment by csilv...@gmail.com on 18 Jul 2011 at 11:42

[GoogleCodeExporter]

I've just tested my changes with Python 2.7.2 and it seemed to run fine. I 
found that a little surprising! I guess 3.x removed support for a bunch of old 
features, but my changes don't rely on anything added for python3.

As for the next() change, when I back that out and run against 3.2.1, I get 
this error:

  AttributeError: 'generator' object has no attribute 'next'

Perhaps support for the old syntax changed between 3.0 and 3.2?

What version of python2 are you running against? I can try to test against this 
before committing.

Original comment by paul.hol...@gmail.com on 19 Jul 2011 at 11:51

[GoogleCodeExporter]

Python 2.7 was written to be compatible with python 3, to the extent possible, 
so that's not a total surprise.  We try to support all the way back to python 
2.4, if possible.  (I think right now we work with python 2.2.1, but I'm not 
sure.)

} Perhaps support for the old syntax changed between 3.0 and 3.2?

Yes, perhaps so.

If you can get code that works with python 2.4 and python 3.2, I'll be happy.

Original comment by csilv...@gmail.com on 19 Jul 2011 at 11:18

[GoogleCodeExporter]

I've added an updated patch which I've tested for both 2.4 and 2.7. The only 
remaining issue to fix for 3.2 is the use of next(). I'll keep looking for a 
fix for that.

One other change I had to make for 3.2 was replacing a couple of calls to 
xrange() with range(). It seems like xrange() can have a performance benefit 
with very long lists, but I don't believe that should be an issue for this code?

Original comment by paul.hol...@gmail.com on 20 Jul 2011 at 10:00

Attachments:

• iwyu_test_util.patch

[GoogleCodeExporter]

You're right, xrange vs range shouldn't be an issue here.

Patch looks good, feel free to commit.

Can you add a comment somewhere that this has been tested on python 2.4, python 
2.7, and python 3.2, and some weird-ish constructs (like parens around the 
print) are so code works in all those environments?

For the next fix, we can certainly do:
   try:
      generator.next()    # python 2.4
   except AttributeError:
      next(generator)     # python 3.2

Perhaps wrap this in our own Next() function.  It's just ugly, so I'm hoping we 
can avoid it. :-)  But it's not worth spending a huge amount of time trying to 
work around this. 

Original comment by csilv...@gmail.com on 20 Jul 2011 at 11:26

[GoogleCodeExporter]

Great - this is applied in r284. I added a fairly detailed comment at the head 
of the file describing the changes required to support python 3.2, and left a 
TODO near the call to next() - if anyone does have problems running this then 
they should have enough info now to resolve the problem locally.

I'll have a quick experiment with the try/except approach that you suggest. It 
is pretty ugly, but perhaps acceptable if that's the only kludge required to 
support 2.4, 2.7 and 3.2.

Original comment by paul.hol...@gmail.com on 21 Jul 2011 at 7:41

[GoogleCodeExporter]

Where are we on this one -- is it all resolved, or are there still issues left?

Original comment by csilv...@gmail.com on 25 Oct 2011 at 1:33

[GoogleCodeExporter]

I fixed the next() problem so it should cover Python 2.x as well as Python 3.x. 
The test script now seems to be fully Python 3-compatible.

Also, I added Windows build paths to the IWYU path probe.

I'd love to get this reviewed and committed so I don't have to update it every 
time I pull out IWYU.

Thanks!

Original comment by kim.gras...@gmail.com on 10 Jun 2012 at 7:52

Attachments:

• python3.patch

[GoogleCodeExporter]

Oops, didn't see the comments at the top of the file. New patch attached with 
cleaned up file header.

Original comment by kim.gras...@gmail.com on 10 Jun 2012 at 7:56

Attachments:

• python3.patch

[GoogleCodeExporter]

Thanks for patch, I've applied it in r354 with minor comment and without 
Windows build paths. I believe paths' changes should be in a separate commit 
and I have a few questions regarding them.

I've expected that added paths would be
'../../../../../build/Debug+Asserts/bin/include-what-you-use',
'../../../../../build/Release+Asserts/bin/include-what-you-use',
'../../../../../build/Release/bin/include-what-you-use'
because these paths correspond to LLVM+Clang+IWYU layout when you check out 
them following the instructions.

First of all, I think that paths
'../../../../../build/bin/MinSizeRel/include-what-you-use',
'../../../../../build/bin/RelWithDebInfo/include-what-you-use'
are specific to your environment and workflow. That's why I doubt these paths 
will be useful for everyone.

Why is it 'bin/Release' and not 'Release/bin', why 'Debug' and not 
'Debug+Asserts'? Is it your customizations or a default projects' layout on 
Windows? I am OK (though slightly reluctant) with including your custom paths, 
but I want everything to work out of the box, without customization, on all 
platforms. The drawback of paths specific to someone's environment is that new 
users can try to emulate this environment unnecessarily assuming they've messed 
up with build.

Original comment by vsap...@gmail.com on 12 Jun 2012 at 9:44

[GoogleCodeExporter]

OK, good point about the paths being in a separate commit.

These are the default CMake-generated paths on Windows, when I follow the 
instructions at http://clang.llvm.org/get_started.html. Note that they 
recommend generating IDE/build projects out-of-tree, so that's why I had an 
extra '..'.

As for the swapped bin <-> Release and why it's Debug vs Debug+Asserts, that 
surprised me too, but must be down to some inconsistency between the CMake 
workflow and the configure workflow. Maybe this should be fixed in Clang/LLVM.

Since the list of paths is like a search path, I figured it couldn't hurt to 
search more places, but I agree with your concern that we shouldn't be 
searching any non-standard locations. It's just that these seem to be the 
standard locations on Windows :-)

Original comment by kim.gras...@gmail.com on 13 Jun 2012 at 5:05

[GoogleCodeExporter]

Added paths in r355, thanks for making IWYU more Windows-friendly. Also I've 
added Linux/Mac OS X paths for out-of-tree build (basically extra '..').

Original comment by vsap...@gmail.com on 19 Jun 2012 at 7:40

[GoogleCodeExporter]

I just noticed that the _PortableNext thing I submitted in r354 has a problem. 
When run with Python 3, catching the AttributeError is not enough -- the thread 
is still in exception state, so if a test subsequently fails, the failing 
callstack will include _PortableNext, which is highly confusing.

I came up with an alternative implementation which checks if the iterator has a 
next method, and otherwise assumes Python 3 style. This makes test failures 
more correct.

Sorry for the bad fix last time.

Original comment by kim.gras...@gmail.com on 13 Jul 2012 at 3:37

Attachments:

• portable-next.patch

[GoogleCodeExporter]

Thanks for the patch, Kim. I will commit it shortly.

I wonder how the failing callstack with _PortableNext looks like, because I 
don't observe it on Mac OS X. Can you provide an example of a relevant 
callstack? It has nothing to do with the patch, I am just curious.

Original comment by vsap...@gmail.com on 19 Jul 2012 at 8:38

[GoogleCodeExporter]

Here's an example:

--
Traceback (most recent call last):
  File "...\iwyu_test_util.py", line 52, in _PortableNext
    iterator.next()  # Python 2.4-2.6
AttributeError: 'generator' object has no attribute 'next'

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "...\iwyu_test_util.py", line 52, in _PortableNext
    iterator.next()  # Python 2.4-2.6
AttributeError: 'generator' object has no attribute 'next'

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "run_iwyu_tests.py", line 112, in <lambda>
    {'runTest': lambda self, f=filename: self.RunOneTest(f)})
  File "run_iwyu_tests.py", line 88, in RunOneTest
    iwyu_flags, verbose=True)
  File "...\iwyu_test_util.py", line 413, in TestIwyuOnRelativeFile
    test_case.assertTrue(not failures, ''.join(failures))
AssertionError:
tests/badinc.cc:547: Unexpected diagnostic:
I2_TemplateClass::~I2_TemplateClass<FOO> is defined in "tests/badinc-i2-inl.h", 
which isn't directly #included.
--

It seems that whenever Python attempts to throw a new exception, it checks if 
there's an active exception on the current thread, and then bakes it into the 
one being thrown with the "During handling of the above exception, another 
exception occurred" message. Could very well be a bug/quality of implementation 
issue in my Python (3.1.3 for Windows).

Original comment by kim.gras...@gmail.com on 20 Jul 2012 at 7:26

[GoogleCodeExporter]

Thanks, Kim. That satisfied my curiosity.

I've committed the patch in r363 and believe this issue can be closed.

Original comment by vsap...@gmail.com on 21 Jul 2012 at 2:09

• Changed state: Verified