Merge lp:~james-w/pkgme/python-dependencies into lp:pkgme

[James Westby]

Hi,

This branch adds a couple of new features to the Python backend in order to
have Build-Depends and Depends filled in correctly.

In testing this I found that pkgme's stated dependencies were incomplete, so
it fixes that too.

Thanks,

James

[Jonathan Lange (jml)]

I think I follow this.

Not sure why build_depends concatenates a string rather than appending to a list and returning ', '.join(dependencies), but it's not a big deal.

Is there a way to avoid hard-coding "/usr/share/python"? I would have thought it was unnecessary after adding the extra dependencies to setup.py.

More generally, I find the 'getattr(self, method_name)()' pattern easier to follow if there's a standard prefix for those methods. (e.g. 'test_', 'cmd_', etc.) It would be churlish to ask you to change it in this branch.

review: Needs Information

[James Westby (james-w)]

> I think I follow this.
>
> Not sure why build_depends concatenates a string rather than appending to a
> list and returning ', '.join(dependencies), but it's not a big deal.

It's mainly that it gets a string as the input. It would still work your way
though, so I'll change it.

> Is there a way to avoid hard-coding "/usr/share/python"? I would have thought
> it was unnecessary after adding the extra dependencies to setup.py.

Actually you raise an interesting point. This isn't provided by python-debian,
it's actually part of the python packages on Debian/Ubuntu (and nowhere else I think).
That's obviously not very portable, but it was the closest thing to hand to get
this functionality.

A better solution is likely to be to re-implement that logic so that it is
portable.

Also, is /usr/share/python usually on sys.path?

> More generally, I find the 'getattr(self, method_name)()' pattern easier to
> follow if there's a standard prefix for those methods. (e.g. 'test_', 'cmd_',
> etc.) It would be churlish to ask you to change it in this branch.

It would be pretty easy to do though.

Thanks,

James

[Jonathan Lange (jml)]

On Fri, Jul 8, 2011 at 9:05 PM, James Westby <email address hidden> wrote:
>> I think I follow this.
>>
>> Not sure why build_depends concatenates a string rather than appending to a
>> list and returning ', '.join(dependencies), but it's not a big deal.
>
> It's mainly that it gets a string as the input. It would still work your way
> though, so I'll change it.
>

Ta.

>> Is there a way to avoid hard-coding "/usr/share/python"? I would have thought
>> it was unnecessary after adding the extra dependencies to setup.py.
>
> Actually you raise an interesting point. This isn't provided by python-debian,
> it's actually part of the python packages on Debian/Ubuntu (and nowhere else I think).
> That's obviously not very portable, but it was the closest thing to hand to get
> this functionality.
>
> A better solution is likely to be to re-implement that logic so that it is
> portable.
>

OK, that makes sense. It's probably worth adding a comment along these lines.

> Also, is /usr/share/python usually on sys.path?
>

No. Or at least, not as far as I can tell.

>> More generally, I find the 'getattr(self, method_name)()' pattern easier to
>> follow if there's a standard prefix for those methods. (e.g. 'test_', 'cmd_',
>> etc.) It would be churlish to ask you to change it in this branch.
>
> It would be pretty easy to do though.

Yeah :)

I'd be happy to do a follow-up branch for this.

jml

[Jonathan Lange (jml)]

review: Approve

=== modified file 'pkgme/distutils_command/pkgme_info.py'
--- pkgme/distutils_command/pkgme_info.py	2011-04-15 17:17:04 +0000
+++ pkgme/distutils_command/pkgme_info.py	2011-07-08 12:55:21 +0000
@@ -3,6 +3,9 @@
 import sys
 import textwrap
 
+sys.path.insert(1, "/usr/share/python")
+from debpython import pydist
+
 
 class pkgme_info(Command):
 
@@ -51,12 +54,12 @@
     def maintainer(self):
         if (self.distribution.get_maintainer() != 'UNKNOWN' and
             self.distribution.get_maintainer_email() != 'UNKNOWN'):
