Multi-stage Docker builds - #37
Conversation
Shadowfiend
left a comment
Shadowfiend
left a comment
There was a problem hiding this comment.
Left a question. Worth noting, once my first PR is merged, we'll be able to try executing the client's main function as a way to see if the basics are working. It runs relatively quickly and exercises the BLS code paths.
Tests to come soon, then we'll really be in bidness.
| RUN dep ensure --vendor-only | ||
|
|
||
| RUN CGO_ENABLED=1 GOOS=linux go build -a -installsuffix cgo -o $APP_NAME . && \ | ||
| mv $APP_NAME /usr/local/bin/ |
There was a problem hiding this comment.
Interesting. So this will build without bn because bn is dynamically linked? May want to investigate (not right now haha) how we might link it statically. Do we think it's worth filing an issue?
Out of curiosity, what's the benefit of doing this before we do all the updates below, vs after? It feels like the right flow organizationally (to me!) to handle dependencies from furthest away (non-Go) to closest (Go) and then conclude by building our own executable, rather than building the executable up top and pulling in additional dependencies further down, but it's entirely possible we gain something I missed :)
There was a problem hiding this comment.
I'd love to get it statically linked- that way the client could work standalone. I spent a ton of time on it early on but couldn't figure it out.
It feels like the right flow organizationally (to me!) to handle dependencies from furthest away (non-Go) to closest (Go) and then conclude by building our own executable
This is where we're getting a benefit. This Dockerfile builds two images, and in the second we have way less stuff, but take the static build artifact from the first. If the library could be statically built and easily moved, we'd build the lib and client in the first and copy both to the new image.
Basically, takeaway- this pattern is two different containers with different base images, installs, etc
There was a problem hiding this comment.
Aha! So basically if we built the bn image first, we'd need to pull a bunch of Go gunk in to do our Go build after, and then it'd be in the final image. Here, we do the Go stuff, create a basic Alpine image, pull in the stuff we need to compile bn + bn itself, build it, nuke the packages that we pulled in to build it, and pull in our static Go artifact. End result: we have the bn compile installed in the system, plus our static Go artifact, and nothing else.
| APP_REPO_DIR=/go/src/keep-network/keep-client \ | ||
| APP_NAME=keep-client | ||
|
|
||
| COPY ./go $APP_REPO_DIR |
There was a problem hiding this comment.
This has to happen after the dep ensure or a *.go file change will rerun the whole apk + dep ensure flow.
l3x
left a comment
l3x
left a comment
There was a problem hiding this comment.
Looks great, with one exception.
The keep-client COPY command should look like this:
COPY --from=0 /usr/local/bin/keep-client /usr/local/bin/
Without this change we'll see something like this when running the container:
docker: Error response from daemon: OCI runtime create failed: container_linux.go:296: starting container process caused "exec: \"keep-client\": executable file not found in $PATH": unknown.
Also added a test-keep-client.sh file, which uses the other 2 scripts. Should the logic in these bash scripts be moved to a Makefile? Merge branch 'docker-build-process' into docker-build-process-multistage * docker-build-process: Add vendor directory to github. Add --force flag for build script and mention including vendor dir Update go-dfinity-crypto package references
|
Hey Matt, I didn't intend to make changes to your branch (docker-build-process-multistage). But I did, and just went with it, merging those changes back into my branch (docker-build-process). I left the 3 bash scripts and added a note about whether their logic should be put into a Makefile . |
New Merkle Distribution for Nov 1st
…in epic Combine the per-PR Keep a Changelog entries (threshold-network#36, threshold-network#37, threshold-network#34, threshold-network#38, threshold-network#39, threshold-network#40, #8, #2, #14) into shared Added/Changed/Fixed/Security sections under [Unreleased].
…in epic Combine the per-PR Keep a Changelog entries (threshold-network#36, threshold-network#37, threshold-network#34, threshold-network#38, threshold-network#39, threshold-network#40, #8, #2, #14) into shared Added/Changed/Fixed/Security sections under [Unreleased].
3 participants
Based off #36,
In the spirit of the original PR, this
go-dfinity-crypto