-
Notifications
You must be signed in to change notification settings - Fork 46
Bring up to equivalent quality as Python images #10
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
Conversation
There are currently failing tests because the tests check for As far as I can see, 2 obvious ways to fix this would be:
|
# runtime dependencies | ||
RUN apt-get update && apt-get install -y --no-install-recommends \ | ||
tcl \ | ||
tk \ |
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.
Note that I'm not installing these in the slim images, as the same is not done in the Python slim images.
You can check for the necessary libraries doing something like:
find /usr/local -type f -iname '*.so' | xargs ldd | grep 'not found'
On the non-slim Python/PyPy images this returns nothing. On the slim PyPy images this returns:
libtcl8.6.so => not found
libtk8.6.so => not found
libgdbm.so.3 => not found
On the slim Python images it returns:
libtk8.6.so => not found
libtcl8.6.so => not found
libX11.so.6 => not found
Not sure if missing gdbm is an issue...
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.
We could open a new issue to research the missing libgdbm
to see if it is a problem. It is missing from the current slim images as well, so it is unrelated to your changes.
As for the test change vs symlink, I would think fixing the test is the correct way. On the other had, do users of pypy expect |
👍 Overall looks like a great improvement. |
Normally one would make a virtualenv I guess, and then this wouldn't be a problem, but that's a bit less common inside a container. I've found it occasionally handy to have So I'm not really sure.. it's usually not necessary I don't think. It's maybe worth noting that the |
📗 💚 LGTM |
&& curl -SL "https://bitbucket.org/pypy/pypy/downloads/pypy2-v${PYPY_VERSION}-linux64.tar.bz2" \ | ||
| tar -xjC /usr/local --strip-components=1 | ||
ENV PYPY_VERSION 5.4.0 | ||
ENV PYPY_SHA256SUM bdfea513d59dcd580970cb6f79f3a250d00191fd46b68133d5327e924ca845f8 |
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.
Where does this value come from, and how is it expected to be updated? With Python, these values come directly from upstream and are updated via update.sh
automatically (anything else is too error prone).
If upstream doesn't provide any means of verification, I'd rather not create our own (and thus just trust https and bitbucket as we've done, and as upstream officially provides).
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's from the downloads page: http://pypy.org/download.html#checksums
I didn't realise this stuff was updated via a script. I could work something into update.sh
that does something like this (accounting for versions and such):
curl http://pypy.org/download.html \
| grep -E '[a-f0-9]{64} pypy2-v5.4.0-linux64.tar.bz2' \
| cut -d' ' -f1
What do you think? Or is it just not worth the effort?
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've added a command to fetch the checksum to update.sh
.
* Run update.sh to update to PyPy2.7 5.4.1
Merged from master and updated to PyPy 5.4.1 |
Thanks @JayH5! 👍 LGTM |
Ports over roughly the following PRs:
Also adds SHA256 checksum checks for the PyPy tarball downloads.
Apologies that this is kind of big but I'm keen that this stuff doesn't get left behind.