tonysimpson / nanomsg-python

nanomsg wrapper for python with multiple backends (CPython and ctypes) should support 2/3 and Pypy
MIT License
382 stars 85 forks source link

Poll support #50

Closed jbester closed 7 years ago

jbester commented 8 years ago

This commits adds support for the nn_poll function in both the ctypes and cpy wrappers. Additionally, it adds a poll function at the top-level of the nanomsg module.

Any code review comments would be appreciated.

jbester commented 8 years ago

I think I messed up the rebase when I repulled with your master branch. This pull request indicates lines changed that are associated with the previous pull request(s) from myself and Ryan Bahneman. Those lines are the same as the master.

tonysimpson commented 8 years ago

Ok this looks amazing, I'll check it out, thanks!!

codypiersall commented 8 years ago

It looks like the only problem with this PR is that it fails on Python 2.6 due to a str.format() issue. Line 20 in test_poll.py needs to be changed from

SOCKET_ADDRESS = os.environ.get('NANOMSG_PY_TEST_ADDRESS', "inproc://{}".format(uuid.uuid4()))

to

SOCKET_ADDRESS = os.environ.get('NANOMSG_PY_TEST_ADDRESS', "inproc://{0}".format(uuid.uuid4()))

This is necessary because Python 2.6 didn't auto-number the format fields. From the docs:

Changed in version 2.7: The positional argument specifiers can be omitted, so '{} {}' is equivalent to '{0} {1}'.

And here is a patch that makes the tiny change:

diff --git a/tests/test_poll.py b/tests/test_poll.py
index 4da4d84..7100f8b 100644
--- a/tests/test_poll.py
+++ b/tests/test_poll.py
@@ -17,7 +17,7 @@ from nanomsg import (
     NanoMsgAPIError
 )

-SOCKET_ADDRESS = os.environ.get('NANOMSG_PY_TEST_ADDRESS', "inproc://{}".format(uuid.uuid4()))
+SOCKET_ADDRESS = os.environ.get('NANOMSG_PY_TEST_ADDRESS', "inproc://{0}".format(uuid.uuid4()))

 class TestPoll(unittest.TestCase):
jbester commented 8 years ago

Thanks, Cody. I amended the pull request.

codypiersall commented 8 years ago

Thanks @jbester! It looks like there's another problem now: Travis is failing because nanomsg now uses cmake as its build tool, so the files ./autogen.sh etc. don't exist.

That's as far as I've gotten on the problem, but once the travis.yml file is updated everything should (hopefully) work. I'll see if I can get to the bottom of this and attach a patch here – it'll give me a good opportunity to figure out how to do stuff with Travis anyway.

jbester commented 8 years ago

@codypiersall Appveyor errors have come up in pull request 46. My understanding is that it's github being overeager to tie services together than anything else.

codypiersall commented 8 years ago

@jbester I'm not sure what the AppVeyor issue is, but the reason the checks for this PR are failing is that nanomsg doesn't support autotools anymore. In a perfectly organized world, a completely separate PR should be given to nanomsg-python to just update the .travis.yml file, and this PR would just be rebased... and actually if that's what @tonysimpson wants I don't mind submitting a separate PR that just fixes the Travis build.

I gave you a pull request that fixes Travis, but if it's easier for you to just apply a patch then I've attached it below. I don't care at all about getting my name in the commit log, so just do whatever's easier for you.

diff --git a/.travis.yml b/.travis.yml
index 2483ba1..7167a59 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -5,16 +5,21 @@ python:
   - "3.4"
   - "2.7"
   - "2.6"
-  
+
 # command to install dependencies, e.g. pip install -r requirements.txt --use-mirrors
+# Build steps taken from
+# https://github.com/nanomsg/nanomsg#build-it-with-cmake
 install:
   - git clone --quiet --depth=100 "https://github.com/nanomsg/nanomsg.git" ~/builds/nanomsg
       && pushd ~/builds/nanomsg
-      && ./autogen.sh
-      && ./configure
-      && make 
-      && sudo make install
-      && popd;
+      && mkdir build
+      && cd build
+      && cmake ..
+      && cmake --build .
+      && ctest -C Debug .
+      && sudo cmake --build . --target install
+      && sudo ldconfig
+      && popd

-# command to run tests, e.g. python setup.py test      
+# command to run tests, e.g. python setup.py test
 script: LD_LIBRARY_PATH=/lib:/usr/lib:/usr/local/lib python setup.py test
codypiersall commented 8 years ago

Woohoo! Thanks @jbester.

tonysimpson commented 7 years ago

Fantastic work both, sorry for taking so long to merge. I will make a release with these changes to PyPI soon. Thanks again!

codypiersall commented 7 years ago

Thanks for accepting!