Skip to content

Conversation

stuaxo
Copy link
Contributor

@stuaxo stuaxo commented Aug 5, 2017

Make python setup.py develop or python setup.py install install cython if it is not already.

Add install_requires with cython, (and make import completely optional).

Make  ```python setup.py develop``` or ```python setup.py install``` install cython if it is not already.

Add install_requires with cython, (and make import completely optional).
@coveralls
Copy link

Coverage Status

Coverage remained the same at 47.969% when pulling a7fc797 on stuaxo:patch-2 into dd7f4bf on swistakm:master.

@stuaxo
Copy link
Contributor Author

stuaxo commented Aug 7, 2017

@swistakm I'm not really sure what the error in appveyor is about, doesn't seem to be introduced by the patch though.

It seems like this bug in git-lfs

git-lfs/git-lfs#1697

Not sure if it is possible to upgrade git-lfs or is that something in appveyor ?

@swistakm
Copy link
Member

swistakm commented Aug 8, 2017

Oh, yeah. This issue happens from time to time on AppVeyor and is caused by git-lfs. I've hit "rebuild" button.

@swistakm
Copy link
Member

swistakm commented Aug 8, 2017

Once build passed we can start the discussion.

I did not add Cython to the package requirements rather on purpose. I primarily wanted to distribute both Cython .pyx and C++ .cpp sources as sdist package and leave the decision whether to use Cython or plain C++ compilation to the final user. This is actually a solution suggested by Cython documentation.

My general idea is to include both Cython and C++ sources in sdist package and trigger Cython installation using setuptools' extras_require option, e.g.:

pip install imgui[Cython]

Question is: should Cython existence (determined with try ... import block) be enough to trigger actual Cython compilation instead of plain C++ compilation? I think we should be fine with this. What do you think @stuaxo? We can always add additional environment variable later to enable/disable such behaviour.

Still, I will merge this PR because we will have to use this cythonize() function stub anyway but I will remove the install_requires and setup_requires soon.

@stuaxo
Copy link
Contributor Author

stuaxo commented Aug 8, 2017

That all sounds really sensible. We should be fine with the try... import + optional environment variable.

@swistakm swistakm merged commit 96b8bd6 into pyimgui:master Aug 8, 2017
@stuaxo stuaxo deleted the patch-2 branch August 8, 2017 11:23
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.

3 participants