Bug 1313306 - Relax the rules for --help dependencies. r=chmanchester
authorMike Hommey <mh+mozilla@glandium.org>
Thu, 27 Oct 2016 13:40:42 +0900
changeset 347106 a801ef597f1fe732469ef362a9a160940e42a5d9
parent 347105 b6be0e9e3e1ead9c62fc04e60d65015aa13cb08c
child 347107 0ac9e88ff47be0989d6a2eb609583c78acac2ade
push id10298
push userraliiev@mozilla.com
push dateMon, 14 Nov 2016 12:33:03 +0000
treeherdermozilla-aurora@7e29173b1641 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerschmanchester
bugs1313306
milestone52.0a1
Bug 1313306 - Relax the rules for --help dependencies. r=chmanchester --help dependencies currently help identify functions that will run when running configure --help, which we don't want to have spreading too much. OTOH, when such functions have no side effect, it's not really that important to have them explicitly marked. So, allow missing --help dependencies for functions that: - don't use @imports - don't have a closure - don't use global variables This is a first step towards entirely removing the --help markings (the end goal being that --help dependencies will indicate actual --help dependencies). As such, we don't really care about updating the lint error message.
python/mozbuild/mozbuild/configure/lint.py
python/mozbuild/mozbuild/configure/lint_util.py
python/mozbuild/mozbuild/test/configure/test_lint.py
--- a/python/mozbuild/mozbuild/configure/lint.py
+++ b/python/mozbuild/mozbuild/configure/lint.py
@@ -1,45 +1,72 @@
 # This Source Code Form is subject to the terms of the Mozilla Public
 # License, v. 2.0. If a copy of the MPL was not distributed with this
 # file, You can obtain one at http://mozilla.org/MPL/2.0/.
 
 from __future__ import absolute_import, print_function, unicode_literals
 
 from StringIO import StringIO
 from . import (
+    CombinedDependsFunction,
     ConfigureError,
     ConfigureSandbox,
     DependsFunction,
 )
+from .lint_util import disassemble_as_iter
 from mozbuild.util import memoize
 
 
 class LintSandbox(ConfigureSandbox):
     def __init__(self, environ=None, argv=None, stdout=None, stderr=None):
         out = StringIO()
         stdout = stdout or out
         stderr = stderr or out
         environ = environ or {}
         argv = argv or []
+        self._wrapped = {}
         super(LintSandbox, self).__init__({}, environ=environ, argv=argv,
                                           stdout=stdout, stderr=stderr)
 
     def run(self, path=None):
         if path:
             self.include_file(path)
 
+    def _missing_help_dependency(self, obj):
+        if isinstance(obj, CombinedDependsFunction):
+            return False
+        if isinstance(obj, DependsFunction):
+            if self._help_option in obj.dependencies:
+                return False
+            func = self._wrapped[obj.func]
+            # We allow missing --help dependencies for functions that:
+            # - don't use @imports
+            # - don't have a closure
+            # - don't use global variables
+            if func in self._imports or func.func_closure:
+                return True
+            for op, arg in disassemble_as_iter(func):
+                if op in ('LOAD_GLOBAL', 'STORE_GLOBAL'):
+                    return True
+        return False
+
     @memoize
     def _value_for_depends(self, obj, need_help_dependency=False):
         with_help = self._help_option in obj.dependencies
         if with_help:
             for arg in obj.dependencies:
-                if isinstance(arg, DependsFunction):
-                    if self._help_option not in arg.dependencies:
-                        raise ConfigureError(
-                            "`%s` depends on '--help' and `%s`. "
-                            "`%s` must depend on '--help'"
-                            % (obj.name, arg.name, arg.name))
-        elif self._help or need_help_dependency:
+                if self._missing_help_dependency(arg):
+                    raise ConfigureError(
+                        "`%s` depends on '--help' and `%s`. "
+                        "`%s` must depend on '--help'"
+                        % (obj.name, arg.name, arg.name))
+        elif ((self._help or need_help_dependency) and
+              self._missing_help_dependency(obj)):
             raise ConfigureError("Missing @depends for `%s`: '--help'" %
                                  obj.name)
         return super(LintSandbox, self)._value_for_depends(
             obj, need_help_dependency)
