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
Add platforms build #9729
Add platforms build #9729
Conversation
006502d
to
460ae51
Compare
460ae51
to
debd390
Compare
2706f74
to
8a46422
Compare
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.
LGTM! I have a couple questions.
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.
Some minor changes.
Also looking at some logic copied over from buildx to this repo I'm afraid we are going to drift in the future.
Just my 2 cents but Compose should not push Docker images at all. Making it platform-neutral to build and run is a valid use-case but it should end here. If users want to build and push Docker images from their compose files they should use Bake.
As this is already part of the specification, I guess we have to move forward. So in a follow-up we should instead forward build requests to docker buildx bake
. Or maybe by using bake.ReadTargets
and convert them to build opts like https://github.com/docker/buildx/blob/30d650862d26932ffa860465bcd61aa455595f60/commands/bake.go#L106-L120 but then you need extra logic to handle drivers and builders. Maybe there is room on our side to have better support for buildx as a library. cc @tonistiigi @jedevc
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.
Few questions from playing around with it but code LGTM at a high level
From a behavior standpoint, I think the most awkward thing is the necessity to create/activate a buildx builder out-of-band:
multiple platforms feature is currently not supported for docker driver. Please switch to a different driver (eg. "docker buildx create --use")
Personally, I'd expect Compose to create & use a project-scoped buildx context, similar to how it creates a network automatically.
if err != nil { | ||
return nil, err | ||
} | ||
plats = append(plats, p) |
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 think we need to make sure we don't add a duplicate if a platform is already present via DOCKER_DEFAULT_PLATFORM
above:
services:
hello:
build:
context: .
platforms:
- linux/amd64
$ DOCKER_DEFAULT_PLATFORM="linux/amd64" docker compose build hello
...
=> ERROR exporting to image 0.0s
------
> exporting to image:
------
failed to solve: number of platforms does not match references 2 1
(Given how cryptic the BuildKit error is, it might also be beneficial [perhaps in compose-go
loader/validate?] to error if duplicates exist in service.build.platforms
, but I'm less concerned about that.)
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.
Also, is it weird that you can "extend" the supported platforms via DOCKER_DEFAULT_PLATFORM
?
e.g. in the same example above, DOCKER_DEFAULT_PLATFORM=linux/arm64 docker compose build hello
would build both AMD64 + ARM64 images even though only linux/amd64
is actually in compose.yaml
.
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.
Does it make sense for you if we manage the DOCKER_DEFAULT_PLATFORM
like the service.platform
, which means refuse to build if the DOCKER_DEFAULT_PLATFORM
isn't defined in the service.build.platforms
section?
pkg/compose/build.go
Outdated
buildOptions.Exports = []bclient.ExportEntry{{ | ||
Type: "image", | ||
Attrs: map[string]string{ | ||
"push": "true", | ||
}, | ||
}} |
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 type: "docker"
+ "load": "true"
like the other one?
I believe this is legitimately trying to push to registry:
$ docker compose build hello
...
=> ERROR exporting to image 0.7s
=> => exporting layers 0.1s
=> => exporting manifest sha256:90bf7923cd97b6dec8f53161c7df1e6d2587ec52ccc060273cdc6b6ae8bd6c9c 0.0s
=> => exporting config sha256:ec0a66e1fe3ca40d1042b63f3f82a0ff10189f86c6b68c1dd2b6f3213ac7057a 0.0s
=> => exporting manifest sha256:8a1a9bea74c44f39ab83dd4a831d74845c1f4eda2c3f2a5c230e0988fa554236 0.0s
=> => exporting config sha256:296e0f515945635ccc81e78c428bdf2b341a725f8ad033afbff0db2d0615aec1 0.0s
=> => exporting manifest list sha256:87e79b4bda828912aba9e22edd32010ab65b56da406c307bf60b61e79616b8cb 0.0s
=> => pushing layers 0.5s
------
> exporting to image:
------
failed to solve: server message: insufficient_scope: authorization failed
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 just realized it's doing this because you can't export multi-platform images with the docker
export type
I wonder if we need a --output
flag on build
with a default of type=docker
that behaves the same as the buildx flag, so it can error out if you try to do docker compose build --output=docker
on a multi-platform image.
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.
And TIL that docker compose push
already exists 🤔
But of course, it does not build:
$ docker compose push
An image does not exist locally with the tag: milas/hello
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
type: "docker"
+"load": "true"
like the other one?I believe this is legitimately trying to push to registry:
$ docker compose build hello ... => ERROR exporting to image 0.7s => => exporting layers 0.1s => => exporting manifest sha256:90bf7923cd97b6dec8f53161c7df1e6d2587ec52ccc060273cdc6b6ae8bd6c9c 0.0s => => exporting config sha256:ec0a66e1fe3ca40d1042b63f3f82a0ff10189f86c6b68c1dd2b6f3213ac7057a 0.0s => => exporting manifest sha256:8a1a9bea74c44f39ab83dd4a831d74845c1f4eda2c3f2a5c230e0988fa554236 0.0s => => exporting config sha256:296e0f515945635ccc81e78c428bdf2b341a725f8ad033afbff0db2d0615aec1 0.0s => => exporting manifest list sha256:87e79b4bda828912aba9e22edd32010ab65b56da406c307bf60b61e79616b8cb 0.0s => => pushing layers 0.5s ------ > exporting to image: ------ failed to solve: server message: insufficient_scope: authorization failed
Nope because docker
exporter doesn't support multi-arch builds, I just need to remove the push
option by default
430efa5
to
7a321d7
Compare
If compose can define its own buildx store that should be possible I think (using |
@milas IHMO buildx is Compose defaut builder, so if users decided to use the legacy builder, they don't expect to some automagic behaviour during the build process, especially with an explicit discarded builder, right? |
0ac4fa2
to
f900a64
Compare
f900a64
to
23c9aa6
Compare
Noticed today while using this build that I can't launch this project: https://github.com/dockersamples/compose-dev-env
(Not actually running via dev envs feature, using the |
Signed-off-by: Guillaume Lours <guillaume.lours@docker.com>
Signed-off-by: Guillaume Lours <705411+glours@users.noreply.github.com>
…ompose file Signed-off-by: Guillaume Lours <705411+glours@users.noreply.github.com>
…sts) support DOCKER_DEFAULT_PLATFORM when 'compose up --build' add tests to check behaviour when DOCKER_DEFAULT_PLATFORM is defined Signed-off-by: Guillaume Lours <705411+glours@users.noreply.github.com>
…mands Signed-off-by: Guillaume Lours <705411+glours@users.noreply.github.com>
43169d5
to
44c55e8
Compare
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.
!!
|
||
// Contains helps to detect if a non-comparable struct is part of an array | ||
// only use this method if you can't rely on existing golang Contains function of slices (https://pkg.go.dev/golang.org/x/exp/slices#Contains) | ||
func Contains[T any](origin []T, element T) bool { |
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 think the awkwardness is for users who are using BuildKit but with the default Docker driver, so they will likely be confused if they don't know about buildx + builder contexts. That said, I think the current approach is fine, as the error message does give you the command to create a buildx context, at which point things will work. In the future, maybe a new flag, e.g. |
Hi, P.S. love the panda picture! |
@lara-ec do your compose services that need to be constrained to amd64 have a build section at all? Would you mind including a sample snippet of your compose file? Thanks. |
@lara-ec can you confirm me that it's broken for all developers (M1 and Intel)? |
@nicksieger Yes, we need a service:
platform: linux/amd64
build:
context: ./service/
dockerfile: Dockerfile.dev @glours I've only tested it on Intel, I can test it on M1 tomorrow. But at least on Intel, the PR fixes it :) |
@lara-ec, @StefanScherer and I reproduced your issue, we confirm on our side that the PR resolve the problem. |
What I did
Add support of the
platforms
attribut, recently introduced in the Compose specification.This feature will allow to build multi-arch images from the
build
command and push those imagines to a registry.Huge thanks to @crazy-max for his help 🙏
Should fix #8531
(not mandatory) A picture of a cute animal, if possible in relation with what you did