[Freeipa-devel] [PATCH 0122-0123] reimplementation of package version comparison code
Jan Cholasta
jcholast at redhat.com
Mon Jan 11 16:58:28 UTC 2016
On 11.1.2016 16:29, Martin Babinsky wrote:
> 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.
Patch 0122: ACK
Pushed to:
master: 7cd99e852053710c64dcb66cd5b15fc8ed4da5de
ipa-4-2: be9af721536b7c05e2ba5ffd7d72cc3727d64ff5
ipa-4-3: 6b6a11fda76e70d390d5c55f3aaeba8f455458c0
Patch 0123: LGTM
--
Jan Cholasta
More information about the Freeipa-devel
mailing list