Merge lp:~james-w/pkgme-devportal/package-name-spaces into lp:pkgme-devportal

[James Westby]

Use the cleaned package name to avoid spaces etc. in filenames.

Hi,

This fixes a bug that John found where packages fail to build because
they have spaces in the paths.

We were using the raw PackageName produced by the backend, but pkgme
does some futzing with the package name to remove spaces and stuff.

We therefore "clean" the package name before we use it to ensure we
are using the final package name.

Thanks,

James

[Jonathan Lange (jml)]

Thanks. Glad it's such a simple fix.

That said, I don't think this will fix bad filenames that come from get_extra_files_from_paths.

jml

review: Needs Fixing

[lp:~james-w/pkgme-devportal/package-name-spaces]

123. By James Westby on 2012-09-17

Also fix the issue with get_extra_files_from_paths. Thanks jml.

124. By James Westby on 2012-09-17

Add an acceptance test for a package name with spaces.

[Jonathan Lange (jml)]

Thanks.

review: Approve

=== added directory 'devportalbinary/acceptance/data/package_name_with_spaces'
=== added file 'devportalbinary/acceptance/data/package_name_with_spaces/devportal-metadata.json'
--- devportalbinary/acceptance/data/package_name_with_spaces/devportal-metadata.json	1970-01-01 00:00:00 +0000
+++ devportalbinary/acceptance/data/package_name_with_spaces/devportal-metadata.json	2012-09-17 16:22:21 +0000
@@ -0,0 +1,4 @@
+{
+    "suggested_package_name": "The Jabberwocky",
+    "description": "The Jabberwocky, by Lewis Carroll"
+}

=== added file 'devportalbinary/acceptance/data/package_name_with_spaces/jabberwocky.pdf'
Binary files devportalbinary/acceptance/data/package_name_with_spaces/jabberwocky.pdf	1970-01-01 00:00:00 +0000 and devportalbinary/acceptance/data/package_name_with_spaces/jabberwocky.pdf	2012-09-17 16:22:21 +0000 differ
=== modified file 'devportalbinary/acceptance/tests/__init__.py'
--- devportalbinary/acceptance/tests/__init__.py	2012-09-12 18:08:24 +0000
+++ devportalbinary/acceptance/tests/__init__.py	2012-09-17 16:22:21 +0000
@@ -1,5 +1,6 @@
 import os
 import shutil
+import subprocess
 
 from fixtures import Fixture, MonkeyPatch, TempDir
 from testtools import TestCase
@@ -48,6 +49,15 @@
         build_source_package(test_data.path, sign=False)
 
 
+def build_binary_package(path):
+    proc = subprocess.Popen(["debuild", "-uc", "-us"], cwd=path,
+            stdout=subprocess.PIPE, stderr=subprocess.STDOUT)
+    out, err = proc.communicate()
+    if proc.returncode != 0:
+        raise AssertionError(
+                "Building binary package from %s failed: %s" % (path, out))
+
+
 class AcceptanceTests(TestCase):
 
     def assertErrorsOutWith(self, test_data, error_message):
@@ -145,6 +155,14 @@
             test_data,
             "Text will never be implemented: You can't package a text file")
 
+    def test_package_name_with_spaces(self):
+        test_data = self.useFixture(TestData("package_name_with_spaces"))
+        run_pkgme(test_data)
+        # Build the binary package, as that's usually where errors with
+        # spaces in paths show up.
+        build_binary_package(test_data.path)
+        self.assertThat(test_data.path, HasFileTree({'debian/control': {}}))
+
 
 def test_suite():
     import unittest

=== modified file 'devportalbinary/metadata.py'
--- devportalbinary/metadata.py	2012-09-12 18:19:35 +0000
+++ devportalbinary/metadata.py	2012-09-17 16:22:21 +0000
@@ -402,9 +402,8 @@
             'debian/%s.lintian-overrides' % (package_name,) : lintian_override_content,
             }
 
-    def get_extra_files_from_paths(self):
+    def get_extra_files_from_paths(self, package_name):
         """Get extra, binary files for the package."""
-        package_name = self._calculate_info_element(PackageName)
         extra_files = self._get_extra_icon_files_from_paths(package_name)
         return extra_files
 
@@ -538,7 +537,6 @@
             Distribution,
             ExplicitCopyright,
             ExtraControlBinaryFields,
-            ExtraFilesFromPaths,
             ExtraTargets,
             Maintainer,
             Section,
@@ -552,9 +550,15 @@
             value = self._calculate_info_element(element)
             if value:
                 info[element] = value
-        # Special-case ExtraFiles since it needs PackageName.
+        # Special-case ExtraFiles and ExtraFilesFromPaths since they need
+        # PackageName.
+        package_name = PackageName.clean(info[PackageName])
         info[ExtraFiles] = self._calculate_info_element(
-            ExtraFiles, info[PackageName])
+            ExtraFiles, package_name)
+        extra_files_from_paths = self._calculate_info_element(
+            ExtraFilesFromPaths, package_name)
+        if extra_files_from_paths:
+            info[ExtraFilesFromPaths] = extra_files_from_paths
         return info
 
     @classmethod

=== modified file 'devportalbinary/tests/test_metadata.py'
--- devportalbinary/tests/test_metadata.py	2012-09-12 18:19:35 +0000
+++ devportalbinary/tests/test_metadata.py	2012-09-17 16:22:21 +0000
@@ -351,14 +351,16 @@
             backend._get_extra_icon_files_from_paths(package_name))
 
     def test_get_extra_files_from_paths_in_info_if_icons(self):
-        metadata = self.make_metadata()
+        package_name = self.getUniqueString()
+        metadata = self.make_metadata(package_name=package_name)
         metadata[MetadataBackend.ICONS] = {
             '48x48': '/foo/bar.png',
             '128x128': '/baz/qux.png',
             }
         backend = self.make_backend(metadata=metadata)
         self.assertEqual(
-            backend.get_extra_files_from_paths(),
+            backend.get_extra_files_from_paths(
+                PackageName.clean(package_name)),
             backend.get_info()[ExtraFilesFromPaths])
 
     def test_get_extra_files_from_paths_lintian_override(self):
@@ -369,6 +371,15 @@
             'debian/%s.lintian-overrides' % package_name,
             backend.get_extra_files(package_name))
 
+    def test_get_info_uses_cleaned_package_name(self):
+        application_name = 'foo bar'
+        expected_package_name = 'foobar'
+        metadata = self.make_metadata(package_name=application_name)
+        backend = self.make_backend(metadata=metadata)
+        self.assertIn(
+            'debian/%s.lintian-overrides' % expected_package_name,
+            backend.get_info()[ExtraFiles])
+
     def test_license_field_not_in_mapping(self):
         metadata = self.make_metadata()
         license = 'not-standard-license-that-is-not-mapped'