launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/gina-remove-source-dir into lp:launchpad.

Commit message:
Clean up gina temporary directories more carefully.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/gina-remove-source-dir/+merge/366624

AbstractPackageData.process_package does clean up the whole temporary directory already, but gina didn't do any finer-grained cleanup.  This caused a test failure on bionic, because dpkg-source now refuses to overwrite a directory that already exists.  We now clean up more carefully in unpack_dsc and read_dsc.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/gina-remove-source-dir into lp:launchpad.

=== modified file 'lib/lp/soyuz/scripts/gina/packages.py'
--- lib/lp/soyuz/scripts/gina/packages.py	2018-05-06 08:52:34 +0000
+++ lib/lp/soyuz/scripts/gina/packages.py	2019-04-28 17:02:38 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2018 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2019 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Package information classes.
@@ -105,14 +105,16 @@
 def unpack_dsc(package, version, component, distro_name, archive_root):
     dsc_name, dsc_path, component = get_dsc_path(package, version,
                                                  component, archive_root)
+    version = re.sub("^\d+:", "", version)
+    version = re.sub("-[^-]+$", "", version)
+    source_dir = "%s-%s" % (package, version)
     try:
         extract_dpkg_source(dsc_path, ".", vendor=distro_name)
     except DpkgSourceError as e:
+        if os.path.isdir(source_dir):
+            shutil.rmtree(source_dir)
         raise ExecutionError("Error %d unpacking source" % e.result)
 
-    version = re.sub("^\d+:", "", version)
-    version = re.sub("-[^-]+$", "", version)
-    source_dir = "%s-%s" % (package, version)
     return source_dir, dsc_path
 
 
@@ -120,28 +122,31 @@
     source_dir, dsc_path = unpack_dsc(package, version, component,
                                       distro_name, archive_root)
 
-    dsc = open(dsc_path).read().strip()
+    try:
+        dsc = open(dsc_path).read().strip()
 
-    fullpath = os.path.join(source_dir, "debian", "changelog")
-    changelog = None
-    if os.path.exists(fullpath):
-        changelog = open(fullpath).read().strip()
-    else:
-        log.warn("No changelog file found for %s in %s" %
-                 (package, source_dir))
+        fullpath = os.path.join(source_dir, "debian", "changelog")
         changelog = None
-
-    copyright = None
-    globpath = os.path.join(source_dir, "debian", "*copyright")
-    for fullpath in glob.glob(globpath):
-        if not os.path.exists(fullpath):
-            continue
-        copyright = open(fullpath).read().strip()
-
-    if copyright is None:
-        log.warn(
-            "No copyright file found for %s in %s" % (package, source_dir))
-        copyright = ''
+        if os.path.exists(fullpath):
+            changelog = open(fullpath).read().strip()
+        else:
+            log.warn("No changelog file found for %s in %s" %
+                     (package, source_dir))
+            changelog = None
+
+        copyright = None
+        globpath = os.path.join(source_dir, "debian", "*copyright")
+        for fullpath in glob.glob(globpath):
+            if not os.path.exists(fullpath):
+                continue
+            copyright = open(fullpath).read().strip()
+
+        if copyright is None:
+            log.warn(
+                "No copyright file found for %s in %s" % (package, source_dir))
+            copyright = ''
+    finally:
+        shutil.rmtree(source_dir)
 
     return dsc, changelog, copyright
 

=== modified file 'lib/lp/soyuz/scripts/tests/test_gina.py'
--- lib/lp/soyuz/scripts/tests/test_gina.py	2016-03-20 22:47:48 +0000
+++ lib/lp/soyuz/scripts/tests/test_gina.py	2019-04-28 17:02:38 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2016 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2019 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 from doctest import DocTestSuite
@@ -20,8 +20,8 @@
 from lp.services.database.constants import UTC_NOW
 from lp.services.features.testing import FeatureFixture
 from lp.services.log.logger import DevNullLogger
+from lp.services.osutils import write_file
 from lp.services.tarfile_helpers import LaunchpadWriteTarFile
-from lp.services.osutils import write_file
 from lp.soyuz.enums import PackagePublishingStatus
 from lp.soyuz.scripts.gina import ExecutionError
 from lp.soyuz.scripts.gina.archive import (
@@ -276,8 +276,10 @@
         # Unpacking this in an Ubuntu context fails.
         self.assertRaises(
             ExecutionError, sp_data.do_package, "ubuntu", archive_root)
+        self.assertFalse(os.path.exists("foo-1.0"))
         # But all is well in a Debian context.
         sp_data.do_package("debian", archive_root)
+        self.assertFalse(os.path.exists("foo-1.0"))
 
     def test_process_package_cleans_up_after_unpack_failure(self):
         archive_root = self.useTempDir()