Opened 6 years ago

Closed 5 years ago

#5129 task closed fixed (fixed)

Replace usage of execfile with t.p.compat version

Reported by: Thijs Triemstra Owned by: Itamar Turner-Trauring
Priority: low Milestone: Python-3.x
Component: core Keywords: py3k
Cc: Thijs Triemstra, Facundo Batista, Gavin Panella, Vladimir Perić Branch: branches/py3-execfile-5129
branch-diff, diff-cov, branch-cov, buildbot
Author: exarkun, allenap

Description (last modified by Thijs Triemstra)

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.

Attachments (7)

compat3k.execfile.diff (5.7 KB) - added by Gavin Panella 6 years ago.
Patch to create t.p.compat3k.execfile and use it everywhere.
compat3k.execfile.2.diff (7.1 KB) - added by Gavin Panella 6 years ago.
Patch to create t.p.compat3k.execfile and use it everywhere (with topfile and lint fixes).
compat3k.execfile.3.diff (6.5 KB) - added by Gavin Panella 6 years ago.
Patch to create t.p.compat3k.execfile and use it everywhere (with review fixes).
compat3k.execfile.4.diff (11.1 KB) - added by Gavin Panella 6 years ago.
Patch to create t.p.compat.execfile and use it everywhere (with all review fixes).
compat3k.execfile.5.diff (11.7 KB) - added by Gavin Panella 6 years ago.
Patch to create t.p.compat.execfile and use it everywhere (with universal newlines).
py3-execfile-5129.apply-to-r33601.diff (3.0 KB) - added by Gavin Panella 6 years ago.
Like compat3k.execfile.5.diff, but can be applied to branches/py3-execfile-5129.
py3-execfile-5129.apply-to-r34464.diff (1.6 KB) - added by Gavin Panella 5 years ago.
Tests for Java-handling code path through getExtensions, and for the "extensions" in ns branch.

Download all attachments as: .zip

Change History (37)

comment:1 Changed 6 years ago by Thijs Triemstra

Cc: Facundo Batista added

comment:2 Changed 6 years ago by Thijs Triemstra

Description: modified (diff)

comment:3 Changed 6 years ago by Tourman

Milestone: Python-3.x

comment:4 Changed 6 years ago by Gavin Panella

Branch: bzr+ssh://bazaar.launchpad.net/~allenap/twisted/execfile-to-compile-exec-5129

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.

Changed 6 years ago by Gavin Panella

Attachment: compat3k.execfile.diff added

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

comment:5 Changed 6 years ago by Gavin Panella

Branch: bzr+ssh://bazaar.launchpad.net/~allenap/twisted/execfile-to-compile-exec-5129
Cc: Gavin Panella added

comment:6 Changed 6 years ago by Thijs Triemstra

Keywords: review added

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

Changed 6 years ago by Gavin Panella

Attachment: compat3k.execfile.2.diff added

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

comment:7 Changed 6 years ago by Jean-Paul Calderone

Author: exarkun
Branch: branches/py3-execfile-5129

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

comment:8 Changed 6 years ago by Jean-Paul Calderone

Author: exarkunallenap
Keywords: review removed
Owner: set to Gavin Panella

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!

Changed 6 years ago by Gavin Panella

Attachment: compat3k.execfile.3.diff added

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

comment:9 Changed 6 years ago by Gavin Panella

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.

Changed 6 years ago by Gavin Panella

Attachment: compat3k.execfile.4.diff added

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

comment:10 Changed 6 years ago by Gavin Panella

Keywords: review added
Owner: Gavin Panella deleted

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 :)

comment:11 Changed 6 years ago by Jean-Paul Calderone

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

comment:12 Changed 6 years ago by Thijs Triemstra

Summary: Replace usage of execfile with t.p.compat3k versionReplace usage of execfile with t.p.compat version

comment:13 Changed 6 years ago by Gavin Panella

It's a diff against trunk.

comment:14 Changed 6 years ago by Thijs Triemstra

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

comment:15 Changed 6 years ago by Gavin Panella

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

comment:16 Changed 6 years ago by Jean-Paul Calderone

Keywords: review removed
Owner: set to Jean-Paul Calderone
  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.

comment:17 Changed 6 years ago by Jean-Paul Calderone

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

refs #5129

comment:18 Changed 6 years ago by Jean-Paul Calderone

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

refs #5129

comment:19 Changed 6 years ago by Jean-Paul Calderone

Author: allenapexarkun, allenap
Owner: changed from Jean-Paul Calderone to Gavin Panella

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.

Changed 6 years ago by Gavin Panella

Attachment: compat3k.execfile.5.diff added

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

comment:20 Changed 6 years ago by Gavin Panella

Keywords: review added
Owner: changed from Gavin Panella to Jean-Paul Calderone

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).

Changed 6 years ago by Gavin Panella

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

comment:21 Changed 6 years ago by Gavin Panella

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).

comment:22 Changed 6 years ago by Thijs Triemstra

Owner: Jean-Paul Calderone deleted

comment:23 Changed 5 years ago by Jean-Paul Calderone

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

refs #5129

comment:24 Changed 5 years ago by Jean-Paul Calderone

Keywords: review removed
Owner: set to Gavin Panella

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!

Changed 5 years ago by Gavin Panella

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

comment:25 in reply to:  24 Changed 5 years ago by Gavin Panella

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.

  1. 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.

  1. 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 :)

comment:26 Changed 5 years ago by Vladimir Perić

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

refs #5129

comment:27 Changed 5 years ago by Vladimir Perić

Cc: Vladimir Perić added
Keywords: review added
Owner: Gavin Panella deleted

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.

comment:28 Changed 5 years ago by Itamar Turner-Trauring

Owner: set to Itamar Turner-Trauring

comment:29 Changed 5 years ago by Itamar Turner-Trauring

Keywords: review removed

Looks good, I will merge.

comment:30 Changed 5 years ago by itamarst

Resolution: fixed
Status: newclosed

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

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

Note: See TracTickets for help on using tickets.