Replace LooseVersion with pkg_resource.parse_version #254

tiran · 2016-11-18T08:31:00Z

pylint is having a hard time with distutils.version in tox's virtual
envs. virtualenv uses some tricks to provide a virtual distutils
package, pylint can't cope with.

pylint-dev/pylint#73 suggests to use pkg_resources
instead. pkg_resources' version parser has some more benefits, e.g. PEP
440 conformity.

Signed-off-by: Christian Heimes cheimes@redhat.com

tiran · 2016-11-18T08:32:41Z

The PR is related to integration improvements and pip-installable effort, https://fedorahosted.org/freeipa/ticket/6468.

martbab · 2016-11-18T08:53:23Z

I have a sneaky suspicion that the parse_version couldn't cope correctly with some of the downstream versioning schemes like RHEL z-stream releases and such.

That should be OK with API version, but client/server versions can pose a problem. There is are some examples in the ipatests/test_ipaserver/test_version_comparison.py I made when solving a bug that was caused by incorrect version comparison. Can you check if parse_version can handle them correctly?

tiran · 2016-11-18T09:51:55Z

sigh, you are right, parse_version does not handle one version comparison as we expect: 4.2.0-15.el7 < 4.2.0-15.el7_2.3

from __future__ import print_function
import operator
import pkg_resources


version_strings = [
    ("3.0.0-1.el6", "3.0.0-2.el6", operator.lt),
    ("3.0.0-1.el6_8", "3.0.0-1.el6_8.1", operator.lt),
    ("3.0.0-42.el6", "3.0.0-1.el6", operator.gt),
    ("3.0.0-1.el6", "3.0.0-42.el6", operator.lt),
    ("3.0.0-42.el6", "3.3.3-1.fc20", operator.lt),
    ("4.2.0-15.el7", "4.2.0-15.el7_2.3", operator.lt),
    ("4.2.0-15.el7_2", "4.2.0-15.el7_2.3", operator.lt),
    ("4.2.0-15.el7_2.3", "4.2.0-15.el7_2.3", operator.eq),
    ("4.2.0-15.el7_2.3", "4.2.0-15.el7_2.2", operator.gt),
    ("4.2.0-1.fc23", "4.2.1-1.fc23", operator.lt),
    ("4.2.3-alpha1.fc23", "4.2.3-2.fc23", operator.lt),
    ("4.3.90.201601080923GIT55aeea7-0.fc23", "4.3.0-1.fc23", operator.gt)
]

for v1, v2, op in version_strings:
    v1 = pkg_resources.parse_version(v1)
    v2 = pkg_resources.parse_version(v2)
    if not op(v1, v2):
        print("failure: ", v1, op.__name__, v2)

$ python v.py 
failure:  4.2.0-15.el7 lt 4.2.0-15.el7_2.3

tiran · 2016-11-18T10:06:39Z

In case you wonder what is going on, LooseVersion is both loose and dumb.

>>> ('el7',) < ('el7_2',)
True

The legacy version parser parses the version strings differently:

(-1, ('00000004', '00000002', '*final-', '00000015', '*el', '00000007', '*final'))
(-1, ('00000004', '00000002', '*final-', '00000015', '*el', '00000007', '*_', '00000002', '00000003', '*final'))

martbab · 2016-11-18T10:33:35Z

Yes that's why we resorted to a direct CFFI call to RPM libs during server version check in upgrade. We simply could not win aside from re-implementing parsing of Z-stream versions etc. from scratch.

tiran · 2016-11-18T11:57:21Z

I reworked my PR to use tasks.parse_ipa_version() for all version number checks. Of course I ran into the import cycle of death. I broke the cycle with a local import. It's not perfect but a completely fine solution. The function isn't called regularly.

martbab · 2016-11-18T12:09:33Z

I was thinking about this a bit, and was wondering whether the platform-specific idiosyncracies of the version handling could be safely confined to the platform-specific code. I.E ipaplatform.base would define version comparisons via standard pkg_resources parser and fedora/redhat would override this with their platform-specific quirks.

The one thing that could be broken by this would be scenarios like Fedora clients talking to RHEL masters etc., but I think those sceniarios are not handled correctly by the current implementation anyway.

