[RB] Fix duplicate logs when the console does not support ANSI escape sequences #6163
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. Join @maggie-lou and the rest of your teammates on |
maggie-lou
force-pushed
the
rb_dup_logging
branch
2 times, most recently
from
8149b4f
to
04711b3
Compare
maggie-lou
force-pushed
the
rb_dup_logging
branch
from
04711b3
to
a607f2a
Compare
There was a problem hiding this comment.
Just as a heads up, I don't think this will fully fix the linked issue, because we're still using ANSI escape sequences to erase and redraw the live log tail. I think we will need some server-side changes to allow configuring an invocation in "non-streaming" mode, which might be a bit more involved. But this PR is needed either way so it's a good starting point 👍
cli/remotebazel/remotebazel.go
Outdated
| @@ -67,6 +67,7 @@ var ( | |||
| remoteRunner = remoteFlagset.String("remote_runner", defaultRemoteExecutionURL, "The Buildbuddy grpc target the remote runner should run on.") | |||
| timeout = remoteFlagset.Duration("timeout", 0, "If set, requests that have exceeded this timeout will be canceled automatically. (Ex. --timeout=15m; --timeout=2h)") | |||
| execPropsFlag = bbflag.New(remoteFlagset, "runner_exec_properties", []string{}, "Exec properties that will apply to the *ci runner execution*. Key-value pairs should be separated by '=' (Ex. --runner_exec_properties=NAME=VALUE). Can be specified more than once. NOTE: If you want to apply an exec property to the bazel command that's run on the runner, just pass at the end of the command (Ex. bb remote build //... --remote_default_exec_properties=OSFamily=linux).") | |||
| interactive = remoteFlagset.Bool("interactive", true, "Whether to run the script in interactive mode (will print real-time progress updates using ANSI escape sequences). This may cause duplicate logs to be printed if the console does not support ANSI escape sequences.") | |||
There was a problem hiding this comment.
Instead of making this an explicit flag, I think it would be better to check whether the command's output is a terminal or not. You can use terminal.IsTTY(os.Stderr) && terminal.IsTTY(os.Stdout) to check (from the cli/terminal package). When running locally in a terminal, this should return true. If you send the output to a pipe, like bb remote ... | cat, it'll return false. On GH actions, I think it will return false, since the actions runner is not running actions inside an interactive terminal.
Simplified the PR by only checking for interactive support in cli/remotebazel |
cli/remotebazel/remotebazel.go
Outdated
| time.Sleep(1 * time.Second) | ||
| continue | ||
| } | ||
| drawChunk(l) |
There was a problem hiding this comment.
I think this could be
os.Stdout.Write(chunkGetBuffer())
and then we could get rid of drawChunk
We run the ci_runner in interactive mode so that it prints real-time updates for git and bazel commands.
However the Github Actions shell does not support ANSI escape sequences (actions/runner#241). When running remote bazel or the ExecuteWorkflow API in a Github Action runner, this causes output lines to be printed several times (because the real-time printing tries to delete and over-write a previous line with an updated progress message, but deletion isn't supported).
Add a configurable option to disable interactive mode
Related issues: Fixes https://github.com/buildbuddy-io/buildbuddy-internal/issues/3143