Skip to content

build the cryptography manylinux1 docker images#98

Merged
alex merged 4 commits into
masterfrom
manylinux1-work
Jun 29, 2017
Merged

build the cryptography manylinux1 docker images#98
alex merged 4 commits into
masterfrom
manylinux1-work

Conversation

@reaperhulk

Copy link
Copy Markdown
Member

No description provided.

@reaperhulk

reaperhulk commented Jun 9, 2017

Copy link
Copy Markdown
Member Author

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
Comment thread config.json Outdated
{
"tag": "pyca/crypto-jenkins",
"path": "jenkins",
"dockerfile": "jenkins/Dockerfile",

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm pretty sure it is in a release: https://github.com/moby/moby/releases/tag/v17.05.0-ce

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

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
Copy Markdown
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!

Comment thread cryptography-manylinux1/Dockerfile-i686 Outdated
@@ -0,0 +1,10 @@
FROM quay.io/pypa/manylinux1_i686

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

alex commented Jun 20, 2017 via email

Copy link
Copy Markdown
Member

@reaperhulk

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

alex commented Jun 20, 2017

Copy link
Copy Markdown
Member

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
Comment thread cryptography-manylinux1/Dockerfile Outdated
@@ -0,0 +1,11 @@
ARG ARCH=x86_64

Copy link
Copy Markdown
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.

Comment thread cryptography-manylinux1/Dockerfile Outdated
ARG ARCH=x86_64
FROM quay.io/pypa/manylinux1_${ARCH}
MAINTAINER Python Cryptographic Authority
ADD install_openssl.sh /root/install_openssl.sh

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member

Choose a reason for hiding this comment

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

same

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member

Choose a reason for hiding this comment

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

same

@alex

alex commented Jun 29, 2017

Copy link
Copy Markdown
Member

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
Copy Markdown
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