WIP Python 3 Support

[mottosso]

Hi all,

This PR adds complete support for Python-2.7,<=3.7.

Closes #495

Caveat

So far, the PRs I've made have been a minimal modification of master, such that it can be easily merged.

But because master is now so far behind, and because this change overlaps many of the current PRs, this PR includes all previous PRs.

Process

I've made an effort to document the progress via the commit messages, but in a nutshell the process was this.

1. Run files through futurize --stage1
2. Run tests and fix what popped up

Here are some noteworthy things.

• The changes does not affect use of Rez with Python 2, which means you can swap to this release immediately and transition to Python 3 at your own pace.
• print "" was converted to print("")
• basestring was converted to str where relevant, although some remnants still exist to leave use with Python 2 unaffected
• subprocess.Popen use universal_newlines=True for cross-platform and cross-Python compatibility, it turns all output into text which is what is expected anyway.
• open was converted to io.open with an encoding of utf8.
• .next() -> __next__, except for Version which uses .next() to fetch the next version. This should probably be renamed.
• Some dependencies were upgraded to support Python 3
  • vendor/memcached -> memcached-1.59 # py2 and py3 compatible
  • vendor/pyparsing-2.0.1 -> pyparsing-2.4.1 # compatible with py2 and py3
  • vendor/pydot -> pydot==1.4.1 # compatible with py2 and py3 and pyparsing 2.4.1
  • vendor/unittest2 -> deleted, in favour of built-in unittest
• Some files didn't require changes
  • vendor/distlib -> unchanged
  • vendor/progress -> unchanged
  • vendor/lockfile -> unchanged
  • vendor/atomicwrites -> unchanged
  • vendor/argcomplete -> unchanged
  • vendor/amqp -> unchanged
  • vendor/argparse -> unchanged
• metaclass -> @six.add_metaclass
• Some vendor libraries didn't have an equivalent supporting both Python 2 and 3 at the same time, primarily PyYAML. Those libraries were stored in subdirectories and dynamically added based on which Python was running, see vendor/_python3

Note on 2to3 versus futurize

I initially gave 2to3 a try, and although it's capable of converting from 2 to 3, it doesn't maintain compatibility with Python 2. So instead I turned to futurize

• Reference

Note on future versus six

Futurize is meant for use with the future library, but we can't use it nor make it a dependency of Rez, as it monkey-patches the standard library. That's normally fine when a library is used standalone, but not a good idea when used together with other libraries in e.g. Maya, especially not if 2 libraries use two different versions of future.

E.g.

from future import standard_library
standard_library.install_aliases()
import configparser

six on the other hand does no monkey-patching and supports vendoring.

basestring

Comparisons with isinstance(obj, basestring) was replaced with isinstance(obj, six.string_types), and because of the frequency I wrote a minor script for it.

basestring script

import os
import io
import re
import argparse

parser = argparse.ArgumentParser()
parser.add_argument("root")
parser.add_argument("--exclude", nargs="+", default=[])
parser.add_argument("--write", action="store_true")

opts = parser.parse_args()
edits = {
    "files": {},
    "changed": 0,
    "unchanged": 0,
    "excluded": 0,
}

for base, _, files in os.walk(opts.root):
    for fname in files:
        if not fname.endswith(".py"):
            continue

        abspath = os.path.join(base, fname)

        if any(ex in abspath for ex in opts.exclude):
            edits["excluded"] += 1
            continue

        changes = {"count": 0, "lines": [], "has_six": False}

        with io.open(abspath, encoding="utf8") as f:
            for line in f:
                if "six." in line:
                    changes["has_six"] = True

                if re.findall("isinstance*.*basestring", line):
                    changes["lines"] += [line.replace("basestring", "six.string_types")]
                    changes["count"] += 1
                else:
                    changes["lines"] += [line]

        if changes["count"]:
            if not changes["has_six"]:
                imp = "from rez.vendor.six import six\n"

                _index = 0
                for index, line in enumerate(changes["lines"]):
                    if line.startswith("from __future__"):
                        _index = index + 1

                changes["lines"].insert(_index, imp)

            edits["changed"] += 1
        else:
            edits["unchanged"] += 1
        edits["files"][abspath] = changes


