Merge lp:~mandel/ubuntuone-dev-tools/point_to_pylint into lp:ubuntuone-dev-tools

[Manuel de la Peña]

Added a function that will return the full path to pylint in a windows system to fix lp:692923

Added a function that will return the full path to pylint in a windows system to fix lp:692923

[dobey (dobey)]

What happens if there is no Python in the registry at all? What happens if WindowsError is raised again by the calls within the except clause? I think you need a couple more try/except clauses. Also, you should combin the 3 final calls into one line as "return os.path.join(path, 'Scripts', 'pylint.bat')" I think, inside an else: statement, and the other OpenKey/QueryValue calls inside a try/except block, where the script fails with a more meaningful error when a WindowsError is raised if any of those calls fails with that exception. You also need to check that pylint.bat exists and is executable, and exit with a meaningful error like "You must have pylint 0.21 or newer installed to use u1lint."

Also, you probably need to handle the case when USE_PYFLAKES is specified in the environment, instead of relying on pylint.

You don't need to assign the result of find_pylint_path() to a variable to insert it in the list. Just replacing the current string with the function call is enough. The same with platform = sys.platform in the function. You don't need to assign another variable to it for use in the if statement.

Perhaps renaming the function to find_python_script(script_name), and checking for that in the path on Windows would be better. Would make it easy to abstract for pyflakes or pylint (or other future scripts). I'd also reverse the if logic in the function, so that you do "if sys.platform is 'win32': blah blah; else: return script_name" instead. It makes it clearer that the special cases are first, and the default is to return the passed in script name.

review: Needs Fixing

[Manuel de la Peña (mandel)]

> What happens if there is no Python in the registry at all? What happens if
> WindowsError is raised again by the calls within the except clause?

I means that the python installation was not correctly done and therefore that the system cannot be trusted. I agree in some sense to defense programming, but not when the installation of the runtime used is wrong. I do agree that a third try should be added just to raise a more meaningful exception but not for any other reason.

> I think
> you need a couple more try/except clauses. Also, you should combin the 3 final
> calls into one line as "return os.path.join(path, 'Scripts', 'pylint.bat')" I
> think, inside an else: statement, and the other OpenKey/QueryValue calls
> inside a try/except block, where the script fails with a more meaningful error
> when a WindowsError is raised if any of those calls fails with that exception.
> You also need to check that pylint.bat exists and is executable, and exit with
> a meaningful error like "You must have pylint 0.21 or newer installed to use
> u1lint."
>
> Also, you probably need to handle the case when USE_PYFLAKES is specified in
> the environment, instead of relying on pylint.
>

Yes, but that is another branches job and a different bug. This branches just fixes the fact that pylint cannot be found by the subprocess call. Lets keep merge proposals small so that the reviews make more sense. I have filled a diff bug for the pyflakes issue (lp:693080)

> You don't need to assign the result of find_pylint_path() to a variable to
> insert it in the list. Just replacing the current string with the function
> call is enough. The same with platform = sys.platform in the function. You
> don't need to assign another variable to it for use in the if statement.
>

I just prefer this style, I'll assign those variables directly.

> Perhaps renaming the function to find_python_script(script_name), and checking
> for that in the path on Windows would be better. Would make it easy to
> abstract for pyflakes or pylint (or other future scripts).

