vendor: github.com/moby/buildkit v0.14.0-rc2-dev #47683

vvoland · 2024-04-05T15:00:16Z

depends on ~~dockerfile/parse: Fix nil dereference of a linter buildkit#4984~~ dockerfile/linter: check for nil linter in linter functions buildkit#4996

full diff: moby/buildkit@v0.13.1...v0.14.0-rc1

A draft PR to follow changes on buildkit side.

Moby integration adjustments

builder: Pass nil linter to instructions.Parse

eea0b41bf4fb1d69e109ff5ff8045c63f0c0d510 added a new argument to
instructions.Parse to support issuing linter warnings.

Classic builder uses it to parse the Dockerfile instructions and its
usage needs adjustment.

The classic builder is deprecated and we won't be adding any new
features to it, so we just pass a nil linter callback.

builder: Adjust usage of shlex.ProcessWord

1b1c5bc08ad81add007eb647e66ed0929693f3a0 extended the function signature
with one additional return value.

builder: Update detect usage for new detect API from buildkit

builder-next: Adjust NewGatewayFrontend invocation

b5c50afa882e2b34aba880fd5028615e2ef94e07 changed the signature of
NewGatewayFrontend to include a slice of allowed repositories.

Docker does not allow to specify this option, so don't place any
restrictions by passing an empty slice.

Signed-off-by: Paweł Gronowski pawel.gronowski@docker.com

crazy-max · 2024-04-25T12:14:50Z

Looks like we forgot a compat check for https://github.com/moby/moby/actions/runs/8830465079/job/24245262993?pr=47683#step:12:2959

=== FAIL: frontend/dockerfile TestIntegration/TestCommandSourceMapping/worker=dockerd/frontend=builtin (5.31s)
    dockerfile_provenance_test.go:1183: 
        	Error Trace:	/src/frontend/dockerfile/dockerfile_provenance_test.go:1183
        	            				/src/util/testutil/integration/run.go:94
        	            				/src/util/testutil/integration/run.go:208
        	Error:      	Received unexpected error:
        	            	exporter "image" could not be found
        	            	github.com/moby/buildkit/util/stack.Enable
        	            		/src/util/stack/stack.go:77
        	            	github.com/moby/buildkit/util/grpcerrors.FromGRPC
        	            		/src/util/grpcerrors/grpcerrors.go:198
        	            	github.com/moby/buildkit/util/grpcerrors.UnaryClientInterceptor
        	            		/src/util/grpcerrors/intercept.go:41
        	            	google.golang.org/grpc.(*ClientConn).Invoke
        	            		/src/vendor/google.golang.org/grpc/call.go:35
        	            	github.com/moby/buildkit/api/services/control.(*controlClient).Solve
        	            		/src/api/services/control/control.pb.go:2234
        	            	github.com/moby/buildkit/client.(*Client).solve.func2
        	            		/src/client/solve.go:274
        	            	golang.org/x/sync/errgroup.(*Group).Go.func1
        	            		/src/vendor/golang.org/x/sync/errgroup/errgroup.go:75
        	            	runtime.goexit
        	            		/usr/local/go/src/runtime/asm_amd64.s:1650
        	            	failed to solve
        	            	github.com/moby/buildkit/client.(*Client).solve.func2
        	            		/src/client/solve.go:290
        	            	golang.org/x/sync/errgroup.(*Group).Go.func1
        	            		/src/vendor/golang.org/x/sync/errgroup/errgroup.go:75
        	            	runtime.goexit
        	            		/usr/local/go/src/runtime/asm_amd64.s:1650
        	Test:       	TestIntegration/TestCommandSourceMapping/worker=dockerd/frontend=builtin

vvoland · 2024-05-13T16:25:49Z

Rebased with the latest master.

Looks like moby/buildkit@228e250 did some major refactoring of the detect which are breaking changes on our side:

builder/builder-next/controller.go:70:25: undefined: detect.Exporter
cmd/dockerd/daemon.go:248:20: undefined: detect.TracerProvider
cmd/dockerd/daemon.go:371:9: undefined: detect.Shutdown

@jsternberg PTAL

- full diff: moby/buildkit@v0.13.1...v0.14.0-rc2 Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>

eea0b41bf4fb1d69e109ff5ff8045c63f0c0d510 added a new argument to `instructions.Parse` to support issuing linter warnings. Classic builder uses it to parse the Dockerfile instructions and its usage needs adjustment. The classic builder is deprecated and we won't be adding any new features to it, so we just pass a nil linter callback. Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>

1b1c5bc08ad81add007eb647e66ed0929693f3a0 extended the function signature with one additional return value. Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>

Signed-off-by: Jonathan A. Sternberg <jonathan.sternberg@docker.com> Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>

b5c50afa882e2b34aba880fd5028615e2ef94e07 changed the signature of NewGatewayFrontend to include a slice of allowed repositories. Docker does not allow to specify this option, so don't place any restrictions by passing an empty slice. Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>

thaJeztah

Should this one be ready for review? Looks like the upstream fix was merged

thaJeztah · 2024-06-06T13:15:48Z

builder/dockerfile/builder.go

