[flutter_tools] correctly forward error only stdout in non-verbose modes #63815

jonahwilliams · 2020-08-14T22:18:28Z

Description

Xcode is internally aware of the distinction between stderr and stdout, but will forward everything to stdout by default. Providing the -quiet flag will cause it to only surface what it considers an "error" as part of stdout.

Additionally, the inclusion of the "touch" after the assemble script meant that in the event assemble failed, the overall status code would still be 0.

Fixes #63748

jonahwilliams · 2020-08-14T22:24:57Z

packages/flutter_tools/templates/app/macos.tmpl/Runner.xcodeproj/project.pbxproj.tmpl

@@ -280,7 +280,7 @@
 			);
 			runOnlyForDeploymentPostprocessing = 0;
 			shellPath = /bin/sh;
-			shellScript = "\"$FLUTTER_ROOT\"/packages/flutter_tools/bin/macos_assemble.sh\ntouch Flutter/ephemeral/tripwire\n";
+			shellScript = "\"$FLUTTER_ROOT\"/packages/flutter_tools/bin/macos_assemble.sh";


A reasonable follow up to this PR would be to add an automatic migration using the tooling that @jmagman already wrote, but its not something I have time for right now

I'm guessing it doesn't matter if it gets "tripped" twice in a row?

No, just as long as it gets modified at least once

Isn't the reason that we were doing this here was that if we fix it to not need the tripwire, we need to update the project with the input/output files? It seems like this sets us up to accidentally change the script later while forgetting that we need to fix all the projects in the wild.

The project input/output files are correct and can work 100% of the time, if you are building through flutter. When building through the xcode UI, changing configurations does not lead to any sort of change in files on disk - so Xcode thinks that our task can be skipped.

I have no idea how to fix this.

changing configurations does not lead to any sort of change in files on disk - so Xcode thinks that our task can be skipped.

I have no idea how to fix this.

I thought we had a potential solution for this now, inspired by @jmagman's iOS work with App.framework: have three different copies of the framework in ephemeral, rather than just one (e.g., ephemeral/debug/..., ephemeral/profile/..., ephemeral/release/...) and control the linking ourselves with linker flags, including a configuration-variable-based linker search directory.

different locations for debug/profile/release artifacts would definitely help. I can't say for sure if it would solve the issue completely

Anyway, that part was a tangent. Going back to my main concern: what is the goal for moving this logic out of the project? This PR seems unrelated to the tripwire, and AFAIK nothing has changed since we made a deliberate decision to put the tripwire logic on this side of the divide (per my comment above).

I think (now I need to double check, because I could have mixed it up) that ending with && touch was causing failures of the actual script to be suppressed. I bet there is some sort of bash-fu to fix it..

Oh, I see. I missed that there wasn't anything else in macos_assemble.sh before, and thought the error exit you added there was to fix that problem.

Yeah, the current Xcode script is definitely wrong, because it's just two statements. Changing the \n to && should be the easy fix there.

jmagman · 2020-08-14T22:42:42Z

packages/flutter_tools/lib/src/macos/build_macos.dart

+        'VERBOSE_SCRIPT_LOGGING=YES'
+      else
+        '-quiet',
      'COMPILER_INDEX_STORE_ENABLE=NO',
      ...environmentVariablesAsXcodeBuildSettings(globals.platform)
-    ], trace: true);
+    ], trace: false);


Would love to combine more logic, iOS and macOS building should look almost the same.

I'm confused here; IIRC you recently turned trace on for Windows and Linux to fix missing Dart failure output. Why do we want it for Windows and Linux but not macOS?

trace only prints stderr by default, and forwards stdout to our verbose logging (hidden by default). with -quiet, xcode writes errors to stdout so we need to surface them.

Why do we want it for Windows and Linux but not macOS?

Everything is doing something different with error logs 🕵️‍♂️

Oh, I see. Could we use the new stdout error regex logic I added for MSBuild here, so the logic is more unified?

with -quiet there shouldn't be any need, since only task stderr should be surfaced

But there are other differences in the code flow. For instance, won't your current approach mean that errors are printed in the normal color on macOS, rather than red on every other platform? And any code, future or current, that hooks into the logger mechanism to know what the errors are won't see the macOS error output as errors.

Whereas if you pass .* as an error regex (along with using -quiet) then macOS errors will use the same path through the logger as errors on other platforms.

Ahh, cool I didn't know it did that :)

jmagman · 2020-08-14T22:44:07Z

packages/flutter_tools/templates/app/macos.tmpl/Runner.xcodeproj/project.pbxproj.tmpl

@@ -280,7 +280,7 @@
 			);
 			runOnlyForDeploymentPostprocessing = 0;
 			shellPath = /bin/sh;
-			shellScript = "\"$FLUTTER_ROOT\"/packages/flutter_tools/bin/macos_assemble.sh\ntouch Flutter/ephemeral/tripwire\n";
+			shellScript = "\"$FLUTTER_ROOT\"/packages/flutter_tools/bin/macos_assemble.sh";


I'm guessing it doesn't matter if it gets "tripped" twice in a row?

jmagman · 2020-08-14T22:54:16Z

packages/flutter_tools/bin/macos_assemble.sh

+  EchoError "Failed to build ${project_path}."
+  exit -1


This makes sense for now.
A better solution for both platforms would be to craft one-line error messages (not like the ======================= errors we have now) and echo prefixed with "error " or "warning ". This will show up as a real Xcode error instead of just a failure of a script exiting with nonzero, as discussed in chat one time.

except that's supposed to say "This is an error message" and I'm too lazy to recreate this.

Is this something we need to do for our printError calls? That is not that most difficult thing to add...

We are way too chatty with out printError calls to just prepend error . For example, I believe every dart compilation failure line is a separate printError, including the _^_ whatever ASCII annotation line that points to the column of the printed line, so there would be like 5 Xcode errors for every dart compilation error, when there should just be one. I may be misremembering though.

I have a branch for this I never finished, for iOS:
logger.dart

