Redirect if internal console #3108
Conversation
gfszr
force-pushed
the
redirect-if-internal-console
branch
2 times, most recently
from
5991d03
to
8679c98
Compare
| @@ -326,7 +326,7 @@ func openRedirects(redirects [3]string, foreground bool) (stdin, stdout, stderr | |||
| return dflt | |||
| } | |||
| var f *os.File | |||
| f, err = os.Create(path) | |||
| f, err = os.OpenFile(path, os.O_WRONLY|os.O_CREATE, 0o666) | |||
There was a problem hiding this comment.
This is a backwards incompatible change because you are not passsing O_TRUNC.
There was a problem hiding this comment.
Will add it - I still need to check whether O_TRUNC has any bad effect when opening a fifo.
| // This is a var for testing | ||
| maxGoroutines = 1 << 10 | ||
| ) | ||
| // Max number of goroutines that we will return. |
There was a problem hiding this comment.
Avoid unrelated reformats, they pollute blames and make merge conflicts unnecessarily harder.
There was a problem hiding this comment.
My editor auto-formats using gofumpt, so it formatted some extra editted files. Will revert such changes.
| @@ -1049,6 +1071,32 @@ func (s *Session) onLaunchRequest(request *dap.LaunchRequest) { | |||
| return | |||
| } | |||
|
|
|||
| func() { | |||
There was a problem hiding this comment.
Followed a similar pattern I've seen later on of wrapping some logic in an inline function to ease breaking, not anything special - I can export it to a different method.
There was a problem hiding this comment.
If you refer to this file, that pattern is to execute a defer, not to group anything.
| ) | ||
|
|
||
| func generateStdioTempPipes() (res [2]string, err error) { | ||
| err = errors.New("Unimplemented") |
There was a problem hiding this comment.
Merging this with just a linux implementation would be weird. This is enough of a prominent feature that having it implemented for one operating system implies that we have to implement it for at least the more important ones. I think it needs to support Windows and macOS.
There was a problem hiding this comment.
Implementing it to all platforms should be relatively easier if we can change the Redirects to not only be paths to stdout/stderr, but io.Writers. I'll implement complete support for native, which should be relatively easy to do soin that backend.
| @@ -1049,6 +1071,32 @@ func (s *Session) onLaunchRequest(request *dap.LaunchRequest) { | |||
| return | |||
| } | |||
|
|
|||
| func() { | |||
| if args.Console == "internalConsole" && runtime.GOOS == "linux" && (args.Backend == "default" || args.Backend == "native") { | |||
There was a problem hiding this comment.
It's weird that using internalConsole on an unsupported configuration fails silently. It's also weird that all values of console that aren't internalConsole are accepted as valid.
There was a problem hiding this comment.
The current behavior is that console is not checked, so it can also be completely invalid and dlv dap would accept it, so I'm not sure we necessarily have to validate the console argument.
There was a problem hiding this comment.
Suppose someone misspells internlaConsole, how do they notice?
There was a problem hiding this comment.
Usually, such syntax can be checked in VSCode itself - it can present an error such as: "internalConsole" is not a valid "console" value".
There was a problem hiding this comment.
Not everyone uses vscode with dap. I thought that was the whole point of this.
There was a problem hiding this comment.
Hm, true. I am not familiar with how every DAP client handles this, but adding an allowed list of declared DAP consoles (I think it is standardized) should be easy enough.
There was a problem hiding this comment.
it should just be the consoles that we support.
|
Thank you @aarzilli for the quick reply! 🙂 |
gfszr
force-pushed
the
redirect-if-internal-console
branch
from
8679c98
to
750a0b4
Compare
|
Is this still being worked on @gfszr ? |
|
Yes - currently having some difficulties on developing and testing on Windows and macOS, as requested in review. |
|
Closing due to age. |
This PR is currently as draft to better improve implementation and pass tests.
This PR partially implements #3094, by implementing redirection of
stdoutandstderrto the debug console via DAP'sOutputEvent's on Linux.It partially does so as current debug backend API support receiving a path to a redirected file only, and not a
io.Writeras an example. It also supports only thenativebackend and not other backends.Achieving full support would require adding a generalized API so that we can pass an
io.Writerto the native backend nicely, but this can be done later on and be discussed more, while Linux support can be easily implemented.