Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Poll support #50

Merged
merged 5 commits into from
Sep 7, 2016
Merged

Poll support #50

merged 5 commits into from
Sep 7, 2016

Conversation

jbester
Copy link
Contributor

@jbester jbester commented Feb 11, 2016

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
Copy link
Contributor Author

jbester commented Feb 11, 2016

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
Copy link
Owner

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

@codypiersall
Copy link
Contributor

codypiersall commented Jul 1, 2016

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
Copy link
Contributor Author

jbester commented Jul 5, 2016

Thanks, Cody. I amended the pull request.

@codypiersall
Copy link
Contributor

codypiersall commented Jul 5, 2016

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
Copy link
Contributor Author

jbester commented Jul 5, 2016

@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
Copy link
Contributor

codypiersall commented Jul 5, 2016

@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
Copy link
Contributor

Woohoo! Thanks @jbester.

@tonysimpson tonysimpson merged commit f47d2ac into tonysimpson:master Sep 7, 2016
@tonysimpson
Copy link
Owner

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

@codypiersall
Copy link
Contributor

Thanks for accepting!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants