Bug 1058036 - Ensure dependency isn't lost when ORing InstallManifest; r=glandium
authorGregory Szorc <gps@mozilla.com>
Sun, 24 Aug 2014 20:19:55 -0400
changeset 201601 af7832580b3ad586c7db86159eb64e755b1728e1
parent 201600 949cf26ba3d5cb6ed17ba5fb626be29051a099ab
child 201602 0843c26857a9d0534a461ec3fdacfd8b6112167e
push id27375
push userryanvm@gmail.com
push dateTue, 26 Aug 2014 19:56:59 +0000
treeherdermozilla-central@f9bfe115fee5 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersglandium
bugs1058036
milestone34.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 1058036 - Ensure dependency isn't lost when ORing InstallManifest; r=glandium The install manifest processor starts with an empty InstallManifest and uses |= to "concatenate" instances. It became pretty obvious when developing some patches that add more preprocessed files to install manifests that the source install manifest dependency was getting lost during the |= operation. This patch fixes it. The solution is not ideal performance wise. But slightly worse performance (only after config.status, however) is better than clobbers. A test has been added to ensure this doesn't regress.
python/mozbuild/mozpack/manifests.py
python/mozbuild/mozpack/test/test_manifests.py
--- a/python/mozbuild/mozpack/manifests.py
+++ b/python/mozbuild/mozpack/manifests.py
@@ -100,21 +100,21 @@ class InstallManifest(object):
         file path.
 
         If fileobj is defined, the manifest will be populated with data read
         from the specified file object.
 
         Both path and fileobj cannot be defined.
         """
         self._dests = {}
-        self._source_file = None
+        self._source_files = set()
 
         if path or fileobj:
             with _auto_fileobj(path, fileobj, 'rb') as fh:
-                self._source_file = fh.name
+                self._source_files.add(fh.name)
                 self._load_from_fileobj(fh)
 
     def _load_from_fileobj(self, fileobj):
         version = fileobj.readline().rstrip()
         if version not in ('1', '2', '3', '4'):
             raise UnreadableInstallManifest('Unknown manifest version: %s' %
                 version)
 
@@ -175,16 +175,24 @@ class InstallManifest(object):
 
     def __neq__(self, other):
         return not self.__eq__(other)
 
     def __ior__(self, other):
         if not isinstance(other, InstallManifest):
             raise ValueError('Can only | with another instance of InstallManifest.')
 
+        # We must copy source files to ourselves so extra dependencies from
+        # the preprocessor are taken into account. Ideally, we would track
+        # which source file each entry came from. However, this is more
+        # complicated and not yet implemented. The current implementation
+        # will result in over invalidation, possibly leading to performance
+        # loss.
+        self._source_files |= other._source_files
+
         for dest in sorted(other._dests):
             self._add_entry(dest, other._dests[dest])
 
         return self
 
     def _encode_field_entry(self, data):
         """Converts an object into a format that can be stored in the manifest file.
 
@@ -283,19 +291,16 @@ class InstallManifest(object):
             (self.PREPROCESS, source, deps, marker, self._encode_field_entry(defines)))
 
     def _add_entry(self, dest, entry):
         if dest in self._dests:
             raise ValueError('Item already in manifest: %s' % dest)
 
         self._dests[dest] = entry
 
-    def _get_deps(self, dest):
-        return {self._source_file} if self._source_file else set()
-
     def populate_registry(self, registry):
         """Populate a mozpack.copier.FileRegistry instance with data from us.
 
         The caller supplied a FileRegistry instance (or at least something that
         conforms to its interface) and that instance is populated with data
         from this manifest.
         """
         for dest in sorted(self._dests):
@@ -334,14 +339,14 @@ class InstallManifest(object):
 
                 continue
 
             if install_type == self.PREPROCESS:
                 registry.add(dest, PreprocessedFile(entry[1],
                     depfile_path=entry[2],
                     marker=entry[3],
                     defines=self._decode_field_entry(entry[4]),
-                    extra_depends=self._get_deps(dest)))
+                    extra_depends=self._source_files))
 
                 continue
 
             raise Exception('Unknown install type defined in manifest: %d' %
                 install_type)
--- a/python/mozbuild/mozpack/test/test_manifests.py
+++ b/python/mozbuild/mozpack/test/test_manifests.py
@@ -350,10 +350,18 @@ class TestInstallManifest(TestWithTmpDir
         m = InstallManifest(path=manifest)
         c = FileCopier()
         m.populate_registry(c)
         c.copy(dest)
 
         with open(destfile, 'rt') as fh:
             self.assertEqual(fh.read(), 'SOURCE\nINCLUDE MODIFIED\n')
 
+        # ORing an InstallManifest should copy file dependencies
+        m = InstallManifest()
+        m |= InstallManifest(path=manifest)
+        c = FileCopier()
+        m.populate_registry(c)
+        e = c._files['p_dest']
+        self.assertEqual(e.extra_depends, [manifest])
+
 if __name__ == '__main__':
     mozunit.main()