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

Best practices for making code compatible with Python2 and Python3 #2184

Open
ale-rt opened this Issue Oct 22, 2017 · 11 comments

Comments

Projects
8 participants
@ale-rt
Member

ale-rt commented Oct 22, 2017

Yesterday we have been discussing (see #2181) if using six is a good option for us and there seems there is consensus about it.

We are investigating ways to automate the code porting and we found out the tool sixer.

We now want to write a script to automatically run sixer on our packages.

Anybody who has suggestions (or is aware of some reasons to not use this approach) is invited to discuss on this issue.

@gforcada

This comment has been minimized.

Contributor

gforcada commented Oct 22, 2017

My personal opinion: using sixer sound a really good idea 👍 👍 👍 👍 run it automatically on all our packages a really bad one 👎 👎 👎

Although I guess, I have barely tried six before, that six is mostly safe, our test matrix is quite complex enough to give it one commit on each package and then have fun trying to figure out which one is responsible of the breakage.

I love this talk from this year's pycon: https://www.youtube.com/watch?v=66XoCk79kjM what relates this talk to this issue is: "one commit one specific change", i.e. don't fix 300 different little changes on a single commit, because even if 300 little changes one on their own are really small and innocent, 300 at the same time are an easy way to slip simple mistakes that go unseen, because, who reviews a boring pull request doing only 300 small cleanups that spans 90 files with a diff of +1000 lines? If that same diff is split into 300 chunks is far more easier to spot where the mistake has been made (if any).

@pbauer pbauer added this to In Progress in Python 3 Jan 22, 2018

@pbauer

This comment has been minimized.

Member

pbauer commented Jan 22, 2018

I started to document the process. Feel free to update as needed.

Porting Plone packages to Python 3

Principles

Main links

Reference material

Porting eggs with six, sixer and python-modernize

Install sixer and modernize

Install modernize (https://pypi.python.org/pypi/modernize) and sixer (https://pypi.python.org/pypi/sixer) in a fresh python3 virtualenv. sixer does not run in python 2.7

$ python3 -m venv py3
$ cd py3
$ ./bin/pip install modernize
$ ./bin/pip install sixer
$ source bin/activate

Migrate a package using sixer

Pick a package to work on. Add it to #2368 with your name. If the package is not yet in checkouts.cfg add it and run buildout. Note: Do not commit this change into the coredev! When a merge-request is merged mr.developer automatically adds the package to this file.

Run sixer for a complete package in coredev to get an idea how much will need to be changed:

$ sixer all src/plone.app.robotframework

Create a new branch for the package you are working on:

$ cd coredev/src/plone.app.robotframework
$ git checkout -b python3

Make all changes in-place:

$ sixer all -w src/plone.app.robotframework

Afterwards run isort with the Plone settings. If the settings are configured globally and isort is in the path, then sixer can be run like this instead::

$ sixer all --app=isort -w src/plone.app.robotframework

If that does not work (for some it does not) you have to check that sixer did not import six in silly places.

Now you need to:

  1. sixer puts import six at the bottom of all imports. We sort it along all other imports.

  2. Check if all the changes sixer made really make sense. Sort the imports according to the Plone coding guidelines: Do not leave import six at the bottom of the imports unless it belongs there.

  3. Run the tests for this package (in python 2.7) e.g.

     $ ./bin/test --all -s plone.app.robotframework
    

sixer gives warnings when it is not sure what to do with a possible problem. You should check those. On http://python-future.org/compatible_idioms.html you may find a good way to write something that is Python 2 and 3 compatible.

sixer is not picking up doctests automatically. It only changes files ending with .py. Others need to be specified explicitly, e.g.:

$ sixer all -w  src/plone.testing/src/plone/testing/*.rst
$ sixer all -w  src/plone.app.content/plone/app/content/*.txt
$ sixer all -w src/Products/CMFPlone/skins/plone_login/*

Also sixer for some reason does not always fix doctests, so you will have to do that by hand 😭 . See #2268 for more discussion about doctests.

Another thing sixer cannot do is change deprecated imports or patterns (like using @implementer()instead of implements(). Most of these issues will probably only appear once the imports and syntax are valid enough so that a instance can start up. So don't put too much work into guessing the right code for py3 while you cannot test it yet in py3.

Migrate a package using modernize

modernize works very similar to sixer but adresses way more issues. Among others it can fix relative imports, exceptions and tuple parameters. For details just give it a try.

$ python-modernize -wn -x libmodernize.fixes.fix_import  .

This runs all fixers except one that adds from __future__ import absolute_import to each file (https://python-modernize.readthedocs.io/en/latest/fixers.html#2to3fixer-import).

Same as with sixer you need to check each change for its necessity and sanity!!!

Wrapping up the changes on a package

If the tests pass in python 2.7 and you think you did your best to prepare the code to be able to at least start up in python 3 do the following:

  1. Add six to the dependencies in setup.py
  2. Add an entry in CHANGES.txt: Prepare for Python 2 / 3 compatibility [yourname]
  3. Commit with the commit message Prepare for Python 2 / 3 compatibility
  4. Make a pull-request (e.g. #2181)
  5. Update the information about the package in #2217
  6. Trigger jenkins-jobs for all Plone-Versions that use this branch. The links to jenkins will show up automatically as a comment in the pull-request.
  7. If the job is green you can merge it and check the box next for that package in #2217

Running Plone on Python 3

Plone does not yet run or even startup in Python 3 but trying it is a nice way to find and fix py3 issues without guessing.

The config py3.cfg (https://github.com/plone/buildout.coredev/blob/5.2/py3.cfg) can
be used to build and run Plone on Python3. It removes the Arcetypes-dependecies that will not be
ported to python3. In your own projects you could add a similar effect by depending only on Products.CMFPlone instead of Plone.

The config depends on wsgi.cfg for wsgi-support and adds checkouts and
branches with python3 compatability that is work-in progress.

It also removes a lot of parts that either break at the moment or are not
necessary before startup works.

Clone coredev and use branch 5.2::

$ git clone git@github.com:plone/buildout.coredev.git coredev_py3
$ cd coredev_py3
$ git checkout 5.2

Create py3 virtualenv::

$ python3.6 -m venv .

Install buildout::

$ ./bin/pip install -r requirements.txt

Run buildout::

$ ./bin/buildout -c py3.cfg

Start Plone::

$ ./bin/wsgi.py

Startup will fail since a lot of code is not python3-compatible yet.

Frequent errors include:

  • relative imports
  • use of ZServer (e.g for FTP or WebDAV)

@pbauer pbauer changed the title from Best practices for making code compatible with Python2 and Python3 to Document best practices for making code compatible with Python2 and Python3 Jan 22, 2018

@pbauer pbauer changed the title from Document best practices for making code compatible with Python2 and Python3 to Best practices for making code compatible with Python2 and Python3 Jan 22, 2018

@gforcada

This comment has been minimized.

Contributor

gforcada commented Jan 22, 2018

@pbauer thanks for putting some documentation!! ❤️ , shouldn't we put that on buildout.coredev docs folder?

May I ask one question? According to wikipedia by 2020, only Python 3.6 will still be supported (ok, 3.5 as well, but only until September).

Does it make sense to support 3.4 at all? Given that our testing setup is complex enough, having to run all the tests against 2.7 and 3.6 will be already a lot of time to wait to merge pull requests, the more python versions we try to support the longer the whole process it will be...

@pbauer

This comment has been minimized.

Member

pbauer commented Jan 23, 2018

@gforcada good question. I simply thought we would support the same version as Zope. But it makes sense to only support 3.5. and 3.6 since 3.4 reaches end of live in march next year.
The ticket is WIP. I'll move the final text to buildout.coredev docs after the sprint in Insbruck this week.

@gforcada

This comment has been minimized.

Contributor

gforcada commented Jan 23, 2018

Cool 👍 have fun there!!

@pbauer pbauer referenced this issue Jan 25, 2018

Closed

Start migrating Plone to Python 3 #2217

139 of 150 tasks complete
@pbauer

This comment has been minimized.

Member

pbauer commented Jan 26, 2018

sixer is not picking up doctests automatically. Files not ending with *.py need to be specified explicitly:

sixer all -w  src/plone.testing/src/plone/testing/*.rst
@mauritsvanrees

This comment has been minimized.

Member

mauritsvanrees commented Jan 27, 2018

Some things that sixer does not pick up:

@MrTango MrTango self-assigned this Mar 22, 2018

@jnns

This comment has been minimized.

jnns commented Mar 23, 2018

As an addendum to @pbauer 's instructions to get coredev-py3 running:

I ran into problems running ./bin/buildout -c py3.cfg when it tried to install zope.security:

ValueError: not enough values to unpack (expected 1, got 0)

Installing it manually via ./bin/pip install zope.security yields:

src/zope/proxy/_zope_proxy_proxy.c:27:10: fatal error: Python.h: File or directory not found

This means that I had to install the Python3 development package (which on my system is done with sudo apt install python3-dev).

Maybe this helps someone.

@gp54321

This comment has been minimized.

gp54321 commented Apr 13, 2018

I think that unicode could have a full blown article in your porting code guidelines.

I tried Plone Python 3 and searching for news, one of the first thing I tried, is crashing.

ERROR:portlets:Error while determining renderer availability of portlet ('context' '/Plone' 'news'): a bytes-like object is required, not 'str'
Traceback (most recent call last):
  File "/home/gp/coredev_py3/eggs/plone.portlets-2.3-py3.6.egg/plone/portlets/manager.py", line 117, in _lazyLoadPortlets
    isAvailable = renderer.available
  File "/home/gp/coredev_py3/src/plone.app.portlets/plone/app/portlets/portlets/news.py", line 91, in available
    return self.data.count > 0 and len(self._data())
  File "/home/gp/coredev_py3/eggs/plone.memoize-1.2.2-py3.6.egg/plone/memoize/instance.py", line 53, in memogetter
    val = func(*args, **kwargs)
  File "/home/gp/coredev_py3/src/plone.app.portlets/plone/app/portlets/portlets/news.py", line 119, in _data
    sort_limit=limit)[:limit]
  File "/home/gp/coredev_py3/src/Products.CMFPlone/Products/CMFPlone/CatalogTool.py", line 458, in searchResults
    if not show_inactive and not self.allow_inactive(kw):
  File "/home/gp/coredev_py3/src/Products.CMFPlone/Products/CMFPlone/CatalogTool.py", line 419, in allow_inactive
    parts = path[len(site_path) + 1:].split('/')
TypeError: a bytes-like object is required, not 'str'

The Python error message is difficult to understand, path[len(site_path) + 1:] is a bytes object, that's the whole problem, if it was a str it would work.

the code:

        if isinstance(paths, six.string_types):
            paths = [paths]

        objs = []
        site = getSite()
        for path in list(paths):
            path = path.encode('utf-8')  # paths must not be unicode
            try:
                site_path = '/'.join(site.getPhysicalPath())
                parts = path[len(site_path) + 1:].split('/')
                parent = site.unrestrictedTraverse('/'.join(parts[:-1]))
                objs.append(parent.restrictedTraverse(parts[-1]))

Removing the
path = path.encode('utf-8') # paths must not be unicode
line makes it work (not that it is a good fix of course)

I grepped the python code of the git-cloned coredev for encode('utf-8') and got about 250 hits.
I have tried without success to search in Github for the plone and collective organisations, I am not sure there is a way.

Not that all these hundred of hits will be bad of course. But they could. And it may not even be especially obvious. So a guideline on Unicode and porting Plone to Python 3 could be good to have, also for add-on writers who could write not compatible code - even right now

@ale-rt

This comment has been minimized.

Member

ale-rt commented Apr 13, 2018

Try:

path[len(site_path) + 1:].split(b'/')

Note the b. Prepending a b should work on both Py2 and Py3.

@gp54321

This comment has been minimized.

gp54321 commented Apr 13, 2018

@ale-rt : yes it seems to work on both Plone for Python 3 and Python 5.1.2.
My main point was that it's a specific task that could be documented in guidelines, with the patterns to search, how to fix...It can't be the one place in the code where unicode will be a problem.

@pbauer pbauer referenced this issue Apr 23, 2018

Open

PLIP: Port Plone to Python 3 #2368

11 of 17 tasks complete

@pbauer pbauer moved this from In Progress to Done in Python 3 Oct 26, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment