-
Notifications
You must be signed in to change notification settings - Fork 52
Use docker buildx bake to build images #669
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
This is much cleaner than maintaining a list of complet shell commands.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
Thanks Drahtbot, added the commit from #606 |
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.
bake
is awesome and did not know about it.
|
||
The Bitcoin Core docker images used by warnet are specified in the *docker-bake.hcl* file. | ||
This uses the (experimental) `bake` build functionality of docker buildx. | ||
We use [HCL language](https://github.com/hashicorp/hcl) in the declaration file itself. | ||
See the `bake` [documentation](https://docs.docker.com/build/bake/) for more information on specifications, and how to e.g. override arguments. | ||
|
||
In order to build (or "bake") a certain image, find the image's target (name) in the *docker-bake.hcl* file, and then run `docker buildx bake <target>`. | ||
|
||
```bash | ||
# build the dummy image that will crash on 5k invs | ||
docker buildx bake bitcoin-5k-inv | ||
|
||
# build the same image, but set platform to only linux/amd64 | ||
docker buildx bake bitcoin-5k-inv --set bitcoin-5k-inv.platform=linux/amd64 |
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'm running the commands provided as example, but can we add a reference on how to create a builder before running the buildx command? Mostly for folks not used to build images for multiple platforms.
Even that I'm used to it I always the forget the commands.
https://docs.docker.com/reference/cli/docker/buildx/create/
https://www.docker.com/blog/how-to-rapidly-build-multi-architecture-images-with-buildx/
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.
Agreed, the instructions should outline all required steps. Assuming we're ok with the docker-container build driver (which supports multi-arch image builds), can simply add this as a preliminary step
docker buildx create --name=warnet-builder --use --bootstrap
dockerfile = "./Dockerfile" | ||
context = "./resources/images/bitcoin" | ||
args = { | ||
REPO = "bitcoin" |
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.
Shouldn't this be as shown below?
REPO = "bitcoin" | |
REPO = "bitcoin/bitcoin" |
@@ -0,0 +1,218 @@ | |||
group "all" { | |||
targets = [ | |||
"bitcoin-28", |
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.
Referring to a comment in src/warnet/constants.py
(replicated below), do you think somehow referencing or parsing the targets here would be a potential solution?
# Constants used throughout the project
# Storing as constants for now but we might want a more sophisticated config management
# at some point.
SUPPORTED_TAGS = ["27.0", "26.0", "25.1", "24.2", "23.2", "22.2"]
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.
Could leverage the tags of @Onyekachukwu-Nweke's suggestion to achieve this: #669 (comment)
I opened this PR to include cmake for future releases. It's against the @willcl-ark's branch so it can be included in this PR. |
@@ -0,0 +1,218 @@ | |||
group "all" { |
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 can eliminate the need to duplicate the target names in the sub-groups (maintained
, practice
, vulnerable
) and the main all
group by using variables
and concat
:
variable "maintained_targets" {
default = ["bitcoin-28", "bitcoin-27", "bitcoin-26"]
}
variable "practice_targets" {
default = ["bitcoin-unknown-message", "bitcoin-invalid-blocks", "bitcoin-50-orphans", "bitcoin-no-mp-trim", "bitcoin-disabled-opcodes", "bitcoin-5k-inv"]
}
variable "vulnerable_targets" {
default = ["v0-21-1", "v0-20-0", "v0-19-2", "v0-17-0", "v0-16-1"]
}
group "maintained" {
targets = maintained_targets
}
group "practice" {
targets = practice_targets
}
group "vulnerable" {
targets = vulnerable_targets
}
group "all" {
targets = concat(maintained_targets, practice_targets, vulnerable_targets)
}
platforms = ["linux/amd64", "linux/arm64", "linux/arm/v7"] | ||
} | ||
|
||
target "bitcoin-28" { |
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.
Implement a matrix pattern for version management
I recommend implementing a matrix pattern for targets that differ primarily by version and commit hash:
// Define a matrix of maintained versions
variable "MAINTAINED_VERSIONS" {
default = {
"28" = {
tag = "28.0"
commit = "110183746150428e6385880c79f8c5733b1361ba"
}
"27" = {
tag = "27.2"
commit = "bf03c458e994abab9be85486ed8a6d8813313579"
}
"26" = {
tag = "26.2"
commit = "7b7041019ba5e7df7bde1416aa6916414a04f3db"
}
}
}
// Then reference in target definitions:
target "bitcoin-maintained" {
name = "bitcoin-${key}"
inherits = ["maintained-base"]
matrix = {
key = keys(variable.MAINTAINED_VERSIONS)
}
tags = ["bitcoindevproject/bitcoin:${variable.MAINTAINED_VERSIONS[key].tag}"]
args = {
COMMIT_SHA = "${variable.MAINTAINED_VERSIONS[key].commit}"
}
}
This pattern could be extended to both practice-base
and cve-base targets
.
This approach:
- Makes the configuration more maintainable
- Reduces duplication
- Makes it easier to add new versions
- It creates a clear separation between configuration and implementation
REPO = "bitcoin" | ||
BUILD_ARGS = "--disable-tests --without-gui --disable-bench --disable-fuzz-binary --enable-suppress-external-warnings " | ||
} | ||
platforms = ["linux/amd64", "linux/arm64", "linux/arm/v7"] |
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.
For improved modularity, consider implementing this as a reusable variable."
variable "DEFAULT_PLATFORMS" {
default = ["linux/amd64", "linux/arm64", "linux/arm/v7"]
}
platforms = "${variable.DEFAULT_PLATFORMS}"
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.
variable. is not needed from my experience to reference a variable
variable "DEFAULT_PLATFORMS" {
default = ["linux/amd64", "linux/arm64", "linux/arm/v7"]
}
platforms = "${DEFAULT_PLATFORMS}"
*Note: platform can be overridden as needed as well when baking
docker buildx bake <target> --set "*.platform="linux/arm64""
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.
Yeah, works like @macgyver13 suggested
EXTRA_RUNTIME_PACKAGES = "" | ||
REPO = "willcl-ark/bitcoin" | ||
} | ||
platforms = ["linux/amd64", "linux/armhf"] |
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.
Following the same pattern as my previous comment, I recommend making this a reusable variable as well.
This is much cleaner than maintaining a list of complete shell commands.