Bug 1147207 - Improve SimplePackager manifest consistency check. r=gps, a=sledru
authorMike Hommey <mh+mozilla@glandium.org>
Thu, 26 Mar 2015 14:56:30 +0900
changeset 258315 46262c24ca5b
parent 258314 fc1e894eec2f
child 258316 7f2d41560360
push id4643
push userryanvm@gmail.com
push date2015-04-07 14:24 +0000
treeherdermozilla-beta@0c29ab096b90 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersgps, sledru
bugs1147207, 910660
milestone38.0
Bug 1147207 - Improve SimplePackager manifest consistency check. r=gps, a=sledru 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
python/mozbuild/mozpack/test/test_packager.py
--- a/python/mozbuild/mozpack/packager/__init__.py
+++ b/python/mozbuild/mozpack/packager/__init__.py
@@ -234,17 +234,17 @@ class SimplePackager(object):
         self._chrome_queue = CallDeque()
         # Queue for formatter.add() calls.
         self._file_queue = CallDeque()
         # All paths containing addons.
         self._addons = set()
         # 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):
         '''
         Add the given BaseFile instance with the given path.
         '''
         assert not self._closed
         if is_manifest(path):
@@ -275,43 +275,50 @@ class SimplePackager(object):
                 self._chrome_queue.append(self.formatter.add_manifest,
                                           e.move(e.base))
             elif not isinstance(e, (Manifest, ManifestInterfaces)):
                 self._queue.append(self.formatter.add_manifest, e.move(e.base))
             if isinstance(e, Manifest):
                 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):
         '''
         Return all paths under which root manifests have been found. Root
         manifests are manifests that are included in no other manifest.
         `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):
         '''
         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()
         self._queue.execute()
         self._file_queue.execute()
 
 
 class SimpleManifestSink(object):
--- a/python/mozbuild/mozpack/test/test_packager.py
+++ b/python/mozbuild/mozpack/test/test_packager.py
@@ -206,16 +206,68 @@ class TestSimplePackager(unittest.TestCa
             (('manifest', 5), 'add', 'foo/bar/foo.html', foo_html),
             (('manifest', 5), 'add', 'foo/bar/bar.html', bar_html),
             (('manifest', 9), 'add', 'addon/install.rdf', install_rdf),
         ])
 
         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):
         formatter = MockFormatter()
         foobar = GeneratedFile('foobar')
         foobaz = GeneratedFile('foobaz')
         fooqux = GeneratedFile('fooqux')
         foozot = GeneratedFile('foozot')