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

build the cryptography manylinux1 docker images #98

Merged
merged 4 commits into from
Jun 29, 2017
Merged

Conversation

reaperhulk
Copy link
Member

No description provided.

@reaperhulk
Copy link
Member Author

reaperhulk commented Jun 9, 2017

Big issue right now is that I have to pass a directory to the build context, but I need that install_openssl.sh command. If there's a way to dedupe this sanely that'd be great.

Update: fixed.

@reaperhulk reaperhulk force-pushed the manylinux1-work branch 2 times, most recently from f51e122 to 20ece69 Compare June 15, 2017 17:46
config.json Outdated
@@ -2,66 +2,91 @@
{
"tag": "pyca/crypto-jenkins",
"path": "jenkins",
"dockerfile": "jenkins/Dockerfile",
Copy link
Member

Choose a reason for hiding this comment

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

We should not need dockerfile and path keys. as far as I can tell they are duplicative.

Copy link
Member Author

Choose a reason for hiding this comment

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

if you supply a -f to docker build then you must supply the Dockerfile name. path then becomes the docker build context. If you don't supply -f it is assumed to be Dockerfile inside the build context, but that won't work when you want multiple dockerfiles to share the build context.

That said, ARG in FROM hypothetically resolves this and makes it unnecessary. Unfortunately while that's merged it's not available in a stable docker CE release yet?

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

(As noted in IRC, 17.05 isn't available as stable, only edge)

Copy link
Member

Choose a reason for hiding this comment

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

Sigh. Do we know when edge next goes stable? cc: @cyli

Copy link
Member Author

Choose a reason for hiding this comment

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

Their docs say quarterly. Latest stable is .03 so presumably we should expect to see a 17.07 in stable in about a month?

Copy link

Choose a reason for hiding this comment

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

Currently in RC5 - should be soon - RC5 was unexpected.

Copy link
Member Author

Choose a reason for hiding this comment

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

Pretty sure we can wait then 😄 Thanks @cyli!

@@ -0,0 +1,10 @@
FROM quay.io/pypa/manylinux1_i686
Copy link
Member

Choose a reason for hiding this comment

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

Now that the latest docker supports ARG in from, I believe these can be consolidated.

@alex
Copy link
Member

alex commented Jun 20, 2017 via email

@reaperhulk
Copy link
Member Author

What's 1 month among friends? Yes, 17.06

https://blog.docker.com/2017/03/docker-enterprise-edition/

So I guess at some point soon there should be a stable update?

@alex
Copy link
Member

alex commented Jun 20, 2017

How do you feel about just waiting for that, since it lets us simplify considerably?

This utilizes ARG in FROM and requires docker 17.06-ce or better to
build
alex
alex previously approved these changes Jun 29, 2017
@@ -0,0 +1,11 @@
ARG ARCH=x86_64
Copy link
Member

Choose a reason for hiding this comment

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

Please remove the default.

ARG ARCH=x86_64
FROM quay.io/pypa/manylinux1_${ARCH}
MAINTAINER Python Cryptographic Authority
ADD install_openssl.sh /root/install_openssl.sh
Copy link
Member

Choose a reason for hiding this comment

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

Nit, move the ADD to right before we use it.

function check_sha256sum {
local fname=$1
local sha256=$2
echo "${sha256} ${fname}" > ${fname}.sha256
Copy link
Member

Choose a reason for hiding this comment

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

Quote the output path

local fname=$1
local sha256=$2
echo "${sha256} ${fname}" > ${fname}.sha256
sha256sum -c ${fname}.sha256
Copy link
Member

Choose a reason for hiding this comment

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

same

Copy link
Member

Choose a reason for hiding this comment

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

Does this function do the right thing on failure?

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 on failure it prints to stderr and since we're set -xe and it returns non-zero error code the script halts.

local sha256=$2
echo "${sha256} ${fname}" > ${fname}.sha256
sha256sum -c ${fname}.sha256
rm ${fname}.sha256
Copy link
Member

Choose a reason for hiding this comment

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

same

@alex
Copy link
Member

alex commented Jun 29, 2017

By approve I mean "please fix all these things"

echo "Configuring for i686"
./Configure linux-generic32 no-comp shared --prefix=/opt/pyca/cryptography/openssl --openssldir=/opt/pyca/cryptography/openssl
fi
make depend
Copy link
Member

Choose a reason for hiding this comment

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

Should there be a -j$(nproc) or something on this?

@alex alex merged commit 19ddf40 into master Jun 29, 2017
@alex alex deleted the manylinux1-work branch June 29, 2017 03:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants