py3 fixes not compatible with Python<2.7.9

[gp54321]

this change triggers a syntax error looking very much like this python bug See this thread

root@tstpy:~# cat a.py
def test():
    def test2():
        return True
    d={"v":"myplone"}
    exec("print(v)",d)
test()
root@tstpy:~# source activate py278
(py278) root@tstpy:~# python
Python 2.7.8 |Continuum Analytics, Inc.| (default, Aug 21 2014, 18:22:21) 
[GCC 4.4.7 20120313 (Red Hat 4.4.7-1)] on linux2
Type "help", "copyright", "credits" or "license" for more information.
Anaconda is brought to you by Continuum Analytics.
Please check out: http://continuum.io/thanks and https://binstar.org
>>> 
(py278) root@tstpy:~# python a.py
  File "a.py", line 5
    exec("print(v)",d)
SyntaxError: unqualified exec is not allowed in function 'test' it contains a nested function with free variables
(py278) root@tstpy:~# source activate py279
(py279) root@tstpy:~# python
Python 2.7.9 |Continuum Analytics, Inc.| (default, Apr 14 2015, 12:54:25) 
[GCC 4.4.7 20120313 (Red Hat 4.4.7-1)] on linux2
Type "help", "copyright", "credits" or "license" for more information.
Anaconda is brought to you by Continuum Analytics.
Please check out: http://continuum.io/thanks and https://binstar.org
>>> 
(py279) root@tstpy:~# python a.py
myplone
(py279) root@tstpy:~#

[mauritsvanrees]

I fail to see how the change you link to would cause the problem. If I see if correctly, it used to be:

try:
    CODE A
except TypeError:
    CODE B

After the change, it is:

CODE A

The error you see is when executing CODE A, which has not changed at all.

Or am I seeing this wrong?

[gp54321]

problem comes from replacing

exec(_buildFacade(spec, callable.doc) in facade_globs) by exec(_buildFacade(name, spec, callable.doc), facade_globs)

it's supposed to be python2 and python 3 compatible but in fact it's only true with a fixed python 2 Edit: I'm not saying plone.protect code is buggy. I'm just saying something should be done, either changing the code or adding a release note.

[mauritsvanrees]

You are already seeing this error when you import plone.protect, right? So not yet using it, but already when importing it?

The _buildFacade function from latest AccessControl does not work unless you pass it three arguments. So you need a name there. That is for a version compatible with Zope 4. Plone 4.3-5.1 is using AccessControl 3.0. But there it has the same function signature, and even in the oldest tag I could find, 2.13.0.

In other words, the AccessControl version that is used in Plone 4.2 and higher, seems not compatible with Python 2.7.8 or lower. I'm not sure about Python 2.6, but let's not even try that. :-)

So I wonder if those lines are actually called. If you use an earlier version of plone.protect which still works with the old Python, and you put a pdb before those lines, does that get triggered? Can you then see what happens when calling the 'normal' code and the fallback code?

Note: I don't have a clue what these lines are trying to do. :-)

[gp54321]

this is a SyntaxError; the code called don't matter, it's the way it is called (by an exec) that's the real problem. In other words, if nothing had been changed to the existing code BUT this simple routine

def test():
    def test2():
        return True
    d={"v":"myplone"}
    exec("print(v)",d)

had been added to utils.py, it would have crashed Plone with python <2.7.9 with this code being obviously never been called. As of this code being used, when using python < 2.7.9 it can't be called since when a Python module is imported the syntax is checked so it never comes to be used. With recent Python Plone starts and run all right without a breakpoint on this code being hit; from a quick grepping of the plone.protect directory, it seems only used in tests/testUtils.py. Running find . -iname "*.py" | xargs grep protect | grep utils on the whole buildout-cache/eggs tree don't return any ref to this protect class so unless it's used by people outside of the Plone project it seems it's only testing code - IOW if it was put in its own module in the testing directory and imported from here it would be all right (you are not testing Plone with old python versions...) I don't think it's worth the bother since updating Python is an obvious fix but Plone is lacking a release note anyway to avoid much pain to users of old Pythons.

FYI I learned about this problem in SO (where else ?): https://stackoverflow.com/questions/4484872/why-doesnt-exec-work-in-a-function-with-a-subfunction

