Merge lp:~abentley/launchpad/no-write-cwd into lp:launchpad

[Aaron Bentley]

Avoid writing to the to the CWD.

= Summary =
Avoid writing to CWD in the archive uploader.

== Proposed fix ==
Introduce a DSCFile.cleanUp method, so that we can have a temp directory that
lives between unpackAndCheckSource and storeInDatabase.

== Pre-implementation notes ==
Mid-implementation discussion with bigjools

== Implementation details ==
None

== Tests ==
bin/test -t nascent -t archiveuploader

== Demo and Q/A ==
Try doing a sourcepackagerecipe build on staging. It should not die like this:
http://staging.launchpadlibrarian.net/51599092/aRg8XXIxzd43E3UO4chC78epwXK.txt

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/archiveuploader/nascentupload.py
  lib/lp/archiveuploader/uploadprocessor.py
  lib/lp/archiveuploader/dscfile.py
  lib/lp/archiveuploader/tests/test_dscfile.py

/home/abentley/launchpad/no-write-cwd/bin/lint.sh: line 161: pocketlint: command not found

[Julian Edwards (julian-edwards)]

Looks good! Just a couple of small things:

1. change the indentation at line 41 of the diff as we discussed.
2. from lp.archiveuploader.tests import datadir
  then remove the hardcoded /lib/lp/archiveuploaders/tests/data

Thanks for making the change.

review: Approve

[Robert Collins (lifeless)]

Looks like this is in tweak mode.

review: Needs Fixing

[Aaron Bentley (abentley)]

> Looks like this is in tweak mode.

If you're going to say that, please say a bit more.

[Robert Collins (lifeless)]

On Mon, Jul 19, 2010 at 4:38 PM, Aaron Bentley <email address hidden> wrote:
>> Looks like this is in tweak mode.
>
> If you're going to say that, please say a bit more.

Another reviewer has asked for some changes and for the branch to be
landed after that - what I meant by 'it is in tweak mode' is that it
should have its status set accordingly: it no longer needs review,
rather it needs those changes made and the branch to be landed.

[Aaron Bentley (abentley)]

> Another reviewer has asked for some changes and for the branch to be
> landed after that - what I meant by 'it is in tweak mode' is that it
> should have its status set accordingly: it no longer needs review,
> rather it needs those changes made and the branch to be landed.

The convention in Launchpad reviews is that approve reviews may be conditional, based on the text of the review. A subsequent "needs fixing" review like yours creates confusion.

[Robert Collins (lifeless)]

On Mon, Jul 19, 2010 at 5:37 PM, Aaron Bentley <email address hidden> wrote:
>> Another reviewer has asked for some changes and for the branch to be
>> landed after that - what I meant by 'it is in tweak mode' is that it
>> should have its status set accordingly: it no longer needs review,
>> rather it needs those changes made and the branch to be landed.
>
> The convention in Launchpad reviews is that approve reviews may be conditional, based on the text of the review.  A subsequent "needs fixing" review like yours creates confusion.

I'll try to avoid confusion in future. I was perhaps getting a bit
brief - we had a significant number of confusing-status merge
proposals in the queue. Sorry.

=== modified file 'lib/lp/archiveuploader/dscfile.py'
--- lib/lp/archiveuploader/dscfile.py	2010-06-25 21:35:15 +0000
+++ lib/lp/archiveuploader/dscfile.py	2010-07-20 14:43:51 +0000
@@ -13,7 +13,7 @@
     'SignableTagFile',
     'DSCFile',
     'DSCUploadedFile',
-    'findAndMoveChangelog',
+    'findChangelog',
     'findCopyright',
     ]
 
@@ -204,6 +204,8 @@
         else:
             self.processSignature()
 
+        self.unpacked_dir = None
+
     #
     # Useful properties.
     #
@@ -483,18 +485,19 @@
             "Verifying uploaded source package by unpacking it.")
 
         # Get a temporary dir together.
-        tmpdir = tempfile.mkdtemp()
+        self.unpacked_dir = tempfile.mkdtemp()
 
         # chdir into it
         cwd = os.getcwd()
-        os.chdir(tmpdir)
-        dsc_in_tmpdir = os.path.join(tmpdir, self.filename)
+        os.chdir(self.unpacked_dir)
+        dsc_in_tmpdir = os.path.join(self.unpacked_dir, self.filename)
 
         package_files = self.files + [self]
         try:
             for source_file in package_files:
-                os.symlink(source_file.filepath,
-                           os.path.join(tmpdir, source_file.filename))
+                os.symlink(
+                    source_file.filepath,
+                    os.path.join(self.unpacked_dir, source_file.filename))
             args = ["dpkg-source", "-sn", "-x", dsc_in_tmpdir]
             dpkg_source = subprocess.Popen(args, stdout=subprocess.PIPE,
                                            stderr=subprocess.PIPE)
