JEG2 / highline

A higher level command-line oriented interface.
Other
1.29k stars 137 forks source link

Issues with echo: false and not on TTY #237

Closed cabello closed 5 years ago

cabello commented 5 years ago

Hi πŸ‘‹ ,

We've been using highline with much success! πŸŽ‰ Thanks for making this project open source.

I just noticed an issue present in both 1.7 and 2.0 when executing scripts not in a TTY and setting echo to false.

Script to reproduce issue script.rb:

require 'bundler/inline'

gemfile do
  source 'https://rubygems.org'
  gem 'highline',  '~> 2' # change this to ~> 1 to test 1.X.
end

cli = HighLine.new
answer = cli.ask("What do you think?") { |q| q.echo = false }
puts "You have answered: #{answer}"

Command to reproduce the issue: echo 'something' | ruby script.rb

Output in 2.x:

What do you think?
Traceback (most recent call last):
    10: from highline_feature_request.rb:9:in `<main>'
     9: from /Users/danilo/.rbenv/versions/2.5.3/lib/ruby/gems/2.5.0/gems/highline-2.0.0/lib/highline.rb:223:in `ask'
     8: from /Users/danilo/.rbenv/versions/2.5.3/lib/ruby/gems/2.5.0/gems/highline-2.0.0/lib/highline/question_asker.rb:30:in `ask_once'
     7: from /Users/danilo/.rbenv/versions/2.5.3/lib/ruby/gems/2.5.0/gems/highline-2.0.0/lib/highline/question.rb:536:in `get_response_or_default'
     6: from /Users/danilo/.rbenv/versions/2.5.3/lib/ruby/gems/2.5.0/gems/highline-2.0.0/lib/highline/question.rb:524:in `get_response'
     5: from /Users/danilo/.rbenv/versions/2.5.3/lib/ruby/gems/2.5.0/gems/highline-2.0.0/lib/highline.rb:518:in `get_response_line_mode'
     4: from /Users/danilo/.rbenv/versions/2.5.3/lib/ruby/gems/2.5.0/gems/highline-2.0.0/lib/highline.rb:538:in `get_line_raw_no_echo_mode'
     3: from /Users/danilo/.rbenv/versions/2.5.3/lib/ruby/gems/2.5.0/gems/highline-2.0.0/lib/highline/terminal.rb:70:in `raw_no_echo_mode_exec'
     2: from /Users/danilo/.rbenv/versions/2.5.3/lib/ruby/gems/2.5.0/gems/highline-2.0.0/lib/highline/terminal.rb:70:in `ensure in raw_no_echo_mode_exec'
     1: from /Users/danilo/.rbenv/versions/2.5.3/lib/ruby/gems/2.5.0/gems/highline-2.0.0/lib/highline/terminal/io_console.rb:20:in `restore_mode'
/Users/danilo/.rbenv/versions/2.5.3/lib/ruby/gems/2.5.0/gems/highline-2.0.0/lib/highline/terminal/io_console.rb:20:in `echo=': Inappropriate ioctl for device (Errno::ENOTTY)

It breaks and there is no recovery after that. It works as expected when echo is true.

Output in 1.X:

What do you think?
stty: stdin isn't a terminal
stty: stdin isn't a terminal
stty: stdin isn't a terminal

You have answered: something

It works even though it includes warnings.

Thanks again πŸ’œ

abinoam commented 5 years ago

Great catch. Confirmed in my machine. I can't promise taking a look at it really quickly. But I'll try it at least next week. (If you see a patch before this, please open a PR). This may have a huge impact in running scripts in pipes.

Thank you.

abinoam commented 5 years ago

Perhaps related to https://github.com/piotrmurach/tty-prompt/issues/49

abinoam commented 5 years ago

Checking here that input.tty? and input.isatty is false when pipeing and true calling the script directly. But perhaps this check is not reliable between platforms. No methods from HighLine::Terminal::IOConsole works correctly (when pipeing. eg: terminal_size) This points to us that we should identify early and use another terminal class to handle it (to be created).

Ruby stdlib io/console has advanced a little from my last touch at this code. Now we have things like getpass (http://ruby-doc.org/stdlib-2.6/libdoc/io/console/rdoc/IO/generic_readable.html#method-i-getpass). Some things in HighLine should be directly relying on it.

abinoam commented 5 years ago

Well, currently we treat input, output as being from the same "type". But when pipeing input can be a file and output a terminal.

abinoam commented 5 years ago

I've just assembled a quick and dirt fix. Released https://rubygems.org/gems/highline/versions/2.0.1 @cabello could you please give me any feedback if this fix is enough for your software. I'll probably look at this later for a better solution.

abinoam commented 5 years ago

The HighLine's assumption that input and output is the same type is broken.

cabello commented 5 years ago

I just confirmed this works as expected. Thanks for the quick fix.