-
Notifications
You must be signed in to change notification settings - Fork 373
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
Interactivity suffers? #58
Comments
Should |
@geertj - Fabric waffled on this in the past (started out with pty=False, then since 1.0 has been pty=True) and it's been problematic either way. The breakdown, IIRC:
It comes down to average use case: if you're using this kind of tool for its programmatic benefits, you're likely to want distinct stdout/stderr, which means pty=False is cleaner and more obvious. But if you're using it as a Make/Rake/etc replacement, you'll expect it to behave more "shelly", which here means pty=True because we can't hook the process up to your terminal's pipes (or we lose ability to capture any output at all, perform output hiding, etc.) I can't say offhand which is more likely in the intended userbase; my gut is honestly leaning towards the make/etc replacement, meaning pty=True, and practically it would also mean less of a break with Fabric 1.x (Fabric 2.x will build on Invoke). OTOH I value the ability to become "purer" with this kind of rewrite. As things stand this ticket is here so I make sure the above duality is documented and in peoples' face first thing if they have problems; idea being that since neither choice is perfect, go with the cleaner one (pty=False) and require folks to identify truly interactive invocations & slap pty=True on them. Will probably poll other smart people before I go 1.0 and see if they have angles I'm not seeing. |
This may be outdated now, can't remember when exactly we got hiding+capturing working with |
Some possible hints in the (stdin-focused) |
Some notes about some of this are in #289 since that ticket's implementation changed more of how streams are handled. |
#361 notes that the use cases they're having issues with worked better under Fabric, but I have a followup question for @hjc1710: Re: Fabric handling it, did you mean |
In answer to your question, @bitprophet, my example of Fabric handling this better was using It makes since that the API's are significantly different, because I distinctly remember As stated, #361 gives my primary use case, which is I want to wrap all my unit test running in invoke, and using interactive debuggers when Invoke is wrapping your unit tests is... pretty difficult right now. Thanks for looking into this and let me know if there's any way I can help or test! |
Adding a note here that my 'real complex and also a thing I want to be able to launch with Invoke pretty often' use case, vim, falls under this - specific notes from #289 are in this comment and subsequent. |
Also, it's clear that (if possible with current architecture) there should be an option to completely circumvent the "smart passthrough/stream mirroring+capturing" behavior in some situations - e.g. to do what This isn't ideal because it's only possible with truly local commands (remote ones cannot do this) but practicality needs to beat purity some of the time. So while I very much want this generally solved, a partway workaround would be e.g. |
Poking a little more at "why isn't our mirroring working quite right for vim"... Don't think I recorded this before but I remember it happening before, and it's still a thing now: hitting up/down arrow acts like I'm hitting "A" and/or "B", and even stranger, it can get into this "delayed" state where switching directions is delayed by 1 keypress. E.g. up-up-up-up ... will record stdin reads of 'A' over and over. Then 'down' will...still record a stdin read of 'A'. But then a subsequent 'down' will record 'B', and then all subsequent 'down' are 'B'. Vice versa for going back to 'up' - 1st 'up' still records 'B', only 2nd thru Nth record 'A'. Suspect this has to do with what my terminal emulator ( |
OK, part of the issue is
So the question is why The main loop in this case is:
Debug output of pressing down-arrow twice in a row after a vim session has loaded is below. Note that I am only debugging when
Condensed into just the stdin-read & stdout-read related bits:
So for some reason, What I don't recall clearly is the exact semantics of |
I can recreate the same Only real difference is that the input is appearing as Rummaging around docs, google, manpages etc hasn't helped me a ton so far re: pinning down exactly what Double-checked what I saw before re: the loop getting back a definitive "nope, nothing to read from Wondering if this is ye olde buffering issues with Python processes & pipes...? |
Brainstorm before I dig into that...possible reasons offhand:
|
Some comments on some SO results from google (1, 2) imply the core problem of nonblockingly reading off stdin is unreliable. Not sure how much I trust that but it's interesting and would certainly explain this behavior (though I usually translate "unreliable" as "intermittent" and the behavior I'm getting is quite consistent, just wrong!) Link 2 there does some wacky stuff with the terminal control structures which I should look into in case it's useful:
And a bunch of code in those and other results do basically what our Some others use |
Read/skimmed
Re:
So None of this yet explains what the hell is up with |
Confirmed that Also confirmed symptoms, & lack of efficacy of |
More googlin' and I found this SO which sounds very similar, but the answer is seemingly unhelpful, since the asker is using What is interesting is the asker mentions using Mucked around some more and got this to get me the
(Translation: you have to specify the size of a buffer handed to Result:
So immediately after our However...what this means is...we still have much more information than we had prior. We know how large we can make the So far, |
Existing tests pass with that fix in (I haven't put serious thought into how I could add new tests for this particular phenomenon, besides "ensure we call ioctl"...which had to suffice for the cbreak stuff, so...) given that I have it fallback to the 1-byte behavior for streams without a fileno. (Arguably, we could read much more at a time if we assume non-fileno-bearing streams are in-memory file-like objects...but that can wait.) (I love parentheticals, apparently.) More importantly, my by-hand tests with the arrow keys is smoother now - 3-byte reads and writes happen, and even with Even more importantly, vim now works flawlessly (well, except for |
TODO:
|
Scanned other linked tickets, happy to report that #361 sounds exactly like the issue I was using as my troubleshoot base case yesterday (re: "delayed" behavior due to the problems with @hjc1710 if you're still following this and want to share the exact app you're using, I can confirm whether this fix applies to it. Otherwise tho, I'm expecting to merge this to master today and release soon. EDIT: I'm dumb, you provided that info originally. Will clone your repo and try to reproduce... |
Yea, both of the tasks in that repo work great under the patch, both the REPL & |
Works reasonably well under Fabric 2; color and such don't happen, but there are no problems with control codes, arrow keys, etc. Strangely - the same is true even with this fix temporarily reverted. Can't think offhand what would be different in this case to make the issue not present. Something to poke more later perhaps. |
Hm, tests not failing under Windows (proof) despite not having added a Windows check to the newly updated FIONREAD bits. Wondering if this is because the Windows input streams lack |
This is super exciting to read @bitprophet! I can't wait for this to get merged into master so I can test it out on my main app (proprietary) that initially caused me to open this issue up. Thank you so much for your hard work, I sincerely appreciate it. We recently had to work on an old project that used Fabric and it made me miss Invoke sooooooooooo much. Returning to Invoke with this fixed will be like Christmas morning =). If you want another test case, feel free to ping me when this goes to master and I'll grab it and run our test suite through it and a few other tasks to make sure everything looks great! (Also, I really enjoyed reading your thoughts as you fixed this! Learned a TON about stream handling.) |
Sanity checked that the FIONREAD stuff does in fact work same on Linux (Ubuntu Trusty). Wasn't worried but while I was on there... |
Closed off #406 too, and this as well (added a changelog entry and such). Have some |
This allows to hit arrows, tab, ctrl-x when a prompt is raised on the remote end. (This is a C/P from Invoke) More context: pyinvoke/invoke#58
When running
run("python")
:>>> print 'hm'
prints the next line's prompt + "hm" on the same line.print >>sys.stderr, 'hm'
correctly deals w/ newlines^[[A
(control chars) instead of interacting with readline.Neither of these problems exist when
pty=True
. I feel like this is the same as in Fabric and any other solution where a pty is not the default behavior. At the very least we should make this explicit in the docs, probably by making an FAQ.The text was updated successfully, but these errors were encountered: