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

[Martin Basti]

On 22.12.2015 14:10, Martin Basti wrote:
>
>
> On 22.12.2015 14:04, Petr Spacek wrote:
>> 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.
>>
> It seems that copy&paste camp won.
>
> Pushed to:
> master: 91913c5ba7c380fe6456e1c3e35fcbfbecef5ff1
> ipa-4-3: a249de3b00f20956214a6ee0c1d614b972826637
>
>
> Patch for ipa-4-2 needs rebase ... wait for it please
>
> Martin^2
>
Pushed to ipa-4-2: dccdade484698d5ef553f03ce6aab5d9db7a7cf0