On 01/11/2016 07:47 AM, Jan Cholasta wrote:
On 8.1.2016 18:30, Lukas Slebodnik wrote:
On (08/01/16 17:56), Martin Babinsky wrote:
On 01/08/2016 05:23 PM, Lukas Slebodnik wrote:
On (08/01/16 16:59), Martin Babinsky wrote:
Patch 0122 reimplements version checking and fixes
https://fedorahosted.org/freeipa/ticket/5572
Patch 0123 contains unit test for version checking code.
Thanks to Martin^1 for the idea of using CFFI for calling rpm C-API
directly.
--
Martin^3 Babinsky
>From c7a5d8970d100d071597b4e1d7cef8a27b8cd485 Mon Sep 17 00:00:00 2001
From: Martin Babinsky <mbabi...@redhat.com>
Date: Fri, 8 Jan 2016 15:54:00 +0100
Subject: [PATCH 2/3] tests for package version comparison
These tests will ensure that our package version handling code can
correctly
decide when to upgrade IPA master.
https://fedorahosted.org/freeipa/ticket/5572
---
ipatests/test_ipaserver/test_version_comparsion.py | 47
++++++++++++++++++++++
1 file changed, 47 insertions(+)
create mode 100644 ipatests/test_ipaserver/test_version_comparsion.py
diff --git a/ipatests/test_ipaserver/test_version_comparsion.py
b/ipatests/test_ipaserver/test_version_comparsion.py
new file mode 100644
index
0000000000000000000000000000000000000000..51d069b23ba389ffce39e948cdbc7a1faaa84563
--- /dev/null
+++ b/ipatests/test_ipaserver/test_version_comparsion.py
@@ -0,0 +1,47 @@
+#
+# Copyright (C) 2015 FreeIPA Contributors see COPYING for license
+#
+
+"""
+tests for correct RPM version comparison
+"""
+
+from ipaplatform.tasks import tasks
+import pytest
+
+version_strings = [
+ ("3.0.el6", "3.0.0.el6", "older"),
+ ("3.0.0.el6", "3.0.0.el6_8.2", "older"),
+ ("3.0.0-42.el6", "3.0.0.el6", "newer"),
+ ("3.0.0-1", "3.0.0-42", "older"),
+ ("3.0.0-42.el6", "3.3.3.fc20", "older"),
+ ("4.2.0-15.el7", "4.2.0-15.el7_2.3", "older"),
+ ("4.2.0-15.el7_2.3", "4.2.0-15.el7_2.3", "equal"),
+ ("4.2.0-1.fc23", "4.2.1.fc23", "older"),
+ ("4.2.3-alpha.fc23", "4.2.3-2.fc23", "older"), # numeric
version elements have
+ # precedence over
letters
+ ("4.3.90.201601080923GIT55aeea7-0.fc23", "4.3.0-1.fc23", "newer")
+]
+
+
+@pytest.fixture(params=version_strings)
+def versions(request):
+ return request.param
+
+class TestVersionComparsion(object):
+
+ def test_versions(self, versions):
+ version_string1, version_string2, expected_comparison =
versions
+
+ ver1 = tasks.parse_ipa_version(version_string1)
+ ver2 = tasks.parse_ipa_version(version_string2)
+
+ if expected_comparison == "newer":
+ assert ver1 > ver2
+ elif expected_comparison == "older":
+ assert ver1 < ver2
+ elif expected_comparison == "equal":
+ assert ver1 == ver2
+ else:
+ raise TypeError(
+ "Unexpected comparison string: {}",
expected_comparison)
--
2.5.0
>From 9677e1a3ca2f5837f1b779780127adf27efa81df Mon Sep 17 00:00:00 2001
From: Martin Babinsky <mbabi...@redhat.com>
Date: Fri, 8 Jan 2016 14:17:14 +0100
Subject: [PATCH 1/3] use FFI call to rpmvercmp function for version
comparison
Stop using rpm-python to compare package versions since the
implicit NSS
initialization upon the module import breaks NSS handling in IPA
code. Call
rpm-libs C-API function via CFFI instead.
Big thanks to Martin Kosek <mko...@redhat.com> for sharing the code
snippet
that spurred this patch.
https://fedorahosted.org/freeipa/ticket/5572
---
freeipa.spec.in | 2 +-
ipaplatform/redhat/tasks.py | 59
+++++++++++++++++++++------------------------
2 files changed, 29 insertions(+), 32 deletions(-)
diff --git a/freeipa.spec.in b/freeipa.spec.in
index
7e956538d0f6c24bab636579303e0c7b5eeec199..7f31d41d16b2a26b404c277595f0994a21123f80
100644
--- a/freeipa.spec.in
+++ b/freeipa.spec.in
@@ -214,7 +214,7 @@ Requires: python-pyasn1
Requires: dbus-python
Requires: python-dns >= 1.11.1
Requires: python-kdcproxy >= 0.3
-Requires: rpm-python
+Requires: rpm-devel
/usr/lib64/librpm.so.7 is provided by package rpm-libs
sh$ rpm -qf /usr/lib64/librpm.so.7
rpm-libs-4.13.0-0.rc1.7.fc23.x86_64
It's not very common to depend on devel packages.
LS
I was basically trying workaround this
http://fpaste.org/308652/22677521/
rpm-libs contains librpm.so.*
and cffi is smart enough to load right library with
C = ffi.dlopen("rpm")
This likely won't be an issue here, but in general, we should use the
versioned library name to have at least some API/ABI guarantee. If for
example a function we depend on changed signature between versions, we
would crash *hard* when it's called.
So it's enough to add "Requires: rpm-libs"
but it would be almost noop because rpm-libs is everytime available
od fedora/rhel :-)
"Requires: rpm-devel"
add unnecessary additional runtime dependencies on pkgconfig, popt-devel
librpm.so.7 belongs to rpm-libs and librpm.so to rpm-devel and I was
lazy
(hey it's friday) to add path to librpm.so.7 to paths and use it i LD
loader.
If you think that it is not optimal I will fix it, but let's wait for
some
more feedback.
Sure wait for another python related comments.
NACK. The ffi.cdef() and ffi.dlopen() should be in the module scope, and
the ffi.new() calls are unnecessary.
Attaching updated patches. I have added specific versions of librpm
shared lib to platform specific namespaces. Both of them were tested and
work. I have also change dependency to rpm-libs.
--
Martin^3 Babinsky
From aefcc00f4150e4423b1631c6236563585f16cbd5 Mon Sep 17 00:00:00 2001
From: Martin Babinsky <mbabi...@redhat.com>
Date: Fri, 8 Jan 2016 15:54:00 +0100
Subject: [PATCH 2/2] tests for package version comparison
These tests will ensure that our package version handling code can correctly
decide when to upgrade IPA master.
https://fedorahosted.org/freeipa/ticket/5572
---
ipatests/test_ipaserver/test_version_comparsion.py | 50 ++++++++++++++++++++++
1 file changed, 50 insertions(+)
create mode 100644 ipatests/test_ipaserver/test_version_comparsion.py
diff --git a/ipatests/test_ipaserver/test_version_comparsion.py b/ipatests/test_ipaserver/test_version_comparsion.py
new file mode 100644
index 0000000000000000000000000000000000000000..7650c8e6b7b7b5c8147ab910e0fa65acfb018fc0
--- /dev/null
+++ b/ipatests/test_ipaserver/test_version_comparsion.py
@@ -0,0 +1,50 @@
+#
+# Copyright (C) 2015 FreeIPA Contributors see COPYING for license
+#
+
+"""
+tests for correct RPM version comparison
+"""
+
+from ipaplatform.tasks import tasks
+import pytest
+
+version_strings = [
+ ("3.0.0-1.el6", "3.0.0-2.el6", "older"),
+ ("3.0.0-1.el6_8", "3.0.0-1.el6_8.1", "older"),
+ ("3.0.0-42.el6", "3.0.0-1.el6", "newer"),
+ ("3.0.0-1.el6", "3.0.0-42.el6", "older"),
+ ("3.0.0-42.el6", "3.3.3-1.fc20", "older"),
+ ("4.2.0-15.el7", "4.2.0-15.el7_2.3", "older"),
+ ("4.2.0-15.el7_2", "4.2.0-15.el7_2.3", "older"),
+ ("4.2.0-15.el7_2.3", "4.2.0-15.el7_2.3", "equal"),
+ ("4.2.0-15.el7_2.3", "4.2.0-15.el7_2.2", "newer"),
+ ("4.2.0-1.fc23", "4.2.1-1.fc23", "older"),
+ ("4.2.3-alpha1.fc23", "4.2.3-2.fc23", "older"), # numeric version elements
+ # have precedence over
+ # non-numeric ones
+ ("4.3.90.201601080923GIT55aeea7-0.fc23", "4.3.0-1.fc23", "newer")
+]
+
+
+@pytest.fixture(params=version_strings)
+def versions(request):
+ return request.param
+
+class TestVersionComparsion(object):
+
+ def test_versions(self, versions):
+ version_string1, version_string2, expected_comparison = versions
+
+ ver1 = tasks.parse_ipa_version(version_string1)
+ ver2 = tasks.parse_ipa_version(version_string2)
+
+ if expected_comparison == "newer":
+ assert ver1 > ver2
+ elif expected_comparison == "older":
+ assert ver1 < ver2
+ elif expected_comparison == "equal":
+ assert ver1 == ver2
+ else:
+ raise TypeError(
+ "Unexpected comparison string: {}", expected_comparison)
--
2.5.0
From bdd765b22fc244df80c291a80c25e46ca6706d12 Mon Sep 17 00:00:00 2001
From: Martin Babinsky <mbabi...@redhat.com>
Date: Fri, 8 Jan 2016 14:17:14 +0100
Subject: [PATCH 1/2] use FFI call to rpmvercmp function for version comparison
Stop using rpm-python to compare package versions since the implicit NSS
initialization upon the module import breaks NSS handling in IPA code. Call
rpm-libs C-API function via CFFI instead.
Big thanks to Martin Kosek <mko...@redhat.com> for sharing the code snippet
that spurred this patch.
https://fedorahosted.org/freeipa/ticket/5572
---
freeipa.spec.in | 2 +-
ipaplatform/fedora/paths.py | 2 +-
ipaplatform/redhat/paths.py | 1 +
ipaplatform/redhat/tasks.py | 57 +++++++++++++++++++++------------------------
4 files changed, 29 insertions(+), 33 deletions(-)
diff --git a/freeipa.spec.in b/freeipa.spec.in
index 7e956538d0f6c24bab636579303e0c7b5eeec199..de3ae2f03c303daacdb97441065c14bec8af1d06 100644
--- a/freeipa.spec.in
+++ b/freeipa.spec.in
@@ -214,7 +214,7 @@ Requires: python-pyasn1
Requires: dbus-python
Requires: python-dns >= 1.11.1
Requires: python-kdcproxy >= 0.3
-Requires: rpm-python
+Requires: rpm-libs
%description -n python2-ipaserver
IPA is an integrated solution to provide centrally managed Identity (users,
diff --git a/ipaplatform/fedora/paths.py b/ipaplatform/fedora/paths.py
index 49a904f2f271097d75be235eb30d614dc1bb41ac..46139565594a21b2e9c895b24d9327d2d677dfd3 100644
--- a/ipaplatform/fedora/paths.py
+++ b/ipaplatform/fedora/paths.py
@@ -27,7 +27,7 @@ from ipaplatform.redhat.paths import RedHatPathNamespace
class FedoraPathNamespace(RedHatPathNamespace):
- pass
+ LIB64_LIBRPM_SO = "/usr/lib64/librpm.so.7"
paths = FedoraPathNamespace()
diff --git a/ipaplatform/redhat/paths.py b/ipaplatform/redhat/paths.py
index b80a1b47a972698bb5a93a45d15460e33fd5c700..a7afae5a871f81f4c8ae22f9ed70c6ba065d2e7b 100644
--- a/ipaplatform/redhat/paths.py
+++ b/ipaplatform/redhat/paths.py
@@ -32,6 +32,7 @@ class RedHatPathNamespace(BasePathNamespace):
# https://docs.python.org/2/library/platform.html#cross-platform
if sys.maxsize > 2**32:
LIBSOFTHSM2_SO = BasePathNamespace.LIBSOFTHSM2_SO_64
+ LIB64_LIBRPM_SO = "/usr/lib64/librpm.so.3"
paths = RedHatPathNamespace()
diff --git a/ipaplatform/redhat/tasks.py b/ipaplatform/redhat/tasks.py
index a0b4060cb26bab66248c4397c24b4d58bf1bf8d6..eed889e20979ffc4e6df6a1d187f1baa7f9d672a 100644
--- a/ipaplatform/redhat/tasks.py
+++ b/ipaplatform/redhat/tasks.py
@@ -30,13 +30,13 @@ import stat
import socket
import sys
import base64
+from cffi import FFI
from functools import total_ordering
from subprocess import CalledProcessError
from nss.error import NSPRError
from pyasn1.error import PyAsn1Error
from six.moves import urllib
-import rpm
from ipapython.ipa_log_manager import root_logger, log_mgr
from ipapython import ipautil
@@ -48,34 +48,29 @@ from ipaplatform.paths import paths
from ipaplatform.redhat.authconfig import RedHatAuthConfig
from ipaplatform.base.tasks import BaseTaskNamespace
+_ffi = FFI()
+_ffi.cdef("""
+int rpmvercmp (const char *a, const char *b);
+""")
-# copied from rpmUtils/miscutils.py
-def stringToVersion(verstring):
- if verstring in [None, '']:
- return (None, None, None)
- i = verstring.find(':')
- if i != -1:
- try:
- epoch = str(long(verstring[:i]))
- except ValueError:
- # look, garbage in the epoch field, how fun, kill it
- epoch = '0' # this is our fallback, deal
- else:
- epoch = '0'
- j = verstring.find('-')
- if j != -1:
- if verstring[i + 1:j] == '':
- version = None
- else:
- version = verstring[i + 1:j]
- release = verstring[j + 1:]
- else:
- if verstring[i + 1:] == '':
- version = None
- else:
- version = verstring[i + 1:]
- release = None
- return (epoch, version, release)
+_librpm = _ffi.dlopen(paths.LIB64_LIBRPM_SO)
+
+
+def compare_rpm_version_strings(ver1, ver2):
+ """
+ compare two RPM version string using a FFI call to rpmvercmp() API function
+
+ :param ver1: version string of the first package
+ :param ver2: version string of the second package
+ :returns the same as rpmvercmp(), i.e 1 when ver1 is newer than ver 2,
+ 0 when they are the same and -1 when ver2 is newer that ver1
+
+ Note that we cannot really use rpm python bindings for this, since
+ importing them messes with NSS library initialization and breaks our
+ certificate manipulation/validation functions
+ """
+
+ return _librpm.rpmvercmp(ver1, ver2)
log = log_mgr.get_logger(__name__)
@@ -101,15 +96,15 @@ def selinux_enabled():
class IPAVersion(object):
def __init__(self, version):
- self.version_tuple = stringToVersion(version)
+ self.version = version
def __eq__(self, other):
assert isinstance(other, IPAVersion)
- return rpm.labelCompare(self.version_tuple, other.version_tuple) == 0
+ return compare_rpm_version_strings(self.version, other.version) == 0
def __lt__(self, other):
assert isinstance(other, IPAVersion)
- return rpm.labelCompare(self.version_tuple, other.version_tuple) == -1
+ return compare_rpm_version_strings(self.version, other.version) == -1
class RedHatTaskNamespace(BaseTaskNamespace):
--
2.5.0
From 785ba475292ee107876973c0fae33cc7414bec67 Mon Sep 17 00:00:00 2001
From: Martin Babinsky <mbabi...@redhat.com>
Date: Fri, 8 Jan 2016 14:17:14 +0100
Subject: [PATCH] use FFI call to rpmvercmp function for version comparison
Stop using rpm-python to compare package versions since the implicit NSS
initialization upon the module import breaks NSS handling in IPA code. Call
rpm-libs C-API function via CFFI instead.
Big thanks to Martin Kosek <mko...@redhat.com> for sharing the code snippet
that spurred this patch.
https://fedorahosted.org/freeipa/ticket/5572
---
freeipa.spec.in | 2 +-
ipaplatform/fedora/paths.py | 2 +-
ipaplatform/redhat/paths.py | 1 +
ipaplatform/redhat/tasks.py | 57 +++++++++++++++++++++------------------------
4 files changed, 29 insertions(+), 33 deletions(-)
diff --git a/freeipa.spec.in b/freeipa.spec.in
index bbf3db7a1cfc6d5a7a1b43edeb0a799875386f77..dcb8607eac2a180817381e439f4f02f574fe3886 100644
--- a/freeipa.spec.in
+++ b/freeipa.spec.in
@@ -159,7 +159,7 @@ Requires: p11-kit
Requires: systemd-python
Requires: %{etc_systemd_dir}
Requires: gzip
-Requires: rpm-python
+Requires: rpm-libs
Conflicts: %{alt_name}-server
Obsoletes: %{alt_name}-server < %{version}
diff --git a/ipaplatform/fedora/paths.py b/ipaplatform/fedora/paths.py
index 49a904f2f271097d75be235eb30d614dc1bb41ac..46139565594a21b2e9c895b24d9327d2d677dfd3 100644
--- a/ipaplatform/fedora/paths.py
+++ b/ipaplatform/fedora/paths.py
@@ -27,7 +27,7 @@ from ipaplatform.redhat.paths import RedHatPathNamespace
class FedoraPathNamespace(RedHatPathNamespace):
- pass
+ LIB64_LIBRPM_SO = "/usr/lib64/librpm.so.7"
paths = FedoraPathNamespace()
diff --git a/ipaplatform/redhat/paths.py b/ipaplatform/redhat/paths.py
index b80a1b47a972698bb5a93a45d15460e33fd5c700..a7afae5a871f81f4c8ae22f9ed70c6ba065d2e7b 100644
--- a/ipaplatform/redhat/paths.py
+++ b/ipaplatform/redhat/paths.py
@@ -32,6 +32,7 @@ class RedHatPathNamespace(BasePathNamespace):
# https://docs.python.org/2/library/platform.html#cross-platform
if sys.maxsize > 2**32:
LIBSOFTHSM2_SO = BasePathNamespace.LIBSOFTHSM2_SO_64
+ LIB64_LIBRPM_SO = "/usr/lib64/librpm.so.3"
paths = RedHatPathNamespace()
diff --git a/ipaplatform/redhat/tasks.py b/ipaplatform/redhat/tasks.py
index 2e894d776dcd5542e6c11cc0210add8ad9d90298..47ae647e5947d67e9c0a4b72a5ad87f2f75f62c2 100644
--- a/ipaplatform/redhat/tasks.py
+++ b/ipaplatform/redhat/tasks.py
@@ -30,7 +30,7 @@ import socket
import sys
import urllib
import base64
-import rpm
+from cffi import FFI
from functools import total_ordering
from subprocess import CalledProcessError
@@ -47,34 +47,29 @@ from ipaplatform.paths import paths
from ipaplatform.redhat.authconfig import RedHatAuthConfig
from ipaplatform.base.tasks import BaseTaskNamespace
+_ffi = FFI()
+_ffi.cdef("""
+int rpmvercmp (const char *a, const char *b);
+""")
-# copied from rpmUtils/miscutils.py
-def stringToVersion(verstring):
- if verstring in [None, '']:
- return (None, None, None)
- i = verstring.find(':')
- if i != -1:
- try:
- epoch = str(long(verstring[:i]))
- except ValueError:
- # look, garbage in the epoch field, how fun, kill it
- epoch = '0' # this is our fallback, deal
- else:
- epoch = '0'
- j = verstring.find('-')
- if j != -1:
- if verstring[i + 1:j] == '':
- version = None
- else:
- version = verstring[i + 1:j]
- release = verstring[j + 1:]
- else:
- if verstring[i + 1:] == '':
- version = None
- else:
- version = verstring[i + 1:]
- release = None
- return (epoch, version, release)
+_librpm = _ffi.dlopen(paths.LIB64_LIBRPM_SO)
+
+
+def compare_rpm_version_strings(ver1, ver2):
+ """
+ compare two RPM version string using a FFI call to rpmvercmp() API function
+
+ :param ver1: version string of the first package
+ :param ver2: version string of the second package
+ :returns the same as rpmvercmp(), i.e 1 when ver1 is newer than ver 2,
+ 0 when they are the same and -1 when ver2 is newer that ver1
+
+ Note that we cannot really use rpm python bindings for this, since
+ importing them messes with NSS library initialization and breaks our
+ certificate manipulation/validation functions
+ """
+
+ return _librpm.rpmvercmp(ver1, ver2)
log = log_mgr.get_logger(__name__)
@@ -100,15 +95,15 @@ def selinux_enabled():
class IPAVersion(object):
def __init__(self, version):
- self.version_tuple = stringToVersion(version)
+ self.version = version
def __eq__(self, other):
assert isinstance(other, IPAVersion)
- return rpm.labelCompare(self.version_tuple, other.version_tuple) == 0
+ return compare_rpm_version_strings(self.version, other.version) == 0
def __lt__(self, other):
assert isinstance(other, IPAVersion)
- return rpm.labelCompare(self.version_tuple, other.version_tuple) == -1
+ return compare_rpm_version_strings(self.version, other.version) == -1
class RedHatTaskNamespace(BaseTaskNamespace):
--
2.5.0
--
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
