Skip to content
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

Retaining color of STDOUT #33

Closed
thisismydesign opened this issue Aug 18, 2017 · 26 comments
Closed

Retaining color of STDOUT #33

thisismydesign opened this issue Aug 18, 2017 · 26 comments

Comments

@thisismydesign
Copy link

Hi there,

First of all, great gem!

I was happy when I read colorful output, however it seems that it only applies to your gem's output and not the original command's.

Do you have any plans to make it possible to retain the color of the original output, or do you know whether that's even possible? According to my limited understanding you should somehow pretend to be a tty.

@piotrmurach
Copy link
Owner

Thanks for using the gem!

Sure thing, I will take a look, it seems like a bug as there shouldn't be any mingling of the output going on and all the coloring should be preserved.

@thisismydesign
Copy link
Author

Sounds good, thanks a lot! I was looking at these discussions (#1, #2) and was under the impression that this is the normal behaviour.

I was experimenting with git branch and git status. Will try to provide more info if you can't reproduce it.

@piotrmurach
Copy link
Owner

It may well be the case that the subprocess that I call will strip away any ansi codes from the output stream. If you find a fix then by all means please submit PR, I will happily review and merge.

@thisismydesign
Copy link
Author

I'm afraid I don't have the time at the moment but if I come across a solution in the future I will let you know.

@piotrmurach
Copy link
Owner

@thisismydesign Please see referenced test and example that demonstrate that the ansi codes are preserved for both the actual output and captured output.

@thisismydesign
Copy link
Author

@piotrmurach Thanks for looking into this. Unfortunately this does not cover / solve my use case.
While you showed that root cause is not that the gem swallows colors, my issue is still present: otherwise colored output will not be colored when used through this gem. See git status or rspec as examples.

The root cause is probably explained in the already linked StackOverflow answer:

I would guess that rspec is examining the stream to which it is writing output to see if it is a tty (ie the console) or not, and if it's not, disabling colour.

I think it would be worthwhile to reopen this issue / add some clarification to the readme so that others stumbling upon this have a reference or can maybe think about providing a solution.

@piotrmurach
Copy link
Owner

@thisismydesign Sorry I got too excited closing tickets 😄 I have no problem keeping this open until a suitable solution is found or we add a note to readme to warn people of this use case.

@piotrmurach
Copy link
Owner

@thisismydesign Turns out that this is possible by running command in pseudo terminal. I've added :pty option (which is false by default) to allow running commands with colored output. I don't believe this should be switched on by default due to its side effects and noticable poor performance. I think in majority of cases people will be interested in executing commands in non-interactive way. I've also written some docs. It would be cool if you could test the master branch?

@thisismydesign
Copy link
Author

Thanks a lot! Sure thing, will get back to you.

@thisismydesign
Copy link
Author

Works great, thanks!

@piotrmurach
Copy link
Owner

I'm still undecided whether the :pty flag should be turned on by default. On one hand, I value intuitive libraries that work out of the box. On the other hand, I don't fully appreciate side effects of pseudo terminals to risk it, including the line feed changes as explained the docs. Do you have any thoughts? Do you think that :pty set to false by default is ok?

@jrochkind
Copy link

I think leaving it false by default probably makes sense (although I agree, hard to say, but you gotta pick something!), but the README should probably include a section explaining the setting and it's consequences.

@thisismydesign
Copy link
Author

I think the line feed changes are ok as devs can expect it but it's definitely a breaking change. I'm worried about other side effects too, e.g. came across this yesterday when reading about the PTY module - not sure if it would have any impact.

I'd not make it default before more testing and feedback. Introducing a breaking change of course complicates things further. If you plan some cool new features for the gem it would suck to place them after a major version bump. Otherwise it might be harmless and actually beneficial to make it default sometime in the future. Deprecation warnings beforehand and providing a feature that normalizes line feeds would definitely help with adaptation. Performance-wise I imagine this adds some overhead but it's not a multiplier for the whole command execution in which case I think it'd be ok to have it on by default.

It's not a black and white decision. Hope this was somewhat helpful and I didn't confuse you even more. :)

@piotrmurach
Copy link
Owner

Thank you both for your thoughts!

@jrochkind I have added the PTY(pseudo terminal) section to docs. Would appreciate any comments?

@thisismydesign I'm using PTY module but in a slightly different way. I don't spawn command in pseudo terminal, but use normal child process with pseudo terminal pipes plugged-in. Therefore I don't think that it is susceptible to the process issues as per the mentioned thread. We should be safe here - no zombie processes.

After reading C source for pty.c I have discovered that you can actually put the pty slave device into raw mode. What that means is that there will be no mingling of the output.

I think I will make the pty option to be false by default and here is why. The pseudo terminals are only Unix thing and they aren't available on Windows. And since this lib works fine on Windows I want to keep it this way, if it means you cannot have colored output from external executable on Windows, so be it. Also, from my little tests it appears that PTY may echo input from the external tool whether you wish to see it or not.

@thisismydesign
Copy link
Author

