nanomsg / nnpy

cffi-based Python bindings for nanomsg
MIT License
117 stars 39 forks source link

support cross compiling #19

Closed mtp401 closed 8 years ago

mtp401 commented 8 years ago

This patch adds support for cross compiling nnpy (e.g. under Buildroot). I modeled the changes after what numpy seemingly does (using a site.cfg file if one is present).

djc commented 8 years ago

Thanks for the patch! I'll try to review it ASAP, probably tomorrow.

From a quick skim, first feedback will be that I don't like that you're sort of randomly rewriting whitespace in parts of the code that you don't otherwise touch. Second, there's a bit too much going on in the global scope, this should go into a function somehow. However, feel free to hold off doing something about this until I can do a more complete review.

djc commented 8 years ago

The approach looks basically sane, but like I said, there's way too much code in the global scope. Just move all of the reading of the config file into create_module() instead. Except the imports, which should just go near the other imports at the top.

The setting of variables seems to be a bit roundabout (set constant to None, then set it from the site config, then set it to the default value if still None). Instead, you could just have the constants with the default values, then overwrite them if specified in the site-specific config. Instead of overwriting the constant values, pass around the local variables (so include_dirs to header_files(), and host_library to symbols()) as needed.

As mentioned before, please leave whitespace intact in places you're not touching (alternatively, if you really dislike trailing whitespace, create a separate commit that cleans up trailing whitespace before you commit your actual changes).

Thanks again, nice to get this in!

mtp401 commented 8 years ago

Hi @djc, please take another look when you get a chance. I've made the changes you suggested and split off the whitespace changes into their own commit.

djc commented 8 years ago

Looks great, thanks!

djc commented 8 years ago

FWIW, I've also put a 1.2 release incorporating your changes on PyPI.

mtp401 commented 8 years ago

Great thanks!