Python bindings - complete implementation

franekp commented 7 years ago

Good place to start: https://github.com/franekp/libcypher-parser/blob/pycypher-wip/pycypher/docs/DeveloperGuide.md

Readme: https://github.com/franekp/libcypher-parser/tree/pycypher-wip/pycypher

franekp commented 7 years ago

Hi Chris,

Sorry for the long delay. I addressed all the issues, please reexamine the PR. As before, for information about how building and generation works see https://github.com/franekp/libcypher-parser/blob/pycypher-wip/pycypher/docs/DeveloperGuide.md

Cheers, Franek

cleishm commented 6 years ago

Hi @franekp,

I did some shuffling in the main branch, so I've rebased this PR for you.

I'd also like to see it a little more integrated with autotools - ideally so that a ./configure --with-python && make all will build the python bindings as well (and make check will test them).

I added a commit (21391e4) that starts that integration, but it's not completed. I'm looking at posts like this for techniques: https://blog.kevin-brown.com/programming/2014/09/24/combining-autotools-and-setuptools.html

I'm not, however, familiar enough with python packaging and the use of setuptools. Perhaps you could take a look and provide some thoughts or add some code?

franekp commented 6 years ago

Hi Chris,

Thank you for the rebase. Here you can see Travis build after the rebase: https://travis-ci.org/franekp/libcypher-parser/builds/307076267 and Travis build of the autotools integration attempt: https://travis-ci.org/franekp/libcypher-parser/builds/305047499.

Regarding ./configure --with-python && make all - since I am not an Autotools expert, this is likely to be a significant amount of work. I can do it, but I need to understand the use-case: what type of users will prefer Autotools-based installation over installing from PyPI and what type of packages these commands should be building.

Since the Autotools integration might involve a lot of work and it is not entirely clear how exactly it should look like, I would very much prefer to do it in a separate PR.

I suggest that:

I fix things that were broken by the rebase.
You point out any other concerns that are fixable in a reasonable amount of time.
I fix them.
This PR is finally merged so that time is not wasted on unnecessarily rebasing and fixing things.
You set up Travis build on the main repo so that future PR-s are automatically tested.
After this, I can do smaller PR-s that are easier to review and in general more manageable.

What do you think about this plan?

cleishm commented 6 years ago

Hey @franekp,

Most of the integration is already done in that final commit I added. I'm just unsure how the packaging using setuptools works from there.

Note that I'm not suggesting users would install pycypher using autotools - only that the appropriate python packages are built and ready for publishing. I think this would involve calling your setuptools script, or the pip utility, in the same manner that you currently do from the shell scripts in /ci? Basically, I'd like to replace those shell scripts with targets in the automake generated Makefiles, and integrate them enough that make all also builds the python packages, and make check runs the tests.

Ideally, that would be in place before we merge this PR into the main trunk. Happy to put it on a branch meanwhile.

Cheers, Chris

franekp commented 6 years ago

How have you been testing the code in Integrating with autotools (WIP) commit?

franekp commented 6 years ago

Btw, make check was already building (in-place, only for testing) and testing the bindings.

franekp commented 6 years ago

So you want make all to build python packages for all Python versions both 32 and 64 bit? Are you sure that this will not be too slow for something that is used often during development?

cleishm commented 6 years ago

The final commit is a work-in-progress. I just pushed a fix for the issue that your travis build picked up, to the branch here: https://github.com/cleishm/libcypher-parser/tree/pycypher-wip

As for the architecture - I think just building using whatever version of python that is picked up by autoconf is fine, at least for the local build/test. When it comes to building a release, it makes sense to do that using your docker environment and all the python versions available in the pypa/manylinux1_i686 image.

franekp commented 6 years ago

Let's do the development on this PR, here at least we have travis builds (although not linked from the PR, they are triggered on each push (except rebases) and can be easily found on travis-ci.org).

franekp commented 6 years ago

I removed the reverting commit.

franekp commented 6 years ago

Please address the comments.

franekp commented 6 years ago

Oh, it seems that GitHub does not allow to comment on renamed files. So: pycypher/README.md → pycypher/doc/README.md

This file contains relative links, they should be adjusted when moving the file.
This file was intended to be linked from PyPI so that users can find documentation easily.

berggren commented 6 years ago

@cleishm What is the status of this PR? Can it be merged in this state, or is there anything that needs fixing before? Please let @franekp know so he can fix any outstanding issues.

I would very much like to see this feature added :)

Thanks for all your work here!

cleishm commented 6 years ago

Hi @berggren,

It's not quite ready to merge - I'm still experimenting with the best way to integrate the python package builds with the existing autotools build chain. I need to learn a bit more about how python packaging works :)

Currently, it builds and installs using autotools directly (rather than python setuptools) - but doesn't run the test suite. For that, I think I need to build the package in the local tree somehow. I think I'll switch back to using setuptools, invoked via make, which will hopefully solve that also (see https://blog.kevin-brown.com/programming/2014/09/24/combining-autotools-and-setuptools.html).

Happy to have any assistance, if this is of interest to you. Otherwise, I expect to take another look at it this weekend.

Cheers, Chris

berggren commented 6 years ago

Thanks @cleishm for the update! Sounds good, I just wanted to make sure this was being worked on as it would be a pitty if this PR was dropped. Lot's of good work went into it and having these bindings is really awesome for the project :)

Thanks for working in this!

berggren commented 6 years ago

Friendly ping @cleishm any updates after the holidays? :) Any ETA on when you can merge this?

berggren commented 6 years ago

Hey @cleishm did you get a chance to look at this PR again?

cleishm commented 6 years ago

@berggren Honestly, no. I've been flat out on another project. I'd love some help on this - feel free to jump in if you can!

berggren commented 6 years ago

Cool, I totally understand. I'll see if I can find time to look into this. I will also reach out to Franek to see if he has some cycles over.

Thanks for the update.

cleishm commented 4 years ago

If this is still valid, please feel free to reopen against the main branch.

cleishm / libcypher-parser

Python bindings - complete implementation #5