-
Notifications
You must be signed in to change notification settings - Fork 23
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鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
BLD: require numpy>=2.0.0rc1 at compile time #144
Conversation
70a2d2b
to
8e92c9b
Compare
setup.cfg
Outdated
zip_safe = False | ||
tests_require = pytest-doctestplus | ||
setup_requires = setuptools_scm | ||
install_requires = numpy>=1.19 | ||
install_requires = numpy>=2.0.0rc1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this line seems entirely redundant with pyproject.toml
, however, it's been a while since I actually used setup.cfg
so I'm only 99% confident. If that's desired, I'm game to pull the band aid and merge those two files together (which is essentially automated with the right tooling)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's do that in a separate PR...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Of course
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't want to pin numpy>2.0rc1 in install_requires
- this is for runtime dependencies!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(this is different from setup_requires
which is the old way of specifying build-time dependencies)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh my you're right. I meant to touch the previous line and didn't realize my mistake in review... will fix !
I'm not convinced that failures are related. I'll try to refresh this later today and see if it's reproducible |
8e92c9b
to
7b72872
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I should have gotten this PR in before trying to put out a new pyerfa version. I had not realized our wheel building was dependent on what exactly was going on - pretty bad form really.
I still find the whole pip
description of building things excruciating, but to be honest cannot quite care enough any more to try to make sense of it. If this works to get the right wheels on pypi, then let's just roll with it.
setup.cfg
Outdated
zip_safe = False | ||
tests_require = pytest-doctestplus | ||
setup_requires = setuptools_scm | ||
install_requires = numpy>=1.19 | ||
install_requires = numpy>=2.0.0rc1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's do that in a separate PR...
setup.py
Outdated
@@ -96,7 +96,10 @@ def get_extensions(): | |||
sources = [os.path.join('erfa', 'ufunc.c')] | |||
include_dirs = [] | |||
libraries = [] | |||
define_macros = [] | |||
define_macros = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this change? It seems reasonable, but perhaps good to add why it is even needed?
setup.cfg
Outdated
@@ -20,11 +20,11 @@ classifiers = | |||
[options] | |||
# We set packages to find: to automatically find all sub-packages | |||
packages = find: | |||
requires = numpy | |||
requires = numpy>=1.19.3 # keep in sync with NPY_TARGET_VERSION (setup.py) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think requires
is recommended anymore
I think this PR would be fine if we remove |
7b72872
to
0f1bc9a
Compare
I just dropped the second commit as it was never necessary for this PR and actually broken (thanks @astrofrog for pointing it out). Now this PR should be minimal and will hopefully resolve the situation at astropy on release. |
0f1bc9a
to
03fbe5a
Compare
As discussed in #143
This isn't just for the sake of following guidelines: it's actually helpful to anyone that would build pyerfa from source and would like to easily switch back and forth between versions of numpy, which, if we can't have pre-built wheels for macOS AMD (see #143 (comment)), includes me 馃槄