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

[Tomas Babej]

On 12/14/2015 10:21 AM, Martin Basti wrote:
> 
> 
> 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.

I tested the patch, and it works fine - so conditional ACK from me for
the current iteration of the patch, given developer consensus which was
not reached yet.

There's a split of opinions (external binary camp vs. copy&paste camp),
so we need to decide if we both camps are OK with proceeding.

Tomas