Improve error reporting in aliBuild clean (#814)

* Improve error reporting in aliBuild clean

1. Report the actual error when cleanup fails.
2. Keep going when one directory can't be deleted. This should stop one
   failing dir from completely blocking all cleanup in CI.
3. Use logging to show messages, to match the rest of aliBuild.
4. Fix decideClean docstring to match its code.
5. Adjust tests to match.

* Test that no files are deleted if none exist

* Test that all directories are attempted to be deleted

...even if some fail.

* Avoid repetition in doClean unittest

alibuild_helpers/clean.py

@@ -1,5 +1,3 @@
-from __future__ import print_function
-
 # Import as function if they do not have any side effects
 from os.path import dirname, basename
 
@@ -9,15 +7,33 @@
 import glob
 import sys
 import shutil
+from alibuild_helpers import log
 
-def print_results(x):
-  print(x)
 
 def decideClean(workDir, architecture, aggressiveCleanup):
-  """ Decides what to delete, without actually doing it:
-      - Find all the symlinks in "BUILD"
-      - Find all the directories in "BUILD"
-      - Schedule a directory for deletion if it does not have a symlink
+  """Decide what to delete, without actually doing it.
+
+  To clean up obsolete build directories:
+  - Find all the symlinks in "BUILD"
+  - Find all the directories in "BUILD"
+  - Schedule a directory for deletion if it does not have a symlink
+
+  Installed packages are deleted from the final installation directory
+  according to the above scheme as well.
+
+  The temporary directory and temporary install roots are always cleaned up.
+
+  In aggressive mode, the following are also cleaned up:
+
+  - Tarballs (but not their symlinks), since these are expected to either be
+    unpacked in the installation directory, available from the remote store
+    for download, or not needed any more if their installation directory is
+    gone.
+  - Git checkouts for specific tags, since we expect to be able to rebuild
+    those easily from the mirror directory.
+
+  In the case of installed packages and tarballs, only those for the given
+  architecture are considered for deletion.
   """
   symlinksBuild = [os.readlink(x) for x in glob.glob("%s/BUILD/*-latest*" % workDir)]
   # $WORK_DIR/TMP should always be cleaned up. This does not happen only
@@ -42,24 +58,26 @@ def decideClean(workDir, architecture, aggressiveCleanup):
   toDelete = [x for x in toDelete if path.exists(x)]
   return toDelete
 
+
 def doClean(workDir, architecture, aggressiveCleanup, dryRun):
   """ CLI API to cleanup build area """
   toDelete = decideClean(workDir, architecture, aggressiveCleanup)
   if not toDelete:
-    print_results("Nothing to delete.")
+    log.info("Nothing to delete.")
     sys.exit(0)
-  finalMessage = "This will delete the following directories:\n\n" + "\n".join(toDelete)
 
+  log.banner("This %s delete the following directories:\n%s",
+             "would" if dryRun else "will", "\n".join(toDelete))
   if dryRun:
-    finalMessage += "\n\n--dry-run / -n specified. Doing nothing."
-    print_results(finalMessage)
+    log.info("--dry-run / -n specified. Doing nothing.")
     sys.exit(0)
 
-  print_results(finalMessage)
-  for x in toDelete:
+  have_error = False
+  for directory in toDelete:
     try:
-      shutil.rmtree(x)
-    except OSError:
-      print_results("Unable to delete %s." % x)
-      sys.exit(1)
-  sys.exit(0)
+      shutil.rmtree(directory)
+    except OSError as exc:
+      have_error = True
+      log.error("Unable to delete %s:", directory, exc_info=exc)
+
+  sys.exit(1 if have_error else 0)

tests/test_clean.py

@@ -53,6 +53,7 @@
   "sw/BUILD/b-latest": "fcdfc2e1c9f0433c60b3b000e0e2737d297a9b1c"
 }
 
+
 class CleanTestCase(unittest.TestCase):
     @patch('alibuild_helpers.clean.glob')
     @patch('alibuild_helpers.clean.os')
@@ -80,50 +81,78 @@ def test_decideClean(self, mock_path, mock_os, mock_glob):
     @patch('alibuild_helpers.clean.os')
     @patch('alibuild_helpers.clean.path')
     @patch('alibuild_helpers.clean.shutil')
-    @patch('alibuild_helpers.clean.print_results')
-    def test_doClean(self, mock_print_results,  mock_shutil, mock_path, mock_os, mock_glob):
-        mock_path.realpath.side_effect = lambda x : REALPATH_WITH_OBSOLETE_FILES[x]
-        mock_path.islink.side_effect = lambda x : "latest" in x
-        mock_glob.glob.side_effect = lambda x : GLOB_WITH_OBSOLETE_FILES[x]
-        mock_os.readlink.side_effect = lambda x : READLINK_MOCKUP_DB[x]
+    @patch('alibuild_helpers.clean.log')
+    def test_doClean(self, mock_log, mock_shutil, mock_path, mock_os, mock_glob):
+        mock_path.realpath.side_effect = lambda x: REALPATH_WITH_OBSOLETE_FILES[x]
+        mock_path.islink.side_effect = lambda x: "latest" in x
+        mock_os.readlink.side_effect = lambda x: READLINK_MOCKUP_DB[x]
+
+        files_to_delete = [
+            "sw/TMP",
+            "sw/INSTALLROOT",
+            "sw/TARS/osx_x86-64/store",
+            "sw/SOURCES",
+            "sw/BUILD/somethingtodelete",
+            "sw/osx_x86-64/b/v1",
+            "sw/osx_x86-64/b/v3",
+        ]
+        remove_files_calls = list(map(call, files_to_delete))
+        files_delete_formatarg = "\n".join(files_to_delete)
+
+        mock_glob.glob.side_effect = lambda x: []
+        # To get rid of default entries like sw/TMP, sw/INSTALLROOT.
+        mock_path.exists.return_value = False
+
         with self.assertRaises(SystemExit) as cm:
           doClean(workDir="sw", architecture="osx_x86-64", aggressiveCleanup=True, dryRun=True)
         self.assertEqual(cm.exception.code, 0)
         mock_shutil.rmtree.assert_not_called()
-        mock_print_results.assert_called_with(dedent("""\
-        This will delete the following directories:
+        mock_log.info.assert_called_with("Nothing to delete.")
+        mock_log.banner.assert_not_called()
 
-        sw/TMP
-        sw/INSTALLROOT
-        sw/TARS/osx_x86-64/store
-        sw/SOURCES
-        sw/BUILD/somethingtodelete
-        sw/osx_x86-64/b/v1
-        sw/osx_x86-64/b/v3
+        mock_log.banner.reset_mock()
+        mock_log.info.reset_mock()
 
-        --dry-run / -n specified. Doing nothing."""))
+        mock_glob.glob.side_effect = lambda x: GLOB_WITH_OBSOLETE_FILES[x]
+        mock_path.exists.return_value = True
+
+        with self.assertRaises(SystemExit) as cm:
+          doClean(workDir="sw", architecture="osx_x86-64", aggressiveCleanup=True, dryRun=True)
+        self.assertEqual(cm.exception.code, 0)
+        mock_shutil.rmtree.assert_not_called()
+        mock_log.banner.assert_called_with("This %s delete the following directories:\n%s",
+                                           "would", files_delete_formatarg)
+        mock_log.info.assert_called_with("--dry-run / -n specified. Doing nothing.")
+
+        mock_log.banner.reset_mock()
+        mock_log.info.reset_mock()
 
         with self.assertRaises(SystemExit) as cm:
           doClean(workDir="sw", architecture="osx_x86-64", aggressiveCleanup=True, dryRun=False)
         self.assertEqual(cm.exception.code, 0)
-        remove_files_calls = [call('sw/TMP'),
-                              call('sw/INSTALLROOT'),
-                              call('sw/TARS/osx_x86-64/store'),
-                              call('sw/SOURCES'),
-                              call('sw/BUILD/somethingtodelete'),
-                              call('sw/osx_x86-64/b/v1'),
-                              call('sw/osx_x86-64/b/v3')]
         self.assertEqual(mock_shutil.rmtree.mock_calls, remove_files_calls)
-        mock_print_results.assert_called_with(dedent("""\
-        This will delete the following directories:
-
-        sw/TMP
-        sw/INSTALLROOT
-        sw/TARS/osx_x86-64/store
-        sw/SOURCES
-        sw/BUILD/somethingtodelete
-        sw/osx_x86-64/b/v1
-        sw/osx_x86-64/b/v3"""))
+        mock_log.banner.assert_called_with("This %s delete the following directories:\n%s",
+                                           "will", files_delete_formatarg)
+        mock_log.info.assert_not_called()
+
+        mock_log.banner.reset_mock()
+        mock_log.info.reset_mock()
+        mock_shutil.rmtree.reset_mock()
+
+        def failing_rmtree(directory):
+            raise OSError("sentinel exception")
+
+        mock_shutil.rmtree.side_effect = failing_rmtree
+
+        with self.assertRaises(SystemExit) as cm:
+          doClean(workDir="sw", architecture="osx_x86-64", aggressiveCleanup=True, dryRun=False)
+        self.assertEqual(cm.exception.code, 1)
+        # Make sure the function still attempts to delete all of the
+        # directories, even if some fail.
+        self.assertEqual(mock_shutil.rmtree.mock_calls, remove_files_calls)
+        mock_log.banner.assert_called_with("This %s delete the following directories:\n%s",
+                                           "will", files_delete_formatarg)
+        mock_log.info.assert_not_called()
 
 
 if __name__ == '__main__':