Pass the exit code through the exited event back to the client and
ensure that the printed text is printed completely.
Previously, the exited event just had a big todo and the printer would
sometimes fail to send messages to the connected client. This moves the
printer wait to before the debug adapter is closed to ensure that all
messages get sent through the connection to the editor. While there, I
also plumbed in the exit code to exited. It's not necessarily the real
exit code but it will produce a zero on build success and a non-zero
code on build failure so that should be good enough.
Signed-off-by: Jonathan A. Sternberg <jonathan.sternberg@docker.com>
When the debug thread was updated to always solve inputs from the
operation that it was tied to it became a bit overeager to evaluate
them. The intention of the steps is to have a single direct parent and
then potentially multiple "function calls" that can be evaluated with
step into and step out to leave.
With the change, that logic stayed in, but the inputs were always being
evaluated before they were stepped into or over. Now, when we construct
the steps, we also attach a list of inputs that we should defer
evaluation on to ensure we don't execute inputs that haven't been
executed yet.
It will then wrap the reference with a version that causes `Evaluate` to
do nothing. This prevents the overeager evaluation but allows the
reference to be evaluated if we need to read the filesystem.
Signed-off-by: Jonathan A. Sternberg <jonathan.sternberg@docker.com>
This refines the check for determining whether exec will succeed to work
when an error occurs. This check previously relied on the `Ref` being
populated in the result context but this would only happen if we were
paused from a breakpoint or by stepping. An error would not fill in this
field.
The check is now refined to use the new gateway filesystem exec API so
we can create the container and then check even if we don't have a
returned gateway reference. The logic to determine which mount to check
has also been moved.
Signed-off-by: Jonathan A. Sternberg <jonathan.sternberg@docker.com>
We erroneously skipped a breakpoint when that breakpoint was the same as
the entrypoint and we did not use stop on entry. This is because we only
started evaluating breakpoints after the first step on the entrypoint
instead of at the entrypoint.
Signed-off-by: Jonathan A. Sternberg <jonathan.sternberg@docker.com>
Skip the load build context step when it doesn't have an associated
source line. This caused an extra branch to be created in an otherwise
pretty straightforward dockerfile where stepping in on a copy
instruction that used the context would stay on the same line because it
"stepped into" the context loading rather than being treated the same as
step next.
This resulted in some bad and confusing ergonomics with the cursor
position that were a bit confusing and unexpected.
There might be more areas to try and prune but the most common one, a
single branch instruction that doesn't have a location, now gets skipped
which is the exact thing that was generated for loading the context.
Signed-off-by: Jonathan A. Sternberg <jonathan.sternberg@docker.com>
The context used for serving the dap server was being canceled too early
because it used defer which would initiate at the end of the function
while every other cleanup function used `t.Cleanup` which executes in
its own goroutine.
One possible solution was to move the cancel to the cleanup, but the
context being passed to serve and start doesn't make sense because if it
ever does get canceled, it'll likely cause a similar race condition with
`Stop`.
This removes the context from the methods that were causing this issue
in favor of just relying on the caller calling `Stop` when they are done
with the adapter and server.
This seems to have only affected tests and I don't believe it affected
the actual dap command.
Signed-off-by: Jonathan A. Sternberg <jonathan.sternberg@docker.com>
Properly map the source paths from the metadata in the solve to the
client side paths. The source path returns is relative to the context
that gets uploaded which is usually a subdirectory. The original code
noticed this when mapping the paths but made the invalid assumption that
the dockerfile would always be in the context path so it combined the
dockerfile name with the context path.
It is possible for the dockerfile to be in a subdirectory of the
context. In which case, we computed the paths incorrectly.
This modifies DAP to instead use the `DockerfileMappingDst` and
`DockerfileMappingSrc` which are special included variables to the
inputs that get filled in during the build for the purpose of mapping
the source path to the client side path.
Tests have also been added for this functionality to ensure it doesn't
break again. This should work with both absolute and relative paths
although absolute paths should probably be preferred for usage just
because they're less likely to result in weird things happening.
The sources are also normalized to always convert the source filenames
to absolute paths and DAP itself will accept relative paths but will
only ever communicate in absolute paths. When you set a breakpoint, it
will convert it to an absolute path and reference it in that way rather
than a relative path.
Signed-off-by: Jonathan A. Sternberg <jonathan.sternberg@docker.com>
Case insensitive filesystems can cause breakpoints to not be seen or
verified. This is particularly true on Windows where the drive letter
can also participate in the filepath.
Change the detection logic for a breakpoint to be case insensitive. At
the same time, report the name of the source as part of the breakpoint
so that the editor can be told which casing we're expecting to be used.
Signed-off-by: Jonathan A. Sternberg <jonathan.sternberg@docker.com>
The container filesystem request API that has been added to buildkit
allows a container created through the `NewContainer` API to also access
the filesystems. This is useful when an error occurs because it allows
us to grab the mutable state of the mounts used during the actual build
rather than the input version copies which don't contain any files that
were added as part of the failed command.
This gives us a more accurate view of the filesystem that was previously
only accessible through using `exec` and `ls`/`cat` commands that may
not always exist.
Signed-off-by: Jonathan A. Sternberg <jonathan.sternberg@docker.com>
Improves the naming for file explorer names when the input relates
directly to a source. This is most common when the input is the context
(which is just usually a simple source like `local://context`).
This should help in most circumstances in determining which input is
which.
Signed-off-by: Jonathan A. Sternberg <jonathan.sternberg@docker.com>
When a breakpoint fails to be verified, it will switch the reason to
"failed". It starts off the reason as "pending".
The `reason` field for a breakpoint was added some time after the last
release of `go-dap` which has only been updated once in the last year so
this uses the `main` branch version which contains the field.
Signed-off-by: Jonathan A. Sternberg <jonathan.sternberg@docker.com>
The mounts for exec operations was misconstruing most input names as the
root mount because it was using the wrong input index to match with the
exec mounts.
The correct input index is now being used so bind mounts and other types
of mounts should now show the correct mount destination rather than only
showing the root mount.
Signed-off-by: Jonathan A. Sternberg <jonathan.sternberg@docker.com>
This adds integration tests for the `dap build` command to test various
behavior associated with the command. We start the build and the
integration test acts as a dap client to send requests and check that
the output is what we expect.
Signed-off-by: Jonathan A. Sternberg <jonathan.sternberg@docker.com>
The old logic wasn't quite right, and a little hard to follow.
Before this, because breakpoints sent from setBreakpoints don't have an
end line or column they are just a single point the comparison would
fail the check due to the column check.
E.g. if I have a breakpoint on line 30.
The content lines range from 29-31.
Now lets say the end column is at postion 10, but my breakpoint is at
L30 column 11 (still within range).
The overlap check would fail because 11 > 10 but these shouldn't be
compared at all in this case.
Signed-off-by: Brian Goff <cpuguy83@gmail.com>
The solve error that gets returned sometimes has a platform when the LLB
returned by `ToState` doesn't. In order to ensure we find the failed
digest, we now search for both the digest of what was returned and also
clear out the platform and search for the digest without that in case
that one matches instead.
Signed-off-by: Jonathan A. Sternberg <jonathan.sternberg@docker.com>
If a breakpoint occurs before the step pointed to by next or out, dap
will now stop there instead of the desired location.
This also updates the loop to always set the breakpoints rather than
only when continue is chosen.
Signed-off-by: Jonathan A. Sternberg <jonathan.sternberg@docker.com>
This change modifies how steps are evaluated in the program. Previously,
the execution relied on looking for the digest we were going to stop at
and evaluating the step right before that digest. This could result in
some gaps where it erroneously didn't execute inputs for the current
step as it skipped those digests.
Now, the evaluation reads the inputs and executed those directly rather
than relying on the evaluation of previous steps in the sequence. This
should make the evaluation of inputs more accurate and allow us to have
better breakpoints on things like the copy operation.
Due to the change, it's also easier for us to include the different
inputs in the file explorer.
Signed-off-by: Jonathan A. Sternberg <jonathan.sternberg@docker.com>
The setBreakpoints response body is defined to be an array of
breakpoints but the debug adapter incorrectly serialized an empty array
as null in the JSON message. Explicitly initializing the array will
force the JSON serialization process to send an empty array back instead
of null.
Signed-off-by: Remy Suen <remy.suen@docker.com>
The run in terminal request sent by exec would not work if `buildx` had
been invoked directly instead of through `docker`. If we don't find the
reexec environment variable, we use `os.Args[0]` to launch buildx
directly when invoking attach.
Signed-off-by: Jonathan A. Sternberg <jonathan.sternberg@docker.com>
It accidentally retrieved the relative path to the mount directory
instead of the full path which resulted in always returning the root
directory.
Signed-off-by: Jonathan A. Sternberg <jonathan.sternberg@docker.com>
The debug adapter will now look at the error and collect the input
mounts from the exec operation and translate them to a gateway reference
to be able to access the filesystem in the file explorer.
When we are attempting to find the real operation that failed and
succeed, we now re-solve the dependencies (which should already have
been solved correctly to begin with) so that we can have the proper
reference and can access the filesystem contents.
Signed-off-by: Jonathan A. Sternberg <jonathan.sternberg@docker.com>
This improves location resolution for duplicate digests by making some
assumptions about the file structure to determine a "best match" when
there are multiple possible locations.
It uses the next operation (that this digest is the input for) to
determine a location. If the location of the current operation is before
the next operation, this location is preferred. If there are multiple
locations that happen before the next operation, the one closest to the
next operation (aka later in the file) is used instead.
This resolves the most common case of multiple identical `FROM`
statements without adding more code to the frontends themselves.
Signed-off-by: Jonathan A. Sternberg <jonathan.sternberg@docker.com>
The dap test wasn't waiting for the client's goroutines to complete
before exiting which caused a race condition that could cause it to log
to the dead test logger. This became apparent when `--count` of greater
than one was used since it caused the test to run long enough to trigger
the behavior. It would have also triggered if we had added more tests.
Add the client close to the cleanup so it waits for the goroutine to
finish before the test exits as it was properly supposed to do.
Signed-off-by: Jonathan A. Sternberg <jonathan.sternberg@docker.com>
Improves the determination of the proper parent for exec and file ops.
With file ops, it will only consider inputs and ignore secondary inputs.
This prevents the following case:
```
FROM busybox AS build1
RUN echo foo > /hello
FROM scratch
COPY --from=build1 /hello .
```
Previously, `build1` would be considered the parent of the copy
instruction. Now, copy properly does not have a parent.
If there are multiple file ops and the operations disagree on the
canonical "parent", we give up on trying to find a canonical parent and
assume there is none.
For exec operations, whichever input is associated with the root mount
is considered the primary parent.
For all other operations, the first parent is considered the primary
parent if it exists.
Signed-off-by: Jonathan A. Sternberg <jonathan.sternberg@docker.com>
When the builder loads a dockerfile, it does it by using the base name
of the dockerfile path and only loads the innermost directory. This
means that the source name we're looking for is the base name and not
the full relative path.
Update the set breakpoints functionality so it takes this into account.
Fixes scenarios where DAP is used with a dockerfile nested in the
context.
Signed-off-by: Jonathan A. Sternberg <jonathan.sternberg@docker.com>
Invoking the shell will cause it to persist across the entire build and
to re-execute whenever the builder pauses at another location again.
This still requires using `exec` to launch the shell. Launching by frame
id is also removed since it no longer applies to this version.
Signed-off-by: Jonathan A. Sternberg <jonathan.sternberg@docker.com>
Add a file explorer to the debugger that allows exploring the filesystem
of the current container. It will show directory contents, file
contents, and symlink destinations. It will also show the file mode
associated with a file.
The file explorer defaults to marking itself as an expensive operation
so the debugger doesn't automatically retrieve the information.
Signed-off-by: Jonathan A. Sternberg <jonathan.sternberg@docker.com>
Change how breakpoints and stepping works. These now work more how you
would expect another programming language to work. Breakpoints happen
before the step has been invoked rather than after which means you can
inspect the state before the command runs.
This has the advantage of being more intuitive for someone familiar with
other debuggers. The negative is that you can't run to after a certain
step as easily as you could before. Instead, you would run to that stage
and then use next to go to the step directly afterwards.
Step in and out also now have different behaviors. When a step has
multiple inputs, the inputs of non-zero index are considered like
"function calls". The most common cause of this is to use `COPY --from`
or a bind mount. Stepping into these will cause it to jump to the
beginning of the call chain for that branch. Using step out will exit
back to the location where step in was used.
This change also makes it so some steps may be invoked multiple times in
the callgraph if multiple steps depend on them. The reused steps will
still be cached, but you may end up stepping through more lines than the
previous implementation.
Stack traces now represent where these step in and step out areas
happen rather than the previous steps. This can help you know from where
a certain step is being used.
Signed-off-by: Jonathan A. Sternberg <jonathan.sternberg@docker.com>
In the repl context, we will now return the error instead of directly
printing it. We also suppress reporting errors from cobra. The logic
flow has also been changed to prevent returning errors from cobra unless
there was something related to the command line invocation so usage will
only be printed when a command was typed wrong and it will not show up
for every error.
Signed-off-by: Jonathan A. Sternberg <jonathan.sternberg@docker.com>
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>
Jonathan A. Sternberg