Fix pip list deprecation warning #187
Conversation
There was a problem hiding this comment.
Looks great, just one minor nit! (and added some clarification about |tac|tac| below)
Overall, looks like pip freeze was what we wanted in the first place when we went for pip list -- I wonder though if we can duplicate the issue from #100 and make sure this still successfully catches it?
2.7/Dockerfile
Outdated
| && rm /tmp/get-pip.py \ | ||
| && wget -O /tmp/get-pip.py 'https://bootstrap.pypa.io/get-pip.py' \ | ||
| && python2 /tmp/get-pip.py "pip==$PYTHON_PIP_VERSION" \ | ||
| && rm /tmp/get-pip.py \ |
There was a problem hiding this comment.
This looks like it was unintentionally added here -- these are kept indented so that diff between this version and the other versions is smaller (no whitespace changes, only removed lines, essentially).
2.7/Dockerfile
Outdated
| # https://github.com/docker-library/python/pull/100 | ||
| && [ "$(pip list |tac|tac| awk -F '[ ()]+' '$1 == "pip" { print $2; exit }')" = "$PYTHON_PIP_VERSION" ] \ | ||
| && [ "$(pip freeze --all |tac|tac| awk -F '==' '$1 == "pip" { print $2; exit }')" = "$PYTHON_PIP_VERSION" ] \ |
There was a problem hiding this comment.
Very nice -- pip freeze is indeed a much better solution. 👍
The |tac|tac| is a cheeky trick to ensure the entire output of pip freeze is read completely -- some applications get really grumpy when we do exit from awk too quickly and standard input is then closed (causing pip freeze to have standard output closed). It's easier to illustrate with curl+head:
$ curl -fsSL 'https://google.com' | head -1 > /dev/null
curl: (23) Failed writing body (867 != 1339)There was a problem hiding this comment.
Aha! Thanks for the clarification 👍
|
Removed the 2.7 whitespace commit. I also had a brief look at replicating the issue in #100 but wasn't able to, due to |
|
I just built a [... snip ...]
Successfully installed pip-7.1.2 setuptools-18.2
+ ldconfig
+ pip3 install --no-cache-dir --upgrade --ignore-installed pip==8.1.1
Collecting pip==8.1.1
Downloading pip-8.1.1-py2.py3-none-any.whl (1.2MB)
Installing collected packages: pip
Successfully installed pip-8.1.1
[... snip ...]# pip freeze
You are using pip version 8.1.1, however version 9.0.1 is available.
You should consider upgrading via the 'pip install --upgrade pip' command.
# pip3 freeze
You are using pip version 8.1.1, however version 9.0.1 is available.
You should consider upgrading via the 'pip install --upgrade pip' command.I was expecting to see Digging a little deeper I found references to At this point I'm at a bit of a loss. The proposed change makes sense in the context of updating for deprecation, but it seems like the reason for this check being present at all could use some further investigation, probably from someone with more familiarity with pip and python module internals than me I'm afraid 😞 |
|
Hmm, OK -- maybe then we should simply remove the check entirely and revisit if the problem comes back. Sound sane? |
|
Think so, yes. If nothing else the |
Following discussion in docker-library#187 it was decided that rather than refactor this check, it should be removed. This was because the original error condition it was designed to prevent could not be reproduced. This is done with the understanding that it may need to be revisited if any recurrance (of docker-library#100) is encountered in future.
- `docker`: experimental multiarch (docker-library/docker#52) - `golang`: experimental multiarch (docker-library/golang#158) - `nextcloud`: 10.0.5, 11.0.3, 9.0.58, update via container updates (nextcloud/docker#65) - `postgres`: ensure `postgres` user's HOME has decent permissions (docker-library/postgres#277) - `python`: fix `pip` install to also install `setuptools` and `wheel` (docker-library/python#186, docker-library/python#187)
Following discussion in docker-library#187 it was decided that rather than refactor this check, it should be removed. This was because the original error condition it was designed to prevent could not be reproduced. This is done with the understanding that it may need to be revisited if any recurrance (of docker-library#100) is encountered in future.
Fix pip list deprecation warning
Following discussion in docker-library/python#187 it was decided that rather than refactor this check, it should be removed. This was because the original error condition it was designed to prevent could not be reproduced. This is done with the understanding that it may need to be revisited if any recurrance (of docker-library/python#100) is encountered in future.
Following discussion in docker-library/python#187 it was decided that rather than refactor this check, it should be removed. This was because the original error condition it was designed to prevent could not be reproduced. This is done with the understanding that it may need to be revisited if any recurrance (of docker-library/python#100) is encountered in future.
While working on another PR I noticed the following deprecation warning in the output:
To pre-empt this change I've moved the check over to using
pip freeze. I thinkpip freeze --allis a better candidate for this check as it not only has an easier to parse explicitly defined format, but is also less likely to change.As an aside, I do not understand the purpose of the
|tac|tac|pipe in the affected line (maybe buffering?), but have left it in place as history seems to suggest it was intentionally added during the move to templates, unfortunately no explicit reasoning is given. In some cursory testing it appeared to make no difference, but if this is some sort of intentional trick I would very much like to understand it, if anyone is willing to share their insight. 😄