Merge lp:~jml/pkgme-service/directory-already-exists into lp:pkgme-service

[Jonathan Lange]

Handle empty package_names

When submitting a PDF to pkgme-service on production, we get the following error:

Traceback (most recent call last):
  File "/usr/lib/pymodules/python2.6/celery/execute/trace.py", line 47, in trace
    return cls(states.SUCCESS, retval=fun(*args, **kwargs))
  File "/usr/lib/pymodules/python2.6/celery/app/task/__init__.py", line 247, in __call__
    return self.run(*args, **kwargs)
  File "/srv/pkgme-service.canonical.com/production/pkgme-service/sourcecode/../src/djpkgme/tasks.py", line 246, in run
    packaged_app_path = self.build_package(metadata, overrides)
  File "/srv/pkgme-service.canonical.com/production/pkgme-service/sourcecode/../src/djpkgme/tasks.py", line 223, in build_package
    download_path, output_dir, metadata['package_name'])
  File "/srv/pkgme-service.canonical.com/production/pkgme-service/sourcecode/../src/djpkgme/tasks.py", line 156, in prepare_file
    os.makedirs(extraction_path)
  File "/usr/lib/python2.6/os.py", line 157, in makedirs
    mkdir(name, mode)
OSError: [Errno 17] File exists

The error is caused by 'package_name' being an empty string. I've attacked this problem from two sides:
 1. Rely on submitted data from MyApps as little as possible. Especially, do not rely on submitted package_name.
 2. Structure our directory & file creation to avoid name clashes where possible.

The old 'extraction_path' metaphor is gone, replaced with the idea of 'working_path'. i.e. the place where pkgme does its work. It's created in a hard-coded location (WORKING_DIRECTORY_NAME) and then renamed after the package_name once we've figured out what that is.

To avoid clashes, the downloaded file is stored in its own directory, DOWNLOAD_DIRECTORY_NAME. This directory is deleted after the source package is built, in order to guarantee that it doesn't appear in our output tarball. This fixes a previously unknown bug, I think.

In addition, I've documented what's going on with the directories, and what keys this part of the chain actually expects.

[James Westby (james-w)]

Hi,

This looks good, and is much clearer.

99 + # | \-- devportal-metadata.json (moved out before packaging built)

I "packaging built" means the source package being put in to a tarball? It
may be confused with running pkgme and building the source package, so maybe
"moved out of the way before output tarball is created"?

Thanks,

James

review: Approve

[Jonathan Lange (jml)]

On Mon, Feb 27, 2012 at 3:00 PM, James Westby <email address hidden> wrote:
> Review: Approve
>
> Hi,
>
> This looks good, and is much clearer.
>
> 99      + # | \-- devportal-metadata.json (moved out before packaging built)
>
> I "packaging built" means the source package being put in to a tarball? It
> may be confused with running pkgme and building the source package, so maybe
> "moved out of the way before output tarball is created"?
>

Actually it means 'after pkgme has made the debian/ dir but before it
actually builds the source package'. Will tweak and land.

jml

=== modified file 'src/djpkgme/tasks.py'
--- src/djpkgme/tasks.py	2012-02-25 02:27:54 +0000
+++ src/djpkgme/tasks.py	2012-02-27 15:19:28 +0000
@@ -99,7 +99,7 @@
     """
     return {
         MetadataBackend.CATEGORIES: metadata.get('categories', ''),
-        MetadataBackend.PACKAGE_NAME: metadata['package_name'],
+        MetadataBackend.PACKAGE_NAME: metadata.get('package_name', ''),
         MetadataBackend.TAGLINE: metadata.get('tagline', ''),
         }
 
@@ -149,31 +149,53 @@
         PkgmeError,
     )
 
-
-    def prepare_file(self, path, output_dir, package_name):
-        extraction_path = os.path.join(output_dir, package_name)
+    # The name of the directory that we store the downloaded file in.
+    DOWNLOAD_DIRECTORY_NAME = 'download'
+
+    # The name of the directory that we create inside of the temporary build
+    # directory that pkgme does all of its work on.
+    WORKING_DIRECTORY_NAME = 'working'
+
+    def prepare_file(self, path, output_dir):
+        """Get 'path' ready for automated packaging inside 'output_dir'.
+
+        If 'path' is a tarball, then preparing it means extracting it into a
+        new directory inside 'output_dir'.
+
+        If 'path' is a single file, like a PDF, then preparing it means
+        copying it into a new directory inside 'output_dir'.
+
+        Either way, the key thing is that there is a new directory inside
+        'output_dir', inside which is the actual contents we want to try to
+        automatically package.
+
+        :param path: The path where the uploaded file lives.  Will be a
+            tarball, PDF or something like that.
+        :param output_dir: The directory in which to put the prepared version.
+        :return: A path to a newly-created directory.
+        """
+        working_path = os.path.join(output_dir, self.WORKING_DIRECTORY_NAME)
         try:
             t = tarfile.open(path)
         except tarfile.ReadError:
-            os.makedirs(extraction_path)
-            shutil.copy(path, extraction_path)
+            os.makedirs(working_path)
+            shutil.copy(path, working_path)
         else:
             try:
-                t.extractall(extraction_path)
+                t.extractall(working_path)
             finally:
                 t.close()
             os.unlink(path)
-        return extraction_path
+        return working_path
 
-    def write_metadata(self, metadata, extraction_path):
+    def write_metadata(self, metadata, working_path):
         """Write metadata in a way that the binary backend expects.
 
         :param metadata: Metadata from the client as a dictionary.
-        :param extraction_path: The directory to store the metadata JSON file
-            in.
+        :param working_path: The directory to store the metadata JSON file in.
         """
         metadata_filename = os.path.join(
-            extraction_path, MetadataBackend.METADATA_FILE)
+            working_path, MetadataBackend.METADATA_FILE)
         with open(metadata_filename, 'w') as metadata_file:
             json.dump(as_pkgme_dict(metadata), metadata_file)
         return metadata_filename
@@ -214,33 +236,82 @@
         return destination
 
     def build_package(self, metadata, overrides):
-        """Build packaging for 'metadata' and store it all in 'output_tar_path'.
+        """Build packaging for 'metadata'.
 
-        :return: The URL that the build source package can be downloaded from.
+        :return: A path on disk to a tarball containing the built source
+            package.
         """
+        # This does a lot of moving files and directories around. A brief
+        # summary might help:
+        #
+        # $output_dir (a temporary directory created by this method)
+        # |
+        # +-+ 'download' (removed after packaging is built)
+        # | \-- <file downloaded from client>
+        # |
+        # +-+ 'working' (removed after packaging is built)
+        # | \-- <extracted contents of downloaded file>
+        # | \-- devportal-metadata.json (moved out after debian/ directory
+        # |                              created but before source package
+        # |                              is built)
+        # +-- $package_name_0.dsc
+        # +-- $package_name_0.tar.gz
+        # +-- $package_name_0_source.build
+        # +-- $package_name_0_source.changes
+        # +-- devportal-metadata.json (moved here source package is built)
+        #
+        # The contents of $output_dir (i.e. the source package files and
+        # devportal-metadata.json) are tarred up and stored in a preconfigured
+        # location: PKGME_OUTPUT_DIRECTORY.
         with TempDir() as temp_dir:
             output_dir = temp_dir.path
-            download_path = download_file(metadata['package_url'], output_dir)
-            extraction_path = self.prepare_file(
-                download_path, output_dir, metadata['package_name'])
-            metadata_filename = self.write_metadata(metadata, extraction_path)
+            download_dir = os.path.join(
+                output_dir, self.DOWNLOAD_DIRECTORY_NAME)
+            os.mkdir(download_dir)
+            download_path = download_file(
+                metadata['package_url'], download_dir)
+            working_path = self.prepare_file(download_path, output_dir)
+            metadata_filename = self.write_metadata(metadata, working_path)
             # Run pkgme: generate the packaging and build a source package.
             write_packaging(
-                extraction_path, allowed_backend_names=ALLOWED_BACKEND_NAMES)
+                working_path, allowed_backend_names=ALLOWED_BACKEND_NAMES)
+            # XXX: This duplicates work done by write_packaging. However, we
+            # need the package_name in order to create the tarball in a way
+            # that looks good.
+            package_name = MetadataBackend(
+                working_path).get_package_name(metadata)
             # Move the metadata file out of the way, so it doesn't appear in
             # the source package.
             os.rename(
                 metadata_filename,
                 os.path.join(output_dir, MetadataBackend.METADATA_FILE))
