Skip to content
This repository

Cleaner minimum version comparison #1028

Merged
merged 3 commits into from over 2 years ago

5 participants

Szabolcs Horvát Thomas Kluyver Robert Kern Min RK Fernando Perez
Szabolcs Horvát

Cleaner minimum version comparison using setuptools to reduce chance of breakage (0.11 broke with pyzmq 2.1.10)

Thomas Kluyver
Collaborator

I don't think we want to depend on setuptools at runtime. It looks like distutils (which is in the standard library) has tools for doing this - have a look at distutils.version.

Szabolcs Horvát

That is reasonable, but just as a note: I did need to install setuptools on windows for IPython to start up.

Thomas Kluyver
Collaborator

To actually start it, or to install it? I know we require setuptools to install from source on Windows (although I don't know why). I think if we need it to start, that's a bug, but there might be some windows specific thing I don't know about.

Robert Kern
Collaborator

setuptools and its spawn have the feature to create .exe files on Windows for the scripts so they can be executed from the command line. This is too useful to avoid the setuptools install-time dependency. The auxiliary scripts it creates uses its pkg_resources module to locate the actual script. Avoiding that runtime dependency is tricky. But nothing else in IPython should depend on setuptools.

Szabolcs Horvát

Yes, to start it. I installed using the binary installer for Windows. On startup complained that "ImportError: No module named pkg_resources".

Thomas Kluyver
Collaborator

Fair enough. So setuptools is a dependency on Windows, but needn't be elsewhere. For similar reasons, distribute (the successor to setuptools) is required on Python 3 for the time being.

Szabolcs Horvát

Only one line needs to be changed to do what you suggested: from pkg_resources import parse_version as V to from distutils.version import LooseVersion as V. I am not sure how to change this. This is my first time on GitHub, or any other similar site. I am sorry if my pull-request reaction to IPython 0.11 not working with the latest stable ZMQ (2.1.10) was too hasty (as it probably was). Please close this request if it is inappropriate.

Thomas Kluyver
Collaborator

Don't worry, it's OK. We've actually already fixed the issue with 2.1.10, but I think it's better to use standard functionality for this rather than splitting version numbers up ourselves.

If you're using git on your own computer, just make another commit and push it back to github - the pull request automatically updates. If you are editing directly on Github, go to your copy of the file here and click the "Edit this File" button.

Min RK
Owner

Yes, this is definitely better than mine, I just didn't know about this function. Thanks for fixing it.

I should note that unfortunately this will not work on Python 3 for users of pyzmq-dev:

from distutils.version import LooseVersion as V
V('2.1dev') > V('2.1.4') # ultimately [2,1,'dev'] > [2,1,4]
TypeError: unorderable types: str() < int()

So the check should be if 'dev' not in pyzmq_version and V(pyzmq_version) < V(minimum_version):, to prevent evaluating the invalid comparison.

re: setuptools

setuptools is considered a temporary install and runtime dependency on Windows (installing with setuptools makes scripts that depend on itself), because during the major reorganization of 0.11 our Windows install voodoo broke, and we didn't have the time or interest to fix it, and just punted to setuptools. We do intend to remove this dependency (see #539), but it is probably going to take an actual Windows user/developer (of which there are zero on the IPython team) to do it.

Thomas Kluyver
Collaborator

I'm surprised that's not accounted for - LooseVersion seems pretty useless without it.

Min RK
Owner

All LooseVersion does is turn a version string into a list of the contiguous number or letter blobs ('2.10.4b5-dev' -> (2,10,4,'b',5,'dev')). It doesn't do anything clever in the comparison, it just compares the lists.

The decision to remove str/int comparison in Python 3 seems a very bad one, to me.

pyzmq 2.1.10 skirts this issue itself by adding a pyzmq_version_info() function for returning a tuple of numbers, turning 'dev' into inf, so it can always be used in comparisons.

Thomas Kluyver
Collaborator
Fernando Perez
Owner

Just to confirm/clarify re. setuptools: we accepted it as a dependency on windows for the reasons Robert pointed out, but that's it. Setuptools will never be a runtime dependency for ipython, and it will never be a dependency, period, on linux.

Fernando Perez
Owner

BTW, @minrk and @takluyver, I'll leave the merge of this one to you guys, since you've already looked at it more than I did. I just wanted to clarify the setuptools question for the OP (thanks, BTW, @szhorvat! :)

Min RK
Owner

Looks perfect, thanks @szhorvat!

Fernando Perez
Owner

OK, since this has @minrk's and @takluyver's review, I'm merging it so we keep our queue moving along... Thanks everyone!

Fernando Perez fperez merged commit f19b131 into from November 24, 2011
Fernando Perez fperez closed this November 24, 2011
Fernando Perez fperez referenced this pull request from a commit January 10, 2012
Commit has since been removed from the repository and is no longer available.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.

Showing 1 changed file with 4 additions and 7 deletions. Show diff stats Hide diff stats

  1. 11  IPython/zmq/__init__.py
11  IPython/zmq/__init__.py
@@ -9,21 +9,18 @@
9 9
 # Verify zmq version dependency >= 2.1.4
10 10
 #-----------------------------------------------------------------------------
11 11
 
12  
-import re
13 12
 import warnings
  13
+from distutils.version import LooseVersion as V
14 14
 
15 15
 def check_for_zmq(minimum_version, module='IPython.zmq'):
16  
-    min_vlist = [int(n) for n in minimum_version.split('.')]
17  
-
18 16
     try:
19 17
         import zmq
20 18
     except ImportError:
21 19
         raise ImportError("%s requires pyzmq >= %s"%(module, minimum_version))
22 20
 
23 21
     pyzmq_version = zmq.__version__
24  
-    vlist = [int(n) for n in re.findall(r'\d+', pyzmq_version)]
25  
-
26  
-    if 'dev' not in pyzmq_version and vlist < min_vlist:
  22
+    
  23
+    if 'dev' not in pyzmq_version and V(pyzmq_version) < V(minimum_version):
27 24
         raise ImportError("%s requires pyzmq >= %s, but you have %s"%(
28 25
                         module, minimum_version, pyzmq_version))
29 26
 
@@ -33,7 +30,7 @@ def check_for_zmq(minimum_version, module='IPython.zmq'):
33 30
     if not hasattr(zmq, 'ROUTER'):
34 31
         zmq.ROUTER = zmq.XREP
35 32
 
36  
-    if zmq.zmq_version() >= '4.0.0':
  33
+    if V(zmq.zmq_version()) >= V('4.0.0'):
37 34
         warnings.warn("""libzmq 4 detected.
38 35
         It is unlikely that IPython's zmq code will work properly.
39 36
         Please install libzmq stable, which is 2.1.x or 2.2.x""",
Commit_comment_tip

Tip: You can add notes to lines in a file. Hover to the left of a line to make a note

Something went wrong with that request. Please try again.