cleishm / libcypher-parser

Cypher Parser Library
Apache License 2.0
147 stars 39 forks source link

[don't merge] Add python bindings. #3

Closed franekp closed 7 years ago

franekp commented 7 years ago

Python bindings using CPython API (https://docs.python.org/2/c-api/index.html). I will be glad if you find a while to review them. I will be happy to fix any issues you find and apply any suggestions to the code. Let me know what are your thoughts and whether there is any chance you could consider merging it.

PS. I know about the existence of these: https://github.com/inonit/libcypher-parser-python but they are quite limited and for example do not provide access to parsed AST.

cleishm commented 7 years ago

Hey @franekp!

Thanks for working on this! I'll take a look at the code soon.

First, I want to question whether we should be trying to include bindings for all the different languages into the one repository and distribution? Do you have examples of other libraries with multi-language bindings we can follow?

Cheers, Chris

cleishm commented 7 years ago

Took a quick look at the code. A couple of additional questions:

1) Is this automatically generated? If so, how does the generation work? 2) How does this integrate with the build and packaging? If it doesn't yet, how do you imagine it will?

Cheers, Chris

franekp commented 7 years ago
  1. No.
  2. You can run setup.py build_ext -i that will build pycypher/bindings.so. After that, you can import the package from source directory. Alternatively, you can do ./setup.py build && ./setup.py install that will install it into current virtualenv or system-wide packages directory if outside virtualenv. To upload to PyPI, you first configure your PyPI credentials in some config file in home directory, then run ./setup.py sdist upload (this does not bump the version automatically)
cleishm commented 7 years ago

Thanks! So given it's not generated, how do you see it being updated when changes to the main library occur?

If they're in the same repository, I would expect them to be kept in sync at all times - everything committed to the codebase should always build and pass tests. That could suggest that having a separate repository (like the inonit one) might be the better approach, as you could update it after new versions of libcypher-parser are released. Thoughts?

Along the same lines, if they do go in the same repository, I'd like to ensure the build system is integrated, so that it's easy to build and run the tests for all languages.

franekp commented 7 years ago

Hi Chris,

You can take a look here: https://github.com/cleishm/libcypher-parser/pull/4 This are proposed changes to libcypher-parser.h that would make auto-generation of bindings possible.

This would be a similar approach that OpenCV uses: http://opencv-python-tutroals.readthedocs.io/en/latest/py_tutorials/py_bindings/py_bindings_basics/py_bindings_basics.html https://github.com/opencv/opencv/tree/master/modules/python https://github.com/opencv/opencv/tree/master/modules/java OpenCV has some annotations in source code and custom scripts to generate bindings for Python and Java that use these annotations to extract necessary information.

Let me know if these annotations is something that you can accept. If yes, I will start working on a script to generate the bindings and will provide documentation on how to use the annotations and the script. If we go this way, most of the changes will be reflected automatically in the bindings, the only functions that will need to stay backwards compatible would be: cypher_astnode_type, cypher_astnode_instanceof, cypher_astnode_nchildren, cypher_astnode_get_child, cypher_astnode_range, cypher_parser_new_config, cypher_fparse, cypher_parse_result_nroots, cypher_parse_result_get_root

Regarding the build system, I can add entries in the makefile for generating, building and testing python bindings. It will not require any dependencies besides Python.

Cheers, Franek

cleishm commented 7 years ago

This sounds great. I like the idea of having generic indicators in the header that a parser will use to generate bindings for other languages.

If you can make a single PR containing a complete implementation, including the build phase, I'd be happy to review and merge.

I'd also like to consider how to do packaging and release. I currently do this largely by hand, as it's only going to a few places. If we add other languages, it would be better if it was scripted.

Cheers, Chris

franekp commented 7 years ago

I can set up a (build / test / PyPI upload) script for python bindings that will run on Travis CI. Build and test will run on every commit and upload will run whenever a tag is pushed to this repository.

Later on, you can extend the Travis config to build and deploy other types of packages. Let me know if this setup is acceptable to you, so that I can start experimenting with Travis.

Note: for this to work, you will need to set up a PyPI account, encrypt your credentials using Travis command line and put them (encrypted) in source control in .travis.yml so that they are accessible as environment variables during the build. Related links: https://docs.travis-ci.com/user/encryption-keys/ https://docs.travis-ci.com/user/pull-requests/#Pull-Requests-and-Security-Restrictions

cleishm commented 7 years ago

That sounds ok to me. I am wondering, however, if there should be a manual approval step in there? I.e. push a tag, have the build run and produce artifacts, but approve before they're uploaded. At least until we've got trust around the build process?

Note that I currently run circleci on this project rather than travis, but I'm not committed to any solution.

franekp commented 7 years ago

Ready for your review: https://github.com/cleishm/libcypher-parser/pull/5

Build and upload is automatic, it is already uploaded on PyPI: https://pypi.python.org/pypi/pycypher

You can see the latest "deploying" build here: https://travis-ci.org/franekp/libcypher-parser/builds/281322968 And the latest "testing" build here: https://travis-ci.org/franekp/libcypher-parser/builds/281321387

Regarding manual approval before upload: you can read the "PyPI deployment" section of dev guide and see if this can convince you to the automatic upload process. I have tested that overriding existing version with a new build tag is working, an example can be found here: https://pypi.python.org/pypi?:action=display&name=pycypher&version=0.5.5 This means that doing a rollback is not a problem.

Please post your PyPI username so I can grant you the permissions.

cleishm commented 7 years ago

Ok. I'll review #5, and we can continue the discussion there.

cleishm commented 7 years ago

I created a PyPI account, with the username 'cleishm'.

franekp commented 6 years ago

I added you as an owner to the pycypher package.