Opened 8 years ago

Closed 13 months ago

#6908 defect closed fixed (fixed)

compileall fails due to bad demo code

Reported by: jlynch Owned by: jlynch
Priority: normal Milestone:
Component: core Keywords: compileall
Cc: Branch:
Author:

Description

If I attempt to run compileall in the base of the twisted package directory:

python -m compileall .

I get three SyntaxErrors in demo code within the docs directory. In particular:

SyntaxError: ('invalid syntax', ('./docs/projects/historic/2003/pycon/deferex/deferex-bad-adding.py', 6, 1, '...\n'))
SyntaxError: ('invalid syntax', ('./docs/projects/historic/2003/pycon/deferex/deferex-listing0.py', 8, 9, '        ...\n'))
SyntaxError: ('invalid syntax', ('./docs/projects/historic/2003/pycon/deferex/deferex-listing2.py', 6, 1, '...\n'))

It makes sense that these are not valid python code as they are just demos, but since the files are still .py files, it makes sense that they should compile.

I have attached a patch as per my understanding of the wiki that makes these files valid python . I did not include a news file because I am not sure which topfiles directory is appropriate as this is a change to the docs subdirectory.

Attachments (1)

fix-compileall-6908.patch (1.5 KB) - added by jlynch 8 years ago.
Patch that makes demo code valid python

Download all attachments as: .zip

Change History (10)

Changed 8 years ago by jlynch

Attachment: fix-compileall-6908.patch added

Patch that makes demo code valid python

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

Keywords: review removed
Owner: set to jlynch

Why does it matter what compileall does to our documentation?

comment:2 Changed 8 years ago by jlynch

It matters because production python shops might want to run compileall before they actually ship their pycs to their production environments.

Some reasons one might want to do this:

  1. Additional line of defense against invalid code making it to production (in addition to pyflakes, flake8, etc ...).
  1. Performance concerns when the code paths first get hit.
  1. Security. Usually the user executing the production python code is allowed read permission but not write permission to the filesystem so that even if someone compromised that user they would be less able to impact the machine. This means that you must generate the pyc's before deployment.

You would be right to assume that in a production system you might be using virtualenv and proper python packaging, which resolves this issue because we would never see a .py from a doc directory, but I happen to be at one of the older python shops that built quite a lot of infrastructure before proper packaging and did a lot of cross repository dependency management with git submodules. Basically we are maintaining a fork right now with this issue fixed and I figured it would be better to just get this contributed back upstream since it is a quick and harmless change to "to the right thing".

Also just in general if you have python files in a python package they should probably be valid python, even if they are in an example directory.

comment:3 Changed 8 years ago by jlynch

Keywords: review added

Bumping this back to review because I think it is a quick easy fix that makes all .py files properly compile in the repository.

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

I'm still not inclined to accept this.

  • The historic documentation is there as a record of what was presented at a conference. Changing it distorts this record.
  • There is no test coverage that demonstrates that "all .py files properly compile". Nor is there any development policy that requires this to be the case.
  • Twisted is in the "twisted" directory in the repository. If you want to compile all of Twisted, you only have to compile that directory, not all of the docs. As a bonus, this will be faster and use less disk space.

I'll let someone else agree or disagree and claim the review though.

comment:5 Changed 8 years ago by jlynch

Ok, I mean it is perfectly reasonable to say that the twisted package is just the directories provided by the twisted python package (which is just the twisted subdirectory and not the twisted directory). It also seems reasonable to say that if you have a .py file in your repository, it should probably be valid python. Like I said above, you are right that using proper python deps would be the "right way" on our end (and save disk sapce, and be faster because you can use wheels, etc ...); unfortunately it's taking a long time to move our large code base towards using virtualenv and proper packaging.

I'm submitting this patch because it is 6 lines and means we don't have to maintain a fork of twisted. To address your concerns, would the probability of this patch getting accepted go up if I got the blessing of glyph, who I believe gave the talk, and/or if I included a test case that asserts compileall succeeds in the base directory?

To be honest I figured this would just be an easy patch since it's in a doc directory and not impacting the api in any way. I know that this is impacting development in at least one large python code base that uses git submodules (so a full checkout of the twisted repo), and I don't believe it is implausible to assume there are others out there.

comment:6 Changed 8 years ago by jlynch

Keywords: review removed
Resolution: wontfix
Status: newclosed

As per the discussion over at #twisted-dev I'm going to try to work -x into our deploy process instead of this patch.

comment:7 Changed 8 years ago by Glyph

Thanks for working through this with us. Even though we didn't merge anything, it's always nice to have users bring us their problems rather than silently raging at Twisted :-). Glad you got it resolved!

comment:8 Changed 13 months ago by Thomas Grainger

Resolution: wontfix
Status: closedreopened

comment:9 Changed 13 months ago by Thomas Grainger

Resolution: fixed
Status: reopenedclosed
Note: See TracTickets for help on using tickets.