Skip to content

[flutter_tools] Allow the tool to suppress compilation errors.#58539

Merged
jonahwilliams merged 6 commits intoflutter:masterfrom
jonahwilliams:fix_double_compiler_error
Jun 3, 2020
Merged

[flutter_tools] Allow the tool to suppress compilation errors.#58539
jonahwilliams merged 6 commits intoflutter:masterfrom
jonahwilliams:fix_double_compiler_error

Conversation

@jonahwilliams
Copy link
Contributor

@jonahwilliams jonahwilliams commented Jun 3, 2020 •
edited
Loading

Description

suppress compilation errors on startup to make the most egregious parts of #58337 better

Specifically:

what I think is happening here is that the tool initializes the frontend_server for the hot reload cycle concurrent with whatever native build step is running (Gradle/Xcode). This speeds up the initialization of flutter run a decent amount, about 5 - 10 seconds depending on how "dirty" the build is. Something I didn't think about when doing this was errors! Now we're getting them twice whenever there is a problem with Dart code.

@fluttergithubbot fluttergithubbot added the tool Affects the "flutter" command-line tool. See also t: labels. label Jun 3, 2020
@jonahwilliams
Copy link
Contributor Author

jonahwilliams commented Jun 3, 2020

This also removes some of the extraneous output and newlines to simplify things a bit. For example, a compile error on startup (android) now looks like:

Launching lib\main.dart on Pixel 4 in debug mode...
lib/main.dart:12:11: Error: Expected ',' before this.
          key: Key('title'),
          ^^^
lib/main.dart:11:13: Error: Too many positional arguments: 1 allowed, but 2 found.
Try removing the extra positional arguments.
        Text('Hello, world!',2
            ^
../../packages/flutter/lib/src/widgets/text.dart:282:9: Context: Found this candidate, but the arguments don't match.
  const Text(
        ^^^^


FAILURE: Build failed with an exception.

* Where:
Script 'C:\Users\Jonah\flutter\packages\flutter_tools\gradle\flutter.gradle' line: 896

* What went wrong:
Execution failed for task ':app:compileFlutterBuildDebug'.
> Process 'command 'C:\Users\Jonah\flutter\bin\flutter.bat'' finished with non-zero exit value 1

* Try:
Run with --stacktrace option to get the stack trace. Run with --info or --debug option to get more log output. Run with --scan to get full insights.

* Get more help at https://help.gradle.org

BUILD FAILED in 4s
Running Gradle task 'assembleDebug'...
Running Gradle task 'assembleDebug'... Done                         4.7s
Exception: Gradle task assembleDebug failed with exit code 1

In a separate PR, we could chop off the footer (seems fairly stable) when the error is app:compileFlutterBuildDebug or another flutter-related task and replace it with a better error message. (i.e. verbose instead of stack-trace

jonahwilliams
jonahwilliams commented Jun 3, 2020
View reviewed changes
packages/flutter_tools/lib/src/build_system/targets/dart.dart
);
if (output == null || output.errorCount != 0) {
throw Exception('Errors during snapshot creation: $output');
throw Exception();
Copy link
Contributor Author

@jonahwilliams jonahwilliams Jun 3, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This added nothing

jonahwilliams
jonahwilliams commented Jun 3, 2020
View reviewed changes
packages/flutter_tools/lib/src/commands/assemble.dart
? measurement.stackTrace
: null,
);
if (measurement.fatal || globals.logger.isVerbose) {
Copy link
Contributor Author

@jonahwilliams jonahwilliams Jun 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only show for verbose errors, actual bugs will get logged to stderr

jonahwilliams
jonahwilliams commented Jun 3, 2020
View reviewed changes
packages/flutter_tools/lib/src/compile.dart
// Invalid state, see commented issue below for more information.
// NB: both the completeError and _badState flags are required to avoid
// filling the console with exceptions.
if (boundaryKey == null) {
Copy link
Contributor Author

@jonahwilliams jonahwilliams Jun 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was fixed several months ago by @aam

Copy link
Contributor Author

@jonahwilliams jonahwilliams Jun 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#35924

jonahwilliams
jonahwilliams commented Jun 3, 2020
View reviewed changes
packages/flutter_tools/test/general.shard/resident_runner_test.dart
<FlutterDevice>[
mockFlutterDevice,
],
applicationBinary: globals.fs.file('app.apk'),
Copy link
Contributor Author

@jonahwilliams jonahwilliams Jun 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When there is an applicationBinary then there was no native build step, so we need to show the flutter run errors as normal

@jonahwilliams
Copy link
Contributor Author

jonahwilliams commented Jun 3, 2020

The xcode output is:

Running Xcode build...                                                  
                                                   
Xcode build done.                                            5.6s
Failed to build iOS app
Error output from Xcode build:
↳
    ** BUILD FAILED **


Xcode's output:
↳
    lib/main.dart:16:5: Error: Expected ';' after this.
        )
        ^
    lib/main.dart:17:1: Error: Unexpected token ''.

    Command PhaseScriptExecution failed with a nonzero exit code
    note: Using new build systemnote: Planning buildnote: Constructing build description

Could not build the application for the simulator.
Error launching application on iPhone 11 Pro Max.

Which is a bit better

@jonahwilliams jonahwilliams requested a review from jmagman June 3, 2020 02:53
jmagman
jmagman reviewed Jun 3, 2020
View reviewed changes
packages/flutter_tools/lib/src/codegen.dart
List<Uri> invalidatedFiles, {
String outputPath,
PackageConfig packageConfig,
bool suppressErrors = false,
Copy link
Member

@jmagman jmagman Jun 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a comment for when you would want to suppress errors? Doesn't sound like something you would generally want to do.

Copy link
Contributor Author

@jonahwilliams jonahwilliams Jun 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done here: https://github.com/flutter/flutter/pull/58539/files#diff-e78651ad5bd5c46811f2b76bc465220bR375

Copy link
Member

@jmagman jmagman Jun 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this PR is the crumb trail for the signature as to why you would want to suppress errors (so they aren't displayed twice in certain scenarios).

jmagman
jmagman approved these changes Jun 3, 2020
View reviewed changes
Copy link
Member

@jmagman jmagman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jonahwilliams jonahwilliams merged commit e216eec into flutter:master Jun 3, 2020
@jonahwilliams jonahwilliams deleted the fix_double_compiler_error branch June 3, 2020 19:00
@Hixie
Copy link
Contributor

Hixie commented Jun 6, 2020

We should consider having end-to-end tests that verify the entire text of common scenarios like error messages, startup, etc, to make sure we don't accidentally regress the actual output.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 31, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@jmagman jmagman jmagman approved these changes

Assignees

No one assigned

Labels

tool Affects the "flutter" command-line tool. See also t: labels.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@jonahwilliams @Hixie @jmagman @googlebot @fluttergithubbot