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