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

[Petr Spacek]

On 14.12.2015 14:29, Tomas Babej wrote:
> 
> 
> 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.

Further inspection shows that rpmdevtools depends on Perl stack which seems to
be too much for such a simple thing. So, I'm hesitantly changing my NACK to
ACK for copy&paste camp.

-- 
Petr^2 Spacek