refactor depfile usage and update linux rule #42487

jonahwilliams · 2019-10-11T00:37:32Z

Description

Pulls the logic for depfile reading and writing into a single class. Uses it to make the linux unpack command smarter.
Updates BuildSystem to not consider missing inputs a fatal error, instead it will rerun as normal. This allows easier usage of depfiles for fairly dynamic rules.
Remove dead code from build bundle.

Fixes #42411
Fixes #42399
Fixes #42697
Fixes #42411
Fixes #42954

packages/flutter_tools/lib/src/build_system/depfile.dart

codecov · 2019-10-11T18:34:27Z

Codecov Report

Merging #42487 into master will decrease coverage by 1.11%.
The diff coverage is 94.11%.

@@            Coverage Diff             @@
##           master   #42487      +/-   ##
==========================================
- Coverage   60.75%   59.64%   -1.12%     
==========================================
  Files         194      195       +1     
  Lines       18885    18905      +20     
==========================================
- Hits        11474    11276     -198     
- Misses       7411     7629     +218

Flag	Coverage Δ
#flutter_tool	`59.64% <94.11%> (-1.12%)`	⬇️

Impacted Files	Coverage Δ
...s/flutter_tools/lib/src/commands/build_bundle.dart	`36.53% <ø> (ø)`	⬆️
...tter_tools/lib/src/build_system/targets/linux.dart	`89.55% <100%> (+4.44%)`	⬆️
...s/flutter_tools/lib/src/tester/flutter_tester.dart	`72.44% <100%> (+0.28%)`	⬆️
...utter_tools/lib/src/build_system/build_system.dart	`89.28% <83.33%> (+0.19%)`	⬆️
packages/flutter_tools/lib/src/bundle.dart	`85.45% <88.88%> (+4.13%)`	⬆️
...es/flutter_tools/lib/src/build_system/depfile.dart	`93.75% <93.75%> (ø)`
...ages/flutter_tools/lib/src/commands/build_web.dart	`29.41% <0%> (-64.71%)`	⬇️
packages/flutter_tools/lib/src/web/compile.dart	`10.34% <0%> (-62.07%)`	⬇️
packages/flutter_tools/lib/src/ios/simulators.dart	`3.26% <0%> (-47.29%)`	⬇️
packages/flutter_tools/lib/src/base/flags.dart	`38.88% <0%> (-11.12%)`	⬇️
... and 17 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 134ac42...d1f1be6. Read the comment docs.

…_directories

jonahwilliams · 2019-10-12T20:38:10Z

packages/flutter_tools/lib/src/bundle.dart

