⚽ Live Match: docker/buildx/pull/

cpuguy83 · 2023-11-14T00:01:56Z

This allows the buildx CLI to act a proxy to the configured instance. It allows external code to use buildx itself as a driver for connecting to buildkitd instances.

Instance and node selection should follow the same semantics as as buildx build, including taking into account the BUILDX_BUILDER env var and the --builder global flag.

jedevc · 2023-11-14T11:43:29Z

+		}
+		defer conn.Close()
+
+		go func() {


Could we extract the logic for dial-stdio in buildkit into a reusable function so we could just call that here after getting the conn? Ideally, we wouldn't end up with diverging implementations in buildctl/buildx (e.g. that would make it a pain to fix bugs in the future, etc).

We're also having the same thing in dagger, where we need dial-stdio for the connhelper, but don't really want to ship all of buildctl, so it would be cool to have just one implementation to call instead of needing to extract the relevant code.

One implementation of what exactly?
the main part of this is just wiring up copy operations.
I'm not sure its worth sharing an implementation of that?

Ah, I think I'm just confused by the differences in handling CloseRead/CloseWrite here versus in buildctl. Given that they seem to be doing the same thing, I'm not quite sure I understand why the code is so different.

@AkihiroSuda I'm curious on this case where its wrapping with a nop-closer.https://github.com/moby/buildkit/blame/5ae9b23c40a926615b6c3cc1da3f5457d2f61fd4/cmd/buildctl/dialstdio.go#L35

I don't think we necessarily need the stream to handle a half-close here, just that it s a little cleaner in terms of the connection semantics (hence why its implemented here).

Other than that I think the implementation is pretty much the same, just this is taking advantage of errgroup and buildctl is basically doing its own hand-rolled equivalent.

From my memory, tty stuff didn't work correctly without handling half-close there

I was trying to test for using tty as stdin/stdout but this broke http2.
Not sure how to check for this.

Fundamentally though, the only real change as compared to buildctl would be to not fallback to Close on when CloseRead is not implemented.

tonistiigi · 2024-01-09T01:45:38Z

+
+
+<!---MARKER_GEN_END-->
+


Let's add a bit more description/usage here. It isn't entirely obvious for new user what "dial stdio" means.

Added extra docs.

Not sure what changed for this

🤔
I wrote stuff... apparently I did not commit it somehow.

Pushed up again.

Oh I know what happened... I did docs the wrong way.

tonistiigi · 2024-02-08T21:31:57Z

+
+## Description
+
+dial-stdio uses the stdin and stoud streams of the command to proxy to the configured builder instance.


tonistiigi · 2024-02-08T21:33:12Z

+# docker buildx dial-stdio
+
+<!---MARKER_GEN_START-->
+Dial stdio


"Proxy current stdio streams to builder instance"

tonistiigi · 2024-02-08T21:34:02Z

+## Description
+
+dial-stdio uses the stdin and stoud streams of the command to proxy to the configured builder instance.
+It is not intended to be used by humans, but rather by other tools that want to interact with the builder instance API.


"builder instance API." -> "builder instance via BuildKit API"

tonistiigi · 2024-02-08T21:36:19Z

+}()
+
+client.New(ctx, "", client.WithContextDialer(func(ctx context.Context, s string) (net.Conn, error) {
+    return c2, nil


Why use static instance here? I think it should invoke new command when dialer gets called.

func dial(...) (net.Conn, error) { _ = exec.Command() ... } client.New(ctx, "", client.WithContextDialer(dial))

tonistiigi · 2024-02-08T21:49:13Z

+    cmd.Stdin = c1
+    cmd.Stdout = c1
+
+    if err := cmd.Start() {


err := cmd.Start(); err != nil

tonistiigi · 2024-02-08T21:49:55Z

+
+```go
+
+client.New(ctx, "", client.WithContextDialer(func(ctx context.Context, s string) (net.Conn, error) {


nit: remove the parameters variable names if they are unused

This allows the buildx CLI to act a proxy to the configured instance. It allows external code to use buildx itself as a driver for connecting to buildkitd instances. Instance and node selection should follow the same semantics as as `buildx build`, including taking into account the `BUILDX_BUILDER` env var and the `--builder` global flag. Signed-off-by: Brian Goff <cpuguy83@gmail.com>

cpuguy83 force-pushed the dialstdio branch from 435fa9d to 0d4ec7c Compare November 14, 2023 00:05

tonistiigi reviewed Nov 14, 2023

View reviewed changes

Comment thread build/dial.go Resolved

Comment thread driver/docker/driver.go Resolved

Comment thread commands/dial_stdio.go Resolved

jedevc reviewed Nov 14, 2023

View reviewed changes

cpuguy83 force-pushed the dialstdio branch 7 times, most recently from 78aac86 to ea99e40 Compare November 16, 2023 21:27

cpuguy83 marked this pull request as ready for review November 16, 2023 21:27

cpuguy83 force-pushed the dialstdio branch from ea99e40 to 1891aec Compare November 17, 2023 17:23

crazy-max added this to the v0.13.0 milestone Dec 23, 2023

tonistiigi reviewed Jan 9, 2024

View reviewed changes

cpuguy83 force-pushed the dialstdio branch 3 times, most recently from dd3ca7c to 94f5985 Compare January 27, 2024 21:55

cpuguy83 force-pushed the dialstdio branch 4 times, most recently from 9b657e9 to 0c5390f Compare February 8, 2024 20:06

tonistiigi reviewed Feb 8, 2024

View reviewed changes

cpuguy83 force-pushed the dialstdio branch from 0c5390f to e2071a5 Compare February 8, 2024 21:42

tonistiigi reviewed Feb 8, 2024

View reviewed changes

cpuguy83 force-pushed the dialstdio branch from e2071a5 to 760244e Compare February 8, 2024 22:16

tonistiigi approved these changes Feb 10, 2024

View reviewed changes

tonistiigi merged commit 481384b into docker:master Feb 10, 2024

cpuguy83 deleted the dialstdio branch February 10, 2024 01:32

cpuguy83 mentioned this pull request Mar 25, 2024

[REQ] Update buildx integration to use new docker buildx dial-stdio project-copacetic/copacetic#543

Closed

1 task done

crazy-max mentioned this pull request Jun 3, 2024

dial-stdio: remove extra cmd.flags() #2496

Merged


		## Description

		dial-stdio uses the stdin and stoud streams of the command to proxy to the configured builder instance.


		```go

		client.New(ctx, "", client.WithContextDialer(func(ctx context.Context, s string) (net.Conn, error) {



		<!---MARKER_GEN_END-->

Conversation

cpuguy83 commented Nov 14, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jedevc Nov 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cpuguy83 Nov 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jedevc Nov 14, 2023 •

edited

Loading

cpuguy83 Nov 14, 2023 •

edited

Loading