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:

Patch attached.
>From 8ef93485d61e8732166fb0c5b6c4559209740f3e Mon Sep 17 00:00:00
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

freeipa.spec.in             |  2 ++
ipaplatform/redhat/tasks.py | 19 +++++++++++++++++++
2 files changed, 21 insertions(+)

diff --git a/freeipa.spec.in b/freeipa.spec.in

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

Thanks for this catch, it is actually located in yum package, I
copy stringToVersion function from there to IPA, to avoid

Updated patch attached.

Looking good. The __cmp__ function, however, is not available in
3. As we will eventually support python3 in RHEL as well, maybe we
should make sure even platform-dependent parts are python3
For the future's sake.



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
is newer.  Other exit statuses indicate problems.

ipa-4.2.0-15.el7 < 4.2.0-15.el7_2.3

which could be directly called from __eq__ or __lt__, since we are in
platform specific code anyway already.

Imho we should generally prefer reaching out to a Python library rather
launching a subprocess to compare the versions, it's unnecessary
I would not be too worried about miliseconds longer execution on a
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
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


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

Reply via email to