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.
--- a/python/mozbuild/mozbuild/configure/lint.py
+++ b/python/mozbuild/mozbuild/configure/lint.py
@@ -4,47 +4,74 @@
from __future__ import absolute_import, print_function, unicode_literals
import inspect
from StringIO import StringIO
from collections import defaultdict
from contextlib import contextmanager
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):
out = StringIO()
environ = environ or {}
argv = argv or []
+ self._wrapped = {}
super(LintSandbox, self).__init__({}, environ=environ, argv=argv,
stdout=out, stderr=out)
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)
@contextmanager
def only_when_impl(self, when):
yield
+
+ 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()