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

Switch to using setuptools. #12

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Conversation

b4hand
Copy link
Contributor

@b4hand b4hand commented May 22, 2014

This allows the package to be built without manually installing Cython
first by requiring it to install Cython as part of the setup process.

Previously, installing hat-trie via pip would fail if Cython was not
already installed. This should make it work automatically.

This allows the package to be built without manually installing Cython
first by requiring it to install Cython as part of the setup process.

Previously, installing hat-trie via pip would fail if Cython was not
already installed.  This should make it work automatically.
@b4hand
Copy link
Contributor Author

b4hand commented May 22, 2014

Hmm, this doesn't seem to actually work as I expected. I'll investigate tomorrow.

@kmike
Copy link
Member

kmike commented May 22, 2014

For the particular problem of failing without Cython I'd prefer just to add generated C code to the source distribution.

This causes the build to fail when Cython is not preinstalled.
@b4hand
Copy link
Contributor Author

b4hand commented May 22, 2014

That would be great. I was curious if you had a reason for excluding the generated C code from the source distro.

@kmike
Copy link
Member

kmike commented May 22, 2014

There is no particular reason for that. I haven't commited generated C files because I thought hat-trie wrapper is not really finished, and building C files via Cython provides a simpler commit history and a simpler workflow. Also, at one point the development was frozen (I didn't have plans to continue it - but this have changed).

Including generated C files in a pypi distribution doesn't require commiting them (it require some changes to setup.py), but the only pypi release (0.1) was a bit hasty - I've just took a version that have been resting on github for a while and put in on pypi. I'm using this wrapper from time to time, but mostly for experiments; for my tasks it turns out other data structures (namely https://github.com/kmike/marisa-trie and https://github.com/kmike/DAWG) are more useful, that's why the hat-trie wrapper is not polished.

I'm fine with adding generated C code to the repo. The updated workflow should probably be mentioned in README.

@b4hand
Copy link
Contributor Author

b4hand commented May 22, 2014

I'm not sure exactly what you'd like to say in the README. If you have a particular blurb in mind, let me know, and I can add it.

@b4hand
Copy link
Contributor Author

b4hand commented May 22, 2014

Also, if you don't mind, I'd really appreciate releasing a new version of hat-trie to pypi after this gets merged.

@kmike
Copy link
Member

kmike commented May 22, 2014

If we put generated C file to the sdist then we don't need cython in setup.py (neither in setup_requires nor in cmdclass) - that's the point of adding C files to sdist (or to the repo).

But if Cython is not regenerating C files when setup.py is executed then developers who make changes should themselves regenerate the files after making changes. C files should be also generated before running tests somehow.

So we need to document how to generate these files, when to generate them and how to commit them. In other projects I'm using policies like this: https://github.com/tpeng/python-crfsuite#contributing; if you have better suggestions they are welcome.


Sure, I'll make a release after this gets merged.

@b4hand
Copy link
Contributor Author

b4hand commented May 23, 2014

So one advantage to the way I have it setup right now is if the .pyx file gets changed, then Cython will automatically be invoked if you already have Cython installed and will update the generated C code. This will show up as a modified file in Git. I like that due to the fact that it makes it obvious that a new file needs committing. However, it does appear that in Travis it is only using the committed C file. I was actually hoping to make it work to always generate the file.

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.

2 participants