# HG changeset patch # User Mike Hommey # Date 1551218740 0 # Node ID 9d7e38697deec620a5cd00ca4c39912526107909 # Parent c16d8fdcf22a9c23fadac2980f0140292f52aaf1 Bug 1529799 - Handle dependency loops involving imply_option more gracefully. r=chmanchester Currently, when a dependency loop involve imply_option, it is possible to end up with an error message saying the implied option is unknown, when it fact it is. So instead of bailing out with a weird error message, try to make things work (if the implied value is not different from what's known), or bail with a more accurate message. Differential Revision: https://phabricator.services.mozilla.com/D20822 diff --git a/python/mozbuild/mozbuild/configure/__init__.py b/python/mozbuild/mozbuild/configure/__init__.py --- a/python/mozbuild/mozbuild/configure/__init__.py +++ b/python/mozbuild/mozbuild/configure/__init__.py @@ -13,20 +13,18 @@ import sys import types from collections import OrderedDict from contextlib import contextmanager from functools import wraps from mozbuild.configure.options import ( CommandLineHelper, ConflictingOptionError, InvalidOptionError, - NegativeOptionValue, Option, OptionValue, - PositiveOptionValue, ) from mozbuild.configure.help import HelpFormatter from mozbuild.configure.util import ( ConfigureOutputHandler, getpreferredencoding, LineIO, ) from mozbuild.util import ( @@ -441,20 +439,38 @@ class ConfigureSandbox(dict): ) self._value_for(option) # All implied options should exist. for implied_option in self._implied_options: value = self._resolve(implied_option.value) if value is not None: - raise ConfigureError( - '`%s`, emitted from `%s` line %d, is unknown.' - % (implied_option.option, implied_option.caller[1], - implied_option.caller[2])) + # There are two ways to end up here: either the implied option + # is unknown, or it's known but there was a dependency loop + # that prevented the implication from being applied. + option = self._options.get(implied_option.name) + if not option: + raise ConfigureError( + '`%s`, emitted from `%s` line %d, is unknown.' + % (implied_option.option, implied_option.caller[1], + implied_option.caller[2])) + # If the option is known, check that the implied value doesn't + # conflict with what value was attributed to the option. + option_value = self._value_for_option(option) + if value != option_value: + reason = implied_option.reason + if isinstance(reason, Option): + reason = self._raw_options.get(reason) or reason.option + reason = reason.split('=', 1)[0] + value = OptionValue.from_(value) + raise InvalidOptionError( + "'%s' implied by '%s' conflicts with '%s' from the %s" + % (value.format(option.option), reason, + option_value.format(option.option), option_value.origin)) # All options should have been removed (handled) by now. for arg in self._helper: without_value = arg.split('=', 1)[0] msg = 'Unknown option: %s' % without_value if self._help: self._logger.warning(msg) else: @@ -528,30 +544,17 @@ class ConfigureSandbox(dict): if (implied_option.when and not self._value_for(implied_option.when)): continue value = self._resolve(implied_option.value) if value is not None: - if isinstance(value, OptionValue): - pass - elif value is True: - value = PositiveOptionValue() - elif value is False or value == (): - value = NegativeOptionValue() - elif isinstance(value, types.StringTypes): - value = PositiveOptionValue((value,)) - elif isinstance(value, tuple): - value = PositiveOptionValue(value) - else: - raise TypeError("Unexpected type: '%s'" - % type(value).__name__) - + value = OptionValue.from_(value) opt = value.format(implied_option.option) self._helper.add(opt, 'implied') implied[opt] = implied_option try: value, option_string = self._helper.handle(option) except ConflictingOptionError as e: reason = implied[e.arg].reason diff --git a/python/mozbuild/mozbuild/configure/options.py b/python/mozbuild/mozbuild/configure/options.py --- a/python/mozbuild/mozbuild/configure/options.py +++ b/python/mozbuild/mozbuild/configure/options.py @@ -64,29 +64,47 @@ class OptionValue(tuple): raise TypeError('cannot compare a populated %s against an %s; ' 'OptionValue instances are tuples - did you mean to ' 'compare against member elements using [x]?' % ( type(other).__name__, type(self).__name__)) # Allow explicit tuples to be compared. if type(other) == tuple: return tuple.__eq__(self, other) + elif isinstance(other, bool): + return bool(self) == other # Else we're likely an OptionValue class. elif type(other) != type(self): return False else: return super(OptionValue, self).__eq__(other) def __ne__(self, other): return not self.__eq__(other) def __repr__(self): return '%s%s' % (self.__class__.__name__, super(OptionValue, self).__repr__()) + @staticmethod + def from_(value): + if isinstance(value, OptionValue): + return value + elif value is True: + return PositiveOptionValue() + elif value is False or value == (): + return NegativeOptionValue() + elif isinstance(value, types.StringTypes): + return PositiveOptionValue((value,)) + elif isinstance(value, tuple): + return PositiveOptionValue(value) + else: + raise TypeError("Unexpected type: '%s'" + % type(value).__name__) + class PositiveOptionValue(OptionValue): '''Represents the value for a positive option (--enable/--with/--foo) in the form of a tuple for when values are given to the option (in the form --option=value[,value2...]. ''' def __nonzero__(self): return True diff --git a/python/mozbuild/mozbuild/test/configure/test_configure.py b/python/mozbuild/mozbuild/test/configure/test_configure.py --- a/python/mozbuild/mozbuild/test/configure/test_configure.py +++ b/python/mozbuild/mozbuild/test/configure/test_configure.py @@ -690,16 +690,108 @@ class TestConfigure(unittest.TestCase): 'QUX': NegativeOptionValue(), }) config = self.get_config(['--with-foo']) self.assertEquals(config, { 'QUX': PositiveOptionValue(), }) + def test_imply_option_dependency_loop(self): + with self.moz_configure(''' + option('--without-foo', help='foo') + + @depends('--with-foo') + def qux_default(foo): + return bool(foo) + + option('--with-qux', default=qux_default, help='qux') + + imply_option('--with-foo', depends('--with-qux')(lambda x: x or None)) + + set_config('FOO', depends('--with-foo')(lambda x: x)) + set_config('QUX', depends('--with-qux')(lambda x: x)) + '''): + config = self.get_config() + self.assertEquals(config, { + 'FOO': PositiveOptionValue(), + 'QUX': PositiveOptionValue(), + }) + + config = self.get_config(['--without-foo']) + self.assertEquals(config, { + 'FOO': NegativeOptionValue(), + 'QUX': NegativeOptionValue(), + }) + + config = self.get_config(['--with-qux']) + self.assertEquals(config, { + 'FOO': PositiveOptionValue(), + 'QUX': PositiveOptionValue(), + }) + + with self.assertRaises(InvalidOptionError) as e: + config = self.get_config(['--without-foo', '--with-qux']) + + self.assertEquals(e.exception.message, + "'--with-foo' implied by '--with-qux' conflicts " + "with '--without-foo' from the command-line") + + config = self.get_config(['--without-qux']) + self.assertEquals(config, { + 'FOO': PositiveOptionValue(), + 'QUX': NegativeOptionValue(), + }) + + with self.moz_configure(''' + option('--with-foo', help='foo') + + @depends('--with-foo') + def qux_default(foo): + return bool(foo) + + option('--with-qux', default=qux_default, help='qux') + + imply_option('--with-foo', depends('--with-qux')(lambda x: x or None)) + + set_config('FOO', depends('--with-foo')(lambda x: x)) + set_config('QUX', depends('--with-qux')(lambda x: x)) + '''): + config = self.get_config() + self.assertEquals(config, { + 'FOO': NegativeOptionValue(), + 'QUX': NegativeOptionValue(), + }) + + config = self.get_config(['--with-foo']) + self.assertEquals(config, { + 'FOO': PositiveOptionValue(), + 'QUX': PositiveOptionValue(), + }) + + with self.assertRaises(InvalidOptionError) as e: + config = self.get_config(['--with-qux']) + + self.assertEquals(e.exception.message, + "'--with-foo' implied by '--with-qux' conflicts " + "with '--without-foo' from the default") + + with self.assertRaises(InvalidOptionError) as e: + config = self.get_config(['--without-foo', '--with-qux']) + + self.assertEquals(e.exception.message, + "'--with-foo' implied by '--with-qux' conflicts " + "with '--without-foo' from the command-line") + + config = self.get_config(['--without-qux']) + self.assertEquals(config, { + 'FOO': NegativeOptionValue(), + 'QUX': NegativeOptionValue(), + }) + def test_option_failures(self): with self.assertRaises(ConfigureError) as e: with self.moz_configure('option("--with-foo", help="foo")'): self.get_config() self.assertEquals( e.exception.message, 'Option `--with-foo` is not handled ; reference it with a @depends'