Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PLIP: Add support for Python 3.8 #2890

Open
pbauer opened this issue Jun 20, 2019 · 18 comments

Comments

6 participants
@pbauer
Copy link
Member

commented Jun 20, 2019

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

  • Pull-requests for all packages that do not work in Python 3.8
  • Create jenkins-jobs for Python 3.8
  • 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

@pbauer pbauer added this to the Plone 5.2 milestone Jun 20, 2019

@pbauer

This comment has been minimized.

Copy link
Member Author

commented Jun 21, 2019

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)

@icemac

This comment has been minimized.

Copy link
Contributor

commented Jun 21, 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

This comment has been minimized.

Copy link
Contributor

commented Jun 21, 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.

@pbauer

This comment has been minimized.

Copy link
Member Author

commented Jun 21, 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 malthe/chameleon#293 the warnings are gone.

@pbauer

This comment has been minimized.

Copy link
Member Author

commented Jun 21, 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?

@icemac

This comment has been minimized.

Copy link
Contributor

commented Jun 22, 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.

@malthe

This comment has been minimized.

Copy link
Member

commented Jun 22, 2019

New Chameleon release 3.6.2 that fixes syntax error.

@pbauer

This comment has been minimized.

Copy link
Member Author

commented Jun 22, 2019

@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

This comment has been minimized.

Copy link
Member Author

commented Jun 22, 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.

@jensens jensens added this to New (drafts) in PLIPs Jun 25, 2019

@jensens

This comment has been minimized.

Copy link
Member

commented Jun 25, 2019

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

@pbauer

This comment has been minimized.

Copy link
Member Author

commented Jun 29, 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 #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.

@tisto

This comment has been minimized.

Copy link
Member

commented Jun 29, 2019

+1

@jensens

This comment has been minimized.

Copy link
Member

commented Jul 1, 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.

@pbauer

This comment has been minimized.

Copy link
Member Author

commented Jul 3, 2019

That's a serious case of inside knowledge.

RestrictedPython and Zope both declare support for 3.8. The respective pull-requests are zopefoundation/RestrictedPython#145, zopefoundation/RestrictedPython#150 and zopefoundation/Zope#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.

@gforcada

This comment has been minimized.

Copy link
Contributor

commented Jul 3, 2019

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

@jensens

This comment has been minimized.

Copy link
Member

commented Jul 3, 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.

@pbauer

This comment has been minimized.

Copy link
Member Author

commented Jul 3, 2019

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

@gforcada

This comment has been minimized.

Copy link
Contributor

commented Jul 4, 2019

jobs are there on PR: plone/plone.dexterity#106

next time though, do we really need to rush to support a beta version of an incremental release? 🤔 🤷‍♂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.