if opts.write:
    for fname, changes in edits["files"].items():
        with io.open(fname, "w", encoding="utf8") as f:
            f.write(u"".join(changes["lines"]))

print(" Changed:")
for abspath, changes in sorted(edits["files"].items()):
    if changes["count"] > 0:
        print("  - %02d changes : %s" % (
            changes["count"],
            abspath,
        ))

print("%d changed, %d unchanged, %d excluded" % (
    edits["changed"],
    edits["unchanged"],
    edits["excluded"],
))

Next Steps

All tests currently pass for Python 2 on both Windows, Linux and MacOS, but Python 3 has a 2 errors left I'm working on figuring out.

======================================================================
FAIL: test_execute_command_environ (rez.tests.test_context.TestContext)
Test that execute_command properly sets environ dict.
----------------------------------------------------------------------
Traceback (most recent call last):
  File "c:\users\manima\dropbox\dev\anima\rez3\lib\site-packages\rez\tests\test_context.py", line 82, in test_execute_command_environ
    self.assertEqual(parts, ["covfefe", "hello"])
AssertionError: Lists differ: [''] != ['covfefe', 'hello']

First differing element 0:
''
'covfefe'

Second list contains 1 additional elements.
First extra element 1:
'hello'

- ['']
+ ['covfefe', 'hello']

======================================================================
FAIL: test_rez_command (rez.tests.test_shells.TestShells)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "c:\users\manima\dropbox\dev\anima\rez3\lib\site-packages\rez\tests\util.py", line 183, in wrapper
    func(self, *args, **kwargs)
  File "c:\users\manima\dropbox\dev\anima\rez3\lib\site-packages\rez\tests\util.py", line 194, in _fn
    fn(self, *args, **kwargs)
  File "c:\users\manima\dropbox\dev\anima\rez3\lib\site-packages\rez\tests\test_shells.py", line 173, in test_rez_command
    self.assertEqual(p.returncode, 0)
AssertionError: 1 != 0

----------------------------------------------------------------------

These both seem related to Python 3 picking up DLLs from Python 2 for some reason.

In 3.7, the word async has become a reserved keyword, renaming that to e.g. _async should be enough for 3.7 compatibility.

[mottosso]

In terms of the vendor packages we might as well update them because some of them are really old.

I think so too, but because this PR is already quite large, I would suggest we try and keep it as minimal as possible and focused on getting Rez running on Python 3 as-is, and make new PRs for things that can be changed separately. That way we'll also ensure that any errors we encounter is more easily spotted.

[mottosso]

it looks like the rez modules are doing PY3 checks to determine whether to use str or basestring (as opposed to deferring to six for that check).

I considered that, but I thought it was important to highlight that under Python 2, the string-type isn't unicode but bytes. The check is literally what six is doing but instead of:

str_type = six.string_types[0]

We can write:

PY3 = sys.version_info >= (3, 0, 0)

if PY3:
    str_type = str
else:
    str_type = basestring

I'm not too fuzzed about it though, the above is certainly shorter (but at what cost?).

[mottosso]

Using this as my default version for the past few days, and it's working well, but I'm using a very small subset of features personally. What would be fantastic was if you could have a look at running this on your machine, with your specific requirements.

Testing

To run this on your machine, try this.

$ pip install git+https://github.com/mottosso/bleeding-rez.git@feature/python-3
$ rez env some requirements
$ cd some_package
$ rez build

This also works with a virtual environment, and the --user flag.

I'm using custom builds commands, but not CMake so that would be helpful to check out.

memcached

This is working as well, except I've noticed cache misses from identical requests between Python 2 and 3, and suspect the hash generated to request a cached version differs between the two. Any idea what I could look out for to ensure hashes are identical? Ideally swapping between Python 2 and 3 shouldn't create two different entries in memcached, as it would double the time taken to generate those, and double the amount of storage used in memcached.

[mottosso]

Found an issue with basestring being replaced by str in schemas.

