check-webkit-style fails due to system pylint

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Mario Sanchez Prada]

Today I hit the following error when trying to run the Tools/Scripts/check-webkit-style script in my Fedora 21 box:

  $ Tools/Scripts/check-webkit-style -g HEAD
  Traceback (most recent call last):
    File "Tools/Scripts/check-webkit-style", line 44, in <module>
      from webkitpy.style.main import CheckWebKitStyle
    File "/home/mario/work/WebKit/Tools/Scripts/webkitpy/style/main.py", line 27, in <module>
      import webkitpy.style.checker as checker
    File "/home/mario/work/WebKit/Tools/Scripts/webkitpy/style/checker.py", line 51, in <module>
      from checkers.python import PythonChecker
    File "/home/mario/work/WebKit/Tools/Scripts/webkitpy/style/checkers/python.py", line 31, in <module>
      from webkitpy.thirdparty.autoinstalled.pylint import lint
    File "/home/mario/work/WebKit/Tools/Scripts/webkitpy/thirdparty/autoinstalled/pylint/lint.py", line 52, in <module>
      from pylint.interfaces import ILinter, IRawChecker, IASTNGChecker
  ImportError: cannot import name ILinter


After some investigation I finally found out that simply removing the pylint module would do the trick:

  $ sudo dnf remove pylint     # If installed from an rpm package

    or

  $ sudo pip uninstall pylint  # If installed via pip


Apparently, the problem is that the thirdparty/autoinstalled/pylint/lint.py file is trying to import the ILinter class from the pylint.interfaces module, which is provided by the thirdparty/autoinstalled/pylint/interfaces.py file as part of pylint 0.25.1. But pylint is also installed in the system under /usr/lib/python2.7, which is picked instead of the one inside thirdparty/autoinstalled... and this other one is a fairly different version (1.3.1 instead of 0.25.1) which, among other things, does not provide an ILinter class, causing the conflict.

So, there should be a way to prevent developers from having this type of conflicts, which can be quite confusing, or at least warning them in some way, so that they can have a working environment out of the box.

[Mario Sanchez Prada]

Created attachment 251942 [details]
Patch proposal

Updating the PYTHONPATH to make sure the third party modules are always found first also 'fixes' the issue, so proposing a small patch that would do just that. Please review, thanks

[Mario Sanchez Prada]

Adding some potential reviewers to CC

[Daniel Bates]

Comment on attachment 251942 [details]
Patch proposal

This doesn't seem like the correct place to put this functionality. Currently we explicitly append the directory thirdparty/autoinstalled to the end of sys.path when installing pylint by <http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/thirdparty/__init__.py?rev=181145#L107> and <http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/thirdparty/__init__.py?rev=181145#L71>.

[Daniel Bates]

On another note, how much effort would it be to make our Python code compatible with the latest pylint and then have autoinstall install the latest pylint?

[Mario Sanchez Prada]

(In reply to comment #3)
> Comment on attachment 251942 [details]
> Patch proposal
> 
> This doesn't seem like the correct place to put this functionality.
> Currently we explicitly append the directory thirdparty/autoinstalled to the
> end of sys.path when installing pylint by
> <http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/thirdparty/
> __init__.py?rev=181145#L107> and
> <http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/thirdparty/
> __init__.py?rev=181145#L71>.

Ah! I was looking for such a place and could not find it for some reason, thanks for pointing it out!

Unfortunately, the problem is precisely that, being at the end of sys.path instead of the beginning, is what's causing the confusion: the checker tries to import ILinter as referenced from third_party/autoinstalled, but then it finds it outside (in the system), which is a very different version not providing that class, and fails.

So, I think it has to be at the beginning of sys.path, so that everything we use from inside third_party/autoinstalled will look first in the same place, before falling back to the system paths.

For the record, I've reverted my previously proposed patch and applied the following one instead:

diff --git a/Tools/Scripts/webkitpy/thirdparty/__init__.py b/Tools/Scripts/webkitpy/thirdparty/__init__.py
index 7e2e4f3..c3baf71 100644
--- a/Tools/Scripts/webkitpy/thirdparty/__init__.py
+++ b/Tools/Scripts/webkitpy/thirdparty/__init__.py
@@ -68,7 +68,7 @@ class AutoinstallImportHook(object):
     def _ensure_autoinstalled_dir_is_in_sys_path(self):
         # Some packages require that the are being put somewhere under a directory in sys.path.
         if not _AUTOINSTALLED_DIR in sys.path:
-            sys.path.append(_AUTOINSTALLED_DIR)
+            sys.path.insert(0, _AUTOINSTALLED_DIR)
 
     def find_module(self, fullname, _):
   

...and that WORKS fine again. Do you think that patch could be acceptable? If so, I'll update the previous one and ask for review right away. Thanks


(In reply to comment #4)
> On another note, how much effort would it be to make our Python code
> compatible with the latest pylint and then have autoinstall install the
> latest pylint?

My understanding of python is relatively basic, and no idea about pylint, so I don't think I can answer this. That's why I focused on getting what we already have to work, but perhaps you're right and, if feasible, that's a better approach.

In any case, I think I solution to this issue would be good, because is very confusing when it happens, and I know I'm not alone (at least another person in #webkitgtk mentioned this recently).

[Mario Sanchez Prada]

Created attachment 252182 [details]
Patch proposal

As I will have to leave in a few minutes, and I'm not sure I will be able to devote time to WebKit next week, I'm attaching a new patch simply prepending the path instead of appending it, in case it's good enough so that it can be integrated. Please review, thanks!

[WebKit Commit Bot]

Comment on attachment 252182 [details]
Patch proposal

Clearing flags on attachment: 252182

Committed r183701: <http://trac.webkit.org/changeset/183701>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.