-            return "%s <%s>"%(
+            return "%s <%s>" % (
                 self.distribution.get_maintainer(),
                 self.distribution.get_maintainer_email())
         elif (self.distribution.get_author() != 'UNKNOWN' and
               self.distribution.get_author_email() != 'UNKNOWN'):
-            return "%s <%s>"%(
+            return "%s <%s>" % (
                 self.distribution.get_author(),
                 self.distribution.get_author_email())
         else:
@@ -72,3 +75,18 @@
 
     def buildsystem(self):
         return "python_distutils"
+
+    def build_depends(self):
+        basics = "python-all, python-setuptools"
+        if getattr(self.distribution, "install_requires", None) is not None:
+            for pkg in self.distribution.install_requires:
+                dep = pydist.guess_dependency(pkg)
+                if dep:
+                    basics += ", %s" % dep
+        return basics
+
+    def debhelper_addons(self):
+        return "python2"
+
+    def depends(self):
+        return "${python:Depends}"

=== modified file 'pkgme/info_elements.py'
--- pkgme/info_elements.py	2011-07-08 12:55:21 +0000
+++ pkgme/info_elements.py	2011-07-08 12:55:21 +0000
@@ -87,6 +87,24 @@
     name = "build_depends"
     description = """The build dependencies of the package."""
 
+    @classmethod
+    def clean(cls, value):
+        if value is None:
+            value = "debhelper (>= 7)"
+        # FIXME: should replace if the dependency isn't strict
+        # enough
+        # FIXME: check is too lax, needs to parse and compare package
+        # names
+        if "debhelper" not in value:
+            value += ", debhelper (>= 7)"
+        return value
+
+
+class DebhelperAddons(InfoElement):
+
+    name = "debhelper_addons"
+    description = """The debhelper addons to use."""
+
 
 class Architecture(InfoElement):
 

=== modified file 'pkgme/package_files.py'
--- pkgme/package_files.py	2011-05-12 21:52:36 +0000
+++ pkgme/package_files.py	2011-07-08 12:55:21 +0000
@@ -7,6 +7,7 @@
     Architecture,
     BuildDepends,
     Buildsystem,
+    DebhelperAddons,
     Depends,
     Description,
     Homepage,
@@ -100,6 +101,7 @@
 
     elements = [
         Buildsystem,
+        DebhelperAddons,
         ]
 
     filename = "rules"

=== modified file 'pkgme/templates/rules'
--- pkgme/templates/rules	2010-11-06 03:53:23 +0000
+++ pkgme/templates/rules	2011-07-08 12:55:21 +0000
@@ -5,4 +5,7 @@
 #if $buildsystem
 --buildsystem=${buildsystem} #slurp
 #end if
+#if $debhelper_addons
+--with=${debhelper_addons} #slurp
+#end if
 

=== modified file 'pkgme/tests/test_distutils_command.py'
--- pkgme/tests/test_distutils_command.py	2010-11-17 21:35:30 +0000
+++ pkgme/tests/test_distutils_command.py	2011-07-08 12:55:21 +0000
@@ -115,3 +115,24 @@
         version = "123"
         cmd = self.get_cmd_with_metadata(dict(version=version))
         self.assertEqual(version, cmd.version())
+
+    def test_build_depends_includes_python_all(self):
+        cmd = self.get_cmd_with_metadata({})
+        self.assertIn("python-all", cmd.build_depends())
+
+    def test_build_depends_includes_python_setuptools(self):
+        cmd = self.get_cmd_with_metadata({})
+        self.assertIn("python-setuptools", cmd.build_depends())
+
+    def test_build_depends_examines_install_requires(self):
+        cmd = self.get_cmd_with_metadata({})
+        cmd.distribution.install_requires = ["foo"]
+        self.assertIn("python-foo", cmd.build_depends())
+
+    def test_debhelper_addons_is_python2(self):
+        cmd = self.get_cmd_with_metadata({})
+        self.assertEqual("python2", cmd.debhelper_addons())
+
+    def test_depends_is_python_depends(self):
+        cmd = self.get_cmd_with_metadata({})
+        self.assertEqual("${python:Depends}", cmd.depends())

=== modified file 'pkgme/tests/test_info_elements.py'
--- pkgme/tests/test_info_elements.py	2011-07-08 12:55:21 +0000
+++ pkgme/tests/test_info_elements.py	2011-07-08 12:55:21 +0000
@@ -1,6 +1,10 @@
 from testtools import TestCase
 
-from pkgme.info_elements import InfoElement, MissingInfoError
+from pkgme.info_elements import (
+    BuildDepends,
+    InfoElement,
+    MissingInfoError,
+    )
 from pkgme.project_info import DictInfo
 
 
@@ -80,3 +84,18 @@
         original_value = self.getUniqueString()
         project_info = DictInfo({TestElement.name: original_value})
         self.assertEqual(original_value, TestElement.get_value(project_info))
+
+
+class BuildDependsTestCase(TestCase):
+
+    def test_clean_inserts_debhelper(self):
+        value = BuildDepends.clean(None)
+        self.assertEqual("debhelper (>= 7)", value)
+
+    def test_clean_adds_debhelper(self):
+        value = BuildDepends.clean("python")
+        self.assertEqual("python, debhelper (>= 7)", value)
+
+    def test_clean_doesnt_add_debhelper_if_present(self):
+        value = BuildDepends.clean("python, debhelper")
+        self.assertEqual("python, debhelper", value)

=== modified file 'pkgme/tests/test_package_files.py'
--- pkgme/tests/test_package_files.py	2011-06-15 18:54:40 +0000
+++ pkgme/tests/test_package_files.py	2011-07-08 12:55:21 +0000
@@ -8,6 +8,7 @@
     Architecture,
     BuildDepends,
     Buildsystem,
+    DebhelperAddons,
     Depends,
     Description,
     Homepage,
@@ -181,6 +182,14 @@
             "%%:\n\tdh $@ --buildsystem=%s \n" % buildsystem,
             package_file.get_contents())
 
+    def test_content_with_debhelper_addons(self):
+        debhelper_addons = self.getUniqueString()
+        args = {DebhelperAddons.name: debhelper_addons}
+        package_file = self.get_object(args)
+        self.assertIn(
+            "%%:\n\tdh $@ --with=%s \n" % debhelper_addons,
+            package_file.get_contents())
+
     def test_overwrite(self):
         package_file = self.get_object()
         self.assertEqual(True, package_file.overwrite)
@@ -328,19 +337,13 @@
 
     def test_get_contents_sets_build_depends(self):
         build_depends = self.getUniqueString()
+        build_depends = BuildDepends.clean(build_depends)
         args = {BuildDepends.name: build_depends}
         package_file = self.get_object(args)
         self.assertThat(
             package_file.get_contents(),
             ControlSourceStanzaHasField("Build-Depends", build_depends))
 
-    def test_get_contents_without_build_depends(self):
-        args = {BuildDepends.name: None}
-        package_file = self.get_object(args)
-        self.assertThat(
-            package_file.get_contents(),
-            ControlSourceStanzaDoesntHaveField("Build-Depends"))
-
     def test_get_contents_sets_architecture(self):
         architecture = self.getUniqueString()
         args = {Architecture.name: architecture}

=== modified file 'setup.py'
--- setup.py	2011-07-08 12:55:21 +0000
+++ setup.py	2011-07-08 12:55:21 +0000
@@ -64,8 +64,10 @@
     test_suite='pkgme.tests',
     install_requires = [
         'argparse',
-        'cheetah',
+        'Cheetah',
+        'python_debian',
         'fixtures',
+        'Markdown', # "Required" by Cheetah, but only a suggests in the packaging.
         'testtools',
         ],
     entry_points = {