8000 BLD: Only allow using Cython module when cythonizing. by charris · Pull Request #14410 · numpy/numpy · GitHub
[go: up one dir, main page]
Skip to content

BLD: Only allow using Cython module when cythonizing. #14410

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Sep 8, 2019
Merged

BLD: Only allow using Cython module when cythonizing. #14410

merged 1 commit into from
Sep 8, 2019

Conversation

charris
Copy link
Member
@charris charris commented Sep 2, 2019

The command line version of cython may point to a different
installation of Cython than that installed in the Python running the
cythonize script. Because the Cython version can be critical, requiring
that the cython used comes from a known place makes it less likely that
the wrong version will be used. This treats Cython as a build dependency
rather than a free standing compiler.

@charris
MAINT: Only allow using Cython module when cythonizing.
393a3a4 
The command line version of `cython` may point to a different
installation of Cython than that installed in the Python running the
cythonize script. Because the Cython version can be critical, requiring
that the cython used comes from a known place makes it less likely that
the wrong version will be used. This treats Cython as a build dependency
rather than a free standing compiler.
@charris charris added 03 - Maintenance 32 - Installation Problems installing or compiling NumPy component: build labels Sep 2, 2019
@charris charris changed the title MAINT: Only allow using Cython module when cythonizing. BLD: Only allow using Cython module when cythonizing. Sep 2, 2019
seberg
seberg approved these changes Sep 2, 2019
View reviewed changes
Copy link
Member
@seberg seberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, not sure if there was discussion elsewhere, but I think this can just go in. Sounds a bit like hitting the PATH version almost certainly gives a wrong one (although I suppose it might be convenient when working with virtual envs).

@tylerjereddy
Copy link
Contributor

So SciPy needs this tools change as well? I usually do cython -V in my conda env before trying to build the sdist & adjust until it gives the version I want.

@charris
Copy link
Member Author
charris commented Sep 2, 2019 •
edited
Loading

So SciPy needs this tools change as well?

In my case, cython -V looked fine and since the last pip update was for Python3.6, that was where where it was checking. Not sure why, the script looked fine, but maybe something was cached? The upshot was that paver called cythonize with Python3.7, which did not have the correct version. So it was the check cython -V check that went wrong together with cythonize checking for an outdated version. Scipy cythonize, unlike NumPy, calls cython by preference rather than the module, but, since the cython version is only checked for the module, that could fail unless the user manually checks which version cython points to -- which you did. SciPy should probably disallow the command line cython just to make sure that cythonize checks the used version against the pyproject.toml file.

@charris
Copy link
Member Author
charris commented Sep 2, 2019

Note that NumPy cythonize has the required version hard wired in. We should look to getting that from the test_requirements.txt file or some other central place.

@rgommers
Copy link
Member
rgommers commented Sep 3, 2019

Note that NumPy cythonize has the required version hard wired in. We should look to getting that from the test_requirements.txt file or some other central place.

We now have a pyproject.toml right? It should go in the build requirements there. As well as in setup_requires in setup.py

rgommers
rgommers previously requested changes Sep 3, 2019
View reviewed changes
Copy link
Member
@rgommers rgommers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

adding the build dependency is needed for this PR, otherwise we're just breaking some use cases that are perfectly fine.

@charris
Copy link
Member Author
charris commented Sep 3, 2019 •
edited
Loading

We now have a pyproject.toml right?

Yes, along with the test_requirements.py file, which is the one that Dependabot patched. Anyone know how Dependabot finds requirements? One can only give it a directory to look in, so I don't know how it found test_requirements.py. Maybe it searches all the files in the directory?

EDIT: Check... Looks like pyproject.toml and test_requirements.py need to be added to MANIFEST.in.

@rgommers
Copy link
Member
rgommers commented Sep 8, 2019

I'm adding the build dependency in gh-14453, so let's leave this as is.

@rgommers rgommers dismissed their stale review September 8, 2019 16:58

requested changes done in gh-14453

@rgommers
Copy link
Member
rgommers commented Sep 8, 2019

Okay, let's give this a try. Thanks Chuck!

@rgommers rgommers merged commit 3fea61f into numpy:master Sep 8, 2019
@rgommers rgommers added this to the 1.18.0 release milestone Sep 8, 2019
@charris charris deleted the remove-cython-fallback branch May 2, 2020 17:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@seberg seberg seberg approved these changes

@rgommers rgommers rgommers left review comments

Assignees
No one assigned
Labels
03 - Maintenance 32 - Installation Problems installing or compiling NumPy component: build
Projects
None yet
Milestone
1.18.0 release
Development

Successfully merging this pull request may close these issues.
4 participants
@charris @tylerjereddy @rgommers @seberg
0