$ rez pip --install Qt.py
09:24:45 INFO     Using python-3.6 (C:\Users\manima\packages\python\3.6\package.py[])
...
rez.vendor.schema.schema.SchemaError: Or(<type 'str'>, And(<class 'rez.vendor.version.version.Version'>, Use(<type 'str'>))) did not validate u'1.1.0'
u'1.1.0' should be instance of <class 'rez.vendor.version.version.Version'>

Full traceback

$ rez pip --install Qt.py
09:24:45 INFO     Using python-3.6 (C:\Users\manima\packages\python\3.6\package.py[])
Collecting Qt.py
  Using cached https://files.pythonhosted.org/packages/78/8f/69ae85559e55b4eecf488c85f87012726d5c7c5829582c8d50abb21bc1fc/Qt.py-1.1.0-py2.py3-none-any.whl
Installing collected packages: Qt.py
Successfully installed Qt.py-1.1.0
WARNING: You are using pip version 19.1, however version 19.1.1 is available.
You should consider upgrading via the 'python -m pip install --upgrade pip' command.
09:24:47 DEBUG    Found c:\users\manima\appdata\local\temp\pip-sr7a79-rez\rez_staging\python\Qt.py-1.1.0.dist-info
Traceback (most recent call last):
  File "c:\Python27\Lib\runpy.py", line 174, in _run_module_as_main
    "__main__", fname, loader, pkg_name)
  File "c:\Python27\Lib\runpy.py", line 72, in _run_code
    exec code in run_globals
  File "C:\Users\manima\Dropbox\dev\anima\rez\Scripts\rez\rez.exe\__main__.py", line 9, in <module>
    sys.exit(run())
  File "c:\users\manima\dropbox\dev\anima\rez\lib\site-packages\rez\cli\_main.py", line 149, in run
    returncode = run_cmd()
  File "c:\users\manima\dropbox\dev\anima\rez\lib\site-packages\rez\cli\_main.py", line 141, in run_cmd
    return opts.func(opts, opts.parser, extra_arg_groups)
  File "c:\users\manima\dropbox\dev\anima\rez\lib\site-packages\rez\cli\pip.py", line 55, in command
    variants=opts.variant
  File "c:\users\manima\dropbox\dev\anima\rez\lib\site-packages\rez\pip.py", line 526, in pip_install_package
    pkg.commands = '\n'.join(commands)
  File "c:\Python27\Lib\contextlib.py", line 24, in __exit__
    self.gen.next()
  File "c:\users\manima\dropbox\dev\anima\rez\lib\site-packages\rez\package_maker__.py", line 182, in make_package
    package = maker.get_package()
  File "c:\users\manima\dropbox\dev\anima\rez\lib\site-packages\rez\package_maker__.py", line 101, in get_package
    package_data = package_schema.validate(package_data)
  File "c:\users\manima\dropbox\dev\anima\rez\lib\site-packages\rez\vendor\schema\schema.py", line 195, in validate
    nvalue = Schema(svalue, error=e).validate(value)
  File "c:\users\manima\dropbox\dev\anima\rez\lib\site-packages\rez\vendor\schema\schema.py", line 235, in validate
    raise SchemaError([None] + x.autos, [e] + x.errors)
rez.vendor.schema.schema.SchemaError: Or(<type 'str'>, And(<class 'rez.vendor.version.version.Version'>, Use(<type 'str'>))) did not validate u'1.1.0'
u'1.1.0' should be instance of <class 'rez.vendor.version.version.Version'>

In this case, Rez is asking pip for a version, which yields a unicode string that doesn't conform to the str requirement. With basestring, a unicode would also be OK.

I'll address this like above, by continuing to use basestring on Python 2, and use str for 3, which should capture all possible characters like before.

[mottosso]

I've noticed more instances of hasattr(args, '__iter__') as a way of determining whether arguments are in the form of a list, or string.

https://github.com/nerdvegas/rez/blob/3c194ba105330b509f9434881abe38b0c23636ad/src/rez/rex.py#L615

args = ["echo", "this"]
args = "echo this"

But as Python 3 yields True to either of these, the query doesn't work. Will need to address that somehow.

1. Could make it isinstance(args, (list, tuple)), but then would exclude instances of iterable classes; would that be a problem? Is anyone interested in passing a custom class here?
2. Could make it isinstance(args, str) which I think covers all bases.