-    if (assets == null) {
-      throwToolExit('Error building assets', exitCode: 1);
+    // Work around for flutter_tester placing kernel artifacts in odd places.
+    if (applicationKernelFilePath != null) {


flutter_tester is currently the only use of this logic. In the long term, we should clean this up to use a tempDir or something similar.

Is there an issue open for this?

jonahwilliams · 2019-10-14T18:22:28Z

This change will also allow us to output a depfile for dart2js, which should cut down on memory usage.

jmagman · 2019-10-14T20:09:24Z

packages/flutter_tools/lib/src/build_system/targets/linux.dart

+          environment.projectDir.path,
+          'linux',
+          'flutter',
+          'ephemeral',


Can we stick

s.path.join( environment.projectDir.path, 'linux', 'flutter', 'ephemeral')

into a local?

jmagman · 2019-10-14T20:11:05Z

packages/flutter_tools/lib/src/build_system/targets/linux.dart

    }
+    final Depfile depfile = Depfile(inputs, outputs);
+    depfile.writeToFile(environment.buildDir.childFile('linux_engine_sources.d'));


Does it matter if the dep file is re-written on every build, even if nothing changes?

No, we don't stat/diff the bytes of the depfile at all, we only read the contents. Changing the order/rewriting shouldn't trigger rebuilds.

jmagman · 2019-10-14T20:29:22Z

packages/flutter_tools/lib/src/build_system/depfile.dart

+  /// Parse the depfile contents from [file].
+  ///
+  /// If the syntax is invalid, returns an empty [Depfile].
+  factory Depfile.parse(File file) {


This looks untested.

It's tested indirectly via integration tests, but I've added some unit tests to cover it

…_directories

zanderso · 2019-10-16T17:35:38Z

packages/flutter_tools/lib/src/build_system/build_system.dart

@@ -772,9 +772,12 @@ class Node {
      }
    }

-    // If we depend on a file that doesnt exist on disk, kill the build.
+    // If we depend on a file that doesnt exist on disk, mark the build as


What happens if the user listed a file by accident, and the fix is to remove the file? How do they find out?

The user here would be the tools team, so ideally we'd have a test to cover it. Long term, I think we should surface the reasons why targets are re-run.

Would it be useful to do a printTrace here with some diagnostic info?

From the description, what is an example of a fairly dynamic rule where a required file doesn't exist at the moment of running the target?

Anything using a depfile, which will be lots of rules

blasten · 2019-10-16T20:52:46Z

packages/flutter_tools/lib/src/build_system/build_system.dart

    if (missingInputs.isNotEmpty) {
-      throw MissingInputException(missingInputs, target.name);
+      _dirty = true;
+      invalidatedReasons.add(InvalidedReason.inputChanged);


does inputChanged mean that a required file is missing? The comment on this file says: An input file has an updated hash..

Added InvalidatedReason.inputMissing with a description

…lutter into match_unpack_directories

blasten · 2019-10-16T21:05:48Z

packages/flutter_tools/lib/src/build_system/depfile.dart

+    final List<String> colonSeparated = contents.split(': ');
+    if (colonSeparated.length != 2) {
+      printError('Invalid depfile: ${file.path}');
+      return const Depfile(<File>[], <File>[]);


In which instance returning an empty Depfile makes the build successful later?

In the case where (for some reason, human intervention) an invalid depfile is written or edited - we don't force the user to flutter clean, instead we allow a new one be written which hopefully doesn't have the same issue

blasten · 2019-10-16T21:09:41Z

packages/flutter_tools/lib/src/build_system/depfile.dart

+    // Expand escape sequences, so that '\ ', for example,ß becomes ' '
+        .map<String>((String path) => path.replaceAllMapped(_escapeExpr, (Match match) => match.group(1)).trim())
+        .where((String path) => path.isNotEmpty)
+        .toSet()


are there duplicated items in the list? If so, would you mind added a comment?

Also added a test case for this

blasten · 2019-10-16T21:15:13Z

packages/flutter_tools/lib/src/build_system/targets/linux.dart

+        final String outputPath = fs.path.join(
+          outputPrefix,
+          fs.path.relative(input.path, from: basePath),
+        );
+        final File destinationFile = fs.file(outputPath);
+        if (!destinationFile.parent.existsSync()) {
+          destinationFile.parent.createSync(recursive: true);
+        }
+        final File inputFile = fs.file(input);
+        inputFile.copySync(destinationFile.path);
+        inputs.add(inputFile);
+        outputs.add(destinationFile);


blasten · 2019-10-16T21:15:43Z

packages/flutter_tools/lib/src/build_system/targets/linux.dart

+      if (fs.isFileSync(entityPath)) {
+        final String outputPath = fs.path.join(
+          outputPrefix,
+          fs.path.relative(entityPath, from: basePath),
+        );
+        final File destinationFile = fs.file(outputPath);
+        if (!destinationFile.parent.existsSync()) {
+          destinationFile.parent.createSync(recursive: true);
+        }
+        final File inputFile = fs.file(entityPath);
+        inputFile.copySync(destinationFile.path);
+        inputs.add(inputFile);
+        outputs.add(destinationFile);
+        continue;


nit: this code might need some comments.

I tried to add some more context here, PTAL

zanderso · 2019-10-17T16:26:25Z

packages/flutter_tools/lib/src/bundle.dart

-    if (assets == null) {
-      throwToolExit('Error building assets', exitCode: 1);
+    // Work around for flutter_tester placing kernel artifacts in odd places.
+    if (applicationKernelFilePath != null) {


Is there an issue open for this?

zanderso · 2019-10-17T16:41:56Z

packages/flutter_tools/test/general.shard/build_system/depfile_test.dart

+import '../../src/common.dart';
+import '../../src/testbed.dart';
+
+void main() {


Some negative tests here would be good.

Added test cases for malformed file, duplicates, and whitespace

jonahwilliams · 2019-10-17T20:58:09Z

Filled #42957 for the tester issue

blasten · 2019-10-17T21:42:27Z

packages/flutter_tools/lib/src/build_system/depfile.dart

+    // Put every file on right-hand side on the separate line
+        .replaceAllMapped(_separatorExpr, (Match match) => '${match.group(1)}\n')
+        .split('\n')
+    // Expand escape sequences, so that '\ ', for example,ß becomes ' '


nit: is the ß intentional?

blasten · 2019-10-17T21:43:20Z

packages/flutter_tools/lib/src/build_system/targets/linux.dart

+        outputs.add(destinationFile);
+        continue;
+      }
+      // If ths artifact is the directory cpp_client_wrapper, recursively


devorso · 2019-10-18T10:14:53Z

I have now this error:

> Compiler message:
/E:/flutter/.pub-cache/hosted/pub.dartlang.org/circular_profile_avatar-1.0.2/lib/circular_profile_avatar.dart:3:8: Error: Error when reading '/E:/flutter/.pub-cache/hosted/pub.dartlang.org/cached_network_image-1.1.2+1/lib/cached_network_image.dart': Le chemin d’accès spécifié est introuvable.

import 'package:cached_network_image/cached_network_image.dart';
^
/E:/flutter/.pub-cache/hosted/pub.dartlang.org/circular_profile_avatar-1.0.2/lib/circular_profile_avatar.dart:70:9: Error: Type 'PlaceholderWidgetBuilder' not found.
final PlaceholderWidgetBuilder placeHolder;
^^^^^^^^^^^^^^^^^^^^^^^^
/E:/flutter/.pub-cache/hosted/pub.dartlang.org/circular_profile_avatar-1.0.2/lib/circular_profile_avatar.dart:73:9: Error: Type 'LoadingErrorWidgetBuilder' not found.
final LoadingErrorWidgetBuilder errorWidget;
^^^^^^^^^^^^^^^^^^^^^^^^^
/E:/flutter/.pub-cache/hosted/pub.dartlang.org/circular_profile_avatar-1.0.2/lib/circular_profile_avatar.dart:76:9: Error: Type 'ImageWidgetBuilder' not found.
final ImageWidgetBuilder imageBuilder;
^^^^^^^^^^^^^^^^^^
/E:/flutter/.pub-cache/hosted/pub.dartlang.org/circular_profile_avatar-1.0.2/lib/circular_profile_avatar.dart:70:9: Error: 'PlaceholderWidgetBuilder' isn't a type.
final PlaceholderWidgetBuilder placeHolder;
^^^^^^^^^^^^^^^^^^^^^^^^
/E:/flutter/.pub-cache/hosted/pub.dartlang.org/circular_profile_avatar-1.0.2/lib/circular_profile_avatar.dart:73:9: Error: 'LoadingErrorWidgetBuilder' isn't a type.
final LoadingErrorWidgetBuilder errorWidget;
^^^^^^^^^^^^^^^^^^^^^^^^^
/E:/flutter/.pub-cache/hosted/pub.dartlang.org/circular_profile_avatar-1.0.2/lib/circular_profile_avatar.dart:76:9: Error: 'ImageWidgetBuilder' isn't a type.
final ImageWidgetBuilder imageBuilder;
^^^^^^^^^^^^^^^^^^
/E:/flutter/.pub-cache/hosted/pub.dartlang.org/circular_profile_avatar-1.0.2/lib/circular_profile_avatar.dart:140:20: Error: The method 'CachedNetworkImage' isn't defined for the class '_CircularProfileAvatarState'.

'_CircularProfileAvatarState' is from 'package:circular_profile_avatar/circular_profile_avatar.dart' ('/E:/flutter/.pub-cache/hosted/pub.dartlang.org/circular_profile_avatar-1.0.2/lib/circular_profile_avatar.dart').
Try correcting the name to the name of an existing method, or defining a method named 'CachedNetworkImage'.
child: CachedNetworkImage(
^^^^^^^^^^^^^^^^^^
Exception: Errors during snapshot creation: null
#0 KernelSnapshot.build (package:flutter_tools/src/build_system/targets/dart.dart:225:7)
#1 _BuildInstance._invokeInternal (package:flutter_tools/src/build_system/build_system.dart:526:25) #2 _BuildInstance.invokeTarget. (package:flutter_tools/src/build_system/build_system.dart:481:35) #3 new Future.sync (dart:async/future.dart:224:31) #4 AsyncMemoizer.runOnce (package:async/src/async_memoizer.dart:43:45) #5 _BuildInstance.invokeTarget (package:flutter_tools/src/build_system/build_system.dart:481:21) #6 BuildSystem.build (package:flutter_tools/src/build_system/build_system.dart:419:36) #7 _AsyncAwaitCompleter.start (dart:async-patch/async_patch.dart:43:6) #8 BuildSystem.build (package:flutter_tools/src/build_system/build_system.dart:400:28) #9 buildWithAssemble (package:flutter_tools/src/bundle.dart:122:48) #10 _AsyncAwaitCompleter.start (dart:async-patch/async_patch.dart:43:6) #11 buildWithAssemble (package:flutter_tools/src/bundle.dart:98:31) #12 BundleBuilder.build (package:flutter_tools/src/bundle.dart:75:11) #13 _AsyncAwaitCompleter.start (dart:async-patch/async_patch.dart:43:6) #14 BundleBuilder.build (package:flutter_tools/src/bundle.dart:52:21) #15 BuildBundleCommand.runCommand (package:flutter_tools/src/commands/build_bundle.dart:126:25) #16 _AsyncAwaitCompleter.start (dart:async-patch/async_patch.dart:43:6) #17 BuildBundleCommand.runCommand (package:flutter_tools/src/commands/build_bundle.dart:97:42) #18 FlutterCommand.verifyThenRunCommand (package:flutter_tools/src/runner/flutter_command.dart:557:18) #19 _asyncThenWrapperHelper. (dart:async-patch/async_patch.dart:71:64) #20 _rootRunUnary (dart:async/zone.dart:1132:38) #21 _CustomZone.runUnary (dart:async/zone.dart:1029:19) #22 _FutureListener.handleValue (dart:async/future_impl.dart:137:18) #23 Future._propagateToListeners.handleValueCallback (dart:async/future_impl.dart:678:45) #24 Future._propagateToListeners (dart:async/future_impl.dart:707:32) #25 Future._completeWithValue (dart:async/future_impl.dart:522:5) #26 Future._asyncComplete. (dart:async/future_impl.dart:552:7) #27 _rootRun (dart:async/zone.dart:1124:13) #28 _CustomZone.run (dart:async/zone.dart:1021:19) #29 _CustomZone.runGuarded (dart:async/zone.dart:923:7) #30 _CustomZone.bindCallbackGuarded. (dart:async/zone.dart:963:23) #31 _microtaskLoop (dart:async/schedule_microtask.dart:41:21) #32 _startMicrotaskLoop (dart:async/schedule_microtask.dart:50:5) #33 _runPendingImmediateCallback (dart:isolate-patch/isolate_patch.dart:116:13) #34 _RawReceivePortImpl._handleMessage (dart:isolate-patch/isolate_patch.dart:173:5)
Failed to build bundle.

FAILURE: Build failed with an exception.

Where:
Script 'E:\flutter\packages\flutter_tools\gradle\flutter.gradle' line: 794

What went wrong:
Execution failed for task ':app:compileFlutterBuildDebugArm64'.

Process 'command 'E:\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 15s
Finished with error: Gradle task assembleDebug failed with exit code 1

refactor depfile usage and update linux rule

b7ff921

fluttergithubbot added the tool Affects the "flutter" command-line tool. See also t: labels. label Oct 11, 2019

googlebot added the cla: yes label Oct 11, 2019

Update depfile.dart

7a3d061

jonahwilliams commented Oct 11, 2019

View reviewed changes

packages/flutter_tools/lib/src/build_system/depfile.dart Show resolved Hide resolved

jonahwilliams added 2 commits October 11, 2019 07:48

Cleanup of bundling and depfile

ff944fa

update tests and remove sync link

24b3080

jonahwilliams added 3 commits October 12, 2019 11:03

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

3743ca8

…_directories

add back workaround for flutter_tester device

c980580

remove commented out code

8396ee7

jonahwilliams commented Oct 12, 2019

View reviewed changes

jonahwilliams marked this pull request as ready for review October 12, 2019 20:38

jonahwilliams requested review from zanderso and jmagman October 14, 2019 14:18

jmagman reviewed Oct 14, 2019

View reviewed changes

jonahwilliams added 3 commits October 14, 2019 18:16

address comments

d20108d

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

6678376

…_directories

Update depfile_test.dart

f9ca906

jonahwilliams requested review from jmagman, blasten, dnfield and zanderso and removed request for zanderso and dnfield October 15, 2019 17:00

zanderso reviewed Oct 16, 2019

View reviewed changes

jonahwilliams requested a review from zanderso October 16, 2019 20:40

blasten reviewed Oct 16, 2019

View reviewed changes

add inputMissing invalidated reason, printTrace for missing files

672d349

jonahwilliams added 2 commits October 16, 2019 14:04

fix typo

c2517e6

Merge branch 'match_unpack_directories' of github.com:jonahwilliams/f…

3f404ac

…lutter into match_unpack_directories

blasten reviewed Oct 16, 2019

View reviewed changes

add comments

44dd365

zanderso reviewed Oct 17, 2019

View reviewed changes

add negative test cases to depfile

237363c

jonahwilliams requested a review from zanderso October 17, 2019 20:56

zanderso approved these changes Oct 17, 2019

View reviewed changes

blasten reviewed Oct 17, 2019

View reviewed changes

Update linux.dart

d1f1be6

jonahwilliams merged commit f53b32e into flutter:master Oct 18, 2019

jonahwilliams deleted the match_unpack_directories branch October 18, 2019 00:42

jonahwilliams mentioned this pull request Oct 29, 2019

Flutter build fails when project source is located on a mapped Samba Share (Network Drive Letter) #43594

Open

Inconnu08 pushed a commit to Inconnu08/flutter that referenced this pull request Nov 26, 2019

refactor depfile usage and update linux rule (flutter#42487)

80da51a

github-actions bot locked as resolved and limited conversation to collaborators Aug 3, 2021

refactor depfile usage and update linux rule #42487

refactor depfile usage and update linux rule #42487

Uh oh!

Conversation

jonahwilliams commented Oct 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

Uh oh!

codecov bot commented Oct 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonahwilliams commented Oct 14, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

blasten Oct 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonahwilliams commented Oct 17, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

devorso commented Oct 18, 2019 • edited by BondarenkoStas Loading Uh oh! There was an error while loading. Please reload this page.

jonahwilliams commented Oct 11, 2019 •

edited

Loading

codecov bot commented Oct 11, 2019 •

edited

Loading

blasten Oct 16, 2019 •

edited

Loading

devorso commented Oct 18, 2019 •

edited by BondarenkoStas

Loading