Code review comment for lp:~jelmer/launchpad/noversion

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 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).

OK, but where and how are the treated as the same package? Or asked the
other way around: _if_ they considered to be identical, isn't the
comparison Version("1.0") == Version("1.0-0") correct? In other words:
Shouldn't we keep the old comparison implementation (or its behaviour)?

Abel