-
Notifications
You must be signed in to change notification settings - Fork 80
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
Fixed build errors due to recent changes (homebrew/pip) #376
base: master
Are you sure you want to change the base?
Conversation
…ig since pyenv may seriously install Python 2.7.0
Codecov Report
@@ Coverage Diff @@
## master #376 +/- ##
==========================================
+ Coverage 91.49% 91.49% +<.01%
==========================================
Files 34 34
Lines 2362 2364 +2
==========================================
+ Hits 2161 2163 +2
Misses 201 201
Continue to review full report at Codecov.
|
…n no longer callable)
It's fixed finally. I initially assumed there was only one error. 🤦♀️ |
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.
@starrify Thanks for looking into this 🙌
I'm not surprised by the homebrew issue, it adds some complexities from time to time. However, in a case of changes in the recent pip, I have a feeling that it'd be easier to update our imports instead of restricting pip version - many people update it regularly and I'd like to avoid the version limitation if possible. Do you agree or there's something I'm missing?
@@ -40,7 +40,8 @@ branches: | |||
before_install: | | |||
if [[ "$TRAVIS_OS_NAME" == "osx" ]]; then | |||
# From https://pythonhosted.org/CodeChat/.travis.yml.html | |||
brew install pyenv-virtualenv | |||
# Homebrew currently fails after updating. See also: https://discuss.circleci.com/t/brew-install-fails-while-updating/32992/4 | |||
HOMEBREW_NO_AUTO_UPDATE=1 brew install pyenv-virtualenv |
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.
Good catch!
retrying | ||
six | ||
tqdm | ||
|
||
scrapinghub>=2.0.3 | ||
|
||
pip<19.3 |
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.
It seems that in the recent pip, the main
function was moved from pip._internal.init to pip._internal.main. It's not the best idea to rely on such internal functions, but I'd say there's a simpler way to fix this without restrictions on pip version
try:
from pip import main as pip_main
except:
try:
from pip._internal.main import main as pip_main
except:
from pip._internal import main as pip_main
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.
Yeah this is good observation! I should have looked into this. Though I'm not sure for now how to write tests for this. 😅
@@ -1,13 +1,15 @@ | |||
click | |||
docker-py | |||
PyYAML | |||
requests | |||
#PyYAML |
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 guess these should be uncommented?
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.
The same file has requests>=2.20.0
and pyyaml>=4.2b1
below which may lead to double requirements issues. 🤔
Hi @vshlapakov , thanks for the review! I was thinking of some quick (and maybe temporary) fixes for the broken CI when creating this pull request, since it affects also pull requests. I didn't look much into |
a290a34
to
b0e4614
Compare
No description provided.