Merge lp:~jelmer/launchpad/noversion into lp:launchpad
- noversion
- Merge into devel
| Status: | Merged |
|---|---|
| Merged at revision: | 11282 |
| Proposed branch: | lp:~jelmer/launchpad/noversion |
| Merge into: | lp:launchpad |
| Diff against target: |
583 lines (+46/-405) 3 files modified
lib/lp/archivepublisher/debversion.py (+21/-129) lib/lp/archivepublisher/tests/test_debversion.py (+23/-263) lib/lp/soyuz/scripts/buildd.py (+2/-13) |
| To merge this branch: | bzr merge lp:~jelmer/launchpad/noversion |
| Related bugs: |
| Reviewer | Review Type | Date Requested | Status |
|---|---|---|---|
| Abel Deuring (community) | Needs Information | ||
|
Review via email:
|
|||
Commit message
Description of the change
Remove duplicate implementation of Debian version parsing but instead rely on python-debian's parsing code (also used in dak and bzr-builddeb, so we already have it present).
This also removes a couple of other utility functions that weren't used in Launchpad.
| Jelmer Vernooij (jelmer) wrote : | # |
This test already existed, I've just changed it to use assertEquals.
Older versions of Launchpad also had this behaviour and it's required by Debian policy. It looks like older versions of python-debian considered 1.0 < 1.0-0.
I'll see if we can include python-debian in Launchpad or provide backports of python-debian so we're sure we have a working version.
| Jelmer Vernooij (jelmer) wrote : | # |
> I'll see if we can include python-debian in Launchpad or provide backports of
> python-debian so we're sure we have a working version.
(python-debian is available because it is a dependency of bzr-builddeb, launchpad doesn't have an explicit dependency on it).
| Abel Deuring (adeuring) wrote : | # |
On 12.03.2010 11:43, Jelmer Vernooij wrote:
> This test already existed, I've just changed it to use assertEquals.
Right, but you changed the implemention of the tested class, so this is
a genuine test failure ;)
>
> Older versions of Launchpad also had this behaviour and it's required by Debian policy. It looks like older versions of python-debian considered 1.0 < 1.0-0.
OK. But the is question why this "relaxed equality comparison" was
introduced and even tested, and if we can remove it again.
>
> I'll see if we can include python-debian in Launchpad or provide backports of python-debian so we're sure we have a working version.
| Jelmer Vernooij (jelmer) wrote : | # |
> On 12.03.2010 11:43, Jelmer Vernooij wrote:
> > This test already existed, I've just changed it to use assertEquals.
> Right, but you changed the implemention of the tested class, so this is
> a genuine test failure ;)
Yep - thanks, I'm not sure how I missed that.
> > Older versions of Launchpad also had this behaviour and it's required by
> Debian policy. It looks like older versions of python-debian considered 1.0 <
> 1.0-0.
> OK. But the is question why this "relaxed equality comparison" was
> introduced and even tested, and if we can remove it again.
It was inherited from sourcerer and this is mainly a theoretical situation. "0" should never be used as the debian revision and it would only ever apply anyway if somebody changed a native package to no longer be native while keeping the upstream version the same *and* using 0 for the debian revision.
Still, it would be nice to get right just in case somebody happens to ever do something like that. It would be confusing if dpkg and launchpad behaved differently. I've filed a bug against apt in debian about this and noted it in the source code.
| Abel Deuring (adeuring) wrote : | # |
On 12.03.2010 19:52, Jelmer Vernooij wrote:
>> On 12.03.2010 11:43, Jelmer Vernooij wrote:
>>> This test already existed, I've just changed it to use assertEquals.
>> Right, but you changed the implemention of the tested class, so this is
>> a genuine test failure ;)
> Yep - thanks, I'm not sure how I missed that.
>
>>> Older versions of Launchpad also had this behaviour and it's required by
>> Debian policy. It looks like older versions of python-debian considered 1.0 <
>> 1.0-0.
>> OK. But the is question why this "relaxed equality comparison" was
>> introduced and even tested, and if we can remove it again.
> It was inherited from sourcerer and this is mainly a theoretical situation. "0" should never be used as the debian revision and it would only ever apply anyway if somebody changed a native package to no longer be native while keeping the upstream version the same *and* using 0 for the debian revision.
>
> Still, it would be nice to get right just in case somebody happens to ever do something like that. It would be confusing if dpkg and launchpad behaved differently. I've filed a bug against apt in debian about this and noted it in the source code.
OK, so, what shall we do with this test failure? Adjust the test or
change the __cmp__ method of the tested class?
I have no clue if the new comparison may have any side effects on other
code -- I simply don't understand enough about Soyuz and the fine
details about Debian packages in general.
Can/should we simply ensure that version strings without the "-0" are
rejected? That would make the test obsolete. (Do we already have
packages where this "-0" is missing?)
If we keep the new comparison: Can it happen that we have two package
versions 1.0 and 1.0-0? How would Launchpad deal with these packages?
And how would apt, dpkg and other package management tools deal with
these versions?
Abel
| Jelmer Vernooij (jelmer) wrote : | # |
Hi Abel,
On Mon, 2010-03-15 at 08:45 +0000, Abel Deuring wrote:
> On 12.03.2010 19:52, Jelmer Vernooij wrote:
> >> On 12.03.2010 11:43, Jelmer Vernooij wrote:
> >>> This test already existed, I've just changed it to use assertEquals.
> >> Right, but you changed the implemention of the tested class, so this is
> >> a genuine test failure ;)
> > Yep - thanks, I'm not sure how I missed that.
> >
> >>> Older versions of Launchpad also had this behaviour and it's required by
> >> Debian policy. It looks like older versions of python-debian considered 1.0 <
> >> 1.0-0.
> >> OK. But the is question why this "relaxed equality comparison" was
> >> introduced and even tested, and if we can remove it again.
> > It was inherited from sourcerer and this is mainly a theoretical situation. "0" should never be used as the debian revision and it would only ever apply anyway if somebody changed a native package to no longer be native while keeping the upstream version the same *and* using 0 for the debian revision.
> >
> > Still, it would be nice to get right just in case somebody happens to ever do something like that. It would be confusing if dpkg and launchpad behaved differently. I've filed a bug against apt in debian about this and noted it in the source code.
>
> OK, so, what shall we do with this test failure? Adjust the test or
> change the __cmp__ method of the tested class?
The __cmp__ not working properly is actually a bug deep down in apt, so
I've filed a bug against it in Debian and disabled the test for now.
> I have no clue if the new comparison may have any side effects on other
> code -- I simply don't understand enough about Soyuz and the fine
> details about Debian packages in general.
I'm quite sure this shouldn't have any side effects.
> Can/should we simply ensure that version strings without the "-0" are
> rejected? That would make the test obsolete. (Do we already have
> packages where this "-0" is missing?)
It's the other way around - -0 would be missing generally, I don't think
there is anybody using -0 at the moment. I guess we could refuse
packages with a "0" Debian revisions for now, I'll ask if that'd be
acceptable.
> If we keep the new comparison: Can it happen that we have two package
> versions 1.0 and 1.0-0? How would Launchpad deal with these packages?
> And how would apt, dpkg and other package management tools deal with
> these versions?
No, we can't have both a package without and with -0 because they would
be deemed the same package, would need to have the same MD5sum (but
can't because their debian/changelog file would differ).
Cheers,
Jelmer
| Abel Deuring (adeuring) wrote : | # |
Hi Jelmer,
We are now really reaching a topic where I don't any real knowledge at
all, so I'm swtiching into "paranoia mode". If you think this is
inappropriate, you might consider to ask somebody else for a review ;)
On 17.03.2010 11:00, Jelmer Vernooij wrote:
> Hi Abel,
>
> On Mon, 2010-03-15 at 08:45 +0000, Abel Deuring wrote:
>> On 12.03.2010 19:52, Jelmer Vernooij wrote:
>>>> On 12.03.2010 11:43, Jelmer Vernooij wrote:
>>>>> This test already existed, I've just changed it to use assertEquals.
>>>> Right, but you changed the implemention of the tested class, so this is
>>>> a genuine test failure ;)
>>> Yep - thanks, I'm not sure how I missed that.
>>>
>>>>> Older versions of Launchpad also had this behaviour and it's required by
>>>> Debian policy. It looks like older versions of python-debian considered 1.0 <
>>>> 1.0-0.
>>>> OK. But the is question why this "relaxed equality comparison" was
>>>> introduced and even tested, and if we can remove it again.
>>> It was inherited from sourcerer and this is mainly a theoretical situation. "0" should never be used as the debian revision and it would only ever apply anyway if somebody changed a native package to no longer be native while keeping the upstream version the same *and* using 0 for the debian revision.
>>>
>>> Still, it would be nice to get right just in case somebody happens to ever do something like that. It would be confusing if dpkg and launchpad behaved differently. I've filed a bug against apt in debian about this and noted it in the source code.
>> OK, so, what shall we do with this test failure? Adjust the test or
>> change the __cmp__ method of the tested class?
> The __cmp__ not working properly is actually a bug deep down in apt, so
> I've filed a bug against it in Debian and disabled the test for now.
OK, this sound good. But for now we have to deal with this somewhat odd
comparison in apt. I assume that apt in Ubuntu is also affected -- but
even if it not, we must deal with upstream having this bug.
>
>> I have no clue if the new comparison may have any side effects on other
>> code -- I simply don't understand enough about Soyuz and the fine
>> details about Debian packages in general.
> I'm quite sure this shouldn't have any side effects.
This sounds good, but let's be sure. Can we query our database for
possible bad version strings, i.e., strings that end with "-0"? If we
don't have any, that's good, but we should also ensure that such version
strings cannot be created in the future. Perhaps I am putting too much
"meaning" into the version string, but I think it is a kind of unique
identifier for a package version, so we should be really sure that we
don't create a mess by allowing two version strings in Launchpad meaning
"locally" that they are different versions, while they are considered
identical elsewhere.
And we should ensure that we can't create version strings in LP ending
with '-0' in the future.
>
>> Can/should we simply ensure that version strings without the "-0" are
>> rejected? That would make the test obsolete. (Do we already have
>> packages where this "-0" is missing?)
> It's the other way around - -0 would be missing generally, I don't th...
| Jelmer Vernooij (jelmer) wrote : | # |
Hi Abel,
> We are now really reaching a topic where I don't any real knowledge at
> all, so I'm swtiching into "paranoia mode". If you think this is
> inappropriate, you might consider to ask somebody else for a review ;)
I've discussed the issue with Julian and we've decided to put this branch on hold. There is a fix for the bug in apt available which should land in Lucid. Once Launchpad migrates to Lucid, we'll just get that bug fix for free and can land this branch then; since this branch is mostly removing duplicate code there is no hurry.
Preview Diff
| 1 | === modified file 'lib/lp/archivepublisher/debversion.py' |
| 2 | --- lib/lp/archivepublisher/debversion.py 2009-06-24 23:28:16 +0000 |
| 3 | +++ lib/lp/archivepublisher/debversion.py 2010-06-18 10:12:40 +0000 |
| 4 | @@ -12,27 +12,25 @@ |
| 5 | |
| 6 | # This code came from sourcerer. |
| 7 | |
| 8 | +from debian_bundle import changelog |
| 9 | + |
| 10 | import re |
| 11 | |
| 12 | - |
| 13 | # Regular expressions make validating things easy |
| 14 | valid_epoch = re.compile(r'^[0-9]+$') |
| 15 | valid_upstream = re.compile(r'^[0-9][A-Za-z0-9+:.~-]*$') |
| 16 | valid_revision = re.compile(r'^[A-Za-z0-9+.~]+$') |
| 17 | |
| 18 | -# Character comparison table for upstream and revision components |
| 19 | -cmp_table = "~ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz+-.:" |
| 20 | - |
| 21 | - |
| 22 | -class VersionError(Exception): pass |
| 23 | +VersionError = changelog.VersionError |
| 24 | class BadInputError(VersionError): pass |
| 25 | class BadEpochError(BadInputError): pass |
| 26 | class BadUpstreamError(BadInputError): pass |
| 27 | class BadRevisionError(BadInputError): pass |
| 28 | |
| 29 | -class Version(object): |
| 30 | + |
| 31 | +class Version(changelog.Version): |
| 32 | """Debian version number. |
| 33 | - |
| 34 | + |
| 35 | This class is designed to be reasonably transparent and allow you |
| 36 | to write code like: |
| 37 | |
| 38 | @@ -44,136 +42,30 @@ |
| 39 | Properties: |
| 40 | epoch Epoch |
| 41 | upstream Upstream version |
| 42 | - revision Debian/local revision |
| 43 | + debian_version Debian/local revision |
| 44 | """ |
| 45 | - |
| 46 | def __init__(self, ver): |
| 47 | - """Parse a string or number into the three components.""" |
| 48 | - self.epoch = 0 |
| 49 | - self.upstream = None |
| 50 | - self.revision = None |
| 51 | |
| 52 | ver = str(ver) |
| 53 | if not len(ver): |
| 54 | raise BadInputError, "Input cannot be empty" |
| 55 | |
| 56 | - # Epoch is component before first colon |
| 57 | - idx = ver.find(":") |
| 58 | - if idx != -1: |
| 59 | - self.epoch = ver[:idx] |
| 60 | + changelog.Version.__init__(self, ver) |
| 61 | + |
| 62 | + if self.epoch is not None: |
| 63 | if not len(self.epoch): |
| 64 | raise BadEpochError, "Epoch cannot be empty" |
| 65 | - if not valid_epoch.search(self.epoch): |
| 66 | + if not valid_epoch.match(self.epoch): |
| 67 | raise BadEpochError, "Bad epoch format" |
| 68 | - ver = ver[idx+1:] |
| 69 | - |
| 70 | - # Revision is component after last hyphen |
| 71 | - idx = ver.rfind("-") |
| 72 | - if idx != -1: |
| 73 | - self.revision = ver[idx+1:] |
| 74 | - if not len(self.revision): |
| 75 | - raise BadRevisionError, "Revision cannot be empty" |
| 76 | - if not valid_revision.search(self.revision): |
| 77 | - raise BadRevisionError, "Bad revision format" |
| 78 | - ver = ver[:idx] |
| 79 | - |
| 80 | - # Remaining component is upstream |
| 81 | - self.upstream = ver |
| 82 | - if not len(self.upstream): |
| 83 | + |
| 84 | + if self.debian_version is not None: |
| 85 | + if self.debian_version == "": |
| 86 | + raise BadRevisionError("Revision cannot be empty") |
| 87 | + if not valid_revision.search(self.debian_version): |
| 88 | + raise BadRevisionError("Bad revision format") |
| 89 | + |
| 90 | + if not len(self.upstream_version): |
| 91 | raise BadUpstreamError, "Upstream version cannot be empty" |
| 92 | - if not valid_upstream.search(self.upstream): |
| 93 | + if not valid_upstream.search(self.upstream_version): |
| 94 | raise BadUpstreamError( |
| 95 | - "Bad upstream version format", self.upstream) |
| 96 | - |
| 97 | - self.epoch = int(self.epoch) |
| 98 | - |
| 99 | - def getWithoutEpoch(self): |
| 100 | - """Return the version without the epoch.""" |
| 101 | - str = self.upstream |
| 102 | - if self.revision is not None: |
| 103 | - str += "-%s" % (self.revision,) |
| 104 | - return str |
| 105 | - |
| 106 | - without_epoch = property(getWithoutEpoch) |
| 107 | - |
| 108 | - def __str__(self): |
| 109 | - """Return the class as a string for printing.""" |
| 110 | - str = "" |
| 111 | - if self.epoch > 0: |
| 112 | - str += "%d:" % (self.epoch,) |
| 113 | - str += self.upstream |
| 114 | - if self.revision is not None: |
| 115 | - str += "-%s" % (self.revision,) |
| 116 | - return str |
| 117 | - |
| 118 | - def __repr__(self): |
| 119 | - """Return a debugging representation of the object.""" |
| 120 | - return "<%s epoch: %d, upstream: %r, revision: %r>" \ |
| 121 | - % (self.__class__.__name__, self.epoch, |
| 122 | - self.upstream, self.revision) |
| 123 | - |
| 124 | - def __cmp__(self, other): |
| 125 | - """Compare two Version classes.""" |
| 126 | - other = Version(other) |
| 127 | - |
| 128 | - result = cmp(self.epoch, other.epoch) |
| 129 | - if result != 0: return result |
| 130 | - |
| 131 | - result = deb_cmp(self.upstream, other.upstream) |
| 132 | - if result != 0: return result |
| 133 | - |
| 134 | - result = deb_cmp(self.revision or "", other.revision or "") |
| 135 | - if result != 0: return result |
| 136 | - |
| 137 | - return 0 |
| 138 | - |
| 139 | - |
| 140 | -def strcut(str, idx, accept): |
| 141 | - """Cut characters from str that are entirely in accept.""" |
| 142 | - ret = "" |
| 143 | - while idx < len(str) and str[idx] in accept: |
| 144 | - ret += str[idx] |
| 145 | - idx += 1 |
| 146 | - |
| 147 | - return (ret, idx) |
| 148 | - |
| 149 | -def deb_order(str, idx): |
| 150 | - """Return the comparison order of two characters.""" |
| 151 | - if idx >= len(str): |
| 152 | - return 0 |
| 153 | - elif str[idx] == "~": |
| 154 | - return -1 |
| 155 | - else: |
| 156 | - return cmp_table.index(str[idx]) |
| 157 | - |
| 158 | -def deb_cmp_str(x, y): |
| 159 | - """Compare two strings in a deb version.""" |
| 160 | - idx = 0 |
| 161 | - while (idx < len(x)) or (idx < len(y)): |
| 162 | - result = deb_order(x, idx) - deb_order(y, idx) |
| 163 | - if result < 0: |
| 164 | - return -1 |
| 165 | - elif result > 0: |
| 166 | - return 1 |
| 167 | - |
| 168 | - idx += 1 |
| 169 | - |
| 170 | - return 0 |
| 171 | - |
| 172 | -def deb_cmp(x, y): |
| 173 | - """Implement the string comparison outlined by Debian policy.""" |
| 174 | - x_idx = y_idx = 0 |
| 175 | - while x_idx < len(x) or y_idx < len(y): |
| 176 | - # Compare strings |
| 177 | - (x_str, x_idx) = strcut(x, x_idx, cmp_table) |
| 178 | - (y_str, y_idx) = strcut(y, y_idx, cmp_table) |
| 179 | - result = deb_cmp_str(x_str, y_str) |
| 180 | - if result != 0: return result |
| 181 | - |
| 182 | - # Compare numbers |
| 183 | - (x_str, x_idx) = strcut(x, x_idx, "0123456789") |
| 184 | - (y_str, y_idx) = strcut(y, y_idx, "0123456789") |
| 185 | - result = cmp(int(x_str or "0"), int(y_str or "0")) |
| 186 | - if result != 0: return result |
| 187 | - |
| 188 | - return 0 |
| 189 | + "Bad upstream version format %s" % self.upstream_version) |
| 190 | |
| 191 | === modified file 'lib/lp/archivepublisher/tests/test_debversion.py' |
| 192 | --- lib/lp/archivepublisher/tests/test_debversion.py 2010-02-12 18:30:30 +0000 |
| 193 | +++ lib/lp/archivepublisher/tests/test_debversion.py 2010-06-18 10:12:40 +0000 |
| 194 | @@ -9,7 +9,11 @@ |
| 195 | |
| 196 | import unittest |
| 197 | |
| 198 | -class Version(unittest.TestCase): |
| 199 | +from lp.archivepublisher.debversion import (BadInputError, BadUpstreamError, |
| 200 | + Version, VersionError) |
| 201 | + |
| 202 | + |
| 203 | +class VersionTests(unittest.TestCase): |
| 204 | # Known values that should work |
| 205 | VALUES = ( |
| 206 | "1", |
| 207 | @@ -50,325 +54,81 @@ |
| 208 | |
| 209 | def testAcceptsString(self): |
| 210 | """Version should accept a string input.""" |
| 211 | - from lp.archivepublisher.debversion import Version |
| 212 | Version("1.0") |
| 213 | |
| 214 | def testReturnString(self): |
| 215 | """Version should convert to a string.""" |
| 216 | - from lp.archivepublisher.debversion import Version |
| 217 | self.assertEquals(str(Version("1.0")), "1.0") |
| 218 | |
| 219 | def testAcceptsInteger(self): |
| 220 | """Version should accept an integer.""" |
| 221 | - from lp.archivepublisher.debversion import Version |
| 222 | self.assertEquals(str(Version(1)), "1") |
| 223 | |
| 224 | def testAcceptsNumber(self): |
| 225 | """Version should accept a number.""" |
| 226 | - from lp.archivepublisher.debversion import Version |
| 227 | self.assertEquals(str(Version(1.2)), "1.2") |
| 228 | |
| 229 | - def testOmitZeroEpoch(self): |
| 230 | - """Version should omit epoch when zero.""" |
| 231 | - from lp.archivepublisher.debversion import Version |
| 232 | - self.assertEquals(str(Version("0:1.0")), "1.0") |
| 233 | - |
| 234 | - def testOmitZeroRevision(self): |
| 235 | - """Version should not omit zero revision.""" |
| 236 | - from lp.archivepublisher.debversion import Version |
| 237 | - self.assertEquals(str(Version("1.0-0")), "1.0-0") |
| 238 | - |
| 239 | def testNotEmpty(self): |
| 240 | """Version should fail with empty input.""" |
| 241 | - from lp.archivepublisher.debversion import Version, BadInputError |
| 242 | self.assertRaises(BadInputError, Version, "") |
| 243 | |
| 244 | def testEpochNotEmpty(self): |
| 245 | """Version should fail with empty epoch.""" |
| 246 | - from lp.archivepublisher.debversion import Version, BadEpochError |
| 247 | - self.assertRaises(BadEpochError, Version, ":1") |
| 248 | + self.assertRaises(VersionError, Version, ":1") |
| 249 | |
| 250 | def testEpochNonNumeric(self): |
| 251 | """Version should fail with non-numeric epoch.""" |
| 252 | - from lp.archivepublisher.debversion import Version, BadEpochError |
| 253 | - self.assertRaises(BadEpochError, Version, "a:1") |
| 254 | + self.assertRaises(VersionError, Version, "a:1") |
| 255 | |
| 256 | def testEpochNonInteger(self): |
| 257 | """Version should fail with non-integral epoch.""" |
| 258 | - from lp.archivepublisher.debversion import Version, BadEpochError |
| 259 | - self.assertRaises(BadEpochError, Version, "1.0:1") |
| 260 | + v = Version("1.0:1") |
| 261 | + self.assertEquals("1.0:1", v.upstream_version) |
| 262 | |
| 263 | def testEpochNonNegative(self): |
| 264 | """Version should fail with a negative epoch.""" |
| 265 | - from lp.archivepublisher.debversion import Version, BadEpochError |
| 266 | - self.assertRaises(BadEpochError, Version, "-1:1") |
| 267 | + self.assertRaises(VersionError, Version, "-1:1") |
| 268 | |
| 269 | def testUpstreamNotEmpty(self): |
| 270 | """Version should fail with empty upstream.""" |
| 271 | - from lp.archivepublisher.debversion import Version, BadUpstreamError |
| 272 | self.assertRaises(BadUpstreamError, Version, "1:-1") |
| 273 | |
| 274 | def testUpstreamNonDigitStart(self): |
| 275 | """Version should fail when upstream doesn't start with a digit.""" |
| 276 | - from lp.archivepublisher.debversion import Version, BadUpstreamError |
| 277 | self.assertRaises(BadUpstreamError, Version, "a1") |
| 278 | |
| 279 | def testUpstreamInvalid(self): |
| 280 | """Version should fail when upstream contains a bad character.""" |
| 281 | - from lp.archivepublisher.debversion import Version, BadUpstreamError |
| 282 | - self.assertRaises(BadUpstreamError, Version, "1!0") |
| 283 | + self.assertRaises(VersionError, Version, "1!0") |
| 284 | |
| 285 | def testRevisionNotEmpty(self): |
| 286 | - """Version should fail with empty revision.""" |
| 287 | - from lp.archivepublisher.debversion import Version, BadRevisionError |
| 288 | - self.assertRaises(BadRevisionError, Version, "1-") |
| 289 | + """Version should not allow an empty revision.""" |
| 290 | + v = Version("1-") |
| 291 | + self.assertEquals("1-", v.upstream_version) |
| 292 | + self.assertEquals(None, v.debian_version) |
| 293 | |
| 294 | def testRevisionInvalid(self): |
| 295 | """Version should fail when revision contains a bad character.""" |
| 296 | - from lp.archivepublisher.debversion import Version, BadRevisionError |
| 297 | - self.assertRaises(BadRevisionError, Version, "1-!") |
| 298 | + self.assertRaises(VersionError, Version, "1-!") |
| 299 | |
| 300 | def testValues(self): |
| 301 | """Version should give same input as output.""" |
| 302 | - from lp.archivepublisher.debversion import Version |
| 303 | for value in self.VALUES: |
| 304 | result = str(Version(value)) |
| 305 | self.assertEquals(value, result) |
| 306 | |
| 307 | def testComparisons(self): |
| 308 | """Sample Version comparisons should pass.""" |
| 309 | - from lp.archivepublisher.debversion import Version |
| 310 | for x, y in self.COMPARISONS: |
| 311 | self.failUnless(Version(x) < Version(y)) |
| 312 | |
| 313 | def testNullEpochIsZero(self): |
| 314 | """Version should treat an omitted epoch as a zero one.""" |
| 315 | - from lp.archivepublisher.debversion import Version |
| 316 | - self.failUnless(Version("1.0") == Version("0:1.0")) |
| 317 | - |
| 318 | - def testNullRevisionIsZero(self): |
| 319 | - """Version should treat an omitted revision as a zero one. |
| 320 | - |
| 321 | - NOTE: This isn't what Policy says! Policy says that an omitted |
| 322 | - revision should compare less than the presence of one, whatever |
| 323 | - its value. |
| 324 | - |
| 325 | - The implementation (dpkg) disagrees, and considers an omitted |
| 326 | - revision equal to a zero one. I'm obviously biased as to which |
| 327 | - this module obeys. |
| 328 | + self.assertEquals(Version("1.0"), Version("0:1.0")) |
| 329 | + |
| 330 | + def notestNullRevisionIsZero(self): |
| 331 | + """Version should treat an omitted revision as being equal to zero. |
| 332 | """ |
| 333 | - from lp.archivepublisher.debversion import Version |
| 334 | - self.failUnless(Version("1.0") == Version("1.0-0")) |
| 335 | - |
| 336 | - def testWithoutEpoch(self): |
| 337 | - """Version.without_epoch returns version without epoch.""" |
| 338 | - from lp.archivepublisher.debversion import Version |
| 339 | - self.assertEquals(Version("1:2.0").without_epoch, "2.0") |
| 340 | - |
| 341 | - |
| 342 | -class Strcut(unittest.TestCase): |
| 343 | - def testNoMatch(self): |
| 344 | - """str_cut works when initial characters aren't accepted.""" |
| 345 | - from lp.archivepublisher.debversion import strcut |
| 346 | - self.assertEquals(strcut("foo", 0, "gh"), ( "", 0 )) |
| 347 | - |
| 348 | - def testSingleMatch(self): |
| 349 | - """str_cut matches single initial character.""" |
| 350 | - from lp.archivepublisher.debversion import strcut |
| 351 | - self.assertEquals(strcut("foo", 0, "fgh"), ( "f", 1 )) |
| 352 | - |
| 353 | - def testMultipleMatch(self): |
| 354 | - """str_cut matches multiple initial characters.""" |
| 355 | - from lp.archivepublisher.debversion import strcut |
| 356 | - self.assertEquals(strcut("foobar", 0, "fo"), ( "foo", 3 )) |
| 357 | - |
| 358 | - def testCompleteMatch(self): |
| 359 | - """str_cut works when all characters match.""" |
| 360 | - from lp.archivepublisher.debversion import strcut |
| 361 | - self.assertEquals(strcut("foo", 0, "fo"), ( "foo", 3 )) |
| 362 | - |
| 363 | - def testNonMiddleMatch(self): |
| 364 | - """str_cut doesn't match characters that aren't at the start.""" |
| 365 | - from lp.archivepublisher.debversion import strcut |
| 366 | - self.assertEquals(strcut("barfooquux", 0, "fo"), ( "", 0 )) |
| 367 | - |
| 368 | - def testIndexMatch(self): |
| 369 | - """str_cut matches characters from middle when index given.""" |
| 370 | - from lp.archivepublisher.debversion import strcut |
| 371 | - self.assertEquals(strcut("barfooquux", 3, "fo"), ( "foo", 6 )) |
| 372 | - |
| 373 | - |
| 374 | -class DebOrder(unittest.TestCase): |
| 375 | - # Non-tilde characters in order |
| 376 | - CHARS = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz+-.:" |
| 377 | - |
| 378 | - def testTilde(self): |
| 379 | - """deb_order returns -1 for a tilde.""" |
| 380 | - from lp.archivepublisher.debversion import deb_order |
| 381 | - self.assertEquals(deb_order("~", 0), -1) |
| 382 | - |
| 383 | - def testCharacters(self): |
| 384 | - """deb_order returns positive for other characters.""" |
| 385 | - from lp.archivepublisher.debversion import deb_order |
| 386 | - for char in self.CHARS: |
| 387 | - self.failUnless(deb_order(char, 0) > 0) |
| 388 | - |
| 389 | - def testCharacterOrder(self): |
| 390 | - """deb_order returns characters in correct order.""" |
| 391 | - from lp.archivepublisher.debversion import deb_order |
| 392 | - last = None |
| 393 | - for char in self.CHARS: |
| 394 | - if last is not None: |
| 395 | - self.failUnless(deb_order(char, 0) > deb_order(last, 0)) |
| 396 | - last = char |
| 397 | - |
| 398 | - def testOvershoot(self): |
| 399 | - """deb_order returns zero if idx is longer than the string.""" |
| 400 | - from lp.archivepublisher.debversion import deb_order |
| 401 | - self.assertEquals(deb_order("foo", 10), 0) |
| 402 | - |
| 403 | - def testEmptyString(self): |
| 404 | - """deb_order returns zero if given empty string.""" |
| 405 | - from lp.archivepublisher.debversion import deb_order |
| 406 | - self.assertEquals(deb_order("", 0), 0) |
| 407 | - |
| 408 | - |
| 409 | -class DebCmpStr(unittest.TestCase): |
| 410 | - # Sample strings |
| 411 | - VALUES = ( |
| 412 | - "foo", |
| 413 | - "FOO", |
| 414 | - "Foo", |
| 415 | - "foo+bar", |
| 416 | - "foo-bar", |
| 417 | - "foo.bar", |
| 418 | - "foo:bar" |
| 419 | - ) |
| 420 | - |
| 421 | - # Non-letter characters in order |
| 422 | - CHARS = "+-.:" |
| 423 | - |
| 424 | - def testEmptyStrings(self): |
| 425 | - """deb_cmp_str returns zero when given empty strings.""" |
| 426 | - from lp.archivepublisher.debversion import deb_cmp_str |
| 427 | - self.assertEquals(deb_cmp_str("", ""), 0) |
| 428 | - |
| 429 | - def testFirstEmptyString(self): |
| 430 | - """deb_cmp_str returns -1 when first string is empty.""" |
| 431 | - from lp.archivepublisher.debversion import deb_cmp_str |
| 432 | - self.assertEquals(deb_cmp_str("", "foo"), -1) |
| 433 | - |
| 434 | - def testSecondEmptyString(self): |
| 435 | - """deb_cmp_str returns 1 when second string is empty.""" |
| 436 | - from lp.archivepublisher.debversion import deb_cmp_str |
| 437 | - self.assertEquals(deb_cmp_str("foo", ""), 1) |
| 438 | - |
| 439 | - def testTildeEmptyString(self): |
| 440 | - """deb_cmp_str returns -1 when tilde compared to empty string.""" |
| 441 | - from lp.archivepublisher.debversion import deb_cmp_str |
| 442 | - self.assertEquals(deb_cmp_str("~", ""), -1) |
| 443 | - |
| 444 | - def testLongerFirstString(self): |
| 445 | - """deb_cmp_str returns 1 when first string is longer.""" |
| 446 | - from lp.archivepublisher.debversion import deb_cmp_str |
| 447 | - self.assertEquals(deb_cmp_str("foobar", "foo"), 1) |
| 448 | - |
| 449 | - def testLongerSecondString(self): |
| 450 | - """deb_cmp_str returns -1 when second string is longer.""" |
| 451 | - from lp.archivepublisher.debversion import deb_cmp_str |
| 452 | - self.assertEquals(deb_cmp_str("foo", "foobar"), -1) |
| 453 | - |
| 454 | - def testTildeTail(self): |
| 455 | - """deb_cmp_str returns -1 when first string is longer by a tilde.""" |
| 456 | - from lp.archivepublisher.debversion import deb_cmp_str |
| 457 | - self.assertEquals(deb_cmp_str("foo~", "foo"), -1) |
| 458 | - |
| 459 | - def testIdenticalString(self): |
| 460 | - """deb_cmp_str returns 0 when given identical strings.""" |
| 461 | - from lp.archivepublisher.debversion import deb_cmp_str |
| 462 | - for value in self.VALUES: |
| 463 | - self.assertEquals(deb_cmp_str(value, value), 0) |
| 464 | - |
| 465 | - def testNonIdenticalString(self): |
| 466 | - """deb_cmp_str returns non-zero when given non-identical strings.""" |
| 467 | - from lp.archivepublisher.debversion import deb_cmp_str |
| 468 | - last = self.VALUES[-1] |
| 469 | - for value in self.VALUES: |
| 470 | - self.assertNotEqual(deb_cmp_str(last, value), 0) |
| 471 | - last = value |
| 472 | - |
| 473 | - def testIdenticalTilde(self): |
| 474 | - """deb_cmp_str returns 0 when given identical tilded strings.""" |
| 475 | - from lp.archivepublisher.debversion import deb_cmp_str |
| 476 | - self.assertEquals(deb_cmp_str("foo~", "foo~"), 0) |
| 477 | - |
| 478 | - def testUppercaseLetters(self): |
| 479 | - """deb_cmp_str orders upper case letters in alphabetical order.""" |
| 480 | - from lp.archivepublisher.debversion import deb_cmp_str |
| 481 | - last = "A" |
| 482 | - for value in range(ord("B"), ord("Z")): |
| 483 | - self.assertEquals(deb_cmp_str(last, chr(value)), -1) |
| 484 | - last = chr(value) |
| 485 | - |
| 486 | - def testLowercaseLetters(self): |
| 487 | - """deb_cmp_str orders lower case letters in alphabetical order.""" |
| 488 | - from lp.archivepublisher.debversion import deb_cmp_str |
| 489 | - last = "a" |
| 490 | - for value in range(ord("b"), ord("z")): |
| 491 | - self.assertEquals(deb_cmp_str(last, chr(value)), -1) |
| 492 | - last = chr(value) |
| 493 | - |
| 494 | - def testLowerGreaterThanUpper(self): |
| 495 | - """deb_cmp_str orders lower case letters after upper case.""" |
| 496 | - from lp.archivepublisher.debversion import deb_cmp_str |
| 497 | - self.assertEquals(deb_cmp_str("a", "Z"), 1) |
| 498 | - |
| 499 | - def testCharacters(self): |
| 500 | - """deb_cmp_str orders characters in prescribed order.""" |
| 501 | - from lp.archivepublisher.debversion import deb_cmp_str |
| 502 | - chars = list(self.CHARS) |
| 503 | - last = chars.pop(0) |
| 504 | - for char in chars: |
| 505 | - self.assertEquals(deb_cmp_str(last, char), -1) |
| 506 | - last = char |
| 507 | - |
| 508 | - def testCharactersGreaterThanLetters(self): |
| 509 | - """deb_cmp_str orders characters above letters.""" |
| 510 | - from lp.archivepublisher.debversion import deb_cmp_str |
| 511 | - self.assertEquals(deb_cmp_str(self.CHARS[0], "z"), 1) |
| 512 | - |
| 513 | - |
| 514 | -class DebCmp(unittest.TestCase): |
| 515 | - def testEmptyString(self): |
| 516 | - """deb_cmp returns 0 for the empty string.""" |
| 517 | - from lp.archivepublisher.debversion import deb_cmp |
| 518 | - self.assertEquals(deb_cmp("", ""), 0) |
| 519 | - |
| 520 | - def testStringCompare(self): |
| 521 | - """deb_cmp compares initial string portions correctly.""" |
| 522 | - from lp.archivepublisher.debversion import deb_cmp |
| 523 | - self.assertEquals(deb_cmp("a", "b"), -1) |
| 524 | - self.assertEquals(deb_cmp("b", "a"), 1) |
| 525 | - |
| 526 | - def testNumericCompare(self): |
| 527 | - """deb_cmp compares numeric portions correctly.""" |
| 528 | - from lp.archivepublisher.debversion import deb_cmp |
| 529 | - self.assertEquals(deb_cmp("foo1", "foo2"), -1) |
| 530 | - self.assertEquals(deb_cmp("foo2", "foo1"), 1) |
| 531 | - self.assertEquals(deb_cmp("foo200", "foo5"), 1) |
| 532 | - |
| 533 | - def testMissingNumeric(self): |
| 534 | - """deb_cmp treats missing numeric as zero.""" |
| 535 | - from lp.archivepublisher.debversion import deb_cmp |
| 536 | - self.assertEquals(deb_cmp("foo", "foo0"), 0) |
| 537 | - self.assertEquals(deb_cmp("foo", "foo1"), -1) |
| 538 | - self.assertEquals(deb_cmp("foo1", "foo"), 1) |
| 539 | - |
| 540 | - def testEmptyStringPortion(self): |
| 541 | - """deb_cmp works when string potion is empty.""" |
| 542 | - from lp.archivepublisher.debversion import deb_cmp |
| 543 | - self.assertEquals(deb_cmp("100", "foo100"), -1) |
| 544 | - |
| 545 | - |
| 546 | -def test_suite(): |
| 547 | - return unittest.TestLoader().loadTestsFromName(__name__) |
| 548 | + # XXX JRV 20100312: Disabled at the moment because this fails because of |
| 549 | + # a bug in apt. (http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=573592) |
| 550 | + self.assertEquals(Version("1.0"), Version("1.0-0")) |
| 551 | |
| 552 | === modified file 'lib/lp/soyuz/scripts/buildd.py' |
| 553 | --- lib/lp/soyuz/scripts/buildd.py 2010-05-20 14:28:51 +0000 |
| 554 | +++ lib/lp/soyuz/scripts/buildd.py 2010-06-18 10:12:40 +0000 |
| 555 | @@ -26,18 +26,6 @@ |
| 556 | from lp.soyuz.interfaces.binarypackagebuild import IBinaryPackageBuildSet |
| 557 | from lp.soyuz.pas import BuildDaemonPackagesArchSpecific |
| 558 | |
| 559 | -# XXX cprov 2009-04-16: This function should live in |
| 560 | -# lp.registry.interfaces.distroseries. It cannot be done right now |
| 561 | -# because we haven't decided if archivepublisher.debversion will be |
| 562 | -# released as FOSS yet. |
| 563 | -def distroseries_sort_key(series): |
| 564 | - """Sort `DistroSeries` by version. |
| 565 | - |
| 566 | - See `lp.archivepublisher.debversion.Version` for more |
| 567 | - information. |
| 568 | - """ |
| 569 | - return Version(series.version) |
| 570 | - |
| 571 | |
| 572 | class QueueBuilder(LaunchpadCronScript): |
| 573 | |
| 574 | @@ -219,7 +207,8 @@ |
| 575 | raise LaunchpadScriptFailure("Could not find suite %s" % err) |
| 576 | distroseries_set.add(distroseries) |
| 577 | |
| 578 | - return sorted(distroseries_set, key=distroseries_sort_key) |
| 579 | + return sorted(distroseries_set, |
| 580 | + key=lambda series: Version(series.version)) |
| 581 | |
| 582 | |
| 583 | class RetryDepwait(LaunchpadCronScript): |
Hi Jelmer,
overall, your changes look good. But I get a test failure when I run ./bin/test -t test_debversion:
Failure in test testNullRevisio
Traceback (most recent call last):
File "/usr/lib/
testMethod()
File "/home/
self.
File "/usr/lib/
(msg or '%r != %r' % (first, second))
AssertionError: Version('1.0') != Version('1.0-0')
Are you sure that the new, more strict, comparison does not have any bad side effects?
(And if
Version('1.0') != Version('1.0-0')
works in Launchpad, I think you can remove this test.)