🐞 terminal fails on Dagger windows distribution

[pjmagee]

What is the issue?

It's been a long while since I have used the terminal feature, but I know it did work for sure around the v0.10. There's a chance a bug was introduced a while back and I can't give a good timeline of when something may have changed, sorry.

The terminal feature is not working on the windows binary and I think it's easy to replicate with just a simple container from and terminal chained command.

I noticed something odd, after trying to debug some adhoc stuff I was learning with C++ build environments in docker, so i thought i'd use Dagger for this. I came across this error and thought maybe it's the docker image I was using, tried a few things, got it working in WSL but now I realised i think the problem is more general and not specific to an image, example below is the basic ubuntu image and that doesn't seem to work.

Dagger version

dagger v0.14.0 (registry.dagger.io/engine:v0.14.0) windows/amd64

Steps to reproduce

dagger core container from --address ubuntu terminal

Swap the image with anything, alpine etc and I'm confident the same result with occur, you are not able to get into a terminal from the dagger windows client binary

Log output

dagger core container from --address ubuntu terminal                            PS  12:30:12 
✔ connect 0.2s
✔ inspecting core types 0.2s
✔ parsing command line arguments 0.0s
✔ container: Container! 0.0s
✔ Container.from(address: "ubuntu"): Container! 0.4s
✘ Container.terminal: Container! 0.7s
! failed to forward exit code: failed to close terminal: EOF
┃ dagger / $
  ✔ start sh 0.7s

Full trace at https://dagger.cloud/mageep/traces/4756d309d0c41f6979d25cff3a6f02eb

Error: response from query: input: container.from.terminal resolve: failed to forward exit code: failed to close terminal: EOF               
Run 'dagger core container from terminal --help' for usage.

Trying on my WSL, with the exact same command, it worked.

mageep@PATRICK-PC:~$ dagger version
dagger v0.14.0 (registry.dagger.io/engine:v0.14.0) linux/amd64
mageep@PATRICK-PC:~$ dagger core container from --address ubuntu terminal
● Attaching terminal:
    container: Container!
    Container.from(address: "docker.io/library/ubuntu:latest@sha256:80dd3c3b9c6cecb9f1667e9290b3bc61b78c2678c02cbdae5f0fea92cc6734ab"): Container!

dagger / $

[grouville]

Hey @pjmagee,
I am bringing the Windows computer to the repair store today ; hopefully it will be fixed soon so that I can test Dagger on Windows ahah 😇

[revosw]

Can confirm from Windows 11 (23H2, OS Build 22631.4541)

 dagger core container from --address ubuntu terminal
✔ connect 44.2s
✔ inspecting core types 11.7s
✔ parsing command line arguments 0.0s
✔ container: Container! 0.0s
✔ Container.from(address: "ubuntu"): Container! 4.4s
✘ Container.terminal: Container! 17.6s
! failed to forward exit code: failed to close terminal: EOF
  ✔ start sh 17.6s

Setup tracing at https://dagger.cloud/traces/setup. To hide: export STOPIT=1

Error: response from query: input: container.from.terminal resolve: failed to forward exit code: failed to close terminal: EOF                                                      
Run 'dagger core container from terminal --help' for usage.

[levlaz]

This came up again today in our community https://discord.com/channels/707636530424053791/1315796470758244372

I was able to reproduce this in any windows terminal. The only place this worked for me was in WSL.

@grouville I am going to bring my framework to Solomon's tomorrow, if you are around can we look into this together?

[rajatjindal]

I am looking into it now. 👍

[rajatjindal]

Hi @jedevc @helderco

I narrowed down the problem to this line here:

dagger/dagql/idtui/frontend_pretty.go

Line 746 in 8c0cefb

oldState, err := term.MakeRaw(ttyFd)

If I revert this commit, the terminal works in Windows.

But I noticed that there was some back and forth on the changes there during dagger shell changes, do you have any insights to share here?

what do you think about conditional code path here for Windows vs !Windows as a workaround?

[pjmagee]

Hi @jedevc @helderco

I narrowed down the problem to this line here:

dagger/dagql/idtui/frontend_pretty.go

Line 746 in 8c0cefb

oldState, err := term.MakeRaw(ttyFd)

If I revert this commit, the terminal works in Windows.

But I noticed that there was some back and forth on the changes there during dagger shell changes, do you have any insights to share here?

what do you think about conditional code path here for Windows vs !Windows as a workaround?

Hey, I have not actively written any code bug fixes, but some others have and when I was involved in the discussion, branching code based on GOOS == Windows was acceptable. I am sure if you did a search, you'll find a few existing places where the OS check is done for other features like exports and the cmd: feature that uses pwsh on windows, otherwise it uses sh

[jedevc]

Huh, what a nice lovely commit message from yours truly 🤔

Bleh, this is really fiddly.

I'm trying to remember all the context - but yes, this change was deliberate in the wider context of #8611.

Essentially - the change to the TUI here was made so that instead of running the entire TUI in raw mode, we rely on bubbletea doing semi-reasonable things. But we still need to put the tty into raw mode when switching for terminal.

I am slightly confused - MakeRaw and Restore should work on windows - before #8611 we also did this exact same operation - which worked. One key difference here though:

• Before, we called MakeRaw on the stdin
• After, we call it on stdout

Perhaps, this is significant? I seem to remember this cropping up before in another form. I'm curious what happens on windows if we switch the MakeRaw operation to use Stdin instead.

[jedevc]

@rajatjindal also, #9037 is another refactoring around this space - might be worth looking into that too.

Note, this is where you could put a conditional if you wanted to.

[rajatjindal]

Perhaps, this is significant? I seem to remember this cropping up before in another form. I'm curious what happens on windows if we switch the MakeRaw operation to use Stdin instead.

yep, that fixes it 🎉