@@ -517,8 +520,9 @@
         # SourcePackageRelease records.
 
         # Check if 'dpkg-source' created only one directory.
-        temp_directories = [dirname for dirname in os.listdir(tmpdir)
-                            if os.path.isdir(dirname)]
+        temp_directories = [
+            dirname for dirname in os.listdir(self.unpacked_dir)
+            if os.path.isdir(dirname)]
         if len(temp_directories) > 1:
             yield UploadError(
                 'Unpacked source contains more than one directory: %r'
@@ -528,31 +532,33 @@
         # name (<sourcename>-<no_epoch(no_revision(version))>).
 
         # Locate both the copyright and changelog files for later processing.
-        for error in findCopyright(self, tmpdir, self.logger):
+        for error in findCopyright(self, self.unpacked_dir, self.logger):
             yield error
 
-        for error in findAndMoveChangelog(self, cwd, tmpdir, self.logger):
+        for error in findChangelog(self, self.unpacked_dir, self.logger):
             yield error
 
         self.logger.debug("Cleaning up source tree.")
+        self.logger.debug("Done")
+
+    def cleanUp(self):
+        if self.unpacked_dir is None:
+            return
         try:
-            shutil.rmtree(tmpdir)
+            shutil.rmtree(self.unpacked_dir)
         except OSError, error:
             # XXX: dsilvers 2006-03-15: We currently lack a test for this.
             if errno.errorcode[error.errno] != 'EACCES':
-                yield UploadError(
+                raise UploadError(
                     "%s: couldn't remove tmp dir %s: code %s" % (
-                    self.filename, tmpdir, error.errno))
+                    self.filename, self.unpacked_dir, error.errno))
             else:
-                yield UploadWarning(
-                    "%s: Couldn't remove tree, fixing up permissions." %
-                    self.filename)
-                result = os.system("chmod -R u+rwx " + tmpdir)
+                result = os.system("chmod -R u+rwx " + self.unpacked_dir)
                 if result != 0:
-                    yield UploadError("chmod failed with %s" % result)
-                shutil.rmtree(tmpdir)
+                    raise UploadError("chmod failed with %s" % result)
+                shutil.rmtree(self.unpacked_dir)
+        self.unpacked_dir = None
 
-        self.logger.debug("Done")
 
     def findBuild(self):
         """Find and return the SourcePackageRecipeBuild, if one is specified.
@@ -733,15 +739,14 @@
     logger.debug("Copying copyright contents.")
     dsc_file.copyright = open(copyright_file).read().strip()
 
-def findAndMoveChangelog(dsc_file, target_dir, source_dir, logger):
+def findChangelog(dsc_file, source_dir, logger):
     """Find and move any debian/changelog.
 
-    This function finds the changelog file within the source package and
-    moves it to target_dir. The changelog file is later uploaded to the 
-    librarian by DSCFile.storeInDatabase().
+    This function finds the changelog file within the source package. The
+    changelog file is later uploaded to the librarian by
+    DSCFile.storeInDatabase().
 
     :param dsc_file: A DSCFile object where the copyright will be stored.
-    :param target_dir: The directory where the changelog will end up.
     :param source_dir: The directory where the source was extracted.
     :param logger: A logger object for debug output.
     """
@@ -756,9 +761,8 @@
         return
 
     # Move the changelog file out of the package direcotry
-    logger.debug("Found changelog contents; moving to root directory")
-    dsc_file.changelog_path = os.path.join(target_dir, "changelog")
-    shutil.move(changelog_file, dsc_file.changelog_path)
+    logger.debug("Found changelog")
+    dsc_file.changelog_path = changelog_file
 
 def check_format_1_0_files(filename, file_type_counts, component_counts,
                            bzip2_count):
 
=== modified file 'lib/lp/archiveuploader/nascentupload.py'
--- lib/lp/archiveuploader/nascentupload.py	2010-05-27 22:18:16 +0000
+++ lib/lp/archiveuploader/nascentupload.py	2010-07-20 14:43:51 +0000
@@ -839,6 +839,12 @@
                 'Exception while accepting:\n %s' % e, exc_info=True)
             self.do_reject(notify)
             return False
+        else:
+            self.cleanUp()
+
+    def cleanUp(self):
+        if self.changes.dsc is not None:
+            self.changes.dsc.cleanUp()
 
     def do_reject(self, notify=True):
         """Reject the current upload given the reason provided."""
@@ -869,6 +875,7 @@
         self.queue_root.notify(summary_text=self.rejection_message,
             changes_file_object=changes_file_object, logger=self.logger)
         changes_file_object.close()
+        self.cleanUp()
 
     def _createQueueEntry(self):
         """Return a PackageUpload object."""
 
=== modified file 'lib/lp/archiveuploader/tests/test_dscfile.py'
--- lib/lp/archiveuploader/tests/test_dscfile.py	2010-07-18 00:26:33 +0000
+++ lib/lp/archiveuploader/tests/test_dscfile.py	2010-07-20 14:43:51 +0000
@@ -7,11 +7,14 @@
 
 import os
 
+from canonical.config import config
+from canonical.launchpad.scripts.logger import QuietFakeLogger
 from canonical.testing.layers import LaunchpadZopelessLayer
 from lp.archiveuploader.dscfile import (
-    findAndMoveChangelog, findCopyright)
+    DSCFile, findChangelog, findCopyright)
 from lp.archiveuploader.nascentuploadfile import UploadError
-from lp.archiveuploader.tests import mock_logger_quiet
+from lp.archiveuploader.tests import datadir, mock_logger_quiet
+from lp.archiveuploader.uploadpolicy import BuildDaemonUploadPolicy
 from lp.testing import TestCase, TestCaseWithFactory
 
 
@@ -27,7 +30,6 @@
         os.makedirs(self.dir_path)
         self.copyright_path = os.path.join(self.dir_path, "copyright")
         self.changelog_path = os.path.join(self.dir_path, "changelog")
-        self.changelog_dest = os.path.join(self.tmpdir, "changelog")
         self.dsc_file = self.MockDSCFile()
 
     def testBadDebianCopyright(self):
@@ -66,8 +68,8 @@
         dangling symlink in an attempt to try and access files on the system
         processing the source packages."""
         os.symlink("/etc/passwd", self.changelog_path)