+
+    def _prepare_function(self, func):
+        wrapped, glob = super(LintSandbox, self)._prepare_function(func)
+        if wrapped not in self._wrapped:
+            self._wrapped[wrapped] = func
+        return wrapped, glob
new file mode 100644
--- /dev/null
+++ b/python/mozbuild/mozbuild/configure/lint_util.py
@@ -0,0 +1,52 @@
+# This Source Code Form is subject to the terms of the Mozilla Public
+# License, v. 2.0. If a copy of the MPL was not distributed with this
+# file, You can obtain one at http://mozilla.org/MPL/2.0/.
+
+from __future__ import absolute_import, print_function, unicode_literals
+
+import dis
+import inspect
+
+
+# dis.dis only outputs to stdout. This is a modified version that
+# returns an iterator.
+def disassemble_as_iter(co):
+    if inspect.ismethod(co):
+        co = co.im_func
+    if inspect.isfunction(co):
+        co = co.func_code
+    code = co.co_code
+    n = len(code)
+    i = 0
+    extended_arg = 0
+    free = None
+    while i < n:
+        c = code[i]
+        op = ord(c)
+        opname = dis.opname[op]
+        i += 1;
+        if op >= dis.HAVE_ARGUMENT:
+            arg = ord(code[i]) + ord(code[i + 1]) * 256 + extended_arg
+            extended_arg = 0
+            i += 2
+            if op == dis.EXTENDED_ARG:
+                extended_arg = arg * 65536L
+                continue
+            if op in dis.hasconst:
+                yield opname, co.co_consts[arg]
+            elif op in dis.hasname:
+                yield opname, co.co_names[arg]
+            elif op in dis.hasjrel:
+                yield opname, i + arg
+            elif op in dis.haslocal:
+                yield opname, co.co_varnames[arg]
+            elif op in dis.hascompare:
+                yield opname, dis.cmp_op[arg]
+            elif op in dis.hasfree:
+                if free is None:
+                    free = co.co_cellvars + co.co_freevars
+                yield opname, free[arg]
+            else:
+                yield opname, None
+        else:
+            yield opname, None
--- a/python/mozbuild/mozbuild/test/configure/test_lint.py
+++ b/python/mozbuild/mozbuild/test/configure/test_lint.py
@@ -31,42 +31,132 @@ class TestLint(unittest.TestCase):
 
     def moz_configure(self, source):
         return MockedOpen({
             os.path.join(test_data_path,
                          'moz.configure'): textwrap.dedent(source)
         })
 
     def test_depends_failures(self):
+        with self.moz_configure('''
+            option('--foo', help='foo')
+            @depends('--foo')
+            def foo(value):
+                return value
+
+            @depends('--help', foo)
+            def bar(help, foo):
+                return
+        '''):
+            self.lint_test()
+
         with self.assertRaises(ConfigureError) as e:
             with self.moz_configure('''
                 option('--foo', help='foo')
                 @depends('--foo')
+                @imports('os')
                 def foo(value):
                     return value
 
                 @depends('--help', foo)
                 def bar(help, foo):
                     return
             '''):
                 self.lint_test()
 
         self.assertEquals(e.exception.message,
                           "`bar` depends on '--help' and `foo`. "
                           "`foo` must depend on '--help'")
 
         with self.assertRaises(ConfigureError) as e:
             with self.moz_configure('''
+                @template
+                def tmpl():
+                    qux = 42
+
+                    option('--foo', help='foo')
+                    @depends('--foo')
+                    def foo(value):
+                        qux
+                        return value
+
+                    @depends('--help', foo)
+                    def bar(help, foo):
+                        return
+                tmpl()
+            '''):
+                self.lint_test()
+
+        self.assertEquals(e.exception.message,
+                          "`bar` depends on '--help' and `foo`. "
+                          "`foo` must depend on '--help'")
+
+        with self.assertRaises(ConfigureError) as e:
+            with self.moz_configure('''
+                @template
+                @imports('os')
+                def tmpl():
+                    option('--foo', help='foo')
+                    @depends('--foo')
+                    def foo(value):
+                        os
+                        return value
+
+                    @depends('--help', foo)
+                    def bar(help, foo):
+                        return
+                tmpl()
+            '''):
+                self.lint_test()
+
+        self.assertEquals(e.exception.message,
+                          "`bar` depends on '--help' and `foo`. "
+                          "`foo` must depend on '--help'")
+
+        with self.moz_configure('''
+            option('--foo', help='foo')
+            @depends('--foo')
+            def foo(value):
+                return value
+
+            include(foo)
+        '''):
+            self.lint_test()
+
+        with self.assertRaises(ConfigureError) as e:
+            with self.moz_configure('''
                 option('--foo', help='foo')
                 @depends('--foo')
+                @imports('os')
                 def foo(value):
                     return value
 
                 include(foo)
             '''):
                 self.lint_test()
 
         self.assertEquals(e.exception.message,
                           "Missing @depends for `foo`: '--help'")
 
+        with self.assertRaises(ConfigureError) as e:
+            with self.moz_configure('''
+                @template
+                @imports('os')
+                def tmpl():
+                    option('--foo', help='foo')
+                    @depends('--foo')
+                    def foo(value):
+                        os
+                        return value
+                    return foo
+
+                foo = tmpl()
+
+                include(foo)
+            '''):
+                self.lint_test()
+
+        self.assertEquals(e.exception.message,
+                          "Missing @depends for `foo`: '--help'")
+
 
 if __name__ == '__main__':
     main()