[Freeipa-devel] [PATCH 0122-0123] reimplementation of package version comparison code
Martin Babinsky
mbabinsk at redhat.com
Tue Jan 12 09:23:09 UTC 2016
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.
--
Martin^3 Babinsky
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-mbabinsk-0123.2-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/b904159a/attachment.bin>
More information about the Freeipa-devel
mailing list