-        errors = list(findAndMoveChangelog(
-            self.dsc_file, self.tmpdir, self.tmpdir, mock_logger_quiet))
+        errors = list(findChangelog(
+            self.dsc_file, self.tmpdir, mock_logger_quiet))
 
         self.assertEqual(len(errors), 1)
         self.assertIsInstance(errors[0], UploadError)
@@ -82,12 +84,12 @@
         file.write(changelog)
         file.close()
 
-        errors = list(findAndMoveChangelog(
-            self.dsc_file, self.tmpdir, self.tmpdir, mock_logger_quiet))
+        errors = list(findChangelog(
+            self.dsc_file, self.tmpdir, mock_logger_quiet))
 
         self.assertEqual(len(errors), 0)
         self.assertEqual(self.dsc_file.changelog_path,
-                         self.changelog_dest)
+                         self.changelog_path)
 
     def testOversizedFile(self):
         """Test that a file larger than 10MiB will fail.
@@ -106,8 +108,8 @@
         file.write(empty_file)
         file.close()
 
-        errors = list(findAndMoveChangelog(
-            self.dsc_file, self.tmpdir, self.tmpdir, mock_logger_quiet))
+        errors = list(findChangelog(
+            self.dsc_file, self.tmpdir, mock_logger_quiet))
 
         self.assertIsInstance(errors[0], UploadError)
         self.assertEqual(
@@ -120,12 +122,27 @@
 
     layer = LaunchpadZopelessLayer
 
+    def getDscFile(self, name):
+        dsc_path = datadir(os.path.join('suite', name, name + '.dsc'))
+        class Changes:
+            architectures = ['source']
+        logger = QuietFakeLogger()
+        policy = BuildDaemonUploadPolicy()
+        policy.distroseries = self.factory.makeDistroSeries()
+        policy.archive = self.factory.makeArchive()
+        policy.distro = policy.distroseries.distribution
+        return DSCFile(dsc_path, 'digest', 0, 'main/editors',
+            'priority', 'package', 'version', Changes, policy, logger)
+
     def test_ReadOnlyCWD(self):
         """Processing a file should work when cwd is read-only."""
         tempdir = self.useTempDir()
-        dsc_file = self.factory.makeDscFile(tempdir)
         os.chmod(tempdir, 0555)
         try:
-            list(dsc_file.unpackAndCheckSource())
+            dsc_file = self.getDscFile('bar_1.0-1')
+            try:
+                list(dsc_file.verify())
+            finally:
+                dsc_file.cleanUp()
         finally:
             os.chmod(tempdir, 0755)
 
=== modified file 'lib/lp/archiveuploader/uploadprocessor.py'
--- lib/lp/archiveuploader/uploadprocessor.py	2010-06-29 14:01:01 +0000
+++ lib/lp/archiveuploader/uploadprocessor.py	2010-07-20 14:43:51 +0000
@@ -293,7 +293,7 @@
             (distribution, suite_name,
              archive) = parse_upload_path(relative_path)
         except UploadPathError, e:
-            # pick some defaults to create the NascentUploap() object.
+            # pick some defaults to create the NascentUpload() object.
             # We will be rejecting the upload so it doesn matter much.
             distribution = getUtility(IDistributionSet)['ubuntu']
             suite_name = None