Bug 971525 - Optionally make FileCopier only delete symlinked directories it needs to replace. r=gps
authorNick Alexander <nalexander@mozilla.com>
Thu, 13 Feb 2014 22:19:48 -0800
changeset 168747 620d43cffffe94db3a845ca19430af00f845a559
parent 168746 c7802c9d6eec999c64c7e37d4bf195e0df364bf2
child 168748 a2ffdc5a947185fa9fdafafd2a91a855cca7c177
push id26215
push userryanvm@gmail.com
push dateFri, 14 Feb 2014 13:54:11 +0000
treeherdermozilla-central@5d7caa093f4f [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersgps
bugs971525
milestone30.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 971525 - Optionally make FileCopier only delete symlinked directories it needs to replace. r=gps
python/mozbuild/mozbuild/action/process_install_manifest.py
python/mozbuild/mozpack/copier.py
python/mozbuild/mozpack/test/test_copier.py
--- a/python/mozbuild/mozbuild/action/process_install_manifest.py
+++ b/python/mozbuild/mozbuild/action/process_install_manifest.py
@@ -11,43 +11,48 @@ from mozpack.manifests import InstallMan
 
 
 COMPLETE = 'From {dest}: Kept {existing} existing; Added/updated {updated}; ' \
     'Removed {rm_files} files and {rm_dirs} directories.'
 
 
 def process_manifest(destdir, paths,
         remove_unaccounted=True,
+        remove_all_directory_symlinks=True,
         remove_empty_directories=True):
     manifest = InstallManifest()
     for path in paths:
         manifest |= InstallManifest(path=path)
 
     copier = FileCopier()
     manifest.populate_registry(copier)
     return copier.copy(destdir,
         remove_unaccounted=remove_unaccounted,
+        remove_all_directory_symlinks=remove_all_directory_symlinks,
         remove_empty_directories=remove_empty_directories)
 
 
 def main(argv):
     parser = argparse.ArgumentParser(
         description='Process install manifest files.')
 
     parser.add_argument('destdir', help='Destination directory.')
     parser.add_argument('manifests', nargs='+', help='Path to manifest file(s).')
     parser.add_argument('--no-remove', action='store_true',
         help='Do not remove unaccounted files from destination.')
+    parser.add_argument('--no-remove-all-directory-symlinks', action='store_true',
+        help='Do not remove all directory symlinks from destination.')
     parser.add_argument('--no-remove-empty-directories', action='store_true',
         help='Do not remove empty directories from destination.')
 
     args = parser.parse_args(argv)
 
     result = process_manifest(args.destdir, args.manifests,
         remove_unaccounted=not args.no_remove,
+        remove_all_directory_symlinks=not args.no_remove_all_directory_symlinks,
         remove_empty_directories=not args.no_remove_empty_directories)
 
     print(COMPLETE.format(dest=args.destdir,
         existing=result.existing_files_count,
         updated=result.updated_files_count,
         rm_files=result.removed_files_count,
         rm_dirs=result.removed_directories_count))
 
--- a/python/mozbuild/mozpack/copier.py
+++ b/python/mozbuild/mozpack/copier.py
@@ -170,28 +170,45 @@ class FileCopyResult(object):
         return len(self.removed_directories)
 
 
 class FileCopier(FileRegistry):
     '''
     FileRegistry with the ability to copy the registered files to a separate
     directory.
     '''
-    def copy(self, destination, skip_if_older=True, remove_unaccounted=True, remove_empty_directories=True):
+    def copy(self, destination, skip_if_older=True,
+             remove_unaccounted=True,
+             remove_all_directory_symlinks=True,
+             remove_empty_directories=True):
         '''
         Copy all registered files to the given destination path. The given
         destination can be an existing directory, or not exist at all. It
         can't be e.g. a file.
         The copy process acts a bit like rsync: files are not copied when they
         don't need to (see mozpack.files for details on file.copy).
 
-        By default, files in the destination directory that aren't registered
-        are removed and empty directories are deleted. To disable removing of
-        unregistered files, pass remove_unaccounted=False. To disable removing
-        empty directories, pass remove_empty_directories=False.
+        By default, files in the destination directory that aren't
+        registered are removed and empty directories are deleted. In
+        addition, all directory symlinks in the destination directory
+        are deleted: this is a conservative approach to ensure that we
+        never accidently write files into a directory that is not the
+        destination directory. In the worst case, we might have a
+        directory symlink in the object directory to the source
+        directory.
+
+        To disable removing of unregistered files, pass
+        remove_unaccounted=False. To disable removing empty
+        directories, pass remove_empty_directories=False. In rare
+        cases, you might want to maintain directory symlinks in the
+        destination directory (at least those that are not required to
+        be regular directories): pass
+        remove_all_directory_symlinks=False. Exercise caution with
+        this flag: you almost certainly do not want to preserve
+        directory symlinks.
 
         Returns a FileCopyResult that details what changed.
         '''
         assert isinstance(destination, basestring)
         assert not os.path.exists(destination) or os.path.isdir(destination)
 
         result = FileCopyResult()
         have_symlinks = hasattr(os, 'symlink')
@@ -270,19 +287,24 @@ class FileCopier(FileRegistry):
             # doesn't follow symlinks into directories by default, so we need
             # to check dirs (we can't wait for root).
             if have_symlinks:
                 filtered = []
                 for d in dirs:
                     full = os.path.join(root, d)
                     st = os.lstat(full)
                     if stat.S_ISLNK(st.st_mode):
-                        os.remove(full)
-                        # We don't need to recreate it because the code above
-                        # would have created it as necessary.
+                        # This directory symlink is not a required
+                        # directory: any such symlink would have been
+                        # removed and a directory created above.
+                        if remove_all_directory_symlinks:
+                            os.remove(full)
+                            result.removed_files.add(os.path.normpath(full))
+                        else:
+                            existing_files.add(os.path.normpath(full))
                     else:
                         filtered.append(d)
 
                 dirs[:] = filtered
 
             existing_dirs.add(os.path.normpath(root))
 
             for d in dirs:
--- a/python/mozbuild/mozpack/test/test_copier.py
+++ b/python/mozbuild/mozpack/test/test_copier.py
@@ -186,18 +186,19 @@ class TestFileCopier(TestWithTmpDir):
         copier.remove('foo')
         copier.add('test', GeneratedFile('test'))
         result = copier.copy(self.tmpdir)
         self.assertEqual(self.all_files(self.tmpdir), set(copier.paths()))
         self.assertEqual(self.all_dirs(self.tmpdir), set(['qux']))
         self.assertEqual(result.removed_files, set(self.tmppath(p) for p in
             ('foo/bar', 'foo/qux', 'foo/deep/nested/directory/file')))
 
-    def test_symlink_directory(self):
-        """Directory symlinks in destination are deleted."""
+    def test_symlink_directory_replaced(self):
+        """Directory symlinks in destination are replaced if they need to be
+        real directories."""
         if not self.symlink_supported:
             return
 
         dest = self.tmppath('dest')
 
         copier = FileCopier()
         copier.add('foo/bar/baz', GeneratedFile('foobarbaz'))
 
@@ -213,16 +214,85 @@ class TestFileCopier(TestWithTmpDir):
         self.assertFalse(stat.S_ISLNK(st.st_mode))
         self.assertTrue(stat.S_ISDIR(st.st_mode))
 
         self.assertEqual(self.all_files(dest), set(copier.paths()))
 
         self.assertEqual(result.removed_directories, set())
         self.assertEqual(len(result.updated_files), 1)
 
+    def test_remove_unaccounted_directory_symlinks(self):
+        """Directory symlinks in destination that are not in the way are
+        deleted according to remove_unaccounted and
+        remove_all_directory_symlinks.
+        """
+        if not self.symlink_supported:
+            return
+
+        dest = self.tmppath('dest')
+
+        copier = FileCopier()
+        copier.add('foo/bar/baz', GeneratedFile('foobarbaz'))
+
+        os.makedirs(self.tmppath('dest/foo'))
+        dummy = self.tmppath('dummy')
+        os.mkdir(dummy)
+
+        os.mkdir(self.tmppath('dest/zot'))
+        link = self.tmppath('dest/zot/zap')
+        os.symlink(dummy, link)
+
+        # If not remove_unaccounted but remove_empty_directories, then
+        # the symlinked directory remains (as does its containing
+        # directory).
+        result = copier.copy(dest, remove_unaccounted=False,
+            remove_empty_directories=True,
+            remove_all_directory_symlinks=False)
+
+        st = os.lstat(link)
+        self.assertTrue(stat.S_ISLNK(st.st_mode))
+        self.assertFalse(stat.S_ISDIR(st.st_mode))
+
+        self.assertEqual(self.all_files(dest), set(copier.paths()))
+        self.assertEqual(self.all_dirs(dest), set(['foo/bar']))
+
+        self.assertEqual(result.removed_directories, set())
+        self.assertEqual(len(result.updated_files), 1)
+
+        # If remove_unaccounted but not remove_empty_directories, then
+        # only the symlinked directory is removed.
+        result = copier.copy(dest, remove_unaccounted=True,
+            remove_empty_directories=False,
+            remove_all_directory_symlinks=False)
+
+        st = os.lstat(self.tmppath('dest/zot'))
+        self.assertFalse(stat.S_ISLNK(st.st_mode))
+        self.assertTrue(stat.S_ISDIR(st.st_mode))
+
+        self.assertEqual(result.removed_files, set([link]))
+        self.assertEqual(result.removed_directories, set())
+
+        self.assertEqual(self.all_files(dest), set(copier.paths()))
+        self.assertEqual(self.all_dirs(dest), set(['foo/bar', 'zot']))
+
+        # If remove_unaccounted and remove_empty_directories, then
+        # both the symlink and its containing directory are removed.
+        link = self.tmppath('dest/zot/zap')
+        os.symlink(dummy, link)
+
+        result = copier.copy(dest, remove_unaccounted=True,
+            remove_empty_directories=True,
+            remove_all_directory_symlinks=False)
+
+        self.assertEqual(result.removed_files, set([link]))
+        self.assertEqual(result.removed_directories, set([self.tmppath('dest/zot')]))
+
+        self.assertEqual(self.all_files(dest), set(copier.paths()))
+        self.assertEqual(self.all_dirs(dest), set(['foo/bar']))
+
     def test_permissions(self):
         """Ensure files without write permission can be deleted."""
         with open(self.tmppath('dummy'), 'a'):
             pass
 
         p = self.tmppath('no_perms')
         with open(p, 'a'):
             pass