Bug 1322025 - Don't wrap the combination function in CombinedDependsFunction. r=chmanchester, a=IanN CLOSED TREE DONTBUILD SEAMONKEY_2_49_ESR_RELBRANCH
authorMike Hommey <mh+mozilla@glandium.org>
Wed, 25 Jan 2017 16:50:29 +0900
changeset 357448 01c6332e22b0ec6858748f8f4a5ffa4e476bd413
parent 357447 4ba744632d51076e2a0d8324fa8a590dcfb5441f
child 357449 e7762fd623b92c2256cf0081860277d9ba920d2b
push id7834
push userfrgrahl@gmx.net
push dateSun, 13 Jan 2019 12:17:02 +0000
treeherdermozilla-esr52@6e4ad8a8f2e8 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerschmanchester, IanN
Bug 1322025 - Don't wrap the combination function in CombinedDependsFunction. r=chmanchester, a=IanN CLOSED TREE DONTBUILD mozilla-esr52 SEAMONKEY_2_49_ESR_RELBRANCH Several things were wrong with the wrapping: - the equality test on functions was actually comparing the memoized functions, which have a type memoize, which inherits from dict. So they weren't comparing actual functions, but the dict used to store the cache of their invocation. - each CombinedDependsFunction created for the same combination function used a different wrapped function, so even if the dict problem wasn't there, the equality test still wouldn't work, except if the function wrapping itself was memoized. - the memoization was not particularly useful. Also, for upcoming changes, we'd actually like the combination function to take an iterable instead of a variable argument list, so that items of the iterable can be skipped.
--- a/python/mozbuild/mozbuild/configure/__init__.py
+++ b/python/mozbuild/mozbuild/configure/__init__.py
@@ -103,54 +103,49 @@ class DependsFunction(object):
             ', '.join(repr(d) for d in self.dependencies),
 class CombinedDependsFunction(DependsFunction):
     def __init__(self, sandbox, func, dependencies):
-        @memoize
-        @wraps(func)
-        def wrapper(*args):
-            return func(args)
         flatten_deps = []
         for d in dependencies:
-            if isinstance(d, CombinedDependsFunction) and d._func == wrapper:
+            if isinstance(d, CombinedDependsFunction) and d._func is func:
                 for d2 in d.dependencies:
                     if d2 not in flatten_deps:
             elif d not in flatten_deps:
         # Automatically add a --help dependency if one of the dependencies
         # depends on it.
         for d in flatten_deps:
             if (isinstance(d, DependsFunction) and
                 sandbox._help_option in d.dependencies):
                 flatten_deps.insert(0, sandbox._help_option)
         super(CombinedDependsFunction, self).__init__(
-            sandbox, wrapper, flatten_deps)
+            sandbox, func, flatten_deps)
     def result(self, need_help_dependency=False):
         # Ignore --help for the combined result
         deps = self.dependencies
         if deps[0] == self.sandbox._help_option:
             deps = deps[1:]
         resolved_args = [self.sandbox._value_for(d, need_help_dependency)
                          for d in deps]
-        return self._func(*resolved_args)
+        return self._func(resolved_args)
     def __eq__(self, other):
         return (isinstance(other, self.__class__) and
-                self._func == other._func and
+                self._func is other._func and
                 set(self.dependencies) == set(other.dependencies))
     def __ne__(self, other):
         return not self == other
 class SandboxedGlobal(dict):
     '''Identifiable dict type for use as function global'''
--- a/python/mozbuild/mozbuild/test/configure/test_configure.py
+++ b/python/mozbuild/mozbuild/test/configure/test_configure.py
@@ -867,16 +867,35 @@ class TestConfigure(unittest.TestCase):
             with self.assertRaises(ConfigureError) as e:
                               '@depends function needs the same `when` as '
                               'options it depends on')
+        with self.moz_configure('''
+            @depends(when=True)
+            def always():
+                return True
+            @depends(when=True)
+            def always2():
+                return True
+            with only_when(always2):
+                option('--with-foo', help='foo', when=always)
+                # include() triggers resolution of its dependencies, and their
+                # side effects.
+                include(depends('--with-foo', when=always)(lambda x: x))
+                # The sandbox should figure that the `when` here is
+                # appropriate. Bad behavior in CombinedDependsFunction.__eq__
+                # made this fail in the past.
+                set_config('FOO', depends('--with-foo', when=always)(lambda x: x))
+        '''):
+            self.get_config()
     def test_include_failures(self):
         with self.assertRaises(ConfigureError) as e:
             with self.moz_configure('include("../foo.configure")'):
             'Cannot include `%s` because it is not in a subdirectory of `%s`'