[mauritsvanrees]

It is very definitely being used by actual code. I added a print statement in Plone coredev 5.1 and started Plone:

plone.protect.utils.protect called for callable with name setMemberProperties
plone.protect.utils.protect called for callable with name changeOwnershipOf
plone.protect.utils.protect called for callable with name acquireLocalRoles
plone.protect.utils.protect called for callable with name deleteObjectsByPaths
plone.protect.utils.protect called for callable with name transitionObjectsByPaths
plone.protect.utils.protect called for callable with name renameObjectsByPaths
plone.protect.utils.protect called for callable with name setPassword
plone.protect.utils.protect called for callable with name setPassword
plone.protect.utils.protect called for callable with name setRoleMapping
plone.protect.utils.protect called for callable with name deleteMemberArea
plone.protect.utils.protect called for callable with name setLocalRoles
plone.protect.utils.protect called for callable with name deleteLocalRoles
plone.protect.utils.protect called for callable with name deleteMembers
plone.protect.utils.protect called for callable with name setProperties
plone.protect.utils.protect called for callable with name editMember
plone.protect.utils.protect called for callable with name addMember
plone.protect.utils.protect called for callable with name removeMember
plone.protect.utils.protect called for callable with name addPrincipalToGroup
plone.protect.utils.protect called for callable with name removePrincipalFromGroup
plone.protect.utils.protect called for callable with name userFolderAddUser
plone.protect.utils.protect called for callable with name _doChangeUser
plone.protect.utils.protect called for callable with name _doDelUsers

It is used in Products/CMFPlone/patches/csrf.py. It escapes your grepping because it does from plone.protect import protect, and the plone.protect __init__.py does from plone.protect.utils import protect. That is already the case in CMFPlone 4.3 so I don't see why you would not get a pdb.

Just to get things straight: I am not trying to prove you wrong or anything, I am just curious to understand what is going on.

I still don't quite get it. I tried a simplified case which does not use nested functions. The newer style with exec as function works in Python 2.4, 2.7 and 3.6:

>>> exec('print(1+a)', {'a': 5})
6

The older style with exec as statement works in Python 2, but that style was never used:

>>> exec 'print(1+a)' in {'a': 5}
6

It fails in Python 3 because exec must be a function there, just like print.

The original style, with exec as function and in as keyword between expression and globals, doesn't work in any Python version:

>>> exec('print(1+1)' in {})
Traceback (most recent call last):
  File "<stdin>", line 1, in ?
TypeError: exec: arg 1 must be a string, file, or code object

Ah, now I see it! The real problem was introduced a bit earlier, in a18cc99ba88cdc3c777801de317eb0072d1787a7. That commit changed the exec statement into the exec function. So that commit resulted in code that didn't work in any Python version. That's okay, because it was fixed in the same PR #72. And it is indeed needed for Python 3.

Anyway, I guess it is fine to keep the change, but a note in the changelog could be nice.

[mauritsvanrees]

I added a note in c51c587e2d74fa8e3081dedd5770eb6c50abf0f6.

[gp54321]

oups! I did setup my debugger wrong :-/. the class is indeed called from patches/csrf.py and it looks like a manual implementation of a Python decorator, with the decorating function being check() in authenticator.py

FTR, I can't agree about the problem origin. You can't test it using a one-liner, it's clear if you see the linked Python issue and the linked SO thread:

(py278) root@tstpy:~# python
Python 2.7.8 |Continuum Analytics, Inc.| (default, Aug 21 2014, 18:22:21) 
[GCC 4.4.7 20120313 (Red Hat 4.4.7-1)] on linux2
Type "help", "copyright", "credits" or "license" for more information.
Anaconda is brought to you by Continuum Analytics.
Please check out: http://continuum.io/thanks and https://binstar.org
>>> exec('print(1+a)', {'a': 5})
6
>>> 
(py278) root@tstpy:~# cat a.py
def test():
    def test2():
        return True
    d={"v":"myplone"}
    exec("print(v)",d)
test()
(py278) root@tstpy:~# python a.py
  File "a.py", line 5
    exec("print(v)",d)
