Bug 1529799 - Handle dependency loops involving imply_option more gracefully. r=chmanchester
authorMike Hommey <mh+mozilla@glandium.org>
Tue, 26 Feb 2019 22:05:40 +0000
changeset 519141 9d7e38697deec620a5cd00ca4c39912526107909
parent 519140 948362bfcaf31849df085dde4967379d4d50a8bc
child 519142 69f758a41070f572edf341089336bc1d4c3168a8
push id10862
push userffxbld-merge
push dateMon, 11 Mar 2019 13:01:11 +0000
treeherdermozilla-beta@a2e7f5c935da [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerschmanchester
bugs1529799
milestone67.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
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
python/mozbuild/mozbuild/configure/__init__.py
python/mozbuild/mozbuild/configure/options.py
python/mozbuild/mozbuild/test/configure/test_configure.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:
@@ -529,30 +545,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
--- 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
--- 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'