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

Fail fast on multi platform build with load #582

Merged
merged 1 commit into from Jun 28, 2021

Conversation

rumpl
Copy link
Member

@rumpl rumpl commented Apr 3, 2021

Avoid doing a build if we cannot load the images at the end.

This doesn't really fix anything but at least users will see the error right away and not after a potentially long build.

@rumpl rumpl force-pushed the feat-fail-fast branch 2 times, most recently from 902304f to 02b8862 Compare April 3, 2021 10:52
@tonistiigi
Copy link
Member

tonistiigi commented Apr 6, 2021

This should fail fast already without building. Should block it in moby level if not. edit: nvm: this is a different case where building in container and exporting to docker

For buildx side, this check should be moved to build/build.go so that builds from bake also pass validation.

@tonistiigi
Copy link
Member

hmm

buildx/build/build.go

Lines 460 to 462 in 84a734d

if len(pp) > 1 && !d.Features()[driver.MultiPlatform] {
return nil, nil, notSupported(d, driver.MultiPlatform)
}

@rumpl
Copy link
Member Author

rumpl commented Apr 7, 2021

Ok so it's building before failing if the driver is docker-container, which has driver.MultiPlatform: true

@tonistiigi
Copy link
Member

@rumpl Ah, ok. That is different. Add this extra condition to https://github.com/docker/buildx/blob/master/build/build.go#L415

Copy link
Member

@tonistiigi tonistiigi left a comment

Choose a reason for hiding this comment

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

^

@rumpl
Copy link
Member Author

rumpl commented Apr 22, 2021

^

👍 I'm moving right now, will get at this next week, sorry

build/build.go Outdated
@@ -520,6 +520,10 @@ func toSolveOpt(ctx context.Context, d driver.Driver, multiDriver bool, opt Opti
so.FrontendAttrs["platform"] = strings.Join(pp, ",")
}

if len(opt.Platforms) != 1 && d.Factory().Name() == "docker-container" {
Copy link
Member

Choose a reason for hiding this comment

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

This does not look correct. Multi-platform builds with docker-container driver are definitely supported. If I understood your description correctly then the condition you want is to check if exporting is happening to docker. Also, we should never check for driver names. There is a features list for that but don't think you need that atm., just check for the exporter.

@rumpl
Copy link
Member Author

rumpl commented Jun 24, 2021 via email

@tonistiigi
Copy link
Member

I should check that the exporter is “docker” right?

Correct

@rumpl rumpl force-pushed the feat-fail-fast branch 3 times, most recently from 43918bf to e78215c Compare June 25, 2021 14:16
build/build.go Outdated
@@ -520,6 +520,10 @@ func toSolveOpt(ctx context.Context, d driver.Driver, multiDriver bool, opt Opti
so.FrontendAttrs["platform"] = strings.Join(pp, ",")
}

if len(opt.Platforms) > 1 && opt.Exports[0].Type == "docker" {
Copy link
Member

Choose a reason for hiding this comment

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

Looks correct but move it to line 458 so all the docker exporter handling is in one place.

Signed-off-by: Djordje Lukic <djordje.lukic@docker.com>
Copy link
Member

@crazy-max crazy-max left a comment

Choose a reason for hiding this comment

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

LGTM

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.

None yet

3 participants