Launchpad itself

Code review comment for lp:~wgrant/launchpad/gina-3.0-support

  1. gina-3.0-support
  2. Merge into db-devel
Revision history for this message
William Grant (wgrant) wrote :

Hi Julian,

Thanks for the review.

On Tue, 2009-12-01 at 16:33 +0000, Julian Edwards wrote:
> Review: Approve
> review approve
> merge approve
>
> > gina needs to support 3.0 (quilt) and 3.0 (native) sources. To do this, it
> > just needs to identify files as the correct types. Since archiveuploader
> > already has a function to do this, I've altered gina to use that rather
> > than the duplicated canonical.launchpad.helpers.getFileType. As gina was
> > the final callsite of getFileType, I've removed that.
> >
> > Since determine_source_file_type only does source files, I've added
> > determine_binary_file_type. It could possibly go along with
> > determine_source_file_type in lp.archiveuploader.utils, but I'm not quite
> > sure.
>
> I think it should go there to be consistent.

Moved.

> >
> > For testing, I've added a 3.0 (quilt) source to the test archive.
> >
>
> Great, thanks for making this change William! It looks good to me so I
> approved it. There's just a couple of minor comments inline.
>
> [snip]
>
> >=== modified file 'lib/lp/soyuz/scripts/gina/handlers.py'
> >--- lib/lp/soyuz/scripts/gina/handlers.py 2009-11-07 09:50:37 +0000
> >+++ lib/lp/soyuz/scripts/gina/handlers.py 2009-12-01 10:10:48 +0000
> >@@ -29,6 +29,7 @@
> >
> > from lp.archivepublisher.diskpool import poolify
> > from lp.archiveuploader.tagfiles import parse_tagfile
> >+from lp.archiveuploader.utils import determine_source_file_type
> >
> > from canonical.database.sqlbase import sqlvalues
> >
> >@@ -46,10 +47,22 @@
> >
> > from lp.registry.interfaces.person import IPersonSet,
> > PersonCreationRationale from lp.registry.interfaces.sourcepackage import
> > SourcePackageType +from lp.soyuz.interfaces.binarypackagerelease import
> > BinaryPackageFileType from lp.soyuz.interfaces.binarypackagename import
> > IBinaryPackageNameSet from lp.soyuz.interfaces.build import BuildStatus
> > from lp.soyuz.interfaces.publishing import PackagePublishingStatus
> >-from canonical.launchpad.helpers import getFileType, getBinaryPackageFormat
> >+from canonical.launchpad.helpers import getBinaryPackageFormat
>
> I think Gina is the only thing using getBinaryPackageFormat - it might be
> worth de-cluttering helpers.py and moving it to the gina module.

Done. I also removed getBinaryPackageExtension, as there were no
callsites.

> >+
> >+
> >+def determine_binary_file_type(filename):
> >+ """Determine the BinaryPackageFileType of the given filename."""
> >+
> >+ if filename.endswith(".deb"):
> >+ return BinaryPackageFileType.DEB
> >+ elif filename.endswith(".udeb"):
> >+ return BinaryPackageFileType.DEB
>
> This should be UDEB I think?

Oops, yes. I've added a test for determine_binary_file_type.

> Everything else looks good! Remind me to land this and the other branch when
> PQM opens.

Both are still blocked on an updated buildbot AMI, but that should
appear on Monday.

The diff on LP is now insanely huge (it looks like it might not have
taken into account the prerequisite branch), but the correct
intermediate diff is attached.

