Update pylint to 1.9.3 (with backwards compatibility) #176

JornVernee · 2018-11-27T22:00:21Z

While looking into python 3 support I ran into a problem where astroid 1.1.0, the version currently needed by mx --strict-compliance gate --strict-mode as a dependency of pylint, does not work under python 3(.6). Luckily, python 3.6 and 2.7 both work with pylint 1.9.3, which is the latest version supported by python 2.7

This PR also allows the use of 1.9.x under strict mode and also let's travis use the 1.9.3 version of pylint instead of using 1.1.0. To make sure there's backwards compatibility with 1.1.x, I'm selecting different pylintrc files to use based on the detected version, since 1.1.x doesn't recognize the newer options.

The pylintrc files I've added is just a copy-paste from the existing one with a few extra disabled messages. The extra disabled messages are:

bad-continuation
Complaining about the way expressions are split across multiple lines. This is a code style thing that was firing a lot, so I've disabled it globally.
wrong-import-position, wrong-import-order, ungrouped-imports, multiple-imports
Bikeshedding about imports.
len-as-condition
Wants us to use not SEQUENCE instead of len(SEQUENCE) to check if a sequence is empty, but this doesn't work when the sequence is None. This seemed like bad advice, so I've disabled the warning.
no-else-return
Wants us to change

if x:
    return foo
else:
    return bar

to

if x:
    return foo
return bar

This seems like a code style thing, and it was firing a lot of times, so I've disabled the warning on the assumption that you don't really care about that. (thoughts?)

cell-var-from-loop
Warns about variables in for loops being closed over by a lambda or function, so when the closure is called the variable will have the value of the latest iteration. There's not really a straight forward fix for this, so I thought it better to leave it up to the developer to keep this in mind.
inconsistent-return-statements
Warns about missing returns statements, which seems useful, but there were just too many false positives in cases where something like abort(...) was being called. Disabling the warning locally everywhere probably gives a false sense of security, so I thought it would be better to leave this up to developers.
too-many-nested-blocks
I didn't think we cared about something like too much nesting, so I've disabled it. (thoughts?)
consider-using-enumerate
A code style suggestion that when followed turned out to change the actual behaviour of the code (and make gate fail). This was firing a lot of times (10-15), so I've just disabled it globally and leave the decision to the developer.
invalid-unary-operand-type, not-callable, unsubscriptable-object
These last 3 were cases where pylint tries to do type checking, but fails if a variable is initialized with for instance None. This was yielding a bunch of false positives, so I've disabled them.

I've also disabled bad-option-value in both files since this lets pylint ignore unrecognized locally disabled warnings/options. This is needed to support both version (and newer ones if t hey are added later)

There were also some warnings that were useful, which I fixed;

fixed too many/few format args
using reference equality to compare to None
using value equality when comparing to literals
collapsed some isinstance conditions
switched type(var) is SomeType to isinstance(var, SomeType)
avoid using async as an identifier, since it's a keyword in python 3.7
removed some redundant calls to keys().
changed a and a.b or c expressions to a.b if a else c
use of set comprehensions where possible
removed unused varargs parameters in mx_benchmark (*args was in the wrong position any ways)

There was 1 I wasn't sure about; there's one definition of the special __add__ function, which is supposed to take 2 arguments (self and one other), but the definition takes 3 [1]. I wasn't sure what to do so I've just disabled the warning for now.

[1] : https://github.com/graalvm/mx/blob/master/mx.py#L1868

JornVernee · 2018-11-28T11:58:12Z

Ok, I've tested with both 1.1.0 pylint and 1.9.3 (current latest), both work on my machine.

Travis is having some trouble downloading the Eclipse jars it seems. I've tried restarting the build a couple of times by closing and opening the PR, but it keeps failing.

I guess you could also test locally and move on without the Travis check, or afaik someone with repo write-access can trigger a restart of the build as well.

dougxc · 2018-11-28T13:17:02Z

Please don't worry too much about the Travis failures where it looks like a Travis infra problem. As long as you can run the tests locally, it should integrate without problems when going through our internal gates.

JornVernee · 2018-11-30T18:30:09Z

I've been experimenting with python 3.7 as well, which came out a few months ago.

3.6 and 2.7 can both use pylint 1.9.3, but 3.7 requires a newer version. 1.9.3 is however the last version supported by python 2.7

I think updating pylint to 1.9.3 is still useful for the sake of better error checking for 2.7, but for python 3 support I think we should aim for 3.7 which is the latest release version currently.

So, I think I will make another PR after this is merged which also adds support for pylint 2.2.x, which is needed for 3.7, and this PR will be for the sake of supporting the latest pylint for python 2.7

~~P.S. I decided to merge the support for 2.2.x into this PR as well (needed for 3.7) since I think the implementation is cleaner overall (using separate pylintrc files)~~

fix pylint warnings Fix warnings Use recent version of pylint in travis undo enumerate change, since it breaks usage fixed spurious diffs tabs -> spaces Undo erroneous change Require at least pylint 1.1.x Ensured 1.1.x pylint keeps working fix bad comment support pylint 2.2.0 added missing pylintrc files 3.7 pylint fixes remove spurious comment bad merge fix

dougxc · 2018-12-20T12:36:07Z

While going through the process of merging it, it's not clear which of the pylint suppression comments are due to false positives. Could you please comment those so it's clear why they are there.

mx.py

JornVernee · 2018-12-20T22:52:17Z

While going through the process of merging it, it's not clear which of the pylint suppression comments are due to false positives. Could you please comment those so it's clear why they are there.

In general the ones that are suppressed globally in the pylintrc files are ones that were complaining about what I considered to be coding style, and not necessarily bugs/errors.

The ones I suppressed locally were about bugs/errors, but false positives.

I can go through the messages and comment on why each one is disabled. But, I've been focused on other projects recently so I will need some time to get back into this (and find some time as well).

- globally disable some warnings that were about type checking

JornVernee · 2018-12-24T16:48:05Z

mx.py


    def isJar(self):
-        cp_repr = self.classpath_repr()
+        cp_repr = self.classpath_repr() #pylint: disable=assignment-from-no-return


self.classpath_repr() does not have a return, but this is because it's throwing a nyi(...). So this is a false positive.

JornVernee · 2018-12-24T16:49:20Z

mx.py


    def exists(self):
-        return exists(self.path) and not self.sourcesPath or exists(self.sourcesPath)
+        return exists(self.path) and not self.sourcesPath or exists(self.sourcesPath) #pylint: disable=consider-using-ternary


I felt this statement made more sense in it's current form then when using the ternary (not self.sourcesPath if exists(self.path) else exists(self.sourcesPath)) since we actually want to do a logical and. So I've disabled the warning.

JornVernee · 2018-12-24T16:49:37Z

mx.py

        }

-    def __add__(self, arcname, contents):
+    def __add__(self, arcname, contents): #pylint: disable=unexpected-special-method-signature


Discussed in PR description

JornVernee · 2018-12-24T16:51:19Z

mx.py

        if _opts.verbose:
            log(' '.join(map(pipes.quote, args)))
-        p = subprocess.Popen(args, preexec_fn=preexec_fn, creationflags=creationflags, stdout=subprocess.PIPE)
+        p = subprocess.Popen(args, preexec_fn=preexec_fn, creationflags=creationflags, stdout=subprocess.PIPE) #pylint: disable=subprocess-popen-preexec-fn


A warning against using preexec_fn because this can apparently lead to dead-locks. I don't really know how to fix it, and it's working any ways, so I've disabled the warning.

JornVernee · 2018-12-24T16:52:08Z

mx.py

        """
-        _release_version = self.release_version_from_tags(vcdir=vcdir, prefix=prefix)
-        return True if _release_version and re.match(r'^[0-9]+[0-9.]+$', _release_version) else False
+        _release_version = self.release_version_from_tags(vcdir=vcdir, prefix=prefix) #pylint: disable=assignment-from-no-return


The called function doesn't have a return statement, since it's using abort(...) so this is a false positive

JornVernee · 2018-12-24T16:52:50Z

mx.py

        out = OutputCapture()
        cmd = ['git', 'log', '{0}origin/master{1}'.format(
-                '..', '' if incoming else '', '..')]
+                '..', '' if incoming else '..')]


format string takes 2 arguments but 3 were provided. Is my fix here correct?

I've fixed this as follows:

if incoming: cmd = ['git', 'log', '..origin/master'] else: cmd = ['git', 'log', 'origin/master..']

It's also clearer.

JornVernee · 2018-12-24T16:53:13Z

mx.py

        stdout = out if not callable(out) else subprocess.PIPE
        stderr = err if not callable(err) else subprocess.PIPE
-        p = subprocess.Popen(args, cwd=cwd, stdout=stdout, stderr=stderr, preexec_fn=preexec_fn, creationflags=creationflags, env=env, **kwargs)
+        p = subprocess.Popen(args, cwd=cwd, stdout=stdout, stderr=stderr, preexec_fn=preexec_fn, creationflags=creationflags, env=env, **kwargs) #pylint: disable=subprocess-popen-preexec-fn


Same as #176 (comment)

JornVernee · 2018-12-24T16:55:10Z

mx.py


 class TempDirCwd(TempDir):
-    def __init__(self, parent_dir=None):
+    def __init__(self, parent_dir=None): #pylint: disable=useless-super-delegation


False positive, see pylint-dev/pylint#1085

JornVernee · 2018-12-24T16:57:08Z

mx.py

            out.element('clean-target', data='clean')
            out.element('id', data='jar')
-            out.close('reference')
+            out.close('reference') #pylint: disable=too-many-function-args


False positive, thinks we're using a different type for out (StringIO) then we actually are (XMLDoc), since it's defined in an enclosing scope

JornVernee · 2018-12-24T17:09:55Z

mx.py

    else:
        def on_error(*args):
-            raise
+            raise #pylint: disable=misplaced-bare-raise


False positive. This depends on which context the function is called in.

JornVernee · 2018-12-24T17:14:23Z

I've commented on the globally disabled warnings in the PR description, and on the locally disabled ones inline.

I've also decided to change some of the locally disabled warnings to globally disabled ones upon further inspection (invalid-unary-operand-type, not-callable and unsubscriptable-object). Hopefully this isn't too much of a problem.

JornVernee · 2018-12-24T21:46:47Z

mx.py

                        moduleXml.element('orderEntry', attributes={'type': 'library', 'name': dep.name, 'level': 'project'})
                    else:
-                        logv("{} skipping {} for {}".format(p, dep, jdk))
+                        logv("{} skipping {} for {}".format(p, dep, jdk)) #pylint: disable=undefined-loop-variable


p is supposedly undefined, but it is actually defined in an enclosing scope.

dougxc · 2018-12-25T22:22:19Z

Sorry for not being clearer in my request for extra comments but I was requesting for the comments to be added to the code, not the PR. Navigating from code back to a comment in a PR is not so straightforward. However, no need to for further fixes in this case - I'll transfer to PR comments to the code where applicable while integrating.

dougxc · 2018-12-25T22:29:18Z

Downstream testing shows that this PR causes the following command to fail in the substratevm suite:

git clone https://github.com/oracle/graal.git
cd graal/substratevm
mx gate --tags build

This is after merging with more recent mx changes (necessary for integrating this PR). Can you please merge the master branch into this PR and see if it also fails for you.

JornVernee · 2018-12-25T22:50:02Z

@dougxc When running that command without this PR and with just the latest mx master branch I'm also seeing this failure:

Traceback (most recent call last):
  File "J:\Projects\mx2\mx_gate.py", line 419, in gate
    _run_gate(cleanArgs, args, tasks)
  File "J:\Projects\mx2\mx_gate.py", line 520, in _run_gate
    mx.command_function('build')(defaultBuildArgs + ['--force-javac'] + args.extra_build_args)
  File "J:\Projects\mx2\mx_commands.py", line 148, in __call__
    return self.command_function(*args, **kwargs)
  File "J:\Projects\graal\substratevm\mx.substratevm\mx_substratevm.py", line 959, in build
    orig_command_build(args, vm)
  File "J:\Projects\mx2\mx_commands.py", line 148, in __call__
    return self.command_function(*args, **kwargs)
  File "J:\Projects\mx2\mx.py", line 12708, in build
    t.execute()
  File "J:\Projects\mx2\mx.py", line 910, in execute
    self.build()
  File "J:\Projects\mx2\mx_native.py", line 247, in build
    self.ninja.build()
  File "J:\Projects\mx2\mx_native.py", line 82, in build
    self._run()
  File "J:\Projects\mx2\mx_native.py", line 110, in _run
    not rc or mx.abort(rc)  # pylint: disable=expression-not-assigned
  File "J:\Projects\mx2\mx.py", line 12230, in abort
    raise SystemExit(error_code)
SystemExit: 1

The sequence of mx commands that were executed until the failure follows:

mx version --oneline
mx sversions
mx clean --all
mx build -p --warning-as-error --force-javac

Though I've not tried building the substratevm suite before, as I thought it wasn't yet supported on Windows. Maybe that's where the failure I'm seeing is actually coming from...

P.S. Seeing the same failure with this PR as well, so the presence or absence of it doesn't seem to have any effect.

dougxc · 2018-12-26T00:34:48Z

Is it possible for you to try this on a non-Windows platform? I think you're right in that SVM is not yet supported on Windows. Although it's hard to tell what exactly the error is from the output you posted - is there no earlier output giving more detail?

JornVernee · 2018-12-26T00:45:25Z

Maybe this

Building com.oracle.svm.native.strictmath with Ninja... [no build manifest]

The first time I ran the command it had [updated dependency] (or something like that) there, but now it's saying "no build manifest" which is probably the source of the non-zero exit code for the command being run? (otherwise, here is the full log: https://gist.github.com/JornVernee/1c4b0fa711cf7c1f9e91152d25418bd4)

It seems like this command is being run in a sub process and the output is not being re-directed. So I was hoping you would maybe recognize the stack trace.

I'm afraid I don't have access to a non-Windows platform to test this on right now.

P.S. I'll try doing a fresh clone of the repo as well to see if that changes anything.
P.P.S. Same problem there I'm afraid, also saying "no build manifest"

JornVernee · 2018-12-26T02:28:36Z

Actually, after some debugging with -V this seems to be a problem with ninja not finding the windows toolchain.

I will continue debugging that some other time.

pejovica · 2018-12-26T15:04:17Z

substratevm suite contains native projects, so building it on Windows requires MSVC 2010 SP1 compiler, and to run mx build from a Command Prompt that already has the development environment set up.

Maybe you could try building with mx build --no-native to see if it is enough for what you need.

JornVernee · 2018-12-26T21:36:34Z

@pejovica Thanks for the tip. I would need to be able to run the mx gate --tags build to reproduce and debug the problem Doug is seeing in the downstream tests.

I wanted to try with MSVC 2010 SP1, but it doesn't seem to run on Windows 10. (I remember trying to install MSVC 2010 in the past for openjdk, but running into compatibility problems as well). Running the compiler; it immediately crashes without any output.

Do you know if there are any plans to support newer/other compilers on Windows?

JornVernee · 2018-12-26T21:40:35Z

@dougxc Can you share the trace of the failure you are seeing? Maybe I can figure out what's causing it from that.

dougxc · 2018-12-27T00:31:25Z

Here's the relevant part of the failure:

Traceback (most recent call last):
  File "/Users/dnsimon/mx/mx_gate.py", line 419, in gate
    _run_gate(cleanArgs, args, tasks)
  File "/Users/dnsimon/mx/mx_gate.py", line 561, in _run_gate
    runner(args, tasks)
  File "/Users/dnsimon/hs/graal/substratevm/mx.substratevm/mx_substratevm.py", line 563, in svm_gate_body
    build_native_image_image()
  File "/Users/dnsimon/hs/graal/substratevm/mx.substratevm/mx_substratevm.py", line 348, in build_native_image_image
    image_path = native_image_path(suite_native_image_root())
  File "/Users/dnsimon/hs/graal/substratevm/mx.substratevm/mx_substratevm.py", line 211, in suite_native_image_root
    layout_native_image_root(root_dir)
  File "/Users/dnsimon/hs/graal/substratevm/mx.substratevm/mx_substratevm.py", line 388, in layout_native_image_root
    symlink_or_copy(join(jvmci_path, symlink_name), join(native_image_root, 'lib', 'jvmci', symlink_name))
  File "/Users/dnsimon/hs/graal/substratevm/mx.substratevm/mx_substratevm.py", line 244, in symlink_or_copy
    copy2(real_target_path, dest_path)
  File "/usr/local/Cellar/python@2/2.7.15_1/Frameworks/Python.framework/Versions/2.7/lib/python2.7/shutil.py", line 144, in copy2
    copyfile(src, dst)
  File "/usr/local/Cellar/python@2/2.7.15_1/Frameworks/Python.framework/Versions/2.7/lib/python2.7/shutil.py", line 97, in copyfile
    with open(dst, 'wb') as fdst:
IOError: [Errno 2] No such file or directory: '/Users/dnsimon/hs/graal/substratevm/svmbuild/llvm-native-image-root-1.8/lib/jvmci/jvmci-hotspot.src.zip'

pejovica · 2018-12-27T13:21:58Z

Well, gates in substratevm suite don't quite work without native parts, but I think that running mx gate --tags build -B--no-native should allow you to reproduce the error.

Yes, we plan to support newer MSVC compilers. Specifically, we will add support for MSVC 2017 since it is used by JDK11.

JornVernee · 2018-12-27T14:20:29Z

Thanks. I've tried with mx gate --tags build -B--no-native and I'm running into this error

Traceback (most recent call last):
  File "H:\Projects-new\mx\mx_gate.py", line 419, in gate
    _run_gate(cleanArgs, args, tasks)
  File "H:\Projects-new\mx\mx_gate.py", line 561, in _run_gate
    runner(args, tasks)
  File "H:\Projects-new\graal\substratevm\mx.substratevm\mx_substratevm.py", line 563, in svm_gate_body
    build_native_image_image()
  File "H:\Projects-new\graal\substratevm\mx.substratevm\mx_substratevm.py", line 348, in build_native_image_image
    image_path = native_image_path(suite_native_image_root())
  File "H:\Projects-new\graal\substratevm\mx.substratevm\mx_substratevm.py", line 211, in suite_native_image_root
    layout_native_image_root(root_dir)
  File "H:\Projects-new\graal\substratevm\mx.substratevm\mx_substratevm.py", line 408, in layout_native_image_root
    copy_tree(clibrary_path, clibraries_dest)
  File "J:\Python27\lib\distutils\dir_util.py", line 128, in copy_tree
    "cannot copy tree '%s': not a directory" % src
DistutilsFileError: cannot copy tree 'H:\Projects-new\graal\substratevm\clibraries': not a directory

It looks similar to what Doug is seeing, but I get the failure slightly later in the same function, which might be up to the difference in platform. But, I get this failure with a clean mx master branch, and switching to the PR branch gives me the exact same error. I've also tried with both JDK 8 and JDK 11. So, this seems like an independent regression?

(Also, thanks for adding support for MSVC 2017! That's also the one I'm currently using for OpenJDK :) )

pejovica · 2018-12-28T17:02:21Z

Indeed, given that this is after the error Doug is seeing, it does seem that everything is fine on your side (clibraries directory is missing because the native parts are not built, so that's unrelated).

graalvmbot added the oca-signed label Nov 27, 2018

JornVernee force-pushed the p3_pylint branch from e4ad946 to fec8ba3 Compare November 27, 2018 22:14

JornVernee mentioned this pull request Nov 27, 2018

Python 3 support. #168

Closed

dougxc self-assigned this Nov 27, 2018

JornVernee force-pushed the p3_pylint branch from 9d2cebc to 1b11347 Compare November 27, 2018 22:34

JornVernee closed this Nov 27, 2018

JornVernee reopened this Nov 27, 2018

JornVernee force-pushed the p3_pylint branch 2 times, most recently from 94e20c0 to b522fdb Compare November 28, 2018 11:46

JornVernee closed this Nov 28, 2018

JornVernee reopened this Nov 28, 2018

JornVernee mentioned this pull request Nov 30, 2018

[WIP] [Python 3] Remove use of types module and basestring #179

Closed

JornVernee force-pushed the p3_pylint branch from b522fdb to 655d918 Compare November 30, 2018 18:41

JornVernee force-pushed the p3_pylint branch from 0ab87a7 to fea20ee Compare November 30, 2018 18:51

JornVernee added 2 commits December 2, 2018 16:57

Log detected pylint version

4ba1940

Typo

43ab2d8

dougxc reviewed Dec 20, 2018

View reviewed changes

mx.py Show resolved Hide resolved

JornVernee added 2 commits December 24, 2018 16:35

- remove pylint 2.2.x support again

23c6108

- globally disable some warnings that were about type checking

pylint fixes

4f5d02e

JornVernee changed the title ~~Allow newer pylint versions~~ Update pylint to 1.9.3 (with backwards compatibility) Dec 24, 2018

JornVernee commented Dec 24, 2018

View reviewed changes

JornVernee added 2 commits December 24, 2018 17:59

Merge branch 'master' of https://github.com/graalvm/mx into p3_pylint

ec543f7

pylint fix after merge with master

de406f7

JornVernee commented Dec 24, 2018

View reviewed changes

Fixes mistaken change

b891a9e

JornVernee commented Dec 24, 2018

View reviewed changes

JornVernee mentioned this pull request Dec 25, 2018

Python 3 Support #184

Merged

dougxc merged commit b891a9e into graalvm:master Jan 8, 2019

Update pylint to 1.9.3 (with backwards compatibility) #176

Update pylint to 1.9.3 (with backwards compatibility) #176

Uh oh!

Conversation

JornVernee commented Nov 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JornVernee commented Nov 28, 2018

Uh oh!

dougxc commented Nov 28, 2018

Uh oh!

JornVernee commented Nov 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dougxc commented Dec 20, 2018

Uh oh!

Uh oh!

JornVernee commented Dec 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JornVernee Dec 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JornVernee Dec 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JornVernee commented Dec 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dougxc commented Dec 25, 2018

Uh oh!

dougxc commented Dec 25, 2018

Uh oh!

JornVernee commented Dec 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dougxc commented Dec 26, 2018

Uh oh!

JornVernee commented Dec 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JornVernee commented Dec 26, 2018

Uh oh!

pejovica commented Dec 26, 2018

Uh oh!

JornVernee commented Dec 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JornVernee commented Dec 26, 2018

Uh oh!

dougxc commented Dec 27, 2018

Uh oh!

pejovica commented Dec 27, 2018

Uh oh!

JornVernee commented Dec 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

JornVernee commented Nov 27, 2018 •

edited

Loading

JornVernee commented Nov 30, 2018 •

edited

Loading

JornVernee commented Dec 20, 2018 •

edited

Loading

JornVernee Dec 24, 2018 •

edited

Loading

JornVernee Dec 24, 2018 •

edited

Loading

JornVernee commented Dec 24, 2018 •

edited

Loading

JornVernee commented Dec 25, 2018 •

edited

Loading

JornVernee commented Dec 26, 2018 •

edited

Loading

JornVernee commented Dec 26, 2018 •

edited

Loading

JornVernee commented Dec 27, 2018 •

edited

Loading