Bug 1147207 - Improve SimplePackager manifest consistency check. r=gps

Bug 910660 added a consistency check that rejects cases where a manifest
inside a directory detected as being the base of an addon is included from
outside the addon. Unfortunately, this triggered false positives when
a manifest is included from within the addon, but just happens to be at
the top-level of that addon.

python/mozbuild/mozpack/packager/__init__.py

@@ -239,7 +239,7 @@ class SimplePackager(object):
         # All manifest paths imported.
         self._manifests = set()
         # All manifest paths included from some other manifest.
-        self._included_manifests = set()
+        self._included_manifests = {}
         self._closed = False
 
     def add(self, path, file):
@@ -280,7 +280,7 @@ class SimplePackager(object):
                 if e.flags:
                     errors.fatal('Flags are not supported on ' +
                                  '"manifest" entries')
-                self._included_manifests.add(e.path)
+                self._included_manifests[e.path] = path
 
     def get_bases(self, addons=True):
         '''
@@ -289,9 +289,14 @@ class SimplePackager(object):
         `addons` indicates whether to include addon bases as well.
         '''
         all_bases = set(mozpath.dirname(m)
-                        for m in self._manifests - self._included_manifests)
+                        for m in self._manifests
+                                 - set(self._included_manifests))
         if not addons:
             all_bases -= self._addons
+        else:
+            # If for some reason some detected addon doesn't have a
+            # non-included manifest.
+            all_bases |= self._addons
         return all_bases
 
     def close(self):
@@ -299,14 +304,16 @@ class SimplePackager(object):
         Push all instructions to the formatter.
         '''
         self._closed = True
-        broken_addons = sorted(m for m in self._included_manifests
-                               if mozpath.dirname(m) in self._addons)
-        if broken_addons:
-            errors.fatal(
-                'Addon base manifest (%s) is included in some other manifest' %
-                ', '.join(broken_addons)
-            )
-        for base in self.get_bases():
+
+        bases = self.get_bases()
+        broken_bases = sorted(
+            m for m, includer in self._included_manifests.iteritems()
+            if mozpath.basedir(m, bases) != mozpath.basedir(includer, bases))
+        for m in broken_bases:
+            errors.fatal('"%s" is included from "%s", which is outside "%s"' %
+                         (m, self._included_manifests[m],
+                          mozpath.basedir(m, bases)))
+        for base in bases:
             if base:
                 self.formatter.add_base(base, base in self._addons)
         self._chrome_queue.execute()

python/mozbuild/mozpack/test/test_packager.py

@@ -211,6 +211,58 @@ class TestSimplePackager(unittest.TestCase):
         self.assertEqual(packager.get_bases(), set(['', 'addon', 'qux']))
         self.assertEqual(packager.get_bases(addons=False), set(['', 'qux']))
 
+    def test_simple_packager_manifest_consistency(self):
+        formatter = MockFormatter()
+        # bar/ is detected as an addon because of install.rdf, but top-level
+        # includes a manifest inside bar/.
+        packager = SimplePackager(formatter)
+        packager.add('base.manifest', GeneratedFile(
+            'manifest foo/bar.manifest\n'
+            'manifest bar/baz.manifest\n'
+        ))
+        packager.add('foo/bar.manifest', GeneratedFile('resource bar bar'))
+        packager.add('bar/baz.manifest', GeneratedFile('resource baz baz'))
+        packager.add('bar/install.rdf', GeneratedFile(''))
+
+        with self.assertRaises(ErrorMessage) as e:
+            packager.close()
+
+        self.assertEqual(e.exception.message,
+            'Error: "bar/baz.manifest" is included from "base.manifest", '
+            'which is outside "bar"')
+
+        # bar/ is detected as a separate base because of chrome.manifest that
+        # is included nowhere, but top-level includes another manifest inside
+        # bar/.
+        packager = SimplePackager(formatter)
+        packager.add('base.manifest', GeneratedFile(
+            'manifest foo/bar.manifest\n'
+            'manifest bar/baz.manifest\n'
+        ))
+        packager.add('foo/bar.manifest', GeneratedFile('resource bar bar'))
+        packager.add('bar/baz.manifest', GeneratedFile('resource baz baz'))
+        packager.add('bar/chrome.manifest', GeneratedFile('resource baz baz'))
+
+        with self.assertRaises(ErrorMessage) as e:
+            packager.close()
+
+        self.assertEqual(e.exception.message,
+            'Error: "bar/baz.manifest" is included from "base.manifest", '
+            'which is outside "bar"')
+
+        # bar/ is detected as a separate base because of chrome.manifest that
+        # is included nowhere, but chrome.manifest includes baz.manifest from
+        # the same directory. This shouldn't error out.
+        packager = SimplePackager(formatter)
+        packager.add('base.manifest', GeneratedFile(
+            'manifest foo/bar.manifest\n'
+        ))
+        packager.add('foo/bar.manifest', GeneratedFile('resource bar bar'))
+        packager.add('bar/baz.manifest', GeneratedFile('resource baz baz'))
+        packager.add('bar/chrome.manifest',
+                     GeneratedFile('manifest baz.manifest'))
+        packager.close()
+
 
 class TestSimpleManifestSink(unittest.TestCase):
     def test_simple_manifest_parser(self):