Tool with `--machine` emits `Downloading ...` before emitting JSON

[yaakovschectman]

et attempts to parse the output of the flutter tool, but when the tool logs that it is downloading dependencies, this parsing breaks. I suspect it would be trivial to add an error message in response to parsing failure suggesting that the user simply re-run et, as it succeeds on the second run once the flutter tool is finished building.

Steps to reproduce

1. Delete <flutter>/bin/cache/flutter_tools.stamp
2. cd into the directory of any Flutter project
3. Run et run

Expected results

The engine builds and the Flutter project runs

Actual results

et run fails because it cannot parse the output logging from the Flutter tools updating. Re-running et after the failure completes, as the Flutter tools is now already built and up to date.

Logs

Logs

```console [2024-08-26 12:35:50.925284] ERROR: Failed to parse flutter devices output: FormatException: Unexpected character (at character 1) Downloading package sky_engine... 388ms ^ Downloading package sky_engine... 388ms Downloading package flutter_gpu... 33ms Downloading flutter_patched_sdk tools... 126ms Downloading flutter_patched_sdk_product tools... 117ms Downloading darwin-arm64 tools... 762ms Downloading darwin-arm64/font-subset tools... 94ms [ { "name": "sdk gphone16k arm64", "id": "emulator-5554", "isSupported": true, "targetPlatform": "android-arm64", "emulator": true, "sdk": "Android 15 (API 35)", "capabilities": { "hotReload": true, "hotRestart": true, "screenshot": true, "fastStart": true, "flutterExit": true, "hardwareRendering": true, "startPaused": true } }, { "name": "macOS", "id": "macos", "isSupported": true, "targetPlatform": "darwin", "emulator": false, "sdk": "macOS 14.6.1 23G93 darwin-arm64", "capabilities": { "hotReload": true, "hotRestart": true, "screenshot": false, "fastStart": false, "flutterExit": true, "hardwareRendering": false, "startPaused": true } }, { "name": "Mac Designed for iPad", "id": "mac-designed-for-ipad", "isSupported": true, "targetPlatform": "darwin", "emulator": false, "sdk": "macOS 14.6.1 23G93 darwin-arm64", "capabilities": { "hotReload": true, "hotRestart": true, "screenshot": false, "fastStart": false, "flutterExit": true, "hardwareRendering": false, "startPaused": true } }, { "name": "Chrome", "id": "chrome", "isSupported": true, "targetPlatform": "web-javascript", "emulator": false, "sdk": "Google Chrome 127.0.6533.122", "capabilities": { "hotReload": true, "hotRestart": true, "screenshot": false, "fastStart": false, "flutterExit": false, "hardwareRendering": false, "startPaused": true } } ] ```

Flutter Doctor output

Doctor output

```console [!] Flutter (Channel master, 3.25.0-1.0.pre.133, on macOS 14.6.1 23G93 darwin-arm64, locale en) • Flutter version 3.25.0-1.0.pre.133 on channel master at /Users/schectman/src/flutter/flutter ! Upstream repository git@github.com:yaakovschectman/flutter.git is not a standard remote. Set environment variable "FLUTTER_GIT_URL" to git@github.com:yaakovschectman/flutter.git to dismiss this error. • Framework revision 6608936e6d (50 minutes ago), 2024-08-26 15:49:25 +0000 • Engine revision 365b0c70fa • Dart version 3.6.0 (build 3.6.0-175.0.dev) • DevTools version 2.39.0-dev.15 • If those were intentional, you can disregard the above warnings; however it is recommended to use "git" directly to perform update checks and upgrades. [✓] Android toolchain - develop for Android devices (Android SDK version 35.0.0-rc4) • Android SDK at /Users/schectman/Library/Android/sdk • Platform android-35, build-tools 35.0.0-rc4 • ANDROID_HOME = /Users/schectman/Library/Android/sdk • Java binary at: /Applications/Android Studio.app/Contents/jbr/Contents/Home/bin/java • Java version OpenJDK Runtime Environment (build 17.0.10+0-17.0.10b1087.21-11572160) • All Android licenses accepted. [!] Xcode - develop for iOS and macOS (Xcode 15.3) • Xcode at /Applications/Xcode.app/Contents/Developer • Build 15E204a ! CocoaPods 1.12.1 out of date (1.13.0 is recommended). CocoaPods is a package manager for iOS or macOS platform code. Without CocoaPods, plugins will not work on iOS or macOS. For more info, see https://flutter.dev/to/platform-plugins To update CocoaPods, see https://guides.cocoapods.org/using/getting-started.html#updating-cocoapods [✓] Chrome - develop for the web • Chrome at /Applications/Google Chrome.app/Contents/MacOS/Google Chrome [✓] Android Studio (version 2023.3) • Android Studio at /Applications/Android Studio.app/Contents • Flutter plugin can be installed from: 🔨 https://plugins.jetbrains.com/plugin/9212-flutter • Dart plugin can be installed from: 🔨 https://plugins.jetbrains.com/plugin/6351-dart • Java version OpenJDK Runtime Environment (build 17.0.10+0-17.0.10b1087.21-11572160) [✓] VS Code (version 1.92.1) • VS Code at /Applications/Visual Studio Code.app/Contents • Flutter extension can be installed from: 🔨 https://marketplace.visualstudio.com/items?itemName=Dart-Code.flutter [✓] Connected device (4 available) • sdk gphone16k arm64 (mobile) • emulator-5554 • android-arm64 • Android 15 (API 35) (emulator) • macOS (desktop) • macos • darwin-arm64 • macOS 14.6.1 23G93 darwin-arm64 • Mac Designed for iPad (desktop) • mac-designed-for-ipad • darwin • macOS 14.6.1 23G93 darwin-arm64 • Chrome (web) • chrome • web-javascript • Google Chrome 127.0.6533.122 [✓] Network resources • All expected network resources are available. ! Doctor found issues in 2 categories. ```

[jonahwilliams]

This might be a machine mode bug where we shouldn't output this info at all.

[christopherfujino]

This might be a machine mode bug where we shouldn't output this info at all.

My understanding is that there are many such instances, and Dart-Code works around this by discarding any line that doesn't parse as JSON

[matanlurey]

This bug is in the flutter tool, not et (yes, we could work around it, but why do that?).

The main executable tries to avoid this situation, but only for flutter run or flutter attach:

final bool runMachine = 
  (args.contains('--machine') && args.contains('run')) ||
  (args.contains('--machine') && args.contains('attach'));

A few options I can think of, but I can't imagine the consequences:

1. Loosen the above to any machine invocation
2. Shift all progress messages to Stderr
3. Change artifact messages specifically (those above) to go to Stderr
4. Shift progress messages if --machine is used, to Stderr

For example a PR from 2021 from @gspencergoog (https://github.com/flutter/flutter/pull/92031) reads:

// This needs to go to stderr to avoid cluttering up stdout if a
// parent process is collecting stdout (e.g. when calling "flutter
// version --machine"),

@andrewkolos Any thoughts? I'm happy to contribute but could use guidance.

[gspencergoog]

For example a PR from 2021 from gspencergoog (https://github.com/flutter/flutter/pull/92031) reads:

I think I looked at making all log messages (not just warnings and errors) go to stderr at the time, but there were plenty of places where tools or tests expected to see specific output on stdout, so I didn't change it. I do think that would be a good idea, at least if --machine is set, so I like option number 4.

[matanlurey]

Thanks Greg! ❤️

[flutter-triage-bot[bot]]

The triaged-engine label is irrelevant if there is no team-engine label or fyi-engine label.

[andrewkolos]

@andrewkolos Any thoughts? I'm happy to contribute but could use guidance.

As a personal principle, CLI commands that have meant-for-machine output should route all meant-for-human status messages to stderr. This aligns with option 2 in https://github.com/flutter/flutter/issues/154119#issuecomment-2379939039. We could have this only be true in the presence of --machine (option 4), though I can't immediately think of a reason to introduce extra logic here (aside from having to update fewer tests). However, my opinion is not firm.

[matanlurey]

If we do (2), we'll have to deal with:

1. Our own unit tests that assert otherwise (doable, I think this is 1-2 hours of work)
2. Our own integration tests, or tests in other repos that assert otherwise (unknown amount of work)
3. End-users that are parsing stdout today assuming certain things that will shift to stderr

So, if we do (2), I'd personally either:

1. Give our larger team a heads up about this change, and a time period to stop doing it
2. Add a flag --direct-status-messages-to-stderr, and do a deprecation warning/change

If we think neither of those is a good thing to do, we could limit to --machine. My concern is adding tons of branch complexity (which flags, which modes) that are hard to test and ultimately end up breaking folks anyway.

/cc @bkonyi @jtmcdole as we've been talking about log scraping on a few threads today.

[jtmcdole]

I like machine silencing the human messages or dumping to stderr. But I also like piping output to other streams (say a socket or file).

If I were running flutter run --machine > tmp.json I'd expect the file to only have json and then the human readable text would be visible on the terminal.

[bkonyi]

If I were running flutter run --machine > tmp.json I'd expect the file to only have json and then the human readable text would be visible on the terminal.

The approach that we've taken for most of our tooling (at least on the Dart side of things) is that --machine should result in the tool only printing machine readable output. If we're not doing that with the Flutter tool, that's a bug as downstream tooling shouldn't have to handle non-JSON strings if they've explicitly specified --machine.

[jtmcdole]

If I were running flutter run --machine > tmp.json I'd expect the file to only have json and then the human readable text would be visible on the terminal.

The approach that we've taken for most of our tooling (at least on the Dart side of things) is that --machine should result in the tool only printing machine readable output. If we're not doing that with the Flutter tool, that's a bug as downstream tooling shouldn't have to handle non-JSON strings if they've explicitly specified --machine.

Yep, I'm agreeing with you on this. > tmp.json just shows stdin flowing into the json file while the human output goes to stderr or some other pipe.