Ensure snapshot rebuild logic takes main path into account #11924

mravn-google · 2017-09-04T09:34:38Z

This is a retake of #11730 which was intercepted by #11820. The latter partially fixes #11400.

Fixes the remaining part of flutter run lib/foo.dart always runs the first thing built #11400. Switch of main entry point would still be ignored, with no new snapshot built, if we happen to switch between two main entry points that are both snapshot dependencies independently of whether they are selected as main entry point. This happens e.g. if the two main entry points import each other. This is fixed by including the main entry point path along with build mode and target platform in the checksum file.
Refactors Checksum into Fingerprint to reflect that the class no longer just handles file checksums, but also build properties.
Separates the general notion of a build Fingerprint from the Snapshotter's knowledge of actual build properties to save.
Prevents the creation of a SnapshotType with null build mode, to avoid having to check for this situation down-stream.

Hixie · 2017-09-05T16:24:49Z

Do you include the identity of the engine as part of this? One thing I've noticed in the past is that if I switch between using --local-engine and not using it, we don't rebuild. Or if I use --local-engine then rebuild the engine, we don't rebuild.

mravn-google · 2017-09-05T20:06:31Z

Intercepted by #11913. I'll stop this race.

cbracken · 2017-09-05T20:20:31Z

Sorry about that! Only spotted #11730 on Friday when Hixie CC'ed me on it, but by that time the merge conflicts were pretty nasty-looking. Should have CC'ed you on #11913 when I sent it out on Friday, but it slipped my mind -- sorry about that!

I really like the idea of renaming Checksum -> Fingerprint and using a map for the additional properties. Probably worth sending that bit out if nothing else. I'm also happy to pick up that bit of cleanup if you've got other things on your plate.

mravn-google · 2017-09-05T20:42:53Z

@cbracken No worries, I just didn't want to keep racing :-). If you are done working on this, I can do the refactoring and address Ian's comment about the engine too. Just let me know.

cbracken · 2017-09-05T20:50:46Z

I'm done for the time being. Refactor + local engine handling would be fantastic. I'm going to be working on disentangling all the AOT variants from build_aot.dart and slowly swinging them over out of the one big build method + adding tests. Thanks again!

cbracken · 2017-09-07T05:27:51Z

packages/flutter_tools/lib/src/base/build.dart


-  String toJson() => JSON.encode(<String, dynamic>{
+  String  toJson() => JSON.encode(<String, dynamic>{


nit: looks like an extra space slipped in after the return type.

cbracken · 2017-09-07T05:29:14Z

packages/flutter_tools/lib/src/base/build.dart

@@ -21,7 +21,9 @@ GenSnapshot get genSnapshot => context.putIfAbsent(GenSnapshot, () => const GenS

 /// A snapshot build configuration.
 class SnapshotType {
-  const SnapshotType(this.platform, this.mode);
+  SnapshotType(this.platform, this.mode) {


Any reason to not keep the const? (and if not, s/new/const/ in buildScriptSnapshot)

Apparently, we cannot have both const and asserts just yet. IntelliJ accepts the code below, but pub run test chokes on it.

const SnapshotType(this.platform, this.mode): assert(mode != null);

dart -v says

assert_initializer: false (Allow asserts in initializer lists.)

I believe this flag will be true by default in the not too distant future.

Ah yep - not sure how many times that's bit me now! The engine is compiled with that flag set, but we don't run the tool that way. Thanks.

Not sure about const, but simple fix for asserts support in pub run test is to set DART_VM_OPTIONS=--assert-initializer for pub run test invocation.

cbracken · 2017-09-07T05:34:16Z

packages/flutter_tools/test/base/build_test.dart

+          'targetPlatform': 'TargetPlatform.android_x64',
+          'entryPoint': 'a.dart',
+        }, <String>['a.dart', 'b.dart']));
+      }, overrides: <Type, Generator>{ FileSystem: () => fs});


nit: s/fs}/fs }/ at the end

cbracken · 2017-09-07T05:35:07Z

packages/flutter_tools/test/base/build_test.dart

@@ -434,10 +423,13 @@ void main() {
      await fs.file('other.dart').writeAsString('import "main.dart";\nvoid main() {}');
      await fs.file('output.snapshot').create();
      await fs.file('output.snapshot.d').writeAsString('output.snapshot : main.dart');
-      await fs.file('output.snapshot.d.checksums').writeAsString(JSON.encode(<String, dynamic>{
-        'version': '$kVersion',


Oops! Thanks for fixing!

cbracken · 2017-09-07T05:36:38Z

Thanks!

mravn-google

Thanks for the review!

mravn-google · 2017-09-07T07:51:04Z

packages/flutter_tools/lib/src/base/build.dart

@@ -21,7 +21,9 @@ GenSnapshot get genSnapshot => context.putIfAbsent(GenSnapshot, () => const GenS

 /// A snapshot build configuration.
 class SnapshotType {
-  const SnapshotType(this.platform, this.mode);
+  SnapshotType(this.platform, this.mode) {


Apparently, we cannot have both const and asserts just yet. IntelliJ accepts the code below, but pub run test chokes on it.

const SnapshotType(this.platform, this.mode): assert(mode != null);

dart -v says

assert_initializer: false (Allow asserts in initializer lists.)

I believe this flag will be true by default in the not too distant future.

mravn-google · 2017-09-07T07:53:12Z

packages/flutter_tools/lib/src/base/build.dart


-  String toJson() => JSON.encode(<String, dynamic>{
+  String  toJson() => JSON.encode(<String, dynamic>{


mravn-google · 2017-09-07T07:53:26Z

packages/flutter_tools/test/base/build_test.dart

+          'targetPlatform': 'TargetPlatform.android_x64',
+          'entryPoint': 'a.dart',
+        }, <String>['a.dart', 'b.dart']));
+      }, overrides: <Type, Generator>{ FileSystem: () => fs});


cbracken · 2017-09-07T15:19:23Z

Thanks again!

Hixie · 2017-09-07T17:46:40Z

We control the VM entrypoint (it's in the bash script) so it should be a trivial thing to just pass the flag to enable const asserts and anything else we want.

googlebot added the cla: yes label Sep 4, 2017

mravn-google mentioned this pull request Sep 4, 2017

Made snapshot rebuild logic take target into account #11730

Closed

mravn-google requested a review from cbracken September 4, 2017 09:36

mravn-google mentioned this pull request Sep 4, 2017

flutter run lib/foo.dart always runs the first thing built #11400

Closed

mravn-google closed this Sep 5, 2017

mravn-google reopened this Sep 5, 2017

mravn-google force-pushed the launch_wrong_target2 branch from c7238e2 to 1a1974a Compare September 6, 2017 12:26

cbracken approved these changes Sep 7, 2017

View reviewed changes

mravn-google commented Sep 7, 2017

View reviewed changes

mravn-google added 5 commits September 7, 2017 12:11

Refactor to Fingerprint

04ae188

Merged with repo changes

416c487

Merge cleanup

783c597

Post-merge cleanup

b47ac43

Review comments

8eb0a49

mravn-google force-pushed the launch_wrong_target2 branch from 51bc0b7 to 8eb0a49 Compare September 7, 2017 10:12

mravn-google merged commit 1401652 into flutter:master Sep 7, 2017

mravn-google mentioned this pull request Sep 7, 2017

Snapshot rebuild logic does not take change of (local) engine into account #11977

Closed

mravn-google deleted the launch_wrong_target2 branch December 18, 2017 21:29

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensure snapshot rebuild logic takes main path into account #11924

Ensure snapshot rebuild logic takes main path into account #11924

mravn-google commented Sep 4, 2017 •

edited

Loading

Hixie commented Sep 5, 2017

mravn-google commented Sep 5, 2017

cbracken commented Sep 5, 2017 •

edited

Loading

mravn-google commented Sep 5, 2017

cbracken commented Sep 5, 2017 •

edited

Loading

cbracken Sep 7, 2017

mravn-google Sep 7, 2017

cbracken Sep 7, 2017

mravn-google Sep 7, 2017 •

edited

Loading

cbracken Sep 7, 2017

aam Sep 7, 2017

cbracken Sep 7, 2017

mravn-google Sep 7, 2017

cbracken Sep 7, 2017

cbracken commented Sep 7, 2017

mravn-google left a comment

mravn-google Sep 7, 2017 •

edited

Loading

mravn-google Sep 7, 2017

mravn-google Sep 7, 2017

cbracken commented Sep 7, 2017

Hixie commented Sep 7, 2017


		String toJson() => JSON.encode(<String, dynamic>{
		String toJson() => JSON.encode(<String, dynamic>{

Ensure snapshot rebuild logic takes main path into account #11924

Ensure snapshot rebuild logic takes main path into account #11924

Conversation

mravn-google commented Sep 4, 2017 • edited Loading

Hixie commented Sep 5, 2017

mravn-google commented Sep 5, 2017

cbracken commented Sep 5, 2017 • edited Loading

mravn-google commented Sep 5, 2017

cbracken commented Sep 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mravn-google Sep 7, 2017 • edited Loading

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

cbracken commented Sep 7, 2017

mravn-google left a comment

Choose a reason for hiding this comment

mravn-google Sep 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cbracken commented Sep 7, 2017

Hixie commented Sep 7, 2017

mravn-google commented Sep 4, 2017 •

edited

Loading

cbracken commented Sep 5, 2017 •

edited

Loading

cbracken commented Sep 5, 2017 •

edited

Loading

mravn-google Sep 7, 2017 •

edited

Loading

mravn-google Sep 7, 2017 •

edited

Loading