libdynd / dynd-python

Python exposure of dynd
http://libdynd.org
Other
119 stars 23 forks source link

Namespace package #701

Closed skrah closed 8 years ago

skrah commented 8 years ago

This (explicit) namespace package works here on Linux/Windows with both Python 2 and Python 3.

The magic switch is in setup.py:

 1) DIST_TARGET = 'all' -> The existing install.

 2) DIST_TARGET = 'ndt' -> Install only libdyndt and the ndt module.

 3) DIST_TARGET = 'nd' -> Install only libdynd and the nd module.

Options 1) and 2) are self-contained, 3) depends on 2).

skrah commented 8 years ago

This includes (and requires) https://github.com/libdynd/dynd-python/pull/699, which builds here on Windows.

izaid commented 8 years ago

As far as I can tell, this is cool. Great work! I'd like at least one of @mwiebe or @insertinterestingnamehere to give it their blessing though.

skrah commented 8 years ago

Since the default install is set to 'all' (pretty much the same as now), perhaps it's easier to have a shell script that shows how the actual namespace packages are supposed to be installed and tested:

# Test the separate dynd.ndt module
git clone dynd-python/ dynd.ndt
cd dynd.ndt/
sed -i "s/DIST_TARGET = 'all'/DIST_TARGET = 'ndt'/" setup.py
python setup.py install
cd ..
python -m dynd.ndt.test

# Test the separate dynd.nd module (after installing the ndt module)
git clone dynd-python/ dynd.nd
cd dynd.nd/
sed -i "s/DIST_TARGET = 'all'/DIST_TARGET = 'nd'/" setup.py
echo "dynd.ndt" >> requirements.txt
python setup.py install
cd ..
python -m dynd.nd.test
mwiebe commented 8 years ago

LGTM, I think my comments all fit in "explain more" and "exceptions as control flow" buckets.

insertinterestingnamehere commented 8 years ago

This looks good to me too. Without the dll preloading logic for config, our python bindings will no longer work if libdynd/libdyndt are in the default install location but not on the path, but we don't necessarily have to fix that in this PR. Moving dynd.config into dynd.common and duplicating the loading logic again or just moving dynd.config into dynd.ndt should fix the problem. Those seem like things that are better done in a separate PR though. Thanks for taking care of this! Great work!

izaid commented 8 years ago

@skrah Looks great. If you could address @mwiebe's minor comments, I'll merge afterwards.

skrah commented 8 years ago

@izaid Thanks, this should be good to go. Let's address the ImportError catching and the config issues in the next PR.

mwiebe commented 8 years ago

I'm happy to merge this, hitting the button.l..

izaid commented 8 years ago

Go for it!