class XcodeScriptPhaseStdoutLogger extends StdoutLogger { XcodeScriptPhaseStdoutLogger({ @required AnsiTerminal terminal, @required Stdio stdio, @required TimeoutConfiguration timeoutConfiguration, StopwatchFactory stopwatchFactory = const StopwatchFactory(), }) : super( terminal: terminal, stdio: stdio, outputPreferences: OutputPreferences( wrapText: false, showColor: false, ), timeoutConfiguration: timeoutConfiguration, stopwatchFactory: stopwatchFactory, ); @override void printError( String message, { StackTrace stackTrace, bool emphasis, TerminalColor color, int indent, int hangingIndent, bool wrap, }) { String xcodeMessage = message; if (message.contains('Error:')) { // <-- "Error:" was the best indicator I had for a dart issue. xcodeMessage = 'error: $message'; } // <-- Probably need more cases here. super.printError( xcodeMessage, stackTrace: stackTrace, emphasis: false, color: color, indent: indent, hangingIndent: hangingIndent, wrap: wrap ); } }

xcode_backend.sh

RunCompileCommand "${FLUTTER_ROOT}/bin/flutter" \ ${verbose_flag} \ ${flutter_engine_flag} \ ${local_engine_flag} \ assemble \ --output="${derived_dir}/" \ ${performance_measurement_option} \ --logging-style=xcode-build-phase \ ...

assemble.dart

argParser.addOption( 'logging-style', hide: true, allowed: <String>['xcode-build-phase'], help: 'Format logging.', );

On the right track?

Yeah that seems like a good place to start, though I would leave out the message introspection and instead configure it via the calls to printError

jmagman · 2020-08-15T01:32:23Z

packages/flutter_tools/bin/macos_assemble.sh

@@ -93,3 +93,12 @@ RunCommand "${FLUTTER_ROOT}/bin/flutter"                                    \
    --build-outputs="${build_outputs_path}"                                 \
    --output="${ephemeral_dir}"                                             \
   "${build_mode}_macos_bundle_flutter_assets"
+
+if [[ $? -ne 0 ]]; then


Hopefully no one ever adds set -e to the top like #50664.

jmagman

LGTM

stuartmorgan · 2020-08-15T01:41:15Z

packages/flutter_tools/templates/app/macos.tmpl/Runner.xcodeproj/project.pbxproj.tmpl

@@ -280,7 +280,7 @@
 			);
 			runOnlyForDeploymentPostprocessing = 0;
 			shellPath = /bin/sh;
-			shellScript = "\"$FLUTTER_ROOT\"/packages/flutter_tools/bin/macos_assemble.sh\ntouch Flutter/ephemeral/tripwire\n";
+			shellScript = "\"$FLUTTER_ROOT\"/packages/flutter_tools/bin/macos_assemble.sh";


Isn't the reason that we were doing this here was that if we fix it to not need the tripwire, we need to update the project with the input/output files? It seems like this sets us up to accidentally change the script later while forgetting that we need to fix all the projects in the wild.

stuartmorgan · 2020-08-15T01:42:44Z

packages/flutter_tools/bin/macos_assemble.sh

@@ -93,3 +93,12 @@ RunCommand "${FLUTTER_ROOT}/bin/flutter"                                    \
    --build-outputs="${build_outputs_path}"                                 \
    --output="${ephemeral_dir}"                                             \
   "${build_mode}_macos_bundle_flutter_assets"
+
+if [[ $? -ne 0 ]]; then


I forget, is there something blocking us from merging this into the Dart script we're using for Windows and Linux? It would be nice to go in that direction instead of adding even more logic to the shell script version.

No, its do-able with some light refactoring besides the tripwire. If we can resolve that its 5 mins of work

Couldn't we combine scripts, and have the tripwire logic based on OS (if we really need the tripwire here rather than in the project, which per my other comment I'm still not convinced of)

Couldn't we combine scripts, and have the tripwire logic based on OS

We could for new projects, with some backwards compat for macos_assemble (unless we add the project migration support, but like I said I don't really have time for that this week)

I was just thinking converting macos_assemble.sh into a thin tool_backend.dart wrapper, not doing an actual migration at this point. Just so we're not adding more bash logic and continuing to maintain bash complexity, when we already have a Dart version that's more testable and more maintainable basically ready to swap in.

stuartmorgan · 2020-08-15T01:44:52Z

packages/flutter_tools/lib/src/macos/build_macos.dart

+        'VERBOSE_SCRIPT_LOGGING=YES'
+      else
+        '-quiet',
      'COMPILER_INDEX_STORE_ENABLE=NO',
      ...environmentVariablesAsXcodeBuildSettings(globals.platform)
-    ], trace: true);
+    ], trace: false);


I'm confused here; IIRC you recently turned trace on for Windows and Linux to fix missing Dart failure output. Why do we want it for Windows and Linux but not macOS?

…ly_macos

jonahwilliams · 2020-08-18T18:14:07Z

Updated to the minimal && change and to use the error regex

stuartmorgan

LGTM

stuartmorgan · 2020-08-18T20:25:32Z

packages/flutter_tools/lib/src/macos/build_macos.dart

@@ -14,6 +14,8 @@ import '../ios/xcodeproj.dart';
 import '../project.dart';
 import 'cocoapod_utils.dart';

+final RegExp _anyOutput = RegExp('.*');


I think it's worth commenting why this is done (i.e., the interaction with -quiet) since it's non-obvious without context.

fluttergithubbot · 2020-08-18T21:41:08Z

This pull request is not suitable for automatic merging in its current state.

