-
Notifications
You must be signed in to change notification settings - Fork 6k
Reland Dart plugin registrant #25496
Conversation
…)" (flutter#25221)" This reverts commit 15b20ee.
runtime/dart_isolate.cc
Outdated
| Dart_Handle result = | ||
| tonic::DartInvokeField(plugin_registrant, "register", {}); | ||
| if (tonic::LogIfError(result)) { | ||
| FML_LOG(ERROR) << "Could not invoke the Dart plugin registrant."; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure this is right. It might have been invoked, but resulted in an unhandled exception. It depends on how tonic::DartInvokeField is written.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. I don't know what would happen if the method threw an exception. I think a test that that throws an exception in register and reasons about it is warranted. Do do this, we can use Settings.unhandled_exception_callback to listen for the error. Or, make sure the generated register method doesn't throw by adding a try-catch around its contents. I suppose, either way, this must be resilient to exceptions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is blocked on the previous comment - throwing from the static method void register() requires a new Dart main file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We might need to put try/catch wrappers around each plugin's registerWith method. flutter/flutter#78964 is doing this for Android.
| Wait(); | ||
| } | ||
|
|
||
| TEST_F(DartIsolateTest, DartPluginRegistrantIsCalled) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make sense to add an explicit test that there is no error when the class is absent?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would require a different location for the test. Since such tests require a new Dart main file, the Dart tooling complains about duplicated output file:
ERROR at //third_party/dart/build/dart/dart_action.gni:45:3: Duplicate output file.
action(target_name) {
^--------------------
Two or more targets generate the same output:
gen/flutter/runtime/assets/kernel_blob.bin
This is can often be fixed by changing one of the target names, or by
setting an output_name on one of them.
Collisions:
//flutter/runtime:_dsk__ds_no_plugin_registrant_fixtures
//flutter/runtime:_dsk__ds_runtime_fixtures
See //third_party/dart/build/dart/dart_action.gni:45:3: Collision.
action(target_name) {
^--------------------Any suggestion @chinmaygarde ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could try to update //testing/testing.gni to support multiple Dart snapshot targets. Or just create another testing target if it's easier - e.g. executable("runtime_plugin_missing_unittests") { .... } that depends on its own test_fixtures("runtime_plugin_missing_fixtures") which specifies a different Dart file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually it might be enough to just add another test_fixtures target and have the existing runtime executable depend on it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh I see, that is probably what you did already. You'll want to update testing.gni so that you can tell it to use a different file name at line 159.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it. Done
runtime/dart_isolate.cc
Outdated
| Dart_Handle result = | ||
| tonic::DartInvokeField(plugin_registrant, "register", {}); | ||
| if (tonic::LogIfError(result)) { | ||
| FML_LOG(ERROR) << "Could not invoke the Dart plugin registrant."; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. I don't know what would happen if the method threw an exception. I think a test that that throws an exception in register and reasons about it is warranted. Do do this, we can use Settings.unhandled_exception_callback to listen for the error. Or, make sure the generated register method doesn't throw by adding a try-catch around its contents. I suppose, either way, this must be resilient to exceptions.
ba4fde4 to
53c0304
Compare
9785d88 to
7562dc3
Compare
7562dc3 to
3d0765e
Compare
9bd5e82 to
6ed1ed8
Compare
zanderso
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The app_elf_snapshot.so file name is hardcoded in the source, so the trick with prepending the GN target_name won't help with avoiding output file conflicts there. Instead, you might need to move the test to a new subdirectory so that the target_gen_dirs can be different. (Or modify the test harness to not hardcode app_elf_snapshot.so, but I don't know how hard that is.)
testing/testing.gni
Outdated
| dart_snapshot_kernel_path = "$target_gen_dir/assets/kernel_blob.bin" | ||
|
|
||
| if (defined(invoker.dart_kernel)) { | ||
| dart_snapshot_kernel_path = invoker.dart_kernel |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of threading this through as a parameter, can you just always prepend the target name to the file name like:
dart_snapshot_kernel_path= "$target_gen_dir/assets/${target_name}_kernel_blob.bin"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the target name has the prefix _ds at this point. I reworked this a bit, so I added a flag use_target_as_artifact_prefix which is set in the root target to just put the root's target name as prefix.
zanderso
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the GN cleanup. It builds now =) But it looks like some tests are failing with:
[ERROR:flutter/testing/dart_isolate_runner.cc(79)] Could not locate kernel file.
|
This pull request is not suitable for automatic merging in its current state.
|
Reworks the Dart plugin registrant to remove the need to use
Dart_GetField.Dart_GetFieldgenerates an exception (NSM) when the field isn't defined. This is seen by the debugger because it isn't aware that the C code is handling the exception in some other way.The suggestion by @zanderso is to define the static method within a class, and then use
Dart_GetClasswhich surprisingly doesn't trigger an exception if the class isn't defined in the app library. The new registrant would look like this:Relands #23813
Fixes flutter/flutter#79810 and flutter/flutter#77680