Skip to content

Conversation

@JornVernee
Copy link
Contributor

@JornVernee JornVernee commented Dec 25, 2018

(Remaking this as one big PR after being requested to per email)

This PR adds python 3 support to mx. It has been tested by running mx --strict-compliance gate --strict-mode on python 2.7.15 and python 3.6.5, both using pylint 1.9.3 (needed for 3.6). So, this PR depends on my pylint PR; #176 . This PR also definitively drops support for python 2.6 (if it was still working).

The cornerstone of this PR is the new mx_portable file. Because there are some modules, functions and types that are not available on both python 2 & 3, this module detects the python version and imports the right thing for that version, exposing it under a common, portable name. Then other mx modules can import that portable name from the mx_portable module [1].

The changes made by this PR are;

  • Changes from the pylint PR, which are described therein.
  • Syntax incompatibility fixes; this is one case of an octal literal, which is changed to a portable syntax. And, another case where the exception aliasing syntax is changed from a comma to using as.
  • Changing calls from dict.iterkeys(), dict.itervalues() and dict.iteritems() to dict.keys(), dict.values() and dict.items(). This slightly changes the behaviour from returning an iterator to returning a list on python 2, and on python 3 these methods return a dictionary 'view'. But, the important part is that the names are portable (the iter* behaviour just doesn't exist in python 3).
  • Some imports are not portable; StringIO, urllib2 and __builtins__, these have been exposed under portable names by the mx_portable file [1].
  • Some types that are not portable are basestring, unicode and long, these have also been exposed under a more portable name through mx_portable [1].
  • Some functions are not portable; itertools.ifilter, cmp, raw_input, function.func_code and dict.viewkeys. I've made portable alternatives for all of them through mx_portable [1].
  • The types module no longer contains the builtin types, instead their builtin names are used. e.g. types.DictType -> dict.
  • Python 3 does not have dict.has_key, but we can use the in keyword instead, so I've made that switch everywhere.
  • OrderedDict.keys() is no longer indexable, This was a problem in two cases. For the case where the first element element was needed I'm creating an iterator and returning the first element. For the case where actual indexing is needed, I'm converting the keys() return to a list first.
  • Python 3 drops support for the builtin __cmp__ function, and instead wants developers to implement every comparison operator separately. I've added backwards compatibility by creating a Comparable class which implements the comparison operators in terms of __cmp__. Then I had the classes using __cmp__ inherit from this Comparable class. This was mostly used for sorting dependencies, and I did find out that in some cases the sorting falls back on identity comparison, which doesn't seem very robust, but I've tried to retain that behaviour as well.
  • Python 3 uses utf-8 string encoding whereas python 2 used ascii (afaik). This can cause some problems with certain apis, so in some cases we have to manually convert between the 2. So, I have added _py3_decode and _py3_encode functions to mx_portable [1] to conditionally go between the 2 encodings, as well as a special version of subprocess.check_output which decodes it's return value into a string. In some other cases I just changed whether a file was opened in text or binary mode.

I've also made gate output the python version being run, and added a travis job for testing under python 3.6

There's a lot of commits right now, and I can try to trim it down some if needed, but I'd rather do that after the review process in case changes need to be made.

[1] : https://github.com/JornVernee/mx/blob/94744aee36e4ae41fbe310f0fb157439cf5f0936/mx_portable.py

@JornVernee
Copy link
Contributor Author

JornVernee commented Dec 25, 2018

Well, I've been testing on Windows and the mx.cmd script just uses whichever python is on the path. But it seems that the mx shell script tries to explicitly select python 2, so the travis job for python 3.6.5 is still being run with python 2.

Doug sent me a patch for that, but I haven't integrated it into this PR yet because it's not my code. There is a commit there with the code, but I decided to remove it again for that reason.

So, just be aware that travis is not actually testing with python 3.6 yet; https://travis-ci.org/graalvm/mx/jobs/472104216#L638

@dougxc dougxc self-assigned this Jan 9, 2019
@JornVernee JornVernee force-pushed the p3_other branch 2 times, most recently from 90b89ea to 2ed1ba8 Compare January 9, 2019 15:38
@JornVernee
Copy link
Contributor Author

@dougxc That should be it.

@JornVernee
Copy link
Contributor Author

JornVernee commented Jan 9, 2019

FWIW I have uploaded a patch that adds (at least partial) python 3 support to the graal/compiler suite: https://github.com/JornVernee/graal/tree/p3_compiler

On Windows I can run gate on python 3.6 until hitting oracle/graal#833 Maybe you could use that patch to test with as well.

@dougxc
Copy link
Member

dougxc commented Jan 11, 2019

Thanks @JornVernee . I have the internal merge ready to go and it should be pushed today.

@dougxc
Copy link
Member

dougxc commented Jan 13, 2019

Hi @JornVernee , discussing this offline with colleagues, we're opting to build on your PR and extend it further in the direction of promoting pure Python 3 code. That is, we want to end up with mx and all downstream suites containing only Python 3 code. This means using only Python 3 module and functions everywhere and having only a Python 2 portability layer. Portability imports and definitions would be guarded by sys.version_info[0] < 3. This means that when we are ready to drop Python 2 altogether, all that should be necessary is to delete this guarded code.

Given the solid start you've provided in this PR means this should not take me too long to complete.

@JornVernee
Copy link
Contributor Author

Hi @dougxc What will that mean for this PR? Will it still be merged?

@dougxc
Copy link
Member

dougxc commented Jan 14, 2019

Yes, this PR will be merged.

@dougxc dougxc merged commit f745be2 into graalvm:master Jan 14, 2019
@JornVernee
Copy link
Contributor Author

JornVernee commented Jan 15, 2019

@dougxc FWIW, I ran gate again after pulling master and it seems that you forgot to add # pylint: disable=undefined-variable to raw_input and unicode in mx.py, and raw_input in select_jdk.py which is making the pylint check fail when running gate on python 3.6.5 (with pylint 1.9.3) (see patch)

I see ci.jsonnet is still using pylint 1.1.0. It was not working for me when running on python 3.6 (that's why I made the pylint patch). I guess the ci is using an earlier version of python 3?

@dougxc
Copy link
Member

dougxc commented Jan 15, 2019

Sorry for the missing and badly formatted pylint suppression comments. I'll apply your patch.
Our internal CI is running pylint 1.1.0 on python2 (my doesn't control what version of python is used to run pylint - the pylint launcher does that). I just installed pylint 1.9.0 on python3 (pip3 install pylint==1.9) and see the following error:

> mx pylint
Detected pylint version: 1.9.0
Running pylint on /Users/dnsimon/mx/mx.py...
Using config file /Users/dnsimon/mx/.pylintrc19
Traceback (most recent call last):
  File "/usr/local/lib/python3.7/site-packages/astroid/decorators.py", line 89, in wrapped
    res = next(generator)
StopIteration

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/usr/local/lib/python3.7/site-packages/astroid/decorators.py", line 104, in wrapped
    raise StopIteration
StopIteration

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "/usr/local/lib/python3.7/site-packages/pylint/lint.py", line 942, in get_ast
    return MANAGER.ast_from_file(filepath, modname, source=True)
  File "/usr/local/lib/python3.7/site-packages/astroid/manager.py", line 80, in ast_from_file
    return AstroidBuilder(self).file_build(filepath, modname)
  File "/usr/local/lib/python3.7/site-packages/astroid/builder.py", line 153, in file_build
    return self._post_build(module, encoding)
  File "/usr/local/lib/python3.7/site-packages/astroid/builder.py", line 173, in _post_build
    self.delayed_assattr(delayed)
  File "/usr/local/lib/python3.7/site-packages/astroid/builder.py", line 232, in delayed_assattr
    for inferred in node.expr.infer():
  File "/usr/local/lib/python3.7/site-packages/astroid/decorators.py", line 89, in wrapped
    res = next(generator)
  File "/usr/local/lib/python3.7/site-packages/astroid/bases.py", line 95, in _infer_stmts
    for inferred in stmt.infer(context=context):
  File "/usr/local/lib/python3.7/site-packages/astroid/context.py", line 71, in cache_generator
    for result in generator:
  File "/usr/local/lib/python3.7/site-packages/astroid/decorators.py", line 86, in wrapped
    generator = _func(node, context, **kwargs)
  File "/usr/local/lib/python3.7/site-packages/astroid/inference.py", line 760, in infer_assign
    stmts = list(self.assigned_stmts(context=context))
  File "/usr/local/lib/python3.7/site-packages/astroid/protocols.py", line 304, in _arguments_infer_argname
    is_metaclass = isinstance(cls, nodes.ClassDef) and cls.type == 'metaclass'
  File "/usr/local/lib/python3.7/site-packages/astroid/scoped_nodes.py", line 1650, in _class_type
    if _is_metaclass(klass):
  File "/usr/local/lib/python3.7/site-packages/astroid/scoped_nodes.py", line 1619, in _is_metaclass
    for baseobj in base.infer():
RuntimeError: generator raised StopIteration
************* Module mx
F:  1, 0: <class 'RuntimeError'>: generator raised StopIteration (astroid-error)

Can you help with that?

@dougxc
Copy link
Member

dougxc commented Jan 15, 2019

I've now installed pylint 1.9.3 on python2 and it works fine so please ignore the above.

@JornVernee
Copy link
Contributor Author

JornVernee commented Jan 15, 2019

@dougxc It looks like you're using python 3.7:

Traceback (most recent call last):
  File "/usr/local/lib/python3.7/site-packages/astroid/decorators.py", line 104, in wrapped

pylint 1.9.3 seems to be the sweet spot for python 2.7 and 3.6, since 1.9.3 is the last version that is supported on python 2. But, afaik python 3.7 requires a later version of pylint to work. I have gotten it working with 2.2.2 which is the latest version of pylint.

After the pylint PR, support for other pylint versions should be easy to add; just look for pylint_ver_map in mx.py and add the version and file name for the new version that needs to be supported. e.g.

diff --git a/mx.py b/mx.py
index e80319e..25cc612 100755
--- a/mx.py
+++ b/mx.py
@@ -13035,6 +13035,10 @@ pylint_ver_map = {
     (1, 9): {
         'rcfile': '.pylintrc19',
         'additional_options': ['--score=n']
+    },
+    (2, 2): {
+        'rcfile': '.pylintrc22',
+        'additional_options': ['--score=n']
     }
 }

The above depends on the presence of a .pylintrc22 file in the case pylint 2.2.x is used. You could also reuse the .pylintrc19 file, but I found that there were some warnings that needed to be disabled globally for pylint 2.2.2, and the older pylint versions don't seem to work with the warning message names from newer pylint versions in the rc file.

I didn't add this to the initial PR to keep it simple in the hope of moving things along. (in hindsight it was probably a better idea to add it right away)

@dougxc
Copy link
Member

dougxc commented Jan 15, 2019

I've added support for pylint 2.2 and applied some of the other small fixes you provided: 0b4566a

@JornVernee
Copy link
Contributor Author

I've added support for pylint 2.2 and applied some of the other small fixes you provided: 0b4566a

Cool, thanks! 👍

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants