Replace usage of execfile with t.p.compat version

[thijs]

From  porting-guide: Starting with Python 3, the execfile statement is no longer available. An alternative is to use the compile() function in conjunction with exec(). compile() can create a code object from a file, and then it can be passed into exec().

exec(compile(source_code, source_file_name, "exec"))

Some of this work has been done by loewis in #4244, but that ticket focuses on getting setup.py to run in py3k, which involves more than execfile compatibility.

Currently these modules use execfile:

./setup.py:                execfile(setup_py, ns, ns)
./twisted/python/test/test_release.py:        execfile("test_project", ns)
./twisted/python/test/test_release.py:        execfile("test_project", ns)
./twisted/python/_release.py:        execfile(self.directory.child("_version.py").path, namespace)
./twisted/python/dist.py:    execfile(vfile, ns)
./twisted/web/script.py:        execfile(path, glob, glob)
./twisted/web/script.py:            execfile(self.filename, namespace, namespace)
./twisted/names/authority.py:        execfile(filename, g, l)
./twisted/lore/htmlbook.py:            execfile(filename)

where lore/htmlbook.py can be ignored because its being phased out, so small enough for this single ticket.

[allenap]

I've had a go at doing this. If this looks reasonable I'm happy to figure out what I need to do to get into into a state that's good to land.

[allenap]

Patch to create t.p.compat3k.execfile and use it everywhere.

[thijs]

Thanks. Putting it up for review as described on [ReviewProcess#Authors:Howtogetyourchangereviewed].

[allenap]

Patch to create t.p.compat3k.execfile and use it everywhere (with topfile and lint fixes).

[exarkun]

(In [33378]) Branching to 'py3-execfile-5129'

[exarkun]

Thanks.

1. There's a remaining use of __builtin__.execfile in twisted/lore/htmlbook.py
2. The redefinition of unused 'StringIO' from line 12 warnings from pyflakes are really pyflakes bugs. The point of pyflakes is to improve your code. I'd rather not see hacks added all over Twisted to suppress meaningless warnings.
3. The change to twisted/python/_release.py to remove the nested subprojects import breaks DocBuilder.
4. The change to inet_ntop in twisted/python/compat.py seems unrelated. Does it fix some bug? Is there a unit test?
5. twisted.python.compat3k shouldn't be a different module from twisted.python.compat.
6. The tests for the new execfile implementation need docstrings and names that follow the naming convention - eg test_execfileGlobals.
7. Ideally, getExtensions in setup.py would have unit tests as well. It would be really awesome if you moved this function into twisted.python.dist and give it a unit test or two.

I did create an svn branch for this ticket, but I didn't end up putting any code in it. Hopefully you can keep using your bzr branch and supply another patch without any trouble.

Thanks again!

[allenap]

Patch to create t.p.compat3k.execfile and use it everywhere (with review fixes).

[allenap]

Thanks for the review. Sorry I've taken a while to respond; I blame Christmas for the disruption :)

I've fixed points #1 to #6 inclusive.

In point #4 you asked:

The change to inet_ntop in twisted/python/compat.py seems unrelated. Does it fix some bug? Is there a unit test?

It was a pure lint fix. I've reverted it as irrelevant to this branch.

I have not yet addressed #7, and I know I ought to per the acceptance criteria. I'll try and get some time for that next week.

[allenap]

Patch to create t.p.compat.execfile and use it everywhere (with all review fixes).

[allenap]

The latest diff (the 4th) has a fix for review point #7 about getExtensions(). Please can you review it again and let me know if anything else needs doing. My fingers are crossed that it's now landable :)

[exarkun]

is compat3k.execfile.4.diff a patch against trunk or against the branch? It applies cleanly to both.

[allenap]

It's a diff against trunk.

[thijs]

(In [33512]) apply compat3k.execfile.4.diff, refs #5129

[allenap]

Is there anything else I can do to help get this landed in trunk?

[exarkun]

1. in twisted/test/test_compat.py
   1. all docstrings, even for classes, should be formatted like """\nfoo\n"""
   2. There's no need to call TestCase.setUp from a subclass's setUp method.
   3. We have a nice helper for setting the contents of a file, twisted.python.filepath.FilePath.setContent.
   4. There's no need to delete the temporary file in tearDown. It's nice to leave testing artifacts around, in case a test fails or someone wants to inspect them for some other reason.
   5. ns_global and ns_local aren't named according to the naming convention.
   6. Methods should be separated from each other by two blank lines.
2. in twisted/python/test/test_dist.py
   1. Thanks for writing these :)
   2. setup_template is not named according to the naming convention
   3. Same comment about setUp
   4. Same comment about FilePath
   5. Same comment about two blank lines separating methods