SyntaxError: unqualified exec is not allowed in function 'test' it contains a nested function with free variables

[zopyx]

The error also occurs on Travis with Python 2.7.14

[mauritsvanrees]

FTR, I can't agree about the problem origin. You can't test it using a one-liner

I know. That is not what I was trying to say. I was wondering how the original code could ever have worked in any Python version when even the simplified exec('print(1+1)' in {}) gives a TypeError. But that version of the code was never in a released version.

[mauritsvanrees]

With the Pythons I have here, it fails in 2.6.9 (Mac), and works in 2.7.12 (Linux), 2.7.13 (Mac) and 2.7.15 (Mac). So it would be strange if it failed in 2.7.14. But apparently that is the case.

[mauritsvanrees]

I compiled a fresh Python 2.7.14 by hand on Mac and it goes fine there. It might depend on the platform, or compile optimizations or whatever.

What I tried is this syntax:

def test():
    def test2():
        return True
    d={"v":"myplone"}
    exec("print(v)",d)

test()

Irritating that I cannot reproduce it on my machine. Sigh.

Perhaps best is to make a 3.x branch, revert the Python 3 fixes there, and use that branch on Plone 5.0 and 5.1. Plone 5.2 can keep using the master branch. Maybe later someone can figure out a way to do the same thing without an ugly exec.

[gp54321]

I tried to install Python 2.7.14 and 2.7.15 using Conda on an Ubuntu 18 and the test code worked without problem. With Ubuntu system python 2 (2.7.15 rc1) no problem too. this 2.7.14 failure seems mysterious indeed. Fedora 26 or 27 are using 2.7.15 as default, I tested on Fedora 26 no problem either. @zopyx can this problem be reproduced out of your specific setup (do you use a generic OS with system-based python) ?

[zopyx]

I can not reproduce the issue locally with Python 2.7.14. As said: it happens for me on Travis with their Python 2.7.14 version.

[mauritsvanrees]

I have created PR #76 to revert the breaking change for Plone 5.0 and 5.1.

[tisto]

@pbauer @mauritsvanrees can we please agree that we always do a major or at least a minor version release for Python 3 compatibility? A breaking change in a minor Plone upgrade is extremely annoying.

@zopyx FYI: you can run Python 2.7.14 on Travis: https://docs.travis-ci.com/user/languages/python/#Specifying-Python-versions

[mauritsvanrees]

Can we please agree that we always do a major or at least a minor version release for Python 3 compatibility?

That doesn't always make sense. For some packages, the changes are only preliminary, fixing code that gives a SyntaxError on Python 3, without really being compatible with Python 3 yet: Plone may start up, but the tests may not yet pass.

Also, if the only change is turning print foo into print(foo), then I see no point in bigger version increases.

Well, we are maybe too shy in updating versions for this. setuptools is at version 39.something, so people are used to version numbers that increase faster.

Note that if we had updated the minor or major version here, it might still have ended up in Plone 5.1.2. There are (probably) lots of packages that got a minor version update between 5.1.1 and 5.1.2. For better or worse, Plone micro/patch/bugfix releases have never been only about fixing bugs. There has always been room for small new features. And the line between a bugfix and a feature is blurry.

Summary: it is not always a clear-cut decision. But yeah, a minor version increase would usually be good.

[mauritsvanrees]

BTW, the reverting fix (PR #76) was merged, and 3.1.4 released.

[pbauer]

Same as with most other packages we should keep the code compatible for py2 and py3. So, I'll change the code in the python-branch in such a way that it will work in both versions even with older 2.7.x.

[pbauer]

For reference, here is the bug in python: https://bugs.python.org/issue21591.

Adding py2-style exec foo will fail in py3 on import (even without forgetting to import six). So to make this run on py3 and py2 < 2.7.9 will require different changes.

But: ZEO 5.0.0 already requires at least python 2.7.9 (https://github.com/zopefoundation/ZEO/commit/77bce36cc61419d18bcc172529e46307f9efc061). Since we use that in Plone 5.1 you cannot really run Plone with a version older than Python 2.7.9 anyway.

@zopyx: maybe you issue is related to https://github.com/plone/bobtemplates.plone/pull/230 (travis is weird).