@@ -187,7 +187,7 @@ func buildLabelOptions(labels map[string]string, stages []instructions.Stage) {
 func (b *Builder) build(ctx context.Context, source builder.Source, dockerfile *parser.Result) (*builder.Result, error) {
 	defer b.imageSources.Unmount()

-	stages, metaArgs, err := instructions.Parse(dockerfile.AST)
+	stages, metaArgs, err := instructions.Parse(dockerfile.AST, nil)


Would the "correct" way be to pass a linter with SkipAll? linter.New(linter.Config{SkipAll: true}), basically a "dummy" linter?

I think both are "correct" from BK perspective - I like the nil though as it saves an unnecessary allocation.

Let me know if you prefer the linter.New though 👍🏻

It's not a blocker for sure!

I'm fine either way; I'm ok with the nil - and this is still something we can change later. I was mostly pondering "what's correct here"

Also (honestly) I was hoping that linter would be an interface here, but it looks to be a type. If it was an interface, then (possibly?) it would make it easier to swap implementations, and we could even have a linter for the classic builder to assist with deprecations, as well as a better way to indicate "this feature is not supported by the classic builder").

cc @daghack @jsternberg

thaJeztah

LGTM

most local changes look trivial afaics. would love to have at least 1 person from the build team to give this a review though!

laurazard

LGTM

thaJeztah · 2024-06-06T14:59:10Z

Thanks @crazy-max !

I'll bring this one in 👍

vvoland added area/builder area/builder/buildkit Issues affecting buildkit area/dependencies labels Apr 5, 2024

vvoland added this to the v-future milestone Apr 5, 2024

vvoland self-assigned this Apr 5, 2024

vvoland force-pushed the buildkit-update branch 3 times, most recently from 6467cc5 to b51705c Compare April 25, 2024 09:47

crazy-max mentioned this pull request Apr 25, 2024

frontend: missing compat check for TestCommandSourceMapping moby/buildkit#4880

Merged

vvoland force-pushed the buildkit-update branch 2 times, most recently from d935fec to c3a6c6e Compare May 13, 2024 16:22

neersighted modified the milestones: v-future, 27.0.0 May 24, 2024

vvoland force-pushed the buildkit-update branch from 2297899 to 9673fbc Compare June 3, 2024 08:39

vvoland changed the title ~~vendor: github.com/moby/buildkit master (v0.14?)~~ vendor: github.com/moby/buildkit v0.14.0-rc1 Jun 3, 2024

This comment was marked as resolved.

Sign in to view

vvoland and others added 5 commits June 6, 2024 11:20

vendor: update github.com/moby/buildkit to v0.14.0-rc2-dev

3bcbb56

- full diff: moby/buildkit@v0.13.1...v0.14.0-rc2 Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>

builder: Adjust usage of shlex.ProcessWord

9956042

1b1c5bc08ad81add007eb647e66ed0929693f3a0 extended the function signature with one additional return value. Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>

builder: Update detect usage for new detect API from buildkit

fa03db1

Signed-off-by: Jonathan A. Sternberg <jonathan.sternberg@docker.com> Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>

vvoland force-pushed the buildkit-update branch from 51ac3ff to 0b5f7b9 Compare June 6, 2024 09:20

vvoland changed the title ~~vendor: github.com/moby/buildkit v0.14.0-rc1~~ vendor: github.com/moby/buildkit v0.14.0-rc2-dev Jun 6, 2024

laurazard mentioned this pull request Jun 6, 2024

otel: remove deprecated usages of otelgrpc #47919

Draft

This was referenced Jun 6, 2024

dockerfile/parse: Fix nil dereference of a linter moby/buildkit#4984

Closed

dockerfile/linter: check for nil linter in linter functions moby/buildkit#4996

Merged

thaJeztah reviewed Jun 6, 2024

View reviewed changes

vvoland marked this pull request as ready for review June 6, 2024 13:26

vvoland requested a review from tonistiigi as a code owner June 6, 2024 13:26

vvoland added the status/2-code-review label Jun 6, 2024

thaJeztah approved these changes Jun 6, 2024

View reviewed changes

laurazard approved these changes Jun 6, 2024

View reviewed changes

crazy-max approved these changes Jun 6, 2024

View reviewed changes

thaJeztah merged commit 6f32dc1 into moby:master Jun 6, 2024
135 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

vendor: github.com/moby/buildkit v0.14.0-rc2-dev #47683

vendor: github.com/moby/buildkit v0.14.0-rc2-dev #47683

vvoland commented Apr 5, 2024 •

edited by thaJeztah

crazy-max commented Apr 25, 2024

vvoland commented May 13, 2024

This comment was marked as resolved.

thaJeztah left a comment

thaJeztah Jun 6, 2024

vvoland Jun 6, 2024

vvoland Jun 6, 2024

thaJeztah Jun 6, 2024

thaJeztah left a comment

laurazard left a comment

thaJeztah commented Jun 6, 2024

vendor: github.com/moby/buildkit v0.14.0-rc2-dev #47683

vendor: github.com/moby/buildkit v0.14.0-rc2-dev #47683

Conversation

vvoland commented Apr 5, 2024 • edited by thaJeztah

Moby integration adjustments

builder: Pass nil linter to instructions.Parse

builder: Adjust usage of shlex.ProcessWord

builder: Update detect usage for new detect API from buildkit

builder-next: Adjust NewGatewayFrontend invocation

crazy-max commented Apr 25, 2024

vvoland commented May 13, 2024

This comment was marked as resolved.

thaJeztah left a comment

Choose a reason for hiding this comment

thaJeztah Jun 6, 2024

Choose a reason for hiding this comment

vvoland Jun 6, 2024

Choose a reason for hiding this comment

vvoland Jun 6, 2024

Choose a reason for hiding this comment

thaJeztah Jun 6, 2024

Choose a reason for hiding this comment

thaJeztah left a comment

Choose a reason for hiding this comment

laurazard left a comment

Choose a reason for hiding this comment

thaJeztah commented Jun 6, 2024

vvoland commented Apr 5, 2024 •

edited by thaJeztah