Products.cmfplone: PLIP: Add support for Python 3.8

Created on 20 Jun 2019 · 21Comments · Source: plone/Products.CMFPlone

PLIP (Plone Improvement Proposal)

Responsible Persons

Proposer: Philip Bauer

Seconder:

Abstract

Python 3.8 is in beta right now and the final final release is scheduled for October 21st 2019. (see https://www.python.org/dev/peps/pep-0569/#schedule)

We should make sure that Plone 5.2 supports it asap. Zope 4.1 already has preliminary support for Python 3.8.

Python 3.8 has many new features and a bunch of spee-optimizations from which we might also benefit. See https://docs.python.org/3.8/whatsnew/3.8.html for details

Motivation

Keeping up-to-date with current Python versions.

Assumptions

Updating our codebase should be relatively easy. Some deprecated imports need to be replaces (most notably from cgi import escape) but I do not expect bigger problems.

Proposal & Implementation

Fix all issues.
Add official support during a release in the Plone 5.2.x series

Deliverables

[x] Pull-requests for all packages that do not work in Python 3.8
[x] Create jenkins-jobs for Python 3.8
[x] Fix all issues to get the build green
[ ] Update documentation to mention 3.8

Risks

Jenkins needs to run 4 jobs instead of three which uses more time and resources

Participants

Philip Bauer

feature (plip)

Source

pbauer

👍5

Most helpful comment

I tested the speed-difference for our tests. I ran ./bin/test locally on my 2017 MacBook Pro using Python 3.7 and 3.8. There is no no speed-difference between Python 3.7 and 3.8.

But the difference when using CHAMELEON_CACHE is huge:

Without CHAMELEON_CACHE:
24 minutes 2.694 seconds.

With CHAMELEON_CACHE:
19 minutes 4.417 seconds.

That's a speed-increase of more than 20% when setting a cache. I think this means we should configure a CHAMELEON_CACHE by default.

pbauer on 22 Jun 2019

👍3 🚀2

All 21 comments

I implemented support for Python 3.8 with the following pull-requests:

With these Plone seems to run fine on Python 3.8.
But there are still some failing tests and a lot of SyntaxWarnings that need to be adressed.

1. SyntaxWarnings

When running the application or the test many warnings such as these appear:

1b119b71d694960f28514f723a94273c.py:305: SyntaxWarning: "is" with a literal. Did you mean "=="?
792ccc5124ee5e8fec45ed441c515f7b.py:117: SyntaxWarning: "is" with a literal. Did you mean "=="?

They appear when identity checks (is and is not) are used with certain types of literals (e.g. strings, ints). In these cases == or != should be used instead. The problem is that 1b119b71d694960f28514f723a94273c.py seems to be a temporary file (maybe created during rendering a template?) and I've had no luck finding the source of the issue yet.

2. Failing Tests

Running the test in Python 3.8 yields some new failing tests. We'll need a jenkins-job to make these publicly visible. But locally you can reproduce them. Errors are testContainerHookRaisesUnauthorized and testContainerHookRaisesConflictError (in Products.CMFPlone/Products/CMFPlone/tests/testCheckId.py).
There may be others but they might only appear due to a weird local lxml. Example: lxml.etree.ParserError: Unicode parsing is not supported on this platform in test_html (from plone.protect.tests.testAuto.TestAutoTransform)

pbauer on 21 Jun 2019

@pbauer The files with the syntax warnings seem to be generated by Chameleon. To persist them to hard disk (an speed up the tests and the server process set the environment variable CHAMELEON_CACHE to an existing directory. Details see https://chameleon.readthedocs.io/en/latest/configuration.html
The default zope.conf.in even sets this variable, see
https://github.com/zopefoundation/Zope/blob/bab9ccadf42c4b7d4ba42cc07b778d958c287b7c/src/Zope2/utilities/skel/etc/zope.conf.in#L38-L55

Ironically the SyntaxWarnings seem to show a paradigm shift in using is resp. == as the latter one now also should be used to compare with None which was a no-go in previous Python versions. Even PEP-8 still tells: „Comparisons to singletons like None should always be done with is or is not, never the equality operators.“

icemac on 21 Jun 2019

@icemac wrote:

Ironically the SyntaxWarnings seem to show a paradigm shift in using is resp. == as the latter one now also should be used to compare with None which was a no-go in previous Python versions. Even PEP-8 still tells: „Comparisons to singletons like None should always be done with is or is not, never the equality operators.“

Maybe is is no longer true for the beta versions of Python 3.8. I've seen it on alpha versions.

icemac on 21 Jun 2019

Thanks for the tip, that helps. After export CHAMELEON_CACHE=/tmp/cache I get the full path and more info. Example:

/tmp/cache/4d4e33ea4ec8106f2bf7146c664510d1.py:934: SyntaxWarning: "is" with a literal. Did you mean "=="?
  if (__attr_name is '__default__'):
/tmp/cache/4d4e33ea4ec8106f2bf7146c664510d1.py:971: SyntaxWarning: "is" with a literal. Did you mean "=="?
  if (__attr_selected is '__default__'):

The issues seems to be in Chameleon and not in the templates. Here is one example:

# <Boolean "python:'selected' if chain == () else None" (162:81)> -> __attr_selected
__token = 7798
__attr_selected = ('selected' if (getitem('chain') == ()) else None)
if (__attr_selected is '__default__'):
    __attr_selected = None
else:
    if __attr_selected:
        __attr_selected = 'selected'
    else:
        __attr_selected = None
if (__attr_selected is not None):
    __append((' selected="%s"' % __attr_selected))
__append('>')
__stream_4752630400 = []
__append_4752630400 = __stream_4752630400.append
__append_4752630400('No workflow')
__msgid_4752630400 = __re_whitespace(''.join(__stream_4752630400)).strip()
if 'label_mapping_no_workflow':
    __append(translate('label_mapping_no_workflow', mapping=None, default=__msgid_4752630400, domain=__i18n_domain, context=__i18n_context, target_language=getitem('target_language')))
__append('</option>')
__append('\n\n                                                ')

The original template here is Products/CMFPlacefulWorkflow/browser/prefs_workflow_policy_mapping.pt and the original statement is selected python:'selected' if chain == () else None;.
Here Chameleon seems to compare values with if (__attr_selected is '__default__') instead of using ==.

With this information I was able to find the code that created that and with https://github.com/malthe/chameleon/pull/293 the warnings are gone.

pbauer on 21 Jun 2019

@icemac can you estimate how significant the performance-benefit of CHAMELEON_CACHE is? I'd like to test it with the Plone coredev but our jenkins is super-busy right now.
Do you usually enable it in production or only for tests?
A problem seems to be that the folder is not automatically created when it does not exist. How do work around that in Zope?

pbauer on 21 Jun 2019

@pbauer I think in union.cms it reduced the time for the test runs by 10 to 20 %. Each template which is used more than 2 to 3 times should benefit from the cache as it does not have to be recompiled.

We are using the caching in development, tests and production. (In development we also use CHAMELEON_RELOAD for automatically updating the compiled files on changes of the template without having to restart the instance.)

Yes, the directory has to exist, it is not created automatically.
When using bin/mkwsgiinstance in Zope the directory is created as part of the skeleton, see https://github.com/zopefoundation/Zope/tree/master/src/Zope2/utilities/skel/var/cache.
I think plone.recipe.zope2instance could do the same.

icemac on 22 Jun 2019

❤1

New Chameleon release 3.6.2 that fixes syntax error.

malthe on 22 Jun 2019

🎉1

@malthe thanks.

@gforcada could you please create a jenkins-job for Python 3.8? It's still too early for automatically run jobs or pull-requests jobs. We also do not need a special branch or plip-config because the coredev already runs on Python 3.8 even though some tests might be failing.

pbauer on 22 Jun 2019

I tested the speed-difference for our tests. I ran ./bin/test locally on my 2017 MacBook Pro using Python 3.7 and 3.8. There is no no speed-difference between Python 3.7 and 3.8.

But the difference when using CHAMELEON_CACHE is huge:

Without CHAMELEON_CACHE:
24 minutes 2.694 seconds.

With CHAMELEON_CACHE:
19 minutes 4.417 seconds.

That's a speed-increase of more than 20% when setting a cache. I think this means we should configure a CHAMELEON_CACHE by default.

pbauer on 22 Jun 2019

👍3 🚀2

cc @loechel may you comment on the RestrictedPython issue with 3.8 please?

jensens on 25 Jun 2019

@gforcada created the jenkins-jobs https://jenkins.plone.org/job/plone-5.2-python-3.8/ and https://jenkins.plone.org/job/plone-5.2-python-3.8-robot-chrome. Thanks! They are not yet triggered by github changes though.

The last failing tests should be fixed with https://github.com/plone/Products.CMFPlone/pull/2903.

After that is merged and the 3.8 tests are green we could hook them up with github changes and create a pull-request-job as well.

Also: RestrictedPython 4.0 supports Python 3.8.

pbauer on 29 Jun 2019

tisto on 29 Jun 2019

@pbauer this is just wrong, we do not have Python 3.8 support w/o having RestrictedPython on 3.8, which is not the case. I spoke with @loechel about this at Buschenschanksprint and there is a serious security issue unsolved. And according to Alexander it is far from trivial to fix.

jensens on 1 Jul 2019

That's a serious case of inside knowledge.

RestrictedPython and Zope both declare support for 3.8. The respective pull-requests are https://github.com/zopefoundation/RestrictedPython/pull/145, https://github.com/zopefoundation/RestrictedPython/pull/150 and https://github.com/zopefoundation/Zope/pull/477. And none mentions any unresolved issues.

If the @plone/security-team says we should not declare support for 3.8 for whatever reason they should please tell me so. Then I'll remove the classifier again.

pbauer on 3 Jul 2019

I will wait to push the jobs until someone confirms python 3.8 support is ready to be tested...

gforcada on 3 Jul 2019

We can test it already, but from a security POV the @plone/security-team still discusses if we better need to wait until 3.8 final is out, check all, and then declare official support.

jensens on 3 Jul 2019

I removed the classifier after discussions with the @plone/security-team.
@gforcada testing for 3.8 should be done though.

pbauer on 3 Jul 2019

jobs are there on PR: https://github.com/plone/plone.dexterity/pull/106

next time though, do we really need to rush to support a beta version of an incremental release? :thinking: :man_shrugging:

gforcada on 4 Jul 2019

@pbauer this PLIP has been approved by the framework team.