[Freeipa-devel] [PATCH 0122-0123] reimplementation of package version comparison code
Martin Basti
mbasti at redhat.com
Tue Jan 12 14:34:17 UTC 2016
On 12.01.2016 13:14, Martin Babinsky wrote:
> On 01/12/2016 01:03 PM, Martin Basti wrote:
>>
>>
>> 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?
>>
>
> Seems like I forgot how to english as well. Attaching updated patch.
>
ACK
Pushed to:
master: ac76644ff6507d6eb792d54148ec8716303774ce
ipa-4-3: e3970bf4ff4af80fb4164cff11a2973c1c951ad8
More information about the Freeipa-devel
mailing list