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

Convert python install from build_ext to setuptools setup_requires. #723

Merged
merged 2 commits into from
Apr 5, 2021

Conversation

marcus1487
Copy link
Contributor

This should make cython installation a bit more robust and allows python3.9 support (on my machine at least).

Resolves #719

@lh3
Copy link
Owner

lh3 commented Apr 2, 2021

Does the change require cython? The current setup.py makes cython optional. When you install with pip, cython is not required. I prefer that way.

@marcus1487
Copy link
Contributor Author

With this change, if cython is not available at install time it will be installed before building the mappy extension as part of the full install command sequence. It does not require that cython is installed/available before pip install mappy or python setup.py install.

This also forces mappy.pyx to be compiled by cython with the version linked with the installing python environment. I think this might be the source of the python 3.9 issue. I'm not sure exactly why it is popping up with python 3.9 specifically, but I got the same error message as posted in the linked issue when trying to install in a python 3.9 environment.

@lh3
Copy link
Owner

lh3 commented Apr 2, 2021

I much prefer people can download the tar-ball and compile without any dependencies. Cython shouldn't be required. I will update the package at PyPI at some point.

@lh3 lh3 closed this Apr 2, 2021
@marcus1487
Copy link
Contributor Author

I appreciate the concern. I am wondering how the mappy.c file is created without cython. It seems that the Makefile removes this file, but I am not following how this file is created without cython. Running the following raises an error indicating no such file or directory: 'python/mappy.c'

git clone https://github.com/lh3/minimap2
cd minimap2
make
pip install .

@lh3
Copy link
Owner

lh3 commented Apr 2, 2021

You create mappy.c with cython. The current setup.py submits this file to PyPI. This way, when users run pip install, they don't need cython as a dependency.

#719 is probably caused by the older cython that was used to create mappy.c.

@marcus1487
Copy link
Contributor Author

I see. Thank you for the detailed explanation!

@lh3 lh3 reopened this Apr 5, 2021
@lh3
Copy link
Owner

lh3 commented Apr 5, 2021

It seems that I am using TABs but you are using SPACEs. Could you change that? I will merge after that.

@marcus1487
Copy link
Contributor Author

Done.

@lh3 lh3 merged commit d3dde2f into lh3:master Apr 5, 2021
@lh3
Copy link
Owner

lh3 commented Apr 5, 2021

Just to confirm: with this PR, pip will automatically install cython. Is that right?

@marcus1487
Copy link
Contributor Author

Yes. When pip install mappy is run cython will be installed automatically (if not already available) before the mappy source is compiled with cython.

@lh3
Copy link
Owner

lh3 commented Apr 5, 2021

Good. Thanks.

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.

Mappy Python3.9 - pip installation error
2 participants