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

use Docker instead of BuildKit #506

Merged
merged 4 commits into from
Nov 14, 2019
Merged

use Docker instead of BuildKit #506

merged 4 commits into from
Nov 14, 2019

Conversation

bcressey
Copy link
Contributor

@bcressey bcressey commented Nov 12, 2019

Issue #, if available:
#470

Description of changes:
Switch to the version of BuildKit integrated with Docker to simplify the build requirements.

Testing done:
Ran cargo make clean && cargo make world; build succeeded.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@bcressey bcressey requested review from jahkeup and zmrow November 12, 2019 00:09
tools/buildsys/src/builder.rs Show resolved Hide resolved
tools/buildsys/src/builder.rs Show resolved Hide resolved
tools/buildsys/src/builder.rs Show resolved Hide resolved
Copy link
Contributor

@zmrow zmrow left a comment

Choose a reason for hiding this comment

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

🍰

Copy link
Member

@jahkeup jahkeup left a comment

Choose a reason for hiding this comment

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

I'd like to hear from @patraw and/or @etungsten about their woes with DOCKER_BUILDKIT to make sure there's not an edge case we're hitting.

Makefile.toml Show resolved Hide resolved
tools/buildsys/src/builder.rs Show resolved Hide resolved
tools/buildsys/src/builder.rs Show resolved Hide resolved
tools/buildsys/src/builder.rs Show resolved Hide resolved
tools/buildsys/src/builder.rs Show resolved Hide resolved
@jahkeup jahkeup mentioned this pull request Nov 12, 2019
@etungsten

This comment has been minimized.

@bcressey
Copy link
Contributor Author

Added a fix for concurrent builds on the same host, and switched to docker build --network=host to work around the buildkit issue.

Copy link
Contributor

@etungsten etungsten left a comment

Choose a reason for hiding this comment

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

🐳

Copy link
Contributor

@tjkirch tjkirch left a comment

Choose a reason for hiding this comment

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

Would you please include an update to INSTALL.md to remove BuildKit setup?

@bcressey
Copy link
Contributor Author

Would you please include an update to INSTALL.md to remove BuildKit setup?

Done. I also filled in a CHANGELOG.md with some relevant entries, and cleaned up a couple of stray BuildKit references.

@bcressey
Copy link
Contributor Author

Backed out CHANGELOG.md changes per discussion.

@jahkeup
Copy link
Member

jahkeup commented Nov 13, 2019

Backed out CHANGELOG.md changes per discussion.

What was the reasoning there? I think I missed this conversation.

@bcressey
Copy link
Contributor Author

Backed out CHANGELOG.md changes per discussion.

What was the reasoning there? I think I missed this conversation.

Essentially, the desire to have changelog entries written by the same author, so the voice and focus would be consistent.

Copy link
Contributor

@tjkirch tjkirch left a comment

Choose a reason for hiding this comment

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

While I don't understand all of the Docker specifics, it's clearly simpler, and fewer dependencies for our users. 🚀

INSTALL.md Show resolved Hide resolved
tools/buildsys/src/builder/error.rs Outdated Show resolved Hide resolved
tools/buildsys/src/builder.rs Show resolved Hide resolved
Dockerfile Show resolved Hide resolved
tools/buildsys/src/builder.rs Show resolved Hide resolved
Newer versions of Docker have BuildKit integrated, and we already
depend on Docker to provide some of our build tools.

BuildKit's ability to copy files directly out of a build image is
convenient, but we can work around that using Docker functionality,
and eliminate the need to run another daemon for builds.

Signed-off-by: Ben Cressey <[email protected]>
Although the cache volume was previously created and was functional,
the subsequent build stage could not see the expected marker file
under either Docker 18.09 and 19.03.

With this change, the marker file is now visible, which verifies that
we are using the cache volume that matches the build stage.

This is a follow-up to the fix in f5c863c.

Signed-off-by: Ben Cressey <[email protected]>
Copy link
Member

@jahkeup jahkeup left a comment

Choose a reason for hiding this comment

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

Builds for me 🔨 👍

@bcressey bcressey merged commit e3cd018 into develop Nov 14, 2019
jahkeup added a commit that referenced this pull request Nov 15, 2019
PR #506 switches us away from needing a daemon running.

Signed-off-by: Jacob Vallejo <[email protected]>
@bcressey bcressey deleted the docker-build branch November 15, 2019 16:33
@iliana iliana added this to the v0.2.0 milestone Nov 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants