regenerate `generated_main` if deps changed in hot reload/restart#77959

Closed

cyanglaz wants to merge 10 commits intoflutter:masterfrom

cyanglaz:dart_restraint_hot_reload

Contributor

cyanglaz commented Mar 11, 2021 •

edited

Loading

When dependencies are changed in the middle of run, hot reload and hot restart should update generated_main accordingly.

Fixes #76219

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
I signed the CLA.
I listed at least one issue that this PR fixes in the description above.
I updated/added relevant documentation (doc comments with ///).
I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

flutter-dashboard bot commented Mar 11, 2021

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat.

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

flutter-dashboard bot added the tool label

google-cla bot added the cla: yes label

cyanglaz marked this pull request as draft

March 11, 2021 20:30

jonahwilliams reviewed

View reviewed changes

packages/flutter_tools/lib/src/build_system/targets/dart_plugin_registraint.dart Outdated

Contributor

jonahwilliams Mar 11, 2021

nit: prefer relative imports for flutter_tools

jonahwilliams reviewed

View reviewed changes

packages/flutter_tools/lib/src/build_system/targets/dart_plugin_registraint.dart Outdated

Contributor

jonahwilliams Mar 11, 2021

nit: doc comment describing what this is for.

jonahwilliams reviewed

View reviewed changes

packages/flutter_tools/lib/src/build_system/targets/dart_plugin_registraint.dart Outdated

Contributor

jonahwilliams Mar 11, 2021

This should be read from the environment configuration - see the KernelSnapshot target for an example, think it is called target or something like that

jonahwilliams reviewed

View reviewed changes

packages/flutter_tools/lib/src/build_system/targets/dart_plugin_registraint.dart Outdated

Contributor

jonahwilliams Mar 11, 2021

I think you should only skip if generateDartPluginRegistry is false

jonahwilliams reviewed

View reviewed changes

packages/flutter_tools/lib/src/build_system/targets/dart_plugin_registraint.dart Outdated

Contributor

jonahwilliams Mar 11, 2021

ahh, smart :D


          Always regenerate dartPluginRestrant

8b62bbd

cyanglaz force-pushed the dart_restraint_hot_reload branch from 5c8472f to 8b62bbd Compare

March 12, 2021 01:16

Chris Yang added 2 commits

March 11, 2021 17:26


          fix typo in name

56639bf


          tests

2c823cc

cyanglaz force-pushed the dart_restraint_hot_reload branch from 843ffba to 2c823cc Compare

March 15, 2021 21:57

cyanglaz marked this pull request as ready for review

March 15, 2021 21:58

cyanglaz requested a review from jonahwilliams

March 15, 2021 21:58

cyanglaz commented

View reviewed changes

packages/flutter_tools/lib/src/build_system/targets/dart_plugin_registrant.dart Outdated

+                /// Construct a [DartPluginRegistrantTarget].
+                ///
+                /// If `project` is unset, a [FlutterProject] based on environment is used.
+                DartPluginRegistrantTarget({FlutterProject project}) {

Contributor Author

cyanglaz Mar 15, 2021 •

edited

Loading

the project param is mainly configurable for testing. In tests, i needed a FlutterProject.fromDirectoryTest

If there are better solution to this, please let me know.

Contributor

jonahwilliams Mar 15, 2021

You can add a @VisibleForTesting annotation on it if it isn't meant to be normally used

Contributor

jonahwilliams commented Mar 15, 2021

Analyzing 3 directories...                                      

   info • Sort directive sections alphabetically • packages/flutter_tools/lib/src/build_system/targets/dart_plugin_registrant.dart:10:1 • directives_ordering
   info • Unused import: 'package:flutter_tools/src/build_system/targets/localizations.dart' • packages/flutter_tools/test/general.shard/build_system/targets/dart_plugin_registrant_test.dart:12:8 • unused_import
   info • Unused import: 'package:flutter_tools/src/localizations/gen_l10n.dart' • packages/flutter_tools/test/general.shard/build_system/targets/dart_plugin_registrant_test.dart:13:8 • unused_import
   info • Unused import: 'package:flutter_tools/src/localizations/localizations_utils.dart' • packages/flutter_tools/test/general.shard/build_system/targets/dart_plugin_registrant_test.dart:14:8 • unused_import
   info • Unused import: 'package:flutter_tools/src/plugins.dart' • packages/flutter_tools/test/general.shard/build_system/targets/dart_plugin_registrant_test.dart:15:8 • unused_import
   info • Unused import: 'package:mockito/mockito.dart' • packages/flutter_tools/test/general.shard/build_system/targets/dart_plugin_registrant_test.dart:17:8 • unused_import

jonahwilliams reviewed

View reviewed changes

packages/flutter_tools/lib/src/build_system/targets/dart_plugin_registrant.dart Outdated

+                ///
+                /// If `project` is unset, a [FlutterProject] based on environment is used.
+                DartPluginRegistrantTarget({FlutterProject project}) {
+                  _project = project;

Contributor

jonahwilliams Mar 15, 2021

do this in an initializer and make it final

jonahwilliams reviewed

View reviewed changes

packages/flutter_tools/lib/src/build_system/targets/dart_plugin_registrant.dart Outdated

+                /// Construct a [DartPluginRegistrantTarget].
+                ///
+                /// If `project` is unset, a [FlutterProject] based on environment is used.
+                DartPluginRegistrantTarget({FlutterProject project}) {

Contributor

jonahwilliams Mar 15, 2021

this constructor should be const

cyanglaz changed the title ~~[wip] regenerate generated_main if deps changed in hot reload/restart~~ regenerate generated_main if deps changed in hot reload/restart

cyanglaz added 2 commits

March 18, 2021 09:29


          review

8bd1f67


          remove extra imports

4ffb974

cyanglaz commented

View reviewed changes

Contributor Author

cyanglaz left a comment

@jonahwilliams Fixed review comments. PTAL

cyanglaz added 2 commits

March 18, 2021 09:40


          Merge branch 'master' into dart_restraint_hot_reload

d4a494e


          fix windows tests

280989a

cyanglaz force-pushed the dart_restraint_hot_reload branch from aa6787d to 280989a Compare

March 22, 2021 18:50

cyanglaz added 3 commits

March 22, 2021 12:28


          remove debugging logs

d4b4a77

fix

ac735d9


          Merge branch 'master' into dart_restraint_hot_reload

0ac1fe5

cyanglaz requested a review from jonahwilliams

March 23, 2021 16:20

Contributor

jonahwilliams commented Mar 25, 2021

Holding off on landing this until we re-visting the dart plugin registrant

cyanglaz requested a review from blasten

March 29, 2021 16:00

jonahwilliams marked this pull request as draft

April 1, 2021 20:34

blasten reviewed

View reviewed changes

packages/flutter_tools/lib/src/compile.dart

                     // so the resident compiler can find it.
                     final File newMainDart = buildDir.parent.childFile('generated_main.dart');
-                    if (await generateMainDartWithPluginRegistrant(
+                    await generateMainDartWithPluginRegistrant(

blasten Apr 2, 2021

I thought this would now be handled by the target DartPluginRegistrantTarget. no?

Contributor Author

cyanglaz Apr 5, 2021

I thought DartPluginRegistrantTarget only happens in hot reload not compile. At least that's the behavior in my manual testing.

@jonahwilliams Is there a way to always handle this in DartPluginRegistrantTarget, both in compile and hot reload/restart?

Contributor

jonahwilliams Apr 5, 2021

I believe it should run before the first compile, but that should be easy to checl

blasten Apr 5, 2021

This is fixed in 829756e

blasten reviewed

View reviewed changes

packages/flutter_tools/lib/src/plugins.dart

                 final String renderedTemplate = globals.templateRenderer
                   .renderString(template, context, htmlEscapeValues: false);
-                final File file = globals.fs.file(filePath);
+                final FileSystem fs = fileSystem ?? globals.fs;

blasten Apr 2, 2021

what about just globals.fs? it'd be more consistent with the use of globals in line 28

Contributor Author

cyanglaz Apr 5, 2021

this adds the possibility to over write fileSystem which makes test easier.

blasten reviewed

View reviewed changes

packages/flutter_tools/lib/src/resident_runner.dart

+                  final CompositeTarget compositeTarget = CompositeTarget(<Target>[
                     const GenerateLocalizationsTarget(),
+                    const DartPluginRegistrantTarget(),

blasten Apr 2, 2021

nice!

blasten mentioned this pull request

Reland the Dart plugin registry #79669

Merged

cyanglaz closed this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

tool