Ticket #5129 task closed fixed

Opened 3 years ago

Last modified 22 months ago

Replace usage of execfile with t.p.compat version

Reported by: thijs Owned by: itamar
Priority: low Milestone: Python-3.x
Component: core Keywords: py3k
Cc: thijs, facundobatista, gavin@…, vlada.peric@… Branch: branches/py3-execfile-5129
(diff, github, buildbot, log)
Author: exarkun, allenap Launchpad Bug:

Description (last modified by thijs) (diff)

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

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

Change History

1

  Changed 3 years ago by thijs

  • cc facundobatista added

2

  Changed 3 years ago by thijs

  • description modified (diff)

3

  Changed 3 years ago by Tourman

  • milestone set to Python-3.x

4

  Changed 2 years ago by allenap

  • branch set to 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 2 years ago by allenap

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

5

  Changed 2 years ago by allenap

  • cc gavin@… added
  • branch bzr+ssh://bazaar.launchpad.net/~allenap/twisted/execfile-to-compile-exec-5129 deleted

6

  Changed 2 years ago by thijs

  • keywords py3k, review added; py3k removed

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

Changed 2 years ago by allenap

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

7

  Changed 2 years ago by exarkun

  • branch set to branches/py3-execfile-5129
  • branch_author set to exarkun

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

8

  Changed 2 years ago by exarkun

  • keywords py3k added; py3k, review removed
  • owner set to allenap
  • branch_author changed from exarkun to allenap

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 2 years ago by allenap

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

9

  Changed 2 years ago by 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.

Changed 2 years ago by allenap

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

10

  Changed 2 years ago by allenap

  • keywords review added
  • owner allenap 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 :)

11

  Changed 2 years ago by exarkun

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

12

  Changed 2 years ago by thijs

  • summary changed from Replace usage of execfile with t.p.compat3k version to Replace usage of execfile with t.p.compat version

13

  Changed 2 years ago by allenap

It's a diff against trunk.

14

  Changed 2 years ago by thijs

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

15

  Changed 2 years ago by allenap

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

16

  Changed 2 years ago by exarkun

  • keywords review removed
  • owner set to 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.

17

  Changed 2 years ago by exarkun

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

refs #5129

18

  Changed 2 years ago by exarkun

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

refs #5129

19

  Changed 2 years ago by exarkun

  • owner changed from exarkun to allenap
  • branch_author changed from allenap to exarkun, allenap

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 2 years ago by allenap

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

20

  Changed 2 years ago by allenap

  • keywords review added
  • owner changed from allenap to exarkun

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 2 years ago by allenap

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

21

  Changed 2 years ago by 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).

22

  Changed 2 years ago by thijs

  • owner exarkun deleted

23

  Changed 2 years ago by exarkun

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

refs #5129

24

follow-up: ↓ 25   Changed 2 years ago by exarkun

  • keywords review removed
  • owner set to allenap

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 2 years ago by allenap

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

25

in reply to: ↑ 24   Changed 2 years ago by 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 :)

26

  Changed 22 months ago by vperic

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

refs #5129

27

  Changed 22 months ago by vperic

  • owner allenap deleted
  • keywords review added
  • cc vlada.peric@… added

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.

28

  Changed 22 months ago by itamar

  • owner set to itamar

29

  Changed 22 months ago by itamar

  • keywords review removed

Looks good, I will merge.

30

  Changed 22 months ago by itamarst

  • status changed from new to closed
  • resolution set to fixed

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