3. in twisted/python/compat.py
   1. The implementation of execfile here does not actually avoid triggering the Python 3 warning! It looks like it would probably work on Python 3, but it's not clear there's any way to confirm that at the moment. Regardless, the implementation would be simpler and would avoid the Python 3 warning if it didn't try to use the builtin execfile when present.

[exarkun]

(In [33600]) Adjustments to meet coding standard; remove the fallback-to-builtin branch of the new execfile function: always use exec().

refs #5129

[exarkun]

(In [33601]) Construct the basedir for the getExtensions tests properly: it needs to be a package-y thing named "twisted".

refs #5129

[exarkun]

I fixed the points I mentioned in my review. Buildbot points out one more problem, which is that the new implementation of execfile (now that it's being used) is incompatible with the builtin execfile in at least one case:

 http://buildbot.twistedmatrix.com/boxes-supported?branch=/branches/py3-execfile-5129

I haven't investigated the specifics of the incompatibility.

[allenap]

Patch to create t.p.compat.execfile and use it everywhere (with universal newlines).

[allenap]

compat3k.execfile.5.diff contains a fix for universal newline support; unrecognised line endings were confusing compile. It might have been easier to stick with the built-in execfile for Python 2.x ;)

exarkun, I didn't make the latest changes against your branch, for which I'm sorry. I just lost track of the conversation and didn't realise that I hadn't acted on your latest review. However, the interdiff between compat3k.execfile.4.diff and compat3k.execfile.5.diff may be of use though; there is a test in there for various newlines (though it's fairly trivial).

[allenap]

Like compat3k.execfile.5.diff, but can be applied to branches/py3-execfile-5129.

[allenap]

The latest patch - py3-execfile-5129.apply-to-r33601.diff - should apply to rev 33601 of branches/py3-execfile-5129, and provide the same stuff as compat3k.execfile.5.diff (though with updated coding style).

[exarkun]

(In [34464]) Apply py3-execfile-5129.apply-to-r33601.diff

refs #5129

[exarkun]

Thanks!

1. It looks like ExecfileCompatTestCase.writeScript should be passed unicode, but it's passed str more often than not.
2. There's no test coverage for the Java-handling code path through getExtensions. I think the code is fine as is, for now, but we should have a new ticket for deleting this special case or finding some way to exercise it to verify it is correct. Will you file that ticket?
3. There's no test coverage for the "extensions" in ns branch in getExtensions, either. This probably should be covered now (it looks correct, fwiw).

I forced a new build with the latest code, < http://buildbot.twistedmatrix.com/boxes-supported?branch=/branches/py3-execfile-5129>. If that turns out well, I think this will be really close to ready. Thanks again!

[allenap]

Tests for Java-handling code path through getExtensions, and for the "extensions" in ns branch.

[allenap]

Replying to exarkun:

Thanks! 1. It looks like ExecfileCompatTestCase.writeScript should be passed unicode, but it's passed str more often than not.

I'm not sure exactly what encoding is used when coercing a unicode to a byte string - is it ASCII, UTF-8, sys.getdefaultencoding(), or something else? - so I was explicit. AFAIK, .encode("ascii") is a no-op for byte strings that are already ASCII, so this code should work correctly in Python 2 or 3.

2. There's no test coverage for the Java-handling code path through getExtensions. I think the code is fine as is, for now, but we should have a new ticket for deleting this special case or finding some way to exercise it to verify it is correct. Will you file that ticket?

The attached patch - py3-execfile-5129.apply-to-r34464.diff - contains a test for this, but I'll file a ticket to remove the special case.

3. There's no test coverage for the "extensions" in ns branch in getExtensions, either. This probably should be covered now (it looks correct, fwiw).

That patch also contains a test for this.

I forced a new build with the latest code, <...>. If that turns out well, I think this will be really close to ready. Thanks again!

My fingers are crossed :)

[vperic]

(In [34603]) Apply py3-execfile-5129.apply-to-r34464.diff

refs #5129

[vperic]

Hi allenap, you forgot to actually mark this for review, so I'm doing that now. I've also applied your patch to the branch and forced another build.

[itamar]

Looks good, I will merge.

[itamarst]

(In [34775]) Merge py3-execfile-5129: Switch to custom execfile() implementation, since it missing from Python 3.

Author: allenap Reviewer: exarkun, itamar Fixes: #5129