Merge "Module import style checking changes"

4 files changed, 111 insertions, 90 deletions

diff --git a/HACKING.rst b/HACKING.rst
index fade33ee4..30f87576f 100644
--- a/HACKING.rst
+++ b/HACKING.rst
@@ -52,6 +52,7 @@ Imports
 -------
 - Do not import objects, only modules (*)
 - Do not import more than one module per line (*)
+- Do not use wildcard ``*`` import (*)
 - Do not make relative imports
 - Do not make new nova.db imports in nova/virt/*
 - Order your imports by the full module path
@@ -62,6 +63,8 @@ Imports
 - imports from ``migrate`` package
 - imports from ``sqlalchemy`` package
 - imports from ``nova.db.sqlalchemy.session`` module
+- imports from ``nova.openstack.common.log.logging`` package
+- imports from ``nova.db.sqlalchemy.migration.versioning_api`` package
 
 Example::
 
diff --git a/run_tests.sh b/run_tests.sh
index be9b0fa73..1f269fbfe 100755
--- a/run_tests.sh
+++ b/run_tests.sh
@@ -93,6 +93,8 @@ export venv_name
 export tools_dir
 export venv=${venv_path}/${venv_dir}
 
+SCRIPT_ROOT=$(dirname $(readlink -f "$0"))
+
 if [ $no_site_packages -eq 1 ]; then
   installvenvopts="--no-site-packages"
 fi
@@ -156,12 +158,11 @@ function run_pep8 {
   srcfiles=`find nova -type f -name "*.py" ! -wholename "nova\/openstack*"`
   srcfiles+=" `find bin -type f ! -name "nova.conf*" ! -name "*api-paste.ini*" ! -name "*~"`"
   srcfiles+=" `find tools -type f -name "*.py"`"
-  srcfiles+=" `find plugins -type f -name "*.py"`"
   srcfiles+=" `find smoketests -type f -name "*.py"`"
   srcfiles+=" setup.py"
 
   # Until all these issues get fixed, ignore.
-  ignore='--ignore=E12,E711,E721,E712,N403,N404'
+  ignore='--ignore=E12,E711,E721,E712,N403,N404,N303'
 
   # First run the hacking selftest, to make sure it's right
   echo "Running hacking.py self test"
@@ -171,6 +172,10 @@ function run_pep8 {
   echo "Running pep8"
   ${wrapper} python tools/hacking.py ${ignore} ${srcfiles}
 
+  PYTHONPATH=$SCRIPT_ROOT/plugins/xenserver/networking/etc/xensource/scripts ${wrapper} python tools/hacking.py ${ignore} ./plugins/xenserver/networking
+
+  PYTHONPATH=$SCRIPT_ROOT/plugins/xenserver/xenapi/etc/xapi.d/plugins ${wrapper} python tools/hacking.py ${ignore} ./plugins/xenserver/xenapi
+
   ${wrapper} bash tools/unused_imports.sh
   # NOTE(sdague): as of grizzly-2 these are passing however leaving the comment
   # in here in case we need to break it out when we get more of our hacking working
diff --git a/tools/hacking.py b/tools/hacking.py
index 42a644e7d..58e246eff 100755
--- a/tools/hacking.py
+++ b/tools/hacking.py
@@ -21,6 +21,7 @@
 Built on top of pep8.py
 """
 
+import imp
 import inspect
 import logging
 import os
@@ -45,7 +46,9 @@ logging.disable('LOG')
 #N8xx git commit messages
 #N9xx other
 
-IMPORT_EXCEPTIONS = ['sqlalchemy', 'migrate', 'nova.db.sqlalchemy.session']
+IMPORT_EXCEPTIONS = ['sqlalchemy', 'migrate', 'nova.db.sqlalchemy.session',
+                     'nova.openstack.common.log.logging',
+                     'nova.db.sqlalchemy.migration.versioning_api']
 START_DOCSTRING_TRIPLE = ['u"""', 'r"""', '"""', "u'''", "r'''", "'''"]
 END_DOCSTRING_TRIPLE = ['"""', "'''"]
 VERBOSE_MISSING_IMPORT = os.getenv('HACKING_VERBOSE_MISSING_IMPORT', 'False')
@@ -150,111 +153,116 @@ def nova_except_format_assert(logical_line):
         yield 1, "N202: assertRaises Exception too broad"
 
 
-def nova_one_import_per_line(logical_line):
-    r"""Check for import format.
+modules_cache = dict((mod, True) for mod in tuple(sys.modules.keys())
+                     + sys.builtin_module_names)
+
+RE_RELATIVE_IMPORT = re.compile('^from\s*[.]')
+
+
+def nova_import_rules(logical_line):
+    r"""Check for imports.
 
     nova HACKING guide recommends one import per line:
     Do not import more than one module per line
 
     Examples:
-    Okay: from nova.rpc.common import RemoteError
-    N301: from nova.rpc.common import RemoteError, LOG
-    """
-    pos = logical_line.find(',')
-    parts = logical_line.split()
-    if (pos > -1 and (parts[0] == "import" or
-                      parts[0] == "from" and parts[2] == "import") and
-        not is_import_exception(parts[1])):
-        yield pos, "N301: one import per line"
+    Okay: from nova.compute import api
+    N301: from nova.compute import api, utils
 
 
-def nova_import_module_only(logical_line):
-    r"""Check for import module only.
+    Imports should usually be on separate lines.
 
     nova HACKING guide recommends importing only modules:
     Do not import objects, only modules
 
+    Examples:
     Okay: from os import path
     Okay: import os.path
+    Okay: from nova.compute import rpcapi
     N302: from os.path import dirname as dirname2
-    N303  from os.path import *
-    N304  import flakes
+    N303: from os.path import *
+    N304: from .compute import rpcapi
     """
-    # N302 import only modules
-    # N303 Invalid Import
-    # N304 Relative Import
-
-    # TODO(sdague) actually get these tests working
-    # TODO(jogo) simplify this code
-    def import_module_check(mod, parent=None, added=False):
-        """Checks for relative, modules and invalid imports.
+    #NOTE(afazekas): An old style relative import example will not be able to
+    # pass the doctest, since the relativity depends on the file's locality
 
-        If can't find module on first try, recursively check for relative
-        imports.
+    def is_module_for_sure(mod, search_path=sys.path):
+        mod_path = mod.replace('.', os.sep)
+        try:
+            imp.find_module(mod_path, search_path)
+        except ImportError:
+            return False
+        return True
+
+    def is_module_for_sure_cached(mod):
+        if mod in modules_cache:
+            return modules_cache[mod]
+        res = is_module_for_sure(mod)
+        modules_cache[mod] = res
+        return res
+
+    def is_module(mod):
+        """Checks for non module imports.
+
+        If can't find module on first try, recursively check for the parent
+        modules.
         When parsing 'from x import y,' x is the parent.
         """
-        current_path = os.path.dirname(pep8.current_file)
-        try:
-            with warnings.catch_warnings():
-                warnings.simplefilter('ignore', DeprecationWarning)
-                valid = True
-                if parent:
-                    parent_mod = __import__(parent, globals(), locals(),
-                        [mod], -1)
-                    valid = inspect.ismodule(getattr(parent_mod, mod))
-                else:
-                    __import__(mod, globals(), locals(), [], -1)
-                    valid = inspect.ismodule(sys.modules[mod])
-                if not valid:
-                    if added:
-                        sys.path.pop()
-                        added = False
-                        return logical_line.find(mod), ("N304: No "
-                            "relative imports. '%s' is a relative import"
-                            % logical_line)
-                    return logical_line.find(mod), ("N302: import only "
-                        "modules. '%s' does not import a module"
-                        % logical_line)
+        if is_module_for_sure_cached(mod):
+            return True
+        parts = mod.split('.')
+        for i in range(len(parts) - 1, 0, -1):
+            path = '.'.join(parts[0:i])
+            if is_module_for_sure_cached(path):
+                return False
+        _missingImport.add(mod)
+        return True
+
+    current_path = os.path.dirname(pep8.current_file)
+    current_mod = os.path.basename(pep8.current_file)
+    if current_mod[-3:] == ".py":
+        current_mod = current_mod[:-3]
 
-        except (ImportError, NameError) as exc:
-            if not added:
-                added = True
-                sys.path.append(current_path)
-                return import_module_check(mod, parent, added)
-            else:
-                name = logical_line.split()[1]
-                if name not in _missingImport:
-                    if VERBOSE_MISSING_IMPORT != 'False':
-                        print >> sys.stderr, ("ERROR: import '%s' in %s "
-                                              "failed: %s" %
-                            (name, pep8.current_file, exc))
-                    _missingImport.add(name)
-                added = False
-                sys.path.pop()
-                return
+    split_line = logical_line.split()
+    split_line_len = len(split_line)
+    if (split_line[0] in ('import', 'from') and split_line_len > 1 and
+        not is_import_exception(split_line[1])):
+        pos = logical_line.find(',')
+        if pos != -1:
+            if split_line[0] == 'from':
+                yield pos, "N301: one import per line"
+            return  # ',' is not supported by the N302 checker yet
+        pos = logical_line.find('*')
+        if pos != -1:
+            yield pos, "N303: No wildcard (*) import."
+            return
 
-        except AttributeError:
-            # Invalid import
-            if "import *" in logical_line:
-                # TODO(jogo): handle "from x import *, by checking all
-                #           "objects in x"
+        if split_line_len in (2, 4, 6) and split_line[1] != "__future__":
+            if 'from' == split_line[0] and split_line_len > 3:
+                mod = '.'.join((split_line[1], split_line[3]))
+                if is_import_exception(mod):
+                        return
+                if RE_RELATIVE_IMPORT.search(logical_line):
+                    yield logical_line.find('.'), ("N304: No "
+                        "relative imports. '%s' is a relative import"
+                        % logical_line)
+                    return
+
+                if not is_module(mod):
+                    yield 0, ("N302: import only modules."
+                              "'%s' does not import a module" % logical_line)
                 return
-            return logical_line.find(mod), ("N303: Invalid import, "
-                "%s" % mod)
 
-    split_line = logical_line.split()
-    if (", " not in logical_line and
-            split_line[0] in ('import', 'from') and
-            (len(split_line) in (2, 4, 6)) and
-            split_line[1] != "__future__"):
-        if is_import_exception(split_line[1]):
-            return
-        if "from" == split_line[0]:
-            rval = import_module_check(split_line[3], parent=split_line[1])
-        else:
-            rval = import_module_check(split_line[1])
-        if rval is not None:
-            yield rval
+        #NOTE(afazekas): import searches first in the package
+        # The import keyword just imports modules
+        # The guestfs module now imports guestfs
+        mod = split_line[1]
+        if (current_mod != mod and
+            not is_module_for_sure_cached(mod) and
+            is_module_for_sure(mod, [current_path])):
+                yield 0, ("N304: No relative imports."
+                          " '%s' is a relative import"
+                          % logical_line)
 
 
 #TODO(jogo): import template: N305
diff --git a/tox.ini b/tox.ini
index f54865601..eed3ae533 100644
--- a/tox.ini
+++ b/tox.ini
@@ -20,10 +20,15 @@ deps=
   pyflakes
 commands =
   python tools/hacking.py --doctest
-  python tools/hacking.py --ignore=E12,E711,E721,E712,N403,N404 --show-source \
-    --exclude=.venv,.git,.tox,dist,doc,*openstack/common*,*lib/python*,*egg,build .
-  python tools/hacking.py --ignore=E12,E711,E721,E712,N403,N404 --show-source \
-    --filename=nova* bin
+  python tools/hacking.py --ignore=E12,E711,E721,E712,N403,N404,N303 \
+    --show-source \
+    --exclude=.venv,.git,.tox,dist,doc,*openstack/common*,*lib/python*,*egg,build,./plugins/xenserver/networking/etc/xensource/scripts,./plugins/xenserver/xenapi/etc/xapi.d/plugins .
+  python tools/hacking.py --ignore=E12,E711,E721,E712,N403,N404,N303,N304 \
+    --show-source \
+    ./plugins/xenserver/networking/etc/xensource/scripts \
+    ./plugins/xenserver/xenapi/etc/xapi.d/plugins
+  python tools/hacking.py --ignore=E12,E711,E721,E712,N403,N404,N303 \
+    --show-source --filename=nova* bin
   bash tools/unused_imports.sh
 
 [testenv:pylint]