@piotrmurach Cool, hope you will include the raw mode by default for the pty option. :)

Definitely agree that it should not break on Windows but to be fair you could easily determine which system the gem is running on. If for nothing else it'd be useful to fail meaningfully when the user tries to use the pty option on Windows. Also I'm not sure whether colors are missing on Windows at all.

Will try to incorporate your gem into mine over the weekend, that should give us some more feedback.

@piotrmurach
Copy link
Owner

to be fair you could easily determine which system the gem is running on

From my experience this is the hardest thing to accomplish and detecting operating system usually produces the least reliable way to handle cross platform support in Ruby. This is predominantly due to different bash-like emulators such as cygwin,git-bash. Please see tty-prompt issue if you have any suggestion how to solve it - simply detecting windows won't help. I tend to fall back on features detection, but I'm not sure how best to detect pty cross platform hence I'm thinking of setting it to false by default.

As far as colors on Windows, I have VM setup for windows xp where I test Ruby libs in powershell and the windows console provides support for basic ANSI color strings.

Thank you trying the gem out, this is super helpful!

@piotrmurach
Copy link
Owner

@thisismydesign been knocked down by flu fore more than a week now and hence failed to finish this.

I've provided a fallback to normal behaviour if pty doesn't exist on whatever the system a command is run. This is definitely a more graceful way to handle this situation. Also, I get to display warning to say what happened - I didn't think it deserved a full blown error message.

Making pty raw requires including new dependency on io-console which I would rather skip for now.

The more I think about this library primary use case which is logging output, it becomes apparent to me that having pty set to false is the right way to go about it. There is a reason why well-behaved tools such as rspec detects if its printing to tty device or not. If you're a sys admin the last thing you want is for output logs from tty-command to contain ansi escape characters.

@thisismydesign
Copy link
Author

@piotrmurach No worries. I also haven't gotten as far yet as I hoped with incorporating your gem.

You made good points and in light of them I also agree that it makes more sense to keeppty off by default.

Is there a way to set the raw mode from the outside so those willing to live with the additional dependency can keep the same functionality?

@piotrmurach
Copy link
Owner

piotrmurach commented Nov 18, 2017

@thisismydesign I was wrong! The io-console is a gem that ships with Ruby by default, so no need to install. I should've known better as I rely on it in another tty library - duhhh. I've changed to disable newlines conversion in pty device by default.

@piotrmurach
Copy link
Owner

@thisismydesign I've release v0.7.0 that includes the new pty option. Please let me know how it works for you. Btw, I'm greatly disappointed that you're usingcolorize instead of pastel 😉

@thisismydesign
Copy link
Author

thisismydesign commented Nov 26, 2017

@piotrmurach Thanks a lot! pastel looks great and since it's already a dependency of tty-command it doesn't make sense to introduce another one. :)

I'm currently facing an issue with hanging commands. I have a very strong suspicion that the hanging is caused by paging. Not sure if this is a terminal behaviour that could be turned off or whether this is coming from the app.

Consider the following command:

vcs_spec_rb_-_autowow_-____repos_autowow_

When executed in a smaller terminal:

image

And finally when pressing a couple enters:

image

What do you think? I can interpret this as expected behaviour. But in this particular case I's see no harm in a workaround - if that's even possible.

This also made me think about some more broad questions. Might not at all be at scope for this gem but wondering whether it's possible / would make sense to:

  • stream print the command output to provide better UX.
  • give the control back to the user upon input request

@thisismydesign
Copy link
Author

thisismydesign commented Nov 26, 2017

Oh and by the way the pty options works fine for me as far as colored output goes. :) The above would probably warrant a separate issue if you're interested.

@thisismydesign
Copy link
Author

Got it meanwhile... https://stackoverflow.com/questions/2183900/how-do-i-prevent-git-diff-from-using-a-pager

It may warrant a warning in the README that using a pseudo terminal can have side effects.

@piotrmurach
Copy link
Owner

@thisismydesign The side effects and the fact that running command in terminal mode is not the intention behind majority of uses cases motivated me to set pty to false by default. When you set pty to true you're essentially running an interactive terminal session, similar to system or exec Ruby calls. To reference myself from this thread:

I don't believe this should be switched on by default due to its side effects and noticable poor performance. I think in majority of cases people will be interested in executing commands in non-interactive way.

I will update the docs further to explain this better!

As far as exposing the streams directly, there is already an issue discussing the possible api which you may wish to check out. I'm not sure I understand the

stream print the command output to provide better UX.

Currently the output is streamed as the command continues to execute. It's in separate thread and keeps reading until the output is exhausted. That was original point of this library to help provide streaming output.

piotrmurach added a commit that referenced this issue Nov 26, 2017
@thisismydesign
Copy link
Author

@piotrmurach Thanks for the doc update!

Currently the output is streamed

You're right, my mistake I used the wrong printer. :)

@piotrmurach
Copy link
Owner

@thisismydesign No problem! If you have any ideas etc... just open new issue so that we don't carry on this monster of a thread 😅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants