dap: support evaluate request to invoke a container #3257
Conversation
jsternberg
force-pushed
the
dap-invoke
branch
2 times, most recently
from
07d9e5a to
bac335d
Compare
commands/dap.go
Outdated
| cobrautil.MarkCommandExperimental(cmd) | ||
|
|
||
| flags := cmd.Flags() | ||
| flags.StringVar(&options.OnFlag, "on", "error", "When to pause the adapter ([always, error])") |
There was a problem hiding this comment.
Isn't this ExceptionBreakMode sent via DAP protocol itself?
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
Why is this returning console.File? I assume detecting the progress mode can be done outside without leaking this into dap layer.
There was a problem hiding this comment.
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 |
commands/build.go
Outdated
| @@ -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. | |||
There was a problem hiding this comment.
Looks like this should be in commands/debug.
dap/adapter.go
Outdated
| name string | ||
|
|
||
| paused chan struct{} | ||
| rCtx *build.ResultHandle |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)) |
There was a problem hiding this comment.
What's the use case of the build subcommand?
There was a problem hiding this comment.
The command is invoked with docker buildx dap build and the bake version will be docker buildx dap bake.
jsternberg
force-pushed
the
dap-invoke
branch
4 times, most recently
from
e75bc33 to
5d72ca2
Compare
There was a problem hiding this comment.
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.
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>
|
Sorry for the slow review. This patch looks good to me, too. |
Supports using the
evaluaterequest in REPL mode to start a containerwith the
execcommand. 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 attachthat attaches to the socket created byevaluate.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.