-            build_source_package(extraction_path, False)
+            # Make sure the directory is named after the package, rather than
+            # a hard-coded string.  This means that when the tarball is
+            # extracted it creates a meaningful directory.
+            new_working_path = os.path.join(output_dir, package_name)
+            os.rename(working_path, new_working_path)
+            build_source_package(new_working_path, False)
             # Create a new tarball containing the source package and the
-            # metadata file.
-            shutil.rmtree(extraction_path)
+            # metadata file, but not the pkgme working set.
+            shutil.rmtree(download_dir)
+            shutil.rmtree(new_working_path)
             return self.save_package(
-                output_dir, settings.PKGME_OUTPUT_DIRECTORY,
-                metadata['package_name'])
+                output_dir, settings.PKGME_OUTPUT_DIRECTORY, package_name)
 
     def run(self, metadata, overrides):
+        """Build a source package.
+
+        Requires 'metadata' to have:
+         * callback_url
+         * package_url
+
+        Uses these if they exist:
+         * errback_url
+         * myapps_id
+
+        Assumes that the backend can determine a non-empty value for
+        'package_name'.
+        """
         self.get_logger().info(
             "Building package for %s" % (
                 metadata.get('myapps_id', '<unknown myapps id>'),))

=== modified file 'src/djpkgme/tests/test_tasks.py'
--- src/djpkgme/tests/test_tasks.py	2012-02-23 12:12:33 +0000
+++ src/djpkgme/tests/test_tasks.py	2012-02-27 15:19:28 +0000
@@ -193,9 +193,14 @@
         metadata = self.factory.make_metadata()
         package_name = metadata['package_name']
         task = BuildPackageTask()
-        self.patch(
-            tasks, 'download_file',
-            lambda url, output_dir: self.factory.make_tarball())
+        def download_file(url, output_dir):
+            # Create a tarball and stick it in 'output_dir'.
+            tarball_location = self.factory.make_tarball()
+            proper_location = os.path.join(
+                output_dir, os.path.basename(tarball_location))
+            os.rename(tarball_location, proper_location)
+            return proper_location
+        self.patch(tasks, 'download_file', download_file)
         pkg_db = self.useFixture(DatabaseFixture())
         self.factory.add_dependencies_for_test_data(pkg_db)
         temp_dir = self.useFixture(TempDir()).path
@@ -243,10 +248,11 @@
         path = self.factory.make_tarball()
         tarball = tarfile.open(path)
         top_level_dir = tarball.getnames()[0]
-        package_name = 'apackage'
         tempdir = self.useFixture(TempDir()).path
-        new_path = task.prepare_file(path, tempdir, package_name)
-        self.assertEqual(os.path.join(tempdir, package_name), new_path)
+        new_path = task.prepare_file(path, tempdir)
+        self.assertEqual(
+            os.path.join(tempdir, BuildPackageTask.WORKING_DIRECTORY_NAME),
+            new_path)
         self.assertThat(new_path, DirExists())
         self.assertThat(os.path.join(new_path, top_level_dir), DirExists())
 
@@ -258,11 +264,12 @@
         content = 'some content\n'
         with open(os.path.join(source_dir, filename), 'w') as f:
             f.write(content)
-        package_name = 'apackage'
         tempdir = self.useFixture(TempDir()).path
-        new_path = task.prepare_file(os.path.join(source_dir, filename),
-                tempdir, package_name)
-        self.assertEqual(os.path.join(tempdir, package_name), new_path)
+        new_path = task.prepare_file(
+            os.path.join(source_dir, filename), tempdir)
+        self.assertEqual(
+            os.path.join(tempdir, BuildPackageTask.WORKING_DIRECTORY_NAME),
+            new_path)
         self.assertThat(new_path, DirExists())
         self.assertThat(os.path.join(new_path, filename),
                 FileContains(content))