-
Notifications
You must be signed in to change notification settings - Fork 562
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
Conversation
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.
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?
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
EvaluateFunc
seems unused.
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.
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 |
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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)) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
.
e75bc33
to
5d72ca2
Compare
There was a problem hiding this 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.
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
evaluate
request in REPL mode to start a containerwith 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 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.