Another thing I was thinking about is whether we could use some proxy object in ipalib/ipaclient/ipapython libs which would use version comparison from ipaplatform if present, and if not use standard Python algorithms.

What I am aiming at that we should reduce the dependency of the PyPI candidate code on ipaplatform madness as much as is humanly possible. IMHO if we really want to use these modules in PyPI, then they ideally should not depend on ipaplatform at all.

martbab · 2016-11-18T12:21:26Z

Upon closer inspection of the affected code it seems that all the code in ipalib/ipaclient/ipapython actually parses and compares API versions, which are sane and parseable by pkg_resources. Since client code should be concerned by API versions and not by platform-specific package versions (that is server's job) it seems that you do not even need to use ipaplatform hammer in this case.

tiran · 2016-11-18T13:39:39Z

I was thinking about this a bit, and was wondering whether the platform-specific idiosyncracies of the version handling could be safely confined to the platform-specific code. I.E ipaplatform.base would define version comparisons via standard pkg_resources parser and fedora/redhat would override this with their platform-specific quirks.

Somebody used a time machine and implemented your proposal already. tasks.parse_ipa_version() is a generic version parsing function. On RPM platforms it returns an object that uses librpm.

IMHO it's ok to use tasks.parse_ipa_version() everywhere.

martbab · 2016-11-18T13:45:12Z

Oh right, ok. My code reading skills are sub-par today.

MartinBasti · 2016-11-18T18:51:58Z

I was thinking about this a bit, and was wondering whether the platform-specific idiosyncracies of the version handling could be safely confined to the platform-specific code. I.E ipaplatform.base would define version comparisons via standard pkg_resources parser and fedora/redhat would override this with their platform-specific quirks.

Somebody used a time machine and implemented your proposal already. tasks.parse_ipa_version() is a generic version parsing function. On RPM platforms it returns an object that uses librpm.

Yeah, I did.

IMHO it's ok to use tasks.parse_ipa_version() everywhere.

As Martin said, we have two versions: package released version and API version.

For released package version we need RPM/platform specific parser, but API version is just 2 numbers and standard python function can be used. It is the same accross platforms.

API version is less important now, because we have versions of commads that scales better, and one day we may drop this overall API version completely. If you want to avoid importing platform then is fine to use standard python functions to compare API versions. Actually that rpmlib scares me.

MartinBasti · 2016-11-18T18:56:16Z

Thank you for PY3 fix, it actually belongs to this ticket https://fedorahosted.org/freeipa/ticket/6473

tiran · 2016-11-21T09:26:59Z

Back to parse_version!

martbab · 2016-11-21T18:06:43Z

It seems that your changes broke IPA upgrade:

Done configuring the web interface (httpd).
Applying LDAP updates
Upgrading IPA:
  [1/9]: stopping directory server
  [2/9]: saving configuration
  [3/9]: disabling listeners
  [4/9]: enabling DS global lock
  [5/9]: starting directory server
  [6/9]: upgrading server
ipa         : ERROR    Upgrade failed with 'SetuptoolsVersion' object has no attribute 'version'
  [error] RuntimeError: 'SetuptoolsVersion' object has no attribute 'version'
  [cleanup]: stopping directory server
  [cleanup]: restoring configuration
ipa.ipapython.install.cli.install_tool(CompatServerMasterInstall): ERROR    Update failed: 'SetuptoolsVersion' object has no attribute 'version'
ipa.ipapython.install.cli.install_tool(CompatServerMasterInstall): ERROR    The ipa-server-install command failed. See /var/log/ipaserver-install.log for more information
11-21 18:49 ipadocker.cli ERROR    Command ipa-server-install -U --domain ipa.test --realm IPA.TEST -p Secret123 -a Secret123 --setup-dns --auto-forwarders failed (exit code 1)

Traceback in ipaserver-install.log:

# tail -n 50  /var/log/ipaserver-install.log 
  File "/usr/lib/python2.7/site-packages/ipapython/install/core.py", line 481, in __runner
    exc_handler(exc_info)
  File "/usr/lib/python2.7/site-packages/ipapython/install/core.py", line 510, in _handle_execute_exception
    self._handle_exception(exc_info)
  File "/usr/lib/python2.7/site-packages/ipapython/install/core.py", line 500, in _handle_exception
    six.reraise(*exc_info)
  File "/usr/lib/python2.7/site-packages/ipapython/install/core.py", line 471, in __runner
    step()
  File "/usr/lib/python2.7/site-packages/ipapython/install/core.py", line 468, in <lambda>
    step = lambda: next(self.__gen)
  File "/usr/lib/python2.7/site-packages/ipapython/install/util.py", line 81, in run_generator_with_yield_from
    six.reraise(*exc_info)
  File "/usr/lib/python2.7/site-packages/ipapython/install/util.py", line 59, in run_generator_with_yield_from
    value = gen.send(prev_value)
  File "/usr/lib/python2.7/site-packages/ipapython/install/core.py", line 705, in _configure
    next(executor)
  File "/usr/lib/python2.7/site-packages/ipapython/install/core.py", line 481, in __runner
    exc_handler(exc_info)
  File "/usr/lib/python2.7/site-packages/ipapython/install/core.py", line 510, in _handle_execute_exception
    self._handle_exception(exc_info)
  File "/usr/lib/python2.7/site-packages/ipapython/install/core.py", line 568, in _handle_exception
    self.__parent._handle_exception(exc_info)
  File "/usr/lib/python2.7/site-packages/ipapython/install/core.py", line 500, in _handle_exception
    six.reraise(*exc_info)
  File "/usr/lib/python2.7/site-packages/ipapython/install/core.py", line 565, in _handle_exception
    super(ComponentBase, self)._handle_exception(exc_info)
  File "/usr/lib/python2.7/site-packages/ipapython/install/core.py", line 500, in _handle_exception
    six.reraise(*exc_info)
  File "/usr/lib/python2.7/site-packages/ipapython/install/core.py", line 471, in __runner
    step()
  File "/usr/lib/python2.7/site-packages/ipapython/install/core.py", line 468, in <lambda>
    step = lambda: next(self.__gen)
  File "/usr/lib/python2.7/site-packages/ipapython/install/util.py", line 81, in run_generator_with_yield_from
    six.reraise(*exc_info)
  File "/usr/lib/python2.7/site-packages/ipapython/install/util.py", line 59, in run_generator_with_yield_from
    value = gen.send(prev_value)
  File "/usr/lib/python2.7/site-packages/ipapython/install/common.py", line 63, in _install
    for _nothing in self._installer(self.parent):
  File "/usr/lib/python2.7/site-packages/ipaserver/install/server/__init__.py", line 575, in main
    master_install(self)
  File "/usr/lib/python2.7/site-packages/ipaserver/install/server/install.py", line 265, in decorated
    func(installer)
  File "/usr/lib/python2.7/site-packages/ipaserver/install/server/install.py", line 851, in install
    ds.apply_updates()
  File "/usr/lib/python2.7/site-packages/ipaserver/install/dsinstance.py", line 693, in apply_updates
    raise RuntimeError("Update failed: %s" % e)

2016-11-21T17:49:45Z DEBUG The ipa-server-install command failed, exception: RuntimeError: Update failed: 'SetuptoolsVersion' object has no attribute 'version'
2016-11-21T17:49:45Z ERROR Update failed: 'SetuptoolsVersion' object has no attribute 'version'
2016-11-21T17:49:45Z ERROR The ipa-server-install command failed. See /var/log/ipaserver-install.log for more information

tiran · 2016-11-21T18:20:36Z

setuptool's version parser does not support slicing. I need to find another solution for verify_client_version().

tiran · 2016-11-22T12:23:50Z

@martbab more fun, the doc string of verify_client_version deviates from the actual implementation. The code does not implement the minor version check. It only compares major version numbers.

https://github.com/freeipa/freeipa/blob/master/ipalib/frontend.py#L764

martbab · 2016-11-23T13:23:02Z

LGTM but please add the relevant ticket number into the commit message.

pylint is having a hard time with distutils.version in tox's virtual envs. virtualenv uses some tricks to provide a virtual distutils package, pylint can't cope with. pylint-dev/pylint#73 suggests to use pkg_resources instead. pkg_resources' version parser has some more benefits, e.g. PEP 440 conformity. But pkg_resources.parse_version() is a heavy weight solution with reduced functionality, e.g. no access to major version. For API_VERSION and plugin version we can use a much simpler and faster approach. https://fedorahosted.org/freeipa/ticket/6468 Signed-off-by: Christian Heimes <cheimes@redhat.com>

martbab · 2016-11-24T14:47:06Z

Fixed upstream
master:
https://fedorahosted.org/freeipa/changeset/2cbaf156045769b54150e4d4c3c1071f164a16fb

tiran force-pushed the parse_version branch 2 times, most recently from 0ae19f1 to 7d5b902 Compare November 18, 2016 11:53

tiran changed the title ~~Replace LooseVersion with pkg_resources.parse_version~~ Replace LooseVersion with parse_ipa_version() Nov 18, 2016

tiran force-pushed the parse_version branch from 7d5b902 to 87265d9 Compare November 18, 2016 13:41

tiran force-pushed the parse_version branch from 87265d9 to 32e4aba Compare November 18, 2016 15:27

tiran mentioned this pull request Nov 21, 2016

Minor fixes for IPAVersion class #259

Closed

tiran force-pushed the parse_version branch from 32e4aba to afd3511 Compare November 21, 2016 09:26

tiran changed the title ~~Replace LooseVersion with parse_ipa_version()~~ Replace LooseVersion with pkg_resource.parse_version Nov 21, 2016

tiran force-pushed the parse_version branch from afd3511 to 19d679d Compare November 21, 2016 10:12

tiran force-pushed the parse_version branch from 19d679d to a2a7d2c Compare November 22, 2016 14:38

MartinBasti assigned martbab Nov 22, 2016

tiran force-pushed the parse_version branch from a2a7d2c to 86a5b4d Compare November 23, 2016 10:18

tiran force-pushed the parse_version branch from 86a5b4d to 4788e90 Compare November 23, 2016 14:04

martbab added ack Pull Request approved, can be merged pushed Pull Request has already been pushed labels Nov 24, 2016

martbab closed this Nov 24, 2016

tiran deleted the parse_version branch November 24, 2016 14:58

Replace LooseVersion with pkg_resource.parse_version #254

Replace LooseVersion with pkg_resource.parse_version #254

tiran commented Nov 18, 2016 •

edited by martbab

tiran commented Nov 18, 2016

martbab commented Nov 18, 2016

tiran commented Nov 18, 2016

tiran commented Nov 18, 2016

martbab commented Nov 18, 2016

tiran commented Nov 18, 2016

martbab commented Nov 18, 2016

martbab commented Nov 18, 2016

tiran commented Nov 18, 2016

martbab commented Nov 18, 2016

MartinBasti commented Nov 18, 2016

MartinBasti commented Nov 18, 2016

tiran commented Nov 21, 2016

martbab commented Nov 21, 2016

tiran commented Nov 21, 2016

tiran commented Nov 22, 2016

martbab commented Nov 23, 2016

martbab commented Nov 24, 2016

Replace LooseVersion with pkg_resource.parse_version #254

Replace LooseVersion with pkg_resource.parse_version #254

Conversation

tiran commented Nov 18, 2016 • edited by martbab

tiran commented Nov 18, 2016

martbab commented Nov 18, 2016

tiran commented Nov 18, 2016

tiran commented Nov 18, 2016

martbab commented Nov 18, 2016

tiran commented Nov 18, 2016

martbab commented Nov 18, 2016

martbab commented Nov 18, 2016

tiran commented Nov 18, 2016

martbab commented Nov 18, 2016

MartinBasti commented Nov 18, 2016

MartinBasti commented Nov 18, 2016

tiran commented Nov 21, 2016

martbab commented Nov 21, 2016

tiran commented Nov 21, 2016

tiran commented Nov 22, 2016

martbab commented Nov 23, 2016

martbab commented Nov 24, 2016

tiran commented Nov 18, 2016 •

edited by martbab