Pyflakes installation using easy_install does not work on windows directly unless the user sets up his system to do so (http://docs.python.org/faq/windows.html#how-do-i-make-python-scripts-executable) otherwise the subprocess call should be to python with the path of pyflakes. Missing this logic with the one of pylint to make a general function does not makes too much sense.

> I'd also reverse
> the if logic in the function, so that you do "if sys.platform is 'win32': blah
> blah; else: return script_name" instead. It makes it clearer that the special
> cases are first, and the default is to return the passed in script name.

I prefer to have the default result at the beginning like when dealing with a recursive call. First the default, later the exceptional case. It can be easily reverted so I do not mind.

[lp:~mandel/ubuntuone-dev-tools/point_to_pylint]

19. By Manuel de la Peña on 2010-12-22

Improve the code to work when pyflakes is enabled rather than pylint.Reverted is statements. Added exception when python path is not found.

[Natalia Bidart (nataliabidart)]

When running the tests I'm getting:

Traceback (most recent call last):
  File "bin/u1lint", line 216, in <module>
    main()
  File "bin/u1lint", line 185, in main
    pylint_args.append('--output-format=parseable')
AttributeError: 'str' object has no attribute 'append'

review: Needs Fixing

[lp:~mandel/ubuntuone-dev-tools/point_to_pylint]

20. By Manuel de la Peña on 2010-12-22

Ensure that an array is returned on linux too.

21. By Manuel de la Peña on 2010-12-22

Ensure that array is returned on linux too.

[Natalia Bidart (nataliabidart)]

Lint notices:

== Python Lint Notices ==

bin/u1lint:
    207: [W0511] XXX Testing that W0511 does not cause a failure
    42: [F0401, find_python_installation_path] Unable to import '_winreg'
    47: [E0602, find_python_installation_path] Undefined variable 'WindowsError'
    53: [E0602, find_python_installation_path] Undefined variable 'WindowsError'
    58: [E1101, find_python_installation_path] Module 'sys' has no 'winver' member

review: Needs Fixing

[lp:~mandel/ubuntuone-dev-tools/point_to_pylint]

22. By Manuel de la Peña on 2010-12-22

Added comments to disable pylint noise on linux.

[Roberto Alsina (ralsina)]

Approved as we discussed on IRC.

review: Approve

[lp:~mandel/ubuntuone-dev-tools/point_to_pylint]

23. By Manuel de la Peña on 2010-12-22

Use new pylint disable and enable.

[Natalia Bidart (nataliabidart)]

Now we're talking!

review: Approve

[dobey (dobey)]

51 + except:

This should probably only catch WindowsError. Other exceptions should not get masked with the InvalidSetupException here.

96 + # append the extra args to the start info
97 + pylint_args.append('--output-format=parseable')
98 + pylint_args.append('--include-ids=yes')

This would be better as pylint_args.extend(['--output-format=parseable', '--include-ids=yes']) I think.

review: Needs Fixing

[lp:~mandel/ubuntuone-dev-tools/point_to_pylint]

24. By Manuel de la Peña on 2010-12-22

Use extend to append a collection to the pylint command. Add WindowsError as the exception to avoid other exceptions to be masked by the InvalidSetupException.

[dobey (dobey)]

review: Approve

=== modified file 'bin/u1lint'
--- bin/u1lint	2010-11-23 14:43:07 +0000
+++ bin/u1lint	2010-12-22 18:36:25 +0000
@@ -23,11 +23,82 @@
 import ConfigParser
 import os
 import subprocess
+import sys
 
 from xdg.BaseDirectory import xdg_data_dirs
 
 SRCDIR = os.environ.get('SRCDIR', os.getcwd())
 
+
+class InvalidSetupException(Exception):
+    """Raised when the env is not correctly setup."""
+
+    
+def find_python_installation_path():
+    """Return the path where python was installed."""
+    assert(sys.platform == 'win32')
+    # To get the correct path of the script we need the installation path
+    # of python. To get the isntallation path we read the registry.
+
+    # pylint: disable=F0401
+    import _winreg
+    # pylint: enable=F0401
+    software_key = _winreg.OpenKey(_winreg.HKEY_LOCAL_MACHINE, 'Software')
+    python_key = None
+    try:
+        python_key = _winreg.OpenKey(software_key, 'Python')
+    # pylint: disable=E0602
+    except WindowsError:
+    # pylint: enable=E0602
+        try:
+            # look in the WoW6432node, we are running python
+            # 32 on a 64 machine
+            wow6432node_key = _winreg.OpenKey(software_key, 'WoW6432Node')
+            python_key = _winreg.OpenKey(wow6432node_key, 'Python')
+        # pylint: disable=E0602
+        except WindowsError:
+        # pylint: enable=E0602
+            raise InvalidSetupException(
+                'Could not located python installation path.')
+    try:
+        core_key = _winreg.OpenKey(python_key, 'PythonCore')
+        # pylint: disable=E1101
+        version_key = _winreg.OpenKey(core_key, sys.winver)
+        # pylint: enable=E1101
+        return _winreg.QueryValue(version_key, 'InstallPath')
+    # pylint: disable=E0602
+    except WindowsError:
+    # pylint: enable=E0602
+        raise InvalidSetupException(
+            'Could not located python installation path.')
+
+
+def find_script_path(script, python_path=None):
+    """Return the path of the given script to be executed by subprocess."""
+    if sys.platform == 'win32':
+        if python_path is None:
+            python_path = find_python_installation_path()
+        return os.path.join(python_path, 'Scripts', script)
+    else:
+        # the default is to return the name of the script beacuse we expect it
+        # to be executable and in the path.
+        return script
+
+
+def get_subprocess_start_info(script):
+    """Return the basic info used by subprocess to start a script."""
+    if sys.platform == 'win32':
+        # the basic setup in windows is not to have python in the path and not
+        # to have .py assigned to be ran with python, therefore we assume this
+        # scenario
+        python_path = find_python_installation_path()
+        return [os.path.join(python_path, 'python.exe'),
+                find_script_path(script, python_path)]
+    else:
+        # the default is to assume that the script is executable and that it
+        # can be found in the path
+        return [script, ]
+    
 def find_pylintrc():
     """Return the first pylintrc found."""
     # Use the pylintrc in the source tree if there is one
@@ -118,11 +189,12 @@
         ignored = []
 
     if os.environ.get('USE_PYFLAKES'):
-        pylint_args = ["pyflakes"]
+        pylint_args = get_subprocess_start_info('pyflakes')
     else:
-        pylint_args = ["pylint",
-                       "--output-format=parseable",
-                       "--include-ids=yes",]
+        pylint_args = get_subprocess_start_info('pylint')
+        # append the extra args to the start info
+        pylint_args.extend(['--output-format=parseable', 
+            '--include-ids=yes'])
         if PYLINTRC:
             pylint_args.append("--rcfile=" + PYLINTRC)