-
Notifications
You must be signed in to change notification settings - Fork 18
BLD Uses pyproject.toml build dependencies in posix #58
BLD Uses pyproject.toml build dependencies in posix #58
Conversation
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.
So this doesn't change anything in practice, since multiuild was calling pip wheel
? Trying to think if this would need a new RC or not.
@@ -64,7 +61,7 @@ jobs: | |||
- bash: | | |||
set -e | |||
pip install virtualenv | |||
BUILD_DEPENDS="$NP_BUILD_DEP $CYTHON_BUILD_DEP $SCIPY_BUILD_DEP" | |||
BUILD_DEPENDS="$CYTHON_BUILD_DEP" |
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.
if multibuild is calling pip wheel
, then do we need to specify a build depend here?
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.
Particularly that if it uses PEP517, this would have no effect. Maybe we should change pin the cython version in pyproject.toml to "Cython>=0.28.5,<3.0.0"
?
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.
if multibuild is calling pip wheel, then do we need to specify a build depend here?
Wheel is already specified in pyproject.toml.
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.
Maybe we should change pin the cython version in pyproject.toml to "Cython>=0.28.5,<3.0.0" ?
This was my concern as well.
multibuild
does something equivalent to this:
pip install $BUILD_DEPENDS
pip wheel --no-deps .
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 this LGTM, should we merge? Our proposed release date is tomorrow :)
But the runs in this PR are installing the latest numpy compatible with the Py version, not the earliest. That could run us into trouble if we are not testing those earlier versions elsewhere...? |
Does it? I don't think so, our "numpy==1.13.3; python_version=='3.6' and platform_system!='AIX'",
"numpy==1.14.5; python_version=='3.7' and platform_system!='AIX'",
"numpy==1.17.3; python_version>='3.8' and platform_system!='AIX'",
"numpy==1.16.0; python_version=='3.6' and platform_system=='AIX'",
"numpy==1.16.0; python_version=='3.7' and platform_system=='AIX'",
"numpy==1.17.3; python_version>='3.8' and platform_system=='AIX'", |
I think we can close this as this is now done in: https://github.com/scikit-learn/scikit-learn/blob/master/.github/workflows/wheels.yml and it does use the pinned build dependencies defined in |
Partially resolves #57
multibuild internally uses
pip wheel ...
which takes advantage of pyproject.tomlThe windows case use more involved since it needs to run
vendor_vcomp140.py
.