The status or check suite tool_tests-general-windows has failed. Please fix the issues identified (or deflake) before re-applying this label.
The status or check suite tool_tests-commands-windows has failed. Please fix the issues identified (or deflake) before re-applying this label.
The status or check suite tool_tests-integration-windows has failed. Please fix the issues identified (or deflake) before re-applying this label.
The status or check suite hostonly_devicelab_tests-0-windows has failed. Please fix the issues identified (or deflake) before re-applying this label.
The status or check suite hostonly_devicelab_tests-1-windows has failed. Please fix the issues identified (or deflake) before re-applying this label.
The status or check suite hostonly_devicelab_tests-2-windows has failed. Please fix the issues identified (or deflake) before re-applying this label.
The status or check suite hostonly_devicelab_tests-3_last-windows has failed. Please fix the issues identified (or deflake) before re-applying this label.
The status or check suite customer_testing-windows has failed. Please fix the issues identified (or deflake) before re-applying this label.

…des (flutter#63815)

[flutter_tools] correctly forward error only stdout in non-verbose modes

ee5ee68

flutter-dashboard bot added team Infra upgrades, team productivity, code health, technical debt. See also team: labels. tool Affects the "flutter" command-line tool. See also t: labels. work in progress; do not review labels Aug 14, 2020

googlebot added the cla: yes label Aug 14, 2020

jonahwilliams removed the work in progress; do not review label Aug 14, 2020

jonahwilliams marked this pull request as ready for review August 14, 2020 22:21

jonahwilliams commented Aug 14, 2020

View reviewed changes

jonahwilliams requested review from stuartmorgan and jmagman August 14, 2020 22:25

jmagman reviewed Aug 14, 2020

View reviewed changes

jmagman reviewed Aug 15, 2020

View reviewed changes

jmagman approved these changes Aug 15, 2020

View reviewed changes

stuartmorgan reviewed Aug 15, 2020

View reviewed changes

jonahwilliams added 3 commits August 17, 2020 14:07

Merge branch 'master' of github.com:flutter/flutter into fix_exit_ear…

6518183

…ly_macos

Address comments

24bb68b

fix unit test

522dd4c

jonahwilliams requested a review from stuartmorgan August 17, 2020 23:30

stuartmorgan approved these changes Aug 18, 2020

View reviewed changes

Update build_macos.dart

ae658e8

jonahwilliams added the waiting for tree to go green label Aug 18, 2020

fluttergithubbot removed the waiting for tree to go green label Aug 18, 2020

jonahwilliams added the waiting for tree to go green label Aug 18, 2020

fluttergithubbot merged commit 39c735f into flutter:master Aug 18, 2020

jonahwilliams deleted the fix_exit_early_macos branch August 18, 2020 23:56

stuartmorgan mentioned this pull request Aug 20, 2020

MacOS builds can swallow errors in non-verbose mode #33582

Closed

DanTup mentioned this pull request Aug 26, 2020

Can not show any error message when Xcode build script return nonzero Dart-Code/Dart-Code#2741

Closed

smadey pushed a commit to smadey/flutter that referenced this pull request Aug 27, 2020

[flutter_tools] correctly forward error only stdout in non-verbose mo…

ecd853f

…des (flutter#63815)

mingwandroid pushed a commit to mingwandroid/flutter that referenced this pull request Sep 6, 2020

[flutter_tools] correctly forward error only stdout in non-verbose mo…

4d6a596

…des (flutter#63815)

jmagman mentioned this pull request Mar 17, 2021

[mac] Failed to initialize Flutter engine: error 2 #72608

Closed

[flutter_tools] correctly forward error only stdout in non-verbose modes #63815

[flutter_tools] correctly forward error only stdout in non-verbose modes #63815

Conversation

jonahwilliams commented Aug 14, 2020 • edited Loading

Description

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmagman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonahwilliams commented Aug 18, 2020

stuartmorgan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fluttergithubbot commented Aug 18, 2020

jonahwilliams commented Aug 14, 2020 •

edited

Loading