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 <mba...@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.

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

Tomas

-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Reply via email to