Implement TTY detection so progress bar is not shown unless in a TTY environment

[AndrewRayCode]

Hello, thank you for the workaround for the progress bar bugs with SFDX_USE_PROGRESS_BAR=false. However this is not a robust solution and is anti-shell-ecosystem.

In shell scripting, we of course send output with stdout and stderr.

Some tools give nice progress indicators of status, for example, docker pull:

Now, to draw a progress bar and refresh the view so that it updates in shell scripting, you need to send special characters to do things like overwrite the current line to implement animation that updates.

Of course this is only done in a "TTY" environment, my understanding is short for "teletype" - referencing old physical machines you type at, really for me this means interactive, as in accepting user input.

There are lots of times where a program isn't sending output to an interactive terminal. The most common one you've probably used is piping commands to another command. Let's take a look at what docker pull does when you pipe the output to another program:

You can see docker correctly detects it's not in an interactive environment, and correctly writes text with newlines to stdout.

All programs that output animation type behavior to stdio have this default detection behavior. This is a requirement for a program to run and play nicely in this ecosystem. You can see this behavior with any other command that has some sort of progress indicator or screen refreshing mechanism.

Where this probably matters most for sfdx users is running sfdx on ci systems, where of course the stdout isn't in an interactive terminal where users can type. Sending clear or overwrite characters to this terminal is incorrect behavior because there's nothing to clear, we're just capturing text from stdout and stderr and showing it to the user. This will of course also be an issue if I want to capture the output of sfdx and do things like grep it, unless I know to use this magic workaround switch.

I have seen some pushback from library maintainers, and I feel worried that SFDX_USE_PROGRESS_BAR will be cemented as a "solution", when in fact this is a workaround that makes sfdx a little harder for everyone to use. I don't think sfdx should forever have inconsistent behavior compared to all other shell tools that have updating behavior.

[AndrewRayCode]

After a little more investigation, I think I understand the behavior a little more

When running something like sfdx force:source:deploy ... 2>&1 > cat, two things happen:

• the stdout and stderror seem to properly pipe to the destination (yay!)
• the progress bar incorrectly shows up

I think the second point is why we still see the progress bar incorrectly in ci environments unless we force it off with the SFDX_USE_PROGRESS_BAR workaround.

[clairebianchi]

@AndrewRayCode are you running the command with --json in CI environments?

[amphro]

@AndrewRayCode I was under the impression we weren't showing progress in a TTY environment. See https://github.com/oclif/cli-ux/blob/master/src/styled/progress.ts#L11 - is that not what you are seeing?

[vazexqi]

Piggy-backing onto this issue. I am on the latest version of the Salesforce CLI (installed using the CircleCI Orb). I also set TERM: dumb in my .circleci/config.yml.

Expected:

Based on the comments and also the link to https://github.com/oclif/cli-ux/blob/master/src/styled/progress.ts#L11 I was expecting that the progress bar will not show up since this is not a TTY environment.

Actual:

The progress bar shows up even thought the output of node -e "console.log(Boolean(process.env.TERM !== 'dumb' && process.stdin.isTTY)" shows that it is not a TTY.

[AndrewRayCode]

@clairebianchi no I am not running with --json, and the example command I posted is a reproduction step. I hope that's enough to remove the more information required tag?

[aheber]

@vazexqi & @AndrewRayCode, I don't think the intention is to not show the progress bar in nonTTY environments but to add a newline to the output so each "poll" interval it will report the current progress and you'll get a stack of progress bars.

This is probably most-easily reviewed here:
https://github.com/AndiDittrich/Node.CLI-Progress/blob/98b0d2dac5c4588d5ba08caf0afb5c29d2def646/lib/single-bar.js#L35

An example of how it now works from my own CI:

I know there have been a few individuals who would rather not have it but I personally appreciate it, even in CI. It is better than no output at all for 30+ minutes which is the alternative in my system.

[amphro]

I thought the newline happened in TTY when the noTTYOutput was not set. It seemed to me that it disabled the render if that option was set... https://github.com/AndiDittrich/Node.CLI-Progress/blob/98b0d2dac5c4588d5ba08caf0afb5c29d2def646/lib/single-bar.js#L52

But I think I must be misunderstanding the code.

It seems though, there are many, many opinions on what people want with output in CI and non-TTY environments.

1. Show no output, except for the final results.
2. Show progress bar separated by newlines.
3. Show "old-style" verbose progress output.

Since people are so opinionated on this, I'm inclined to change the SFDX_USE_PROGRESS_BAR to be SFDX_PROGRESS=none|bar|text with bar being the default. This was proposed by @peternhale

Hopefully, those are explanatory but none would show nothing, bar would show the progress bar, text would show the output as SFDX_USE_PROGRESS_BAR=false does. This would allow us to add more options later for those who want something different...

In a non-TTY environment, bar would continue to show newlines as the above screenshot.

Comments?

[AndrewRayCode]

The progress bar should not appear by default in non TTY environments, be it a bash pipe situation or regular non-TTY stdout. We shouldn't rely on a progress bar for CI systems as it's a bug.

The question is then, for TTY environments, should the default be progress bar (sounds right), and for non-TTY, should the default be text, or none? I think none, as getting verbose output feels like an opt-in, not an opt-out, sort of flag

It looks like some tools default to text (never animated style) progress indicators, see this maven ticket with 150 upvotes of how to disable the text progress

[amphro]

Thanks for the input @AndrewRayCode - after hearing feedback from a lot of stakeholders for the past few weeks, I tend to agree with you. We are discussing internally and hopefully, you'll see something soon.