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

[et] Allow disabling the "cute" output mode, so folks can grep the logs #147903

Closed
matanlurey opened this issue May 7, 2024 · 8 comments · Fixed by flutter/engine#52681
Closed
Assignees
Labels
e: engine-tool Engine-specific tooling (i.e. `tools/engine_tool`). P1 High-priority issues at the top of the work list team-engine Owned by Engine team

Comments

@matanlurey
Copy link
Contributor

Forked from #145711 (comment).

It should be reasonable to do something like:

et build --verbose | grep <something>

We could make this yet-another flag, i.e. --no-animations, but I think verbose implying it is also fine?

Wdut?

/cc @gaaclarke

@matanlurey matanlurey added team-engine Owned by Engine team e: engine-tool Engine-specific tooling (i.e. `tools/engine_tool`). labels May 7, 2024
@gaaclarke
Copy link
Member

Oh man, well tools like ag will change their output format when they detect they are being piped to another process. git uses the flag --porcelain to indicate output that is verbose and meant to be parsed by another tool.

I think making --verbose imply that is fine and I don't recommend those other approaches.

@matanlurey matanlurey self-assigned this May 7, 2024
@matanlurey matanlurey added the P1 High-priority issues at the top of the work list label May 7, 2024
@chinmaygarde
Copy link
Member

chinmaygarde commented May 7, 2024

Ah, I just filed #147936 which is a duplicate. We need to check that stdout is a terminal type and disable the pretty printing if its not. It is also ok to allow manually disabling pretty printing to terminal types too.

@chinmaygarde
Copy link
Member

As an aside, we need to go a bit out our way to fool Xcode since it pretends to be a terminal but can't handle being a terminal quite right. GN does this detection.

@chinmaygarde
Copy link
Member

https://api.dart.dev/stable/3.3.4/dart-io/Stdout/hasTerminal.html may be what we need (and the one for stderr).

auto-submit bot pushed a commit to flutter/engine that referenced this issue May 7, 2024
… provided. (#52619)

Closes flutter/flutter#147894.

While doing this PR I realized we were basically passing `(bool verbose, Envrionment)` as a tuple around, so I just moved the concept _into_ `Environment` directly, and made the necessary code changes across the tool and tests.

To clarify, this does _not_ mimic the output of `ninja --verbose` _today_, because we also don't stream the output directly, and instead do terminal magic. Combined with a hypothetical fix for flutter/flutter#147903, this would work exactly the same as before.

/cc @loic-sharma @johnmccutchan
@matanlurey
Copy link
Contributor Author

Update: We do some checks for hasTerminal and supportsAnsiEscapes, but it sounds like those are either insufficient, or give false-positives.

@matanlurey
Copy link
Contributor Author

When I do:

flutter % et build -v | grep supportsAnsiEscapes
supportsAnsiEscapes: false | hasTerminal: false

... it appears detection does work, so I'm assuming we're not guarding something we should be.

@matanlurey
Copy link
Contributor Author

The fix is as simple as:

diff --git a/tools/engine_tool/lib/src/logger.dart b/tools/engine_tool/lib/src/logger.dart
index 28bd847885..8b4565d94b 100644
--- a/tools/engine_tool/lib/src/logger.dart
+++ b/tools/engine_tool/lib/src/logger.dart
@@ -169,6 +169,8 @@ class Logger {
   /// and emits a carriage return.
   void clearLine() {
     if (!io.stdout.hasTerminal || _test) {
+      // Just emit a newline in tests or when not connected to a terminal.
+      _ioSinkWrite(io.stdout, '\n');
       return;
     }

However, it is next to impossible to test because the Logger does a bunch of things with global state.

I'm going to provide a bit more TLC to logger.dart and then land the fix.

auto-submit bot pushed a commit to flutter/engine that referenced this issue May 9, 2024
… updates. (#52681)

For illustrative purposes:

```sh
$ et build | grep '.*'
```

... should still get line-per-line status updates, but it does not without this patch.

It's hard to write tests because of global state, so I've declined to do so at the moment.

Closes flutter/flutter#147903.
Copy link

This thread has been automatically locked since there has not been any recent activity after it was closed. If you are still experiencing a similar issue, please open a new bug, including the output of flutter doctor -v and a minimal reproduction of the issue.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 23, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
e: engine-tool Engine-specific tooling (i.e. `tools/engine_tool`). P1 High-priority issues at the top of the work list team-engine Owned by Engine team
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants