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

Fixed build errors due to recent changes (homebrew/pip) #376

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

starrify
Copy link
Member

No description provided.

@codecov
Copy link

codecov bot commented Oct 24, 2019

Codecov Report

Merging #376 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #376      +/-   ##
==========================================
+ Coverage   91.49%   91.49%   +<.01%     
==========================================
  Files          34       34              
  Lines        2362     2364       +2     
==========================================
+ Hits         2161     2163       +2     
  Misses        201      201
Impacted Files Coverage Δ
shub/utils.py 90.47% <100%> (+0.03%) ⬆️

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 2871384...97acf9f. Read the comment docs.

@starrify starrify changed the title Fixed build error due to recent homebrew update Fixed build errors due to recent changes (homebrew/pip) Oct 25, 2019
@starrify
Copy link
Member Author

It's fixed finally. I initially assumed there was only one error. 🤦‍♀️

Copy link
Contributor

@vshlapakov vshlapakov left a 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
Copy link
Contributor

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
Copy link
Contributor

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

Copy link
Member Author

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
Copy link
Contributor

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?

Copy link
Member Author

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. 🤔

@starrify
Copy link
Member Author

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 pip's changes, assuming someone (maybe I myself) would soon provide a better approach than restricting the version -- but back then I intended to have the failed CIs resolved ASAP.

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

Successfully merging this pull request may close these issues.

2 participants