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

Fix pip bootstrapping on Python 2.7 #475

Merged
merged 1 commit into from
Jan 26, 2021
Merged

Fix pip bootstrapping on Python 2.7 #475

merged 1 commit into from
Jan 26, 2021

Conversation

VivekPanyam
Copy link
Collaborator

Summary:

The latest version of pip drops support for Python 2.7 (pypa/pip#6148).

This PR modifies install_python_deps.sh to install a Python 2.7 compatible version of pip.

We should also explore dropping Python 2.7 support in the future as it's been over a year since its EOL.

Test Plan:

CI

@@ -5,7 +5,7 @@ set -e
NEUROPOD_PYTHON_BINARY="python${NEUROPOD_PYTHON_VERSION}"

# Install pip
wget https://bootstrap.pypa.io/get-pip.py -O /tmp/get-pip.py
wget https://bootstrap.pypa.io/2.7/get-pip.py -O /tmp/get-pip.py
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. I see it is used at build/ci/install_lint_deps.sh too.
  2. Does it make sense to cache it? like:
tools/lint.sh-    if [ ! -f "/tmp/buildifier" ]; then
tools/lint.sh:        wget https://github.com/bazelbuild/buildtools/releases/download/2.2.1/buildifier -O /tmp/buildifier
tools/lint.sh-        chmod +x /tmp/buildifier
tools/lint.sh-    fi

or it is not used often?

Copy link
Collaborator Author

@VivekPanyam VivekPanyam Jan 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not used often. The reason I built the caching in the lint script above (and some other places) is because those can be used a lot

Copy link
Contributor

@vkuzmin-uber vkuzmin-uber left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left comment, though you can go ahead if you need to update here only

@codecov
Copy link

codecov bot commented Jan 26, 2021

Codecov Report

Merging #475 (7c63f98) into master (c15727e) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #475   +/-   ##
=======================================
  Coverage   88.23%   88.23%           
=======================================
  Files         106      106           
  Lines        6852     6852           
=======================================
  Hits         6046     6046           
  Misses        806      806           

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 c15727e...7c63f98. Read the comment docs.

@VivekPanyam VivekPanyam merged commit 9199206 into master Jan 26, 2021
@VivekPanyam VivekPanyam deleted the fix_py2 branch January 26, 2021 06:09
vkuzmin-uber pushed a commit that referenced this pull request Mar 15, 2021
### Summary:

The latest version of pip drops support for Python 2.7 (pypa/pip#6148).

This PR modifies `install_python_deps.sh` to install a Python 2.7 compatible version of pip.

We should also explore dropping Python 2.7 support in the future as it's been over a year since its EOL.

### Test Plan:

CI
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