[Freeipa-devel] [PATCH 0373] Upgrade: Fix IPA version comparison

[Martin Basti]

On 14.12.2015 09:24, Martin Kosek wrote:
> On 12/14/2015 07:21 AM, Jan Cholasta wrote:
>> On 11.12.2015 19:01, Tomas Babej wrote:
>>>
>>> On 12/11/2015 09:36 AM, Martin Kosek wrote:
>>>> On 12/10/2015 05:09 PM, Martin Basti wrote:
>>>>>
>>>>> On 10.12.2015 15:49, Tomas Babej wrote:
>>>>>> On 12/10/2015 11:23 AM, Martin Basti wrote:
>>>>>>> On 10.12.2015 09:13, Lukas Slebodnik wrote:
>>>>>>>> On (09/12/15 19:22), Martin Basti wrote:
>>>>>>>>> https://fedorahosted.org/freeipa/ticket/5535
>>>>>>>>>
>>>>>>>>> Patch attached.
>>>>>>>> >From 8ef93485d61e8732166fb0c5b6c4559209740f3e Mon Sep 17 00:00:00
>>>>>>>> 2001
>>>>>>>>> From: Martin Basti <mbasti at redhat.com>
>>>>>>>>> Date: Wed, 9 Dec 2015 18:53:35 +0100
>>>>>>>>> Subject: [PATCH] Fix version comparison
>>>>>>>>>
>>>>>>>>> Use RPM library to compare vendor versions of IPA for redhat platform
>>>>>>>>>
>>>>>>>>> https://fedorahosted.org/freeipa/ticket/5535
>>>>>>>>> ---
>>>>>>>>> freeipa.spec.in             |  2 ++
>>>>>>>>> ipaplatform/redhat/tasks.py | 19 +++++++++++++++++++
>>>>>>>>> 2 files changed, 21 insertions(+)
>>>>>>>>>
>>>>>>>>> diff --git a/freeipa.spec.in b/freeipa.spec.in
>>>>>>>>> index
>>>>>>>>> 9f82b3695fb10c4db65cc31278364b3b34e26098..09feba7b8324f5e645da3e8010de86b6c3ee5ab9
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> 100644
>>>>>>>>> --- a/freeipa.spec.in
>>>>>>>>> +++ b/freeipa.spec.in
>>>>>>>>> @@ -166,6 +166,8 @@ Requires: %{etc_systemd_dir}
>>>>>>>>> Requires: gzip
>>>>>>>>> Requires: python-gssapi >= 1.1.0
>>>>>>>>> Requires: custodia
>>>>>>>>> +Requires: rpm-python
>>>>>>>>> +Requires: rpmdevtools
>>>>>>>> Could you explain why do you need the 2nd package?
>>>>>>>> It does not contains any python modules
>>>>>>>> and I cannot see usage of any binary in this patch
>>>>>>>>
>>>>>>>> LS
>>>>>>> Thanks for this catch, it is actually located in yum package, I rather
>>>>>>> copy stringToVersion function from there to IPA, to avoid dependency
>>>>>>> hell
>>>>>>>
>>>>>>> Updated patch attached.
>>>>>>>
>>>>>>>
>>>>>> Looking good. The __cmp__ function, however, is not available in Python
>>>>>> 3. As we will eventually support python3 in RHEL as well, maybe we
>>>>>> should make sure even platform-dependent parts are python3 compatible?
>>>>>> For the future's sake.
>>>>>>
>>>>>> Tomas
>>>>>>
>>>>> Thanks,
>>>>>
>>>>> python 3 compatible patch attached.
>>>> I wonder why we have to add so much code here and reimplement RPM
>>>> version checking if it is already provided by rpmdevtools:
>>>>
>>>> ~~~
>>>> $ /usr/bin/rpmdev-vercmp ipa-4.2.0-15.el7 4.2.0-15.el7_2.3; echo $?
>>>> WARNING: hyphen in release1: 4.2.0-15.el7
>>>>
>>>> rpmdev-vercmp <epoch1> <ver1> <release1> <epoch2> <ver2> <release2>
>>>> rpmdev-vercmp <EVR1> <EVR2>
>>>> rpmdev-vercmp # with no arguments, prompt
>>>>
>>>> Exit status is 0 if the EVR's are equal, 11 if EVR1 is newer, and 12 if
>>>> EVR2
>>>> is newer.  Other exit statuses indicate problems.
>>>>
>>>> ipa-4.2.0-15.el7 < 4.2.0-15.el7_2.3
>>>> 12
>>>> ~~~
>>>>
>>>> which could be directly called from __eq__ or __lt__, since we are in
>>>> platform specific code anyway already.
>>>>
>>>> Martin
>>> Imho we should generally prefer reaching out to a Python library rather
>>> launching a subprocess to compare the versions, it's unnecessary overhead.
> I would not be too worried about miliseconds longer execution on a function
> called during RPM upgrade.
>
>>> That said, the main argument for the usage of rpm-python over
>>> rpmdevtools I see is that rpm-python is very likely to be present on the
>>> system (i.e. it is yum's own dependency), while rpmdevtools will not be
>>> present by default.
>>>
>>>   From the standpoint of trying to minimize the size of freeipa
>>> installation (i.e recent wget -> curl migration), we should keep the
>>> spirit here and do not introduce a dependency for a collection of
>>> developer tools.
>>>
>>> /2 cents
>> +1, also a single function is hardly "much code".
> Ok then. If you all want to add yet another N-V-R parsing function in the
> FreeIPA code, I can live with it (with raised eyebrows though).

Rebased patch attached.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-mbasti-0373.4-Fix-version-comparison.patch
Type: text/x-patch
Size: 3644 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20151214/167301f9/attachment.bin>