[Freeipa-devel] [PATCH 0122-0123] reimplementation of package version comparison code
Martin Babinsky
mbabinsk at redhat.com
Mon Jan 11 15:29:58 UTC 2016
On 01/11/2016 02:27 PM, Martin Babinsky wrote:
> On 01/11/2016 02:01 PM, Jan Cholasta wrote:
>> 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.
>>
>
> Updated patches attached.
>
>
>
Attaching another version.
--
Martin^3 Babinsky
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-mbabinsk-0123.1-tests-for-package-version-comparison.patch
Type: text/x-patch
Size: 2622 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160111/f1f12e0c/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-mbabinsk-0122.3-use-FFI-call-to-rpmvercmp-function-for-version-compa.patch
Type: text/x-patch
Size: 3887 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160111/f1f12e0c/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-4-2-mbabinsk-0122.3-use-FFI-call-to-rpmvercmp-function-for-version-compa.patch
Type: text/x-patch
Size: 3629 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160111/f1f12e0c/attachment-0002.bin>
More information about the Freeipa-devel
mailing list