[Freeipa-devel] [PATCH 0122-0123] reimplementation of package version comparison code
Martin Babinsky
mbabinsk at redhat.com
Tue Jan 12 12:14:38 UTC 2016
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.
--
Martin^3 Babinsky
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-mbabinsk-0123.3-tests-for-package-version-comparison.patch
Type: text/x-patch
Size: 2640 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160112/8b225dee/attachment.bin>
More information about the Freeipa-devel
mailing list