8659..8662.diff
1=== modified file 'lib/canonical/launchpad/helpers.py'
2--- lib/canonical/launchpad/helpers.py 2009-11-25 10:26:03 +0000
3+++ lib/canonical/launchpad/helpers.py 2009-12-10 12:14:13 +0000
4@@ -26,8 +26,7 @@
5
6 import canonical
7 from canonical.launchpad.interfaces import (
8- BinaryPackageFormat, ILaunchBag, IRequestPreferredLanguages,
9- IRequestLocalLanguages)
10+ ILaunchBag, IRequestPreferredLanguages, IRequestLocalLanguages)
11
12
13 # pylint: disable-msg=W0102
14@@ -466,52 +465,6 @@
15 long(sha.new(message_id).hexdigest(), 16), 62))
16
17
18-BINARYPACKAGE_EXTENSIONS = {
19- BinaryPackageFormat.DEB: '.deb',
20- BinaryPackageFormat.UDEB: '.udeb',
21- BinaryPackageFormat.RPM: '.rpm'}
22-
23-
24-class UnrecognizedBinaryFormat(Exception):
25-
26- def __init__(self, fname, *args):
27- Exception.__init__(self, *args)
28- self.fname = fname
29-
30- def __str__(self):
31- return '%s is not recognized as a binary file.' % self.fname
32-
33-
34-def getBinaryPackageFormat(fname):
35- """Return the BinaryPackageFormat for the given filename.
36-
37- >>> getBinaryPackageFormat('mozilla-firefox_0.9_i386.deb').name
38- 'DEB'
39- >>> getBinaryPackageFormat('debian-installer.9_all.udeb').name
40- 'UDEB'
41- >>> getBinaryPackageFormat('network-manager.9_i386.rpm').name
42- 'RPM'
43- """
44- for key, value in BINARYPACKAGE_EXTENSIONS.items():
45- if fname.endswith(value):
46- return key
47-
48- raise UnrecognizedBinaryFormat(fname)
49-
50-
51-def getBinaryPackageExtension(format):
52- """Return the file extension for the given BinaryPackageFormat.
53-
54- >>> getBinaryPackageExtension(BinaryPackageFormat.DEB)
55- '.deb'
56- >>> getBinaryPackageExtension(BinaryPackageFormat.UDEB)
57- '.udeb'
58- >>> getBinaryPackageExtension(BinaryPackageFormat.RPM)
59- '.rpm'
60- """
61- return BINARYPACKAGE_EXTENSIONS[format]
62-
63-
64 def intOrZero(value):
65 """Return int(value) or 0 if the conversion fails.
66
67
68=== modified file 'lib/lp/archiveuploader/tests/test_utils.py'
69--- lib/lp/archiveuploader/tests/test_utils.py 2009-11-18 02:58:23 +0000
70+++ lib/lp/archiveuploader/tests/test_utils.py 2009-12-10 13:32:27 +0000
71@@ -10,6 +10,7 @@
72 import shutil
73
74 from lp.registry.interfaces.sourcepackage import SourcePackageFileType
75+from lp.soyuz.interfaces.binarypackagerelease import BinaryPackageFileType
76 from lp.archiveuploader.tests import datadir
77
78
79@@ -72,6 +73,23 @@
80 self.assertEquals(None, determine_source_file_type('foo_1.0'))
81 self.assertEquals(None, determine_source_file_type('foo_1.0.blah.gz'))
82
83+ def test_determine_binary_file_type(self):
84+ """lp.archiveuploader.utils.determine_binary_file_type should work."""
85+ from lp.archiveuploader.utils import determine_binary_file_type
86+
87+ # .deb -> DEB
88+ self.assertEquals(
89+ determine_binary_file_type('foo_1.0-1_all.deb'),
90+ BinaryPackageFileType.DEB)
91+
92+ # .udeb -> UDEB
93+ self.assertEquals(
94+ determine_binary_file_type('foo_1.0-1_all.udeb'),
95+ BinaryPackageFileType.UDEB)
96+
97+ self.assertEquals(determine_binary_file_type('foo_1.0'), None)
98+ self.assertEquals(determine_binary_file_type('foo_1.0.notdeb'), None)
99+
100 def testPrefixMultilineString(self):
101 """lp.archiveuploader.utils.prefix_multi_line_string should work"""
102 from lp.archiveuploader.utils import prefix_multi_line_string
103
104=== modified file 'lib/lp/archiveuploader/utils.py'
105--- lib/lp/archiveuploader/utils.py 2009-11-14 02:59:34 +0000
106+++ lib/lp/archiveuploader/utils.py 2009-12-10 13:34:56 +0000
107@@ -17,6 +17,7 @@
108 're_changes_file_name',
109 're_extract_src_version',
110 'get_source_file_extension',
111+ 'determine_binary_file_type',
112 'determine_source_file_type',
113 'prefix_multi_line_string',
114 'safe_fix_maintainer',
115@@ -88,6 +89,19 @@
116 return None
117
118
119+def determine_binary_file_type(filename):
120+ """Determine the BinaryPackageFileType of the given filename."""
121+ # Avoid circular imports.
122+ from lp.soyuz.interfaces.binarypackagerelease import BinaryPackageFileType
123+
124+ if filename.endswith(".deb"):
125+ return BinaryPackageFileType.DEB
126+ elif filename.endswith(".udeb"):
127+ return BinaryPackageFileType.UDEB
128+ else:
129+ return None
130+
131+
132 def prefix_multi_line_string(str, prefix, include_blank_lines=0):
133 """Utility function to split an input string and prefix,
134
135
136=== modified file 'lib/lp/soyuz/scripts/gina/handlers.py'
137--- lib/lp/soyuz/scripts/gina/handlers.py 2009-11-25 10:26:03 +0000
138+++ lib/lp/soyuz/scripts/gina/handlers.py 2009-12-10 12:15:49 +0000
139@@ -29,7 +29,8 @@
140
141 from lp.archivepublisher.diskpool import poolify
142 from lp.archiveuploader.tagfiles import parse_tagfile
143-from lp.archiveuploader.utils import determine_source_file_type
144+from lp.archiveuploader.utils import (determine_binary_file_type,
145+ determine_source_file_type)
146
147 from canonical.database.sqlbase import sqlvalues
148
149@@ -47,22 +48,10 @@
150
151 from lp.registry.interfaces.person import IPersonSet, PersonCreationRationale
152 from lp.registry.interfaces.sourcepackage import SourcePackageType
153-from lp.soyuz.interfaces.binarypackagerelease import BinaryPackageFileType
154 from lp.soyuz.interfaces.binarypackagename import IBinaryPackageNameSet
155+from lp.soyuz.interfaces.binarypackagerelease import BinaryPackageFormat
156 from lp.soyuz.interfaces.build import BuildStatus
157 from lp.soyuz.interfaces.publishing import PackagePublishingStatus
158-from canonical.launchpad.helpers import getBinaryPackageFormat
159-
160-
161-def determine_binary_file_type(filename):
162- """Determine the BinaryPackageFileType of the given filename."""
163-
164- if filename.endswith(".deb"):
165- return BinaryPackageFileType.DEB
166- elif filename.endswith(".udeb"):
167- return BinaryPackageFileType.DEB
168- else:
169- return None
170
171
172 def check_not_in_librarian(files, archive_root, directory):
173@@ -91,6 +80,39 @@
174 return to_upload
175
176
177+BINARYPACKAGE_EXTENSIONS = {
178+ BinaryPackageFormat.DEB: '.deb',
179+ BinaryPackageFormat.UDEB: '.udeb',
180+ BinaryPackageFormat.RPM: '.rpm'}
181+
182+
183+class UnrecognizedBinaryFormat(Exception):
184+
185+ def __init__(self, fname, *args):
186+ Exception.__init__(self, *args)
187+ self.fname = fname
188+
189+ def __str__(self):
190+ return '%s is not recognized as a binary file.' % self.fname
191+
192+
193+def getBinaryPackageFormat(fname):
194+ """Return the BinaryPackageFormat for the given filename.
195+
196+ >>> getBinaryPackageFormat('mozilla-firefox_0.9_i386.deb').name
197+ 'DEB'
198+ >>> getBinaryPackageFormat('debian-installer.9_all.udeb').name
199+ 'UDEB'
200+ >>> getBinaryPackageFormat('network-manager.9_i386.rpm').name
201+ 'RPM'
202+ """
203+ for key, value in BINARYPACKAGE_EXTENSIONS.items():
204+ if fname.endswith(value):
205+ return key
206+
207+ raise UnrecognizedBinaryFormat(fname)
208+
209+
210 class DataSetupError(Exception):
211 """Raised when required data is found to be missing in the database"""
212
213
214=== added file 'lib/lp/soyuz/scripts/tests/test_gina.py'
215--- lib/lp/soyuz/scripts/tests/test_gina.py 1970-01-01 00:00:00 +0000
216+++ lib/lp/soyuz/scripts/tests/test_gina.py 2009-12-10 12:19:14 +0000
217@@ -0,0 +1,15 @@
218+# Copyright 2009 Canonical Ltd. This software is licensed under the
219+# GNU Affero General Public License version 3 (see the file LICENSE).
220+
221+import unittest
222+
223+from zope.testing.doctest import DocTestSuite
224+
225+import lp.soyuz.scripts.gina.handlers
226+
227+
228+def test_suite():
229+ suite = unittest.TestSuite()
230+ suite.addTest(DocTestSuite(lp.soyuz.scripts.gina.handlers))
231+ return suite
232+

« Back to merge proposal