зеркало из https://github.com/mozilla/gecko-dev.git
Bug 1526062 - Improve error reporting by the configure lint. r=nalexander
Currently, running the configure lint will raise errors with little context, if any, which can make finding the cause of the errors tedious. So instead of raising exceptions directly, we use a hack to make the exception appear as if they had been thrown at the code location an issue is reported for, which makes the test harness print more useful context. With more context, printed out, some exception messages can be made lighter. The added _raise_from method does more than is necessary here, but will be fully used in a new check shortly. Depends on D19111 Differential Revision: https://phabricator.services.mozilla.com/D19112 --HG-- extra : moz-landing-system : lando
This commit is contained in:
Mike Hommey
Родитель
845718811d
Коммит
83b2adb600
|
|
@ -45,19 +45,82 @@ class LintSandbox(ConfigureSandbox):
|
|||
for dep in self._depends.itervalues():
|
||||
self._check_dependencies(dep)
|
||||
|
||||
def _raise_from(self, exception, obj, line=0):
|
||||
'''
|
||||
Raises the given exception as if it were emitted from the given
|
||||
location.
|
||||
|
||||
The location is determined from the values of obj and line.
|
||||
- `obj` can be a function or DependsFunction, in which case
|
||||
`line` corresponds to the line within the function the exception
|
||||
will be raised from (as an offset from the function's firstlineno).
|
||||
- `obj` can be a stack frame, in which case `line` is ignored.
|
||||
'''
|
||||
def thrower(e):
|
||||
raise e
|
||||
|
||||
if isinstance(obj, DependsFunction):
|
||||
obj, _ = self.unwrap(obj._func)
|
||||
|
||||
if inspect.isfunction(obj):
|
||||
funcname = obj.__name__
|
||||
filename = obj.func_code.co_filename
|
||||
firstline = obj.func_code.co_firstlineno
|
||||
line += firstline
|
||||
elif inspect.isframe(obj):
|
||||
funcname = obj.f_code.co_name
|
||||
filename = obj.f_code.co_filename
|
||||
firstline = obj.f_code.co_firstlineno
|
||||
line = obj.f_lineno
|
||||
else:
|
||||
# Don't know how to handle the given location, still raise the
|
||||
# exception.
|
||||
raise exception
|
||||
|
||||
# Create a new function from the above thrower that pretends
|
||||
# the `def` line is on the first line of the function given as
|
||||
# argument, and the `raise` line is on the line given as argument.
|
||||
|
||||
offset = line - firstline
|
||||
# co_lnotab is a string where each pair of consecutive character is
|
||||
# (chr(byte_increment), chr(line_increment)), mapping bytes in co_code
|
||||
# to line numbers relative to co_firstlineno.
|
||||
# If the offset we need to encode is larger than 255, we need to split it.
|
||||
co_lnotab = (chr(0) + chr(255)) * (offset / 255) + chr(0) + chr(offset % 255)
|
||||
code = thrower.func_code
|
||||
code = types.CodeType(
|
||||
code.co_argcount,
|
||||
code.co_nlocals,
|
||||
code.co_stacksize,
|
||||
code.co_flags,
|
||||
code.co_code,
|
||||
code.co_consts,
|
||||
code.co_names,
|
||||
code.co_varnames,
|
||||
filename,
|
||||
funcname,
|
||||
firstline,
|
||||
co_lnotab
|
||||
)
|
||||
thrower = types.FunctionType(
|
||||
code,
|
||||
thrower.func_globals,
|
||||
funcname,
|
||||
thrower.func_defaults,
|
||||
thrower.func_closure
|
||||
)
|
||||
thrower(exception)
|
||||
|
||||
def _check_dependencies(self, obj):
|
||||
if isinstance(obj, CombinedDependsFunction) or obj in (self._always,
|
||||
self._never):
|
||||
return
|
||||
func, glob = self.unwrap(obj._func)
|
||||
loc = '%s:%d' % (func.func_code.co_filename,
|
||||
func.func_code.co_firstlineno)
|
||||
func_args = inspect.getargspec(func)
|
||||
if func_args.keywords:
|
||||
raise ConfigureError(
|
||||
'%s: Keyword arguments are not allowed in @depends functions'
|
||||
% loc
|
||||
)
|
||||
e = ConfigureError(
|
||||
'Keyword arguments are not allowed in @depends functions')
|
||||
self._raise_from(e, func)
|
||||
|
||||
all_args = list(func_args.args)
|
||||
if func_args.varargs:
|
||||
|
|
@ -77,10 +140,8 @@ class LintSandbox(ConfigureSandbox):
|
|||
dep = dep.name
|
||||
else:
|
||||
dep = dep.option
|
||||
raise ConfigureError(
|
||||
'%s: The dependency on `%s` is unused.'
|
||||
% (loc, dep)
|
||||
)
|
||||
e = ConfigureError('The dependency on `%s` is unused' % dep)
|
||||
self._raise_from(e, func)
|
||||
|
||||
def _need_help_dependency(self, obj):
|
||||
if isinstance(obj, (CombinedDependsFunction, TrivialDependsFunction)):
|
||||
|
|
@ -119,13 +180,13 @@ class LintSandbox(ConfigureSandbox):
|
|||
if with_help:
|
||||
for arg in obj.dependencies:
|
||||
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))
|
||||
e = ConfigureError(
|
||||
"Missing '--help' dependency because `%s` depends on "
|
||||
"'--help' and `%s`" % (obj.name, arg.name))
|
||||
self._raise_from(e, arg)
|
||||
elif self._missing_help_dependency(obj):
|
||||
raise ConfigureError("Missing @depends for `%s`: '--help'" %
|
||||
obj.name)
|
||||
e = ConfigureError("Missing '--help' dependency")
|
||||
self._raise_from(e, obj)
|
||||
return super(LintSandbox, self)._value_for_depends(obj)
|
||||
|
||||
def option_impl(self, *args, **kwargs):
|
||||
|
|
@ -166,14 +227,18 @@ class LintSandbox(ConfigureSandbox):
|
|||
}
|
||||
for prefix, replacement in table[default].iteritems():
|
||||
if name.startswith('--{}-'.format(prefix)):
|
||||
raise ConfigureError(('{} should be used instead of '
|
||||
'{} with default={}').format(
|
||||
name.replace('--{}-'.format(prefix),
|
||||
'--{}-'.format(replacement)),
|
||||
name, default))
|
||||
frame = inspect.currentframe()
|
||||
while frame and frame.f_code.co_name != self.option_impl.__name__:
|
||||
frame = frame.f_back
|
||||
e = ConfigureError('{} should be used instead of '
|
||||
'{} with default={}'.format(
|
||||
name.replace('--{}-'.format(prefix),
|
||||
'--{}-'.format(replacement)),
|
||||
name, default))
|
||||
self._raise_from(e, frame.f_back if frame else None)
|
||||
|
||||
|
||||
def _check_help_for_option_with_func_default(self, option, *args, **kwargs):
|
||||
name = args[0]
|
||||
default = kwargs['default']
|
||||
|
||||
if not isinstance(default, SandboxDependsFunction):
|
||||
|
|
@ -196,8 +261,13 @@ class LintSandbox(ConfigureSandbox):
|
|||
else:
|
||||
rule = '{With|Without}'
|
||||
|
||||
raise ConfigureError(('{} has a non-constant default. '
|
||||
'Its help should contain "{}"').format(name, rule))
|
||||
frame = inspect.currentframe()
|
||||
while frame and frame.f_code.co_name != self.option_impl.__name__:
|
||||
frame = frame.f_back
|
||||
e = ConfigureError(
|
||||
'`help` should contain "{}" because of non-constant default'
|
||||
.format(rule))
|
||||
self._raise_from(e, frame.f_back if frame else None)
|
||||
|
||||
def unwrap(self, func):
|
||||
glob = func.func_globals
|
||||
|
|
|
|||
|
|
@ -5,8 +5,11 @@
|
|||
from __future__ import absolute_import, print_function, unicode_literals
|
||||
|
||||
from StringIO import StringIO
|
||||
import contextlib
|
||||
import os
|
||||
import sys
|
||||
import textwrap
|
||||
import traceback
|
||||
import unittest
|
||||
|
||||
from mozunit import (
|
||||
|
|
@ -35,6 +38,16 @@ class TestLint(unittest.TestCase):
|
|||
'moz.configure'): textwrap.dedent(source)
|
||||
})
|
||||
|
||||
@contextlib.contextmanager
|
||||
def assertRaisesFromLine(self, exc_type, line):
|
||||
with self.assertRaises(exc_type) as e:
|
||||
yield e
|
||||
|
||||
_, _, tb = sys.exc_info()
|
||||
self.assertEquals(
|
||||
traceback.extract_tb(tb)[-1][:2],
|
||||
(mozpath.join(test_data_path, 'moz.configure'), line))
|
||||
|
||||
def test_configure_testcase(self):
|
||||
# Lint python/mozbuild/mozbuild/test/configure/data/moz.configure
|
||||
self.lint_test()
|
||||
|
|
@ -53,7 +66,7 @@ class TestLint(unittest.TestCase):
|
|||
'''):
|
||||
self.lint_test()
|
||||
|
||||
with self.assertRaises(ConfigureError) as e:
|
||||
with self.assertRaisesFromLine(ConfigureError, 7) as e:
|
||||
with self.moz_configure('''
|
||||
option('--foo', help='foo')
|
||||
@depends('--foo')
|
||||
|
|
@ -67,10 +80,9 @@ class TestLint(unittest.TestCase):
|
|||
self.lint_test()
|
||||
|
||||
self.assertEquals(e.exception.message,
|
||||
"%s:7: The dependency on `--help` is unused."
|
||||
% mozpath.join(test_data_path, 'moz.configure'))
|
||||
"The dependency on `--help` is unused")
|
||||
|
||||
with self.assertRaises(ConfigureError) as e:
|
||||
with self.assertRaisesFromLine(ConfigureError, 3) as e:
|
||||
with self.moz_configure('''
|
||||
option('--foo', help='foo')
|
||||
@depends('--foo')
|
||||
|
|
@ -85,11 +97,11 @@ class TestLint(unittest.TestCase):
|
|||
'''):
|
||||
self.lint_test()
|
||||
|
||||
self.assertEquals(e.exception.message,
|
||||
"`bar` depends on '--help' and `foo`. "
|
||||
"`foo` must depend on '--help'")
|
||||
self.assertEquals(
|
||||
e.exception.message,
|
||||
"Missing '--help' dependency because `bar` depends on '--help' and `foo`")
|
||||
|
||||
with self.assertRaises(ConfigureError) as e:
|
||||
with self.assertRaisesFromLine(ConfigureError, 7) as e:
|
||||
with self.moz_configure('''
|
||||
@template
|
||||
def tmpl():
|
||||
|
|
@ -109,9 +121,9 @@ class TestLint(unittest.TestCase):
|
|||
'''):
|
||||
self.lint_test()
|
||||
|
||||
self.assertEquals(e.exception.message,
|
||||
"`bar` depends on '--help' and `foo`. "
|
||||
"`foo` must depend on '--help'")
|
||||
self.assertEquals(
|
||||
e.exception.message,
|
||||
"Missing '--help' dependency because `bar` depends on '--help' and `foo`")
|
||||
|
||||
with self.moz_configure('''
|
||||
option('--foo', help='foo')
|
||||
|
|
@ -123,7 +135,7 @@ class TestLint(unittest.TestCase):
|
|||
'''):
|
||||
self.lint_test()
|
||||
|
||||
with self.assertRaises(ConfigureError) as e:
|
||||
with self.assertRaisesFromLine(ConfigureError, 3) as e:
|
||||
with self.moz_configure('''
|
||||
option('--foo', help='foo')
|
||||
@depends('--foo')
|
||||
|
|
@ -136,9 +148,9 @@ class TestLint(unittest.TestCase):
|
|||
self.lint_test()
|
||||
|
||||
self.assertEquals(e.exception.message,
|
||||
"Missing @depends for `foo`: '--help'")
|
||||
"Missing '--help' dependency")
|
||||
|
||||
with self.assertRaises(ConfigureError) as e:
|
||||
with self.assertRaisesFromLine(ConfigureError, 3) as e:
|
||||
with self.moz_configure('''
|
||||
option('--foo', help='foo')
|
||||
@depends('--foo')
|
||||
|
|
@ -155,9 +167,9 @@ class TestLint(unittest.TestCase):
|
|||
self.lint_test()
|
||||
|
||||
self.assertEquals(e.exception.message,
|
||||
"Missing @depends for `foo`: '--help'")
|
||||
"Missing '--help' dependency")
|
||||
|
||||
with self.assertRaises(ConfigureError) as e:
|
||||
with self.assertRaisesFromLine(ConfigureError, 3) as e:
|
||||
with self.moz_configure('''
|
||||
option('--foo', help='foo')
|
||||
@depends('--foo')
|
||||
|
|
@ -170,9 +182,9 @@ class TestLint(unittest.TestCase):
|
|||
self.lint_test()
|
||||
|
||||
self.assertEquals(e.exception.message,
|
||||
"Missing @depends for `foo`: '--help'")
|
||||
"Missing '--help' dependency")
|
||||
|
||||
# This would have failed with "Missing @depends for `foo`: '--help'"
|
||||
# This would have failed with "Missing '--help' dependency"
|
||||
# in the past, because of the reference to the builtin False.
|
||||
with self.moz_configure('''
|
||||
option('--foo', help='foo')
|
||||
|
|
@ -186,7 +198,7 @@ class TestLint(unittest.TestCase):
|
|||
|
||||
# However, when something that is normally a builtin is overridden,
|
||||
# we should still want the dependency on --help.
|
||||
with self.assertRaises(ConfigureError) as e:
|
||||
with self.assertRaisesFromLine(ConfigureError, 7) as e:
|
||||
with self.moz_configure('''
|
||||
@template
|
||||
def tmpl():
|
||||
|
|
@ -203,7 +215,7 @@ class TestLint(unittest.TestCase):
|
|||
self.lint_test()
|
||||
|
||||
self.assertEquals(e.exception.message,
|
||||
"Missing @depends for `foo`: '--help'")
|
||||
"Missing '--help' dependency")
|
||||
|
||||
# There is a default restricted `os` module when there is no explicit
|
||||
# @imports, and it's fine to use it without a dependency on --help.
|
||||
|
|
@ -218,7 +230,7 @@ class TestLint(unittest.TestCase):
|
|||
'''):
|
||||
self.lint_test()
|
||||
|
||||
with self.assertRaises(ConfigureError) as e:
|
||||
with self.assertRaisesFromLine(ConfigureError, 3) as e:
|
||||
with self.moz_configure('''
|
||||
option('--foo', help='foo')
|
||||
@depends('--foo')
|
||||
|
|
@ -230,10 +242,9 @@ class TestLint(unittest.TestCase):
|
|||
self.lint_test()
|
||||
|
||||
self.assertEquals(e.exception.message,
|
||||
"%s:3: The dependency on `--foo` is unused."
|
||||
% mozpath.join(test_data_path, 'moz.configure'))
|
||||
"The dependency on `--foo` is unused")
|
||||
|
||||
with self.assertRaises(ConfigureError) as e:
|
||||
with self.assertRaisesFromLine(ConfigureError, 5) as e:
|
||||
with self.moz_configure('''
|
||||
@depends(when=True)
|
||||
def bar():
|
||||
|
|
@ -247,10 +258,9 @@ class TestLint(unittest.TestCase):
|
|||
self.lint_test()
|
||||
|
||||
self.assertEquals(e.exception.message,
|
||||
"%s:5: The dependency on `bar` is unused."
|
||||
% mozpath.join(test_data_path, 'moz.configure'))
|
||||
"The dependency on `bar` is unused")
|
||||
|
||||
with self.assertRaises(ConfigureError) as e:
|
||||
with self.assertRaisesFromLine(ConfigureError, 2) as e:
|
||||
with self.moz_configure('''
|
||||
@depends(depends(when=True)(lambda: None))
|
||||
def foo(value):
|
||||
|
|
@ -261,10 +271,9 @@ class TestLint(unittest.TestCase):
|
|||
self.lint_test()
|
||||
|
||||
self.assertEquals(e.exception.message,
|
||||
"%s:2: The dependency on `<lambda>` is unused."
|
||||
% mozpath.join(test_data_path, 'moz.configure'))
|
||||
"The dependency on `<lambda>` is unused")
|
||||
|
||||
with self.assertRaises(ConfigureError) as e:
|
||||
with self.assertRaisesFromLine(ConfigureError, 9) as e:
|
||||
with self.moz_configure('''
|
||||
@template
|
||||
def tmpl():
|
||||
|
|
@ -282,8 +291,7 @@ class TestLint(unittest.TestCase):
|
|||
self.lint_test()
|
||||
|
||||
self.assertEquals(e.exception.message,
|
||||
"%s:9: The dependency on `qux` is unused."
|
||||
% mozpath.join(test_data_path, 'moz.configure'))
|
||||
"The dependency on `qux` is unused")
|
||||
|
||||
def test_default_enable(self):
|
||||
# --enable-* with default=True is not allowed.
|
||||
|
|
@ -291,7 +299,7 @@ class TestLint(unittest.TestCase):
|
|||
option('--enable-foo', default=False, help='foo')
|
||||
'''):
|
||||
self.lint_test()
|
||||
with self.assertRaises(ConfigureError) as e:
|
||||
with self.assertRaisesFromLine(ConfigureError, 2) as e:
|
||||
with self.moz_configure('''
|
||||
option('--enable-foo', default=True, help='foo')
|
||||
'''):
|
||||
|
|
@ -306,7 +314,7 @@ class TestLint(unittest.TestCase):
|
|||
option('--disable-foo', default=True, help='foo')
|
||||
'''):
|
||||
self.lint_test()
|
||||
with self.assertRaises(ConfigureError) as e:
|
||||
with self.assertRaisesFromLine(ConfigureError, 2) as e:
|
||||
with self.moz_configure('''
|
||||
option('--disable-foo', default=False, help='foo')
|
||||
'''):
|
||||
|
|
@ -321,7 +329,7 @@ class TestLint(unittest.TestCase):
|
|||
option('--with-foo', default=False, help='foo')
|
||||
'''):
|
||||
self.lint_test()
|
||||
with self.assertRaises(ConfigureError) as e:
|
||||
with self.assertRaisesFromLine(ConfigureError, 2) as e:
|
||||
with self.moz_configure('''
|
||||
option('--with-foo', default=True, help='foo')
|
||||
'''):
|
||||
|
|
@ -336,7 +344,7 @@ class TestLint(unittest.TestCase):
|
|||
option('--without-foo', default=True, help='foo')
|
||||
'''):
|
||||
self.lint_test()
|
||||
with self.assertRaises(ConfigureError) as e:
|
||||
with self.assertRaisesFromLine(ConfigureError, 2) as e:
|
||||
with self.moz_configure('''
|
||||
option('--without-foo', default=False, help='foo')
|
||||
'''):
|
||||
|
|
@ -354,7 +362,7 @@ class TestLint(unittest.TestCase):
|
|||
help='{Enable|Disable} bar')
|
||||
'''):
|
||||
self.lint_test()
|
||||
with self.assertRaises(ConfigureError) as e:
|
||||
with self.assertRaisesFromLine(ConfigureError, 4) as e:
|
||||
with self.moz_configure('''
|
||||
option(env='FOO', help='foo')
|
||||
option('--enable-bar', default=depends('FOO')(lambda x: bool(x)),
|
||||
|
|
@ -362,8 +370,8 @@ class TestLint(unittest.TestCase):
|
|||
'''):
|
||||
self.lint_test()
|
||||
self.assertEquals(e.exception.message,
|
||||
'--enable-bar has a non-constant default. '
|
||||
'Its help should contain "{Enable|Disable}"')
|
||||
'`help` should contain "{Enable|Disable}" because of '
|
||||
'non-constant default')
|
||||
|
||||
if __name__ == '__main__':
|
||||
main()
|
||||
|
|
|
|||
Загрузка…
Ссылка в новой задаче