[mottosso]

rez bind --quickstart apparently throws a TypeError in Python 3 with this PR, will need to look into that.

[davidlatwe]

rez bind --quickstart apparently throws a TypeError in Python 3 with this PR

Pasting my error message here :)

(rez3) C:\Users\David>rez-bind --quickstart
Binding platform into C:\Users\David\packages...
Traceback (most recent call last):
  File "c:\miniconda3\envs\rez3\lib\site-packages\rez\package_bind.py", line 113, in bind_package
    quiet=quiet)
  File "c:\miniconda3\envs\rez3\lib\site-packages\rez\package_bind.py", line 154, in _bind_package
    exec(stream, namespace)
TypeError: exec() arg 1 must be a string, bytes or code object

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "c:\miniconda3\envs\rez3\lib\runpy.py", line 193, in _run_module_as_main
    "__main__", mod_spec)
  File "c:\miniconda3\envs\rez3\lib\runpy.py", line 85, in _run_code
    exec(code, run_globals)
  File "C:\miniconda3\envs\rez3\Scripts\rez-bind.exe\__main__.py", line 9, in <module>
  File "c:\miniconda3\envs\rez3\lib\site-packages\rez\cli\_main.py", line 198, in rez_bind
    return run("bind")
  File "c:\miniconda3\envs\rez3\lib\site-packages\rez\cli\_main.py", line 149, in run
    returncode = run_cmd()
  File "c:\miniconda3\envs\rez3\lib\site-packages\rez\cli\_main.py", line 141, in run_cmd
    return opts.func(opts, opts.parser, extra_arg_groups)
  File "c:\miniconda3\envs\rez3\lib\site-packages\rez\cli\bind.py", line 76, in command
    quiet=True)
  File "c:\miniconda3\envs\rez3\lib\site-packages\rez\package_bind.py", line 114, in bind_package
    except exc_type as e:
TypeError: catching classes that do not inherit from BaseException is not allowed

[JeanChristopheMorinPerso]

I've noticed more instances of hasattr(args, 'iter') as a way of determining whether arguments are in the form of a list, or string.
...
But as Python 3 yields True to either of these, the query doesn't work. Will need to address that somehow.

Why not:

import collections.abc
shell_mode = isinstance(args, collections.abc.Iterable) and not isinstance(args, str)

?

[mottosso]

I'm not 100% confident plain iterables and strings covers everything passed to these functions, there may for example be custom iterable classes used throughout Rez that I don't know about; are those picked up with that expression?

I've had a solution in place similar to that for a few days that seems to work and passes all tests:

def iterable(arg):
    return (
        isinstance(arg, collections.Iterable)
        and not isinstance(arg, six.string_types)
    )

Not sure abc.Iterable is preferable, but six.string_types accounts for unicode in Python 2 which I think we want. The real test would be using it in a bigger context, with real production data, are you able to test this out?

[mottosso]

Encountered an incompatibility I'm unsure of how to solve.

https://github.com/nerdvegas/rez/blob/3a5afd86dbf88801a44d2fc4f828140ef1be6b34/src/rez/resolved_context.py#L1673-L1676

Here, error_class is either a valid exception, or None. Catching None isn't valid in Python 3, and throws a:

TypeError: catching classes that do not inherit from BaseException is not allowed

The error_class variable is coming from here:

https://github.com/nerdvegas/rez/blob/3a5afd86dbf88801a44d2fc4f828140ef1be6b34/src/rez/resolved_context.py#L1630

And it's clear enough what it means. But if errors aren't caught, should they silently pass or get raised?

[nerdvegas]

"""Catching None isn't valid in Python 3""" Oh that's annoying. Catching None is just a convenient way of dynamically disabling the exception handling (see in this case, error is only caught if config.catch_rex_errors is enabled). A simple way to keep this working would be to add a `_NeverRaisedError` class that rez uses internally (and never raises), and use this instead of None (see https://github.com/nerdvegas/rez/blob/master/src/rez/resolved_context.py#L1630 ). Just want to make two points: * Any rez-specific py2/3 compatibility code that's reused anywhere, we should put into some obvious place, like utils/py2_to_3.py or similar * RE the checking __iter__ for list vs string - I don't recall if there was a specific reason for this; there's a chance it's just really old code that should have just been an isinstance(basestring). But in keeping with the goal of this PR, we should probably keep the replacement code analogous * What's the take on merging this PR? I can't do that until either all the PRs it also encompasses are merged to master, or if this PR is redone without those PRs included. Thx A

…

On Tue, Jun 4, 2019 at 11:30 PM Marcus Ottosson ***@***.***> wrote: Encountered an incompatibility I'm unsure of how to solve. https://github.com/nerdvegas/rez/blob/master/src/rez/resolved_context.py#L1673-L1676 Here, error_class is either a valid exception, or None. Catching None isn't valid in Python 3, and throws a: TypeError: catching classes that do not inherit from BaseException is not allowed The error_class variable is coming from here: https://github.com/nerdvegas/rez/blob/master/src/rez/resolved_context.py#L1630 And it's clear enough what it means. But if errors aren't caught, should they silently pass or get raised? — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#629>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAMOUSSD4MKOITE7FXMDZH3PYZU57ANCNFSM4HN7T2XA> .

[mottosso]

What's the take on merging this PR?

Although it looks like a 270 commits, the ones actually implementing Python 3 support are rather narrow (about 85), the majority of which are very specific single-file edits, and they've all been made with cherry-picking in mind.

Once the prior PRs have been merge, my intent was rebasing those changes.

Having said that, I noticed there was another PR just merged, #495, that overlaps with this one quite drastically, almost like a shotgun blast of minor edits to almost every file. That could complicate this process, and so if possible it would help reduce the amount of work rebasing if Python 3-related changes could be made to this PR instead.

As a side-note, once you grant contributor permissions, we'll all be able to push straight to this PR as though it was our own which I think could be helpful as in seeing things merged as well.

[nerdvegas]

No worries. PR #495 just updates print statements, it's an old PR that brendan just updated, so I got it out the way. I'd imagine it's changes would match those in this PR so hopefully it doesn't cause many conflicts. Been looking into permissions. Unfortunately it isn't possible to restrict access to master in personal GH repos (unless you want to start fudging it with pre-receive hooks, which I don't). So I am considering an organization. The fact that star count is kept intact makes that more of a possibility. I still would rather not move the repo, but if that's necessary to get the controls that we need, then so be it. A

…

On Wed, Jun 5, 2019 at 3:57 PM Marcus Ottosson ***@***.***> wrote: What's the take on merging this PR? Although it looks like a 270 commits, the ones actually implementing Python 3 support are rather narrow (about 85 <https://github.com/mottosso/bleeding-rez/branches>), the majority of which are very specific single-file edits, and they've all been made with cherry-picking in mind. Once the prior PRs have been merge, my intent was rebasing those changes. Having said that, I noticed there was another PR just merged, #495 <#495>, that overlaps with this one quite drastically, almost like a shotgun blast of minor edits to almost every file. That could complicate this process, and so if possible it would help reduce the amount of work rebasing if Python 3-related changes could be made to this PR instead. As a side-note, once you grant contributor permissions, we'll all be able to push straight to this PR as though it was our own which I think could be helpful as in seeing things merged as well. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#629>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAMOUSU7BA3FKW2LCPFNYQDPY5IVRANCNFSM4HN7T2XA> .

[mottosso]

I'd imagine it's changes would match those in this PR so hopefully it doesn't cause many conflicts.

I wish it was as simple as that, but those print >> statements have a few ways of being interpreted, and it's unlikely we've interpreted them the same way.

It's not a showstopper, it just means sifting through that many more conflicts during the rebase.

Unfortunately it isn't possible to restrict access to master in personal GH repos

Probably a discussion better had in #630, but I expect that alongside granting permissions comes some form of trust, and reliance of the fact that Git lets you rollback just about anything.

[mottosso]

Python 3 support completed and at feature parity with Python 2 (all tests pass on all platforms) in bleeding-rez 2.31.4.

• bleeding-rez
• build results

# Windows, Linux or OSX, Python 2.7+,<4
$ pip install bleeding-rez==2.31.4