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 <[email protected]> >>>>>>>>> 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).
+1 We already tried and failed in custom parsing (presumably caused by request to not add any new dependency). I agree with Martin that copy&pasting code does not look like a proper fix. -- 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
