charmbracelet / vhs

Your CLI home video recorder 📼
MIT License
14.36k stars 240 forks source link

calling `vhs record` prints an error message instead of recording a new tape #250

Open Jonathan-Zollinger opened 1 year ago

Jonathan-Zollinger commented 1 year ago

Brief Overview

calling vhs's record prints an error message and doesn't produce a vhs tape

Expected Behavior

Calling vhs record creates a vhs tape and records the user's terminal actions in the vhs tape.

Actual Behavior

Calling vhs record prints out unsupported and stops. (see gif below)

Environment

OS Terminal Terminal Emulator VHS Version
Windows 11 Home
Build 22621.1344
PowerShell 7.3.2 Windows Terminal
as Administrator
v0.3.0 (7cb57d1)

installed via scoop. currently upgraded to latest available build scoop offers

I'm happy to help debug however I can or provide any other information. logging this bug per @bashbunni in discord

picatz commented 1 year ago

This is an unfortunate error from github.com/creack/pty, which doesn't seem to support Windows:

https://github.com/charmbracelet/vhs/blob/1d202a46b770170420a3c29983d8cd5b0d43d0d6/record.go#L72-L75

https://github.com/creack/pty/blob/0d412c9fbeb14954aa65830dcfdb84005cd0cd96/doc.go#L11

bashbunni commented 1 year ago

Looks like there's an open PR for Windows support for Pty. If this is time sensitive, you might be better off building VHS from source and adjusting the dependency to use the upstream that supports Windows. Hopefully that all gets resolved on our upstream soon, but as it involves breaking changes to their API, I can understand why it is taking a while to consider.

https://github.com/creack/pty/pull/155

I'll talk to the team about what can be done on our end to support Windows users without breaking our project 😅

Jonathan-Zollinger commented 1 year ago

sounds great! thanks !

tristanisham commented 2 months ago

Would it make sense to add a warning on Windows to this section? It's a bit jarring and has turned my off to using VHS in the past.

terminal, err := pty.Start(command) 
 if err != nil { 
        if runtime.GOOS == "windows" {
                log.Warnf("Windows dependency pty : %q\n", err)
        }
        return err 
 } 

With the progress on this issue (about an update a month) it doesn't seem like this will be quickly resolved. The last comment is a reference to a PR on the Go Gerrit to add support for Window's PsudoConsole Syscall. No response on the parent issue regarding this, plus depending on if we're in a Go feature freeze we might have a few months to wait regardless if this new SysCall is Pty's solution not counting the time for Pty to finish their tests. Current tests are still failing on Linux according to their CI.

bashbunni commented 2 months ago

@tristanisham that's a good idea, I think it would be worth adding