Re: [Freeipa-devel] [PATCH 0122-0123] reimplementation of package version comparison code

[Jan Cholasta]

On 11.1.2016 13:14, Martin Babinsky wrote:
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.

You haven't tested on any 32 bit architecture, have you? I'm sure /lib64 
will not work there.

Note that when you dlopen just "librpm.so.$N", the correct path 
according to dynamic linker configuration will be used.

I have also change dependency to rpm-libs.

Given librpm is dlopened by full path, I think we should depend directly 
on the "librpm.so.$N()" soname rather than rpm-libs.

--
Jan Cholasta

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