[Freeipa-devel] [PATCH 0122-0123] reimplementation of package version comparison code
Martin Babinsky
mbabinsk at redhat.com
Mon Jan 11 13:27:11 UTC 2016
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.
--
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/3cf9e62b/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-mbabinsk-0122.2-use-FFI-call-to-rpmvercmp-function-for-version-compa.patch
Type: text/x-patch
Size: 4369 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160111/3cf9e62b/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-4-2-mbabinsk-0122.2-use-FFI-call-to-rpmvercmp-function-for-version-compa.patch
Type: text/x-patch
Size: 4111 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160111/3cf9e62b/attachment-0002.bin>
More information about the Freeipa-devel
mailing list