[Freeipa-devel] [PATCH 0122-0123] reimplementation of package version comparison code
Martin Basti
mbasti at redhat.com
Tue Jan 12 12:03:18 UTC 2016
On 12.01.2016 10:23, Martin Babinsky wrote:
> On 01/11/2016 06:38 PM, Martin Basti wrote:
>>
>>
>> On 11.01.2016 17:58, Jan Cholasta wrote:
>>> 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
>>>
>> Patch 0123:
>>
>> + raise TypeError(
>> + "Unexpected comparison string: {}",
>> expected_comparison)
>>
>> Did you mean:
>> raise TypeError(
>> "Unexpected comparison string: {}".format(expected_comparison))
>> ?
>
> Sorry I temporarily forgot how to python over there. Attaching updated
> patch.
>
Works for me, but shouldn't be the file named test_version_compaRISOn.py
instead of test_version_compaRSIOn.py?
More information about the Freeipa-devel
mailing list