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

BLD: add pyproject.toml file (PEP 518 support). #321

Merged
merged 1 commit into from
Nov 28, 2017

Conversation

rgommers
Copy link
Member

@rgommers rgommers commented Jul 1, 2017

WIP because the debate is not completely finalized (pypa/pip#4582). But wanted to get this in to not forget before the next release.

EDIT: not WIP anymore as commented on in a comment below.

Closes gh-335

@rgommers rgommers added the build label Jul 1, 2017
@rgommers rgommers added this to the v1.0 milestone Jul 1, 2017
@codecov-io
Copy link

codecov-io commented Jul 1, 2017

Codecov Report

Merging #321 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #321   +/-   ##
=======================================
  Coverage   86.22%   86.22%           
=======================================
  Files          21       21           
  Lines        3099     3099           
  Branches      562      562           
=======================================
  Hits         2672     2672           
  Misses        374      374           
  Partials       53       53

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 69c1fa9...5e6d53a. Read the comment docs.

@rgommers rgommers changed the title WIP: BLD: add pyproject.toml file (PEP 518 support). BLD: add pyproject.toml file (PEP 518 support). Jul 13, 2017
@rgommers
Copy link
Member Author

Okay not WIP anymore, debate seems about settled. Rationale for the choice for Python 3.7 is at pypa/pip#4582 (comment)

@grlee77
Copy link
Contributor

grlee77 commented Jul 21, 2017

I have no prior experience with PEP 518. Is Cython not here because the generated .c files are already included in the source distribution on PyPI? If someone checks out the github source, though, the .c files are not included and Cython would required as well.

@rgommers
Copy link
Member Author

Is Cython not here because the generated .c files are already included in the source distribution on PyPI?

Correct (plus, see below)

If someone checks out the github source, though, the .c files are not included and Cython would required as well.

I'm not a fan of treating Cython like a Python package. It's a compiler, and it's only coincidental that pip can install it. I prefer to treat Cython the same as you'd treat a C compiler - with build docs.

@grlee77
Copy link
Contributor

grlee77 commented Jul 21, 2017

I take it you would be against this suggestion over at pyFFTW/pyFFTW#31 to remove the cython-generated .c file from source distributions then?. I see that the Cython docs do suggest including the .c code as we have done in PyWavelets and PyFFTW: http://cython.readthedocs.io/en/latest/src/reference/compilation.html#distributing-cython-modules.

@rgommers
Copy link
Member Author

I think those comments on the pyFFTW issue are correct in principle but not in practice. This was also discussed a week or so ago on distutils-sig, and one of the points made there was that distributors should anyway not start from a PyPI tarball but from the git tag for the release. sdists will always contain generated files.

Maybe in a few years this'll change, but I think it's fundamentally about removing a build dependency and avoiding issues with different versions of Cython, at almost no cost (a couple of Mb larger downloads isn't a big deal).

@grlee77
Copy link
Contributor

grlee77 commented Jul 21, 2017

Sounds reasonable. Thanks for the feedback.

@rgommers
Copy link
Member Author

Updated for final format. @grlee77 any objections to merging this once TravisCI is happy?

@grlee77 grlee77 merged commit 02b3fc6 into PyWavelets:master Nov 28, 2017
@grlee77
Copy link
Contributor

grlee77 commented Nov 28, 2017

Looks good. Thanks

@grlee77
Copy link
Contributor

grlee77 commented Mar 30, 2018

Hi @rgommers , one other question related to the implications of this PR:

I see in this comment a recommendation to update the install_requires in wheels to match the NumPy used at build time. Is this necessary in practice? How is scipy currently handling this?

It seems that the versions chosen here tend to be the oldest ones for which NumPy wheels exist for a given Python version, so I think if a user pip installs everything they would not encounter issues.
However, I suppose there may be some corner cases such as one where a user manually compiled an older NumPy but then tries to pip install PyWavelets.

(I am currently trying to sort this out for pyFFTW too in pyFFTW/pyFFTW#204)

@rgommers
Copy link
Member Author

Yes, the correct version specifier should be >= of the version that is in pyproject.toml as == (because numpy ABI is backwards but not forwards compatible). This should be safe to add now with pip 10; the previous worries about poor pip --upgrade behavior have been resolved by a change in its defaults.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants