Skip to content

dap: support evaluate request to invoke a container #3257

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

Merged
merged 1 commit into from
Jul 15, 2025

Conversation

jsternberg
Copy link
Collaborator

Supports using the evaluate request in REPL mode to start a container
with the exec command. Presently doesn't support any arguments.

This improves the dap server so it is capable of sending reverse
requests and receiving the response. It also adds a hidden command
dap attach that attaches to the socket created by evaluate.

This requires the client to support runInTerminal.

Likely needs some additional work to make sure resources are cleaned up
cleanly especially when the build is unpaused or terminated, but it
should work as a decent base.

commands/dap.go Outdated
cobrautil.MarkCommandExperimental(cmd)

flags := cmd.Flags()
flags.StringVar(&options.OnFlag, "on", "error", "When to pause the adapter ([always, error])")
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this ExceptionBreakMode sent via DAP protocol itself?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not currently implemented but yes ideally we would remove this and use ExceptionBreakMode to set the behavior.

commands/dap.go Outdated
return nil
}

func (d *adapterProtocolDebugger) Out() console.File {
Copy link
Member

Choose a reason for hiding this comment

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

Why is this returning console.File? I assume detecting the progress mode can be done outside without leaking this into dap layer.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think this is cited somewhere else in the PR but it's mostly because the NewPrinter takes a console.File even though it shouldn't. progressui.NewDisplay takes an io.Writer and creates a console.File from that so NewPrinter is just going console.File -> io.Writer -> console.File.

We can change NewPrinter to take an io.Writer and then this can change to an io.Writer.

build/build.go Outdated
type (
EvaluateFunc func(ctx context.Context, name string, c gateway.Client, res *gateway.Result) error
Handler struct {
Evaluate EvaluateFunc
Copy link
Member

Choose a reason for hiding this comment

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

EvaluateFunc seems unused.

@@ -1210,3 +1126,24 @@ func RunBuild(ctx context.Context, dockerCli command.Cli, in *BuildOptions, inSt
}
return resp[defaultTargetName], inputs, nil
}

// debuggerOptions will start a debuggerOptions instance.
Copy link
Member

Choose a reason for hiding this comment

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

Looks like this should be in commands/debug.

dap/adapter.go Outdated
name string

paused chan struct{}
rCtx *build.ResultHandle
Copy link
Member

Choose a reason for hiding this comment

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

follow-up: this ResultHandle seems incorrectly named. I'm not sure if we need it at all but if we do then it should be called something else and should not be defined in the build pkg, as it is basically the transformation of the Evaluate() parameters into struct form and therefore duplicate in the context of build pkg.

Ctx variable naming also makes it confusing as it looks like some form of context.Context.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Agreed. It's mostly been kept around because the code exists and it was useful to reuse it. It can be renamed and probably should be renamed.

commands/dap.go Outdated

cobrautil.MarkFlagsExperimental(flags, "on")

cmd.AddCommand(buildCmd(dockerCli, rootOpts, &options))
Copy link
Member

Choose a reason for hiding this comment

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

What's the use case of the build subcommand?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The command is invoked with docker buildx dap build and the bake version will be docker buildx dap bake.

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.

As discussed offline, we probably want a different variant of this where the exec is not per container, but there is one terminal exec that is associated with the dap session. When the status of dap session changes between running and paused, the terminal should internally switch to new container without the exec process itself shutting down. Later we can add extra capabilities that would keep the state from the previous container by carrying over things like bash history and cwd.

I think we can still merge this and test both flows during the experimental period. Eg. with a config switch in the vscode extension side.

@tonistiigi tonistiigi requested a review from ktock July 15, 2025 06:29
@tonistiigi tonistiigi added this to the v0.26.0 milestone Jul 15, 2025
Supports using the `evaluate` request in REPL mode to start a container
with the `exec` command. Presently doesn't support any arguments.

This improves the dap server so it is capable of sending reverse
requests and receiving the response. It also adds a hidden command
`dap attach` that attaches to the socket created by `evaluate`.

This requires the client to support `runInTerminal`.

Likely needs some additional work to make sure resources are cleaned up
cleanly especially when the build is unpaused or terminated, but it
should work as a decent base.

Signed-off-by: Jonathan A. Sternberg <jonathan.sternberg@docker.com>
@tonistiigi tonistiigi merged commit 32a9b90 into docker:master Jul 15, 2025
138 checks passed
@jsternberg jsternberg deleted the dap-invoke branch July 15, 2025 16:50
@ktock
Copy link
Collaborator

ktock commented Jul 16, 2025

Sorry for the slow review. This patch looks good to me, too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants