Bug 971525 - Optionally make FileCopier only delete symlinked directories it needs to replace. r=gps

python/mozbuild/mozbuild/action/process_install_manifest.py

@@ -16,6 +16,7 @@ COMPLETE = 'From {dest}: Kept {existing} existing; Added/updated {updated}; ' \
 
 def process_manifest(destdir, paths,
         remove_unaccounted=True,
+        remove_all_directory_symlinks=True,
         remove_empty_directories=True):
     manifest = InstallManifest()
     for path in paths:
@@ -25,6 +26,7 @@ def process_manifest(destdir, paths,
     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)
 
 
@@ -36,6 +38,8 @@ def main(argv):
     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.')
 
@@ -43,6 +47,7 @@ def main(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,

python/mozbuild/mozpack/copier.py

@@ -175,7 +175,10 @@ 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
@@ -183,10 +186,24 @@ class FileCopier(FileRegistry):
         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.
         '''
@@ -275,9 +292,14 @@ class FileCopier(FileRegistry):
                     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)
 

python/mozbuild/mozpack/test/test_copier.py

@@ -191,8 +191,9 @@ class TestFileCopier(TestWithTmpDir):
         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
 
@@ -218,6 +219,75 @@ class TestFileCopier(TestWithTmpDir):
         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'):