-
Notifications
You must be signed in to change notification settings - Fork 87
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
Changed TEST_DEPENDS pip install to install one package at a time #182
Conversation
common_utils.sh
Outdated
@@ -316,7 +316,9 @@ function install_wheel { | |||
# MANYLINUX_URL (optional, default "") (via pip_opts function) | |||
local wheelhouse=$(abspath ${WHEEL_SDIR:-wheelhouse}) | |||
if [ -n "$TEST_DEPENDS" ]; then | |||
pip install $(pip_opts) $@ $TEST_DEPENDS | |||
for TEST_DEPENDENCY in $TEST_DEPENDS; do |
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.
Does this work OK for every possible way of spelling a dependency?
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.
echo "Empty string"
TEST_DEPENDS=""
for TEST_DEPENDENCY in $TEST_DEPENDS; do
echo "install" $TEST_DEPENDENCY
done
echo ""
echo "One package"
TEST_DEPENDS="a"
for TEST_DEPENDENCY in $TEST_DEPENDS; do
echo "install" $TEST_DEPENDENCY
done
echo ""
echo "Several packages"
TEST_DEPENDS="a b==12.3 c~=1.4 d>=1,<2"
for TEST_DEPENDENCY in $TEST_DEPENDS; do
echo "install" $TEST_DEPENDENCY
done
outputs
Empty string
One package
install a
Several packages
install a
install b==12.3
install c~=1.4
install d>=1,<2
So that all seems correct to me. Let me know if there is a use case I haven't considered.
@matthew-brett We're due for a quarterly Pillow release today (python-pillow/Pillow#3154), it would be great to get multibuild updated so we could include Python 3.7 wheels. Re:
@radarhere, do you know if this change will fix python-pillow/pillow-wheels#90? Thanks! |
@hugovk this only helps the macOS build in pillow-wheels, not the Linux builds. The problem in the macOS build is really that there are no Python 3.7 wheels for numpy and scipy yet.
|
|
Python 3.7 wheels for SciPy are now available: scipy/scipy#8987 |
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.
Installing everything with one command is fine in most of the cases so I'd suggest having a toggle to turn on this behavior and have it turned off by default
9d6a952
to
d505afb
Compare
Okay. I have added a commit so that instead of splitting up the packages into separate install commands by each word, instead it splits into separate install commands by each line. |
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.
LGTM
Note that I've since realised that newlines are not as convenient as I had presumed - |
You can probably do smth like - |
TEST_DEPENDS="pytest pytest-cov numpy
scipy" Or even use mapping instead of sequence in yaml env:
TEST_DEPENDS: |
'pytest pytest-cov numpy
scipy' |
In https://github.com/python-pillow/Pillow-wheels,
TEST_DEPENDS="pytest pytest-cov numpy scipy"
. scipy depends on numpy, and so if you try to install scipy and numpy at the same time, you receive an error that numpy is not present - https://travis-ci.org/python-pillow/pillow-wheels/jobs/397965939#L4956. This is not normally the case - it would seem to be happening here because wheels do not yet exist for Python 3.7.So instead, for this scenario, I suggest installing TEST_DEPENDS packages one at a time.
Note that this doesn't solve that pillow-wheels build completely, but it's a step in the right direction, and worthwhile as a change in it's own right.