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

[Jan Cholasta]

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 <mbabinsk at 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")
>>>> +]
>>>> +
>>>> +
>>>> + at 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 <mbabinsk at 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 <mkosek at 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.

-- 
Jan Cholasta