[Freeipa-devel] [PATCH 0122-0123] reimplementation of package version comparison code
Jan Cholasta
jcholast at redhat.com
Mon Jan 11 13:01:02 UTC 2016
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 <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.
>>
>
> 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
More information about the Freeipa-devel
mailing list