Add Linux support to Pigeon

[robert-ancell]

Add Linux support for Pigeon. This has minimal work porting the generator and some manually written generated code so we can review this before implementing the generator support.

https://github.com/flutter/flutter/issues/73740

[stuartmorgan-g]

Is this blocked on anything now that custom codec support has landed, or just waiting for you to have time to get back to it?

[robert-ancell]

Just blocked on getting back to it after the GTK4 work.

[robert-ancell]

@stuartmorgan I think this PR is now close enough to be ready to land. I think I've included what was previously discussed, but this is probably worth doing a full review as it might have changed since last time. There's one feature I know that isn't there, which is nullable values, might be worth doing in a follow up PR.

[robert-ancell]

The main thing we'll need in order to land this is wiring up Linux in platform_tests/test_plugin/ and the test suites in tool so that we're running integration tests and have confidence that everything is working end to end. Let me know if you'd like me to do that part.

If you have the time please do work on this.

[robert-ancell]

We need some new API in the engine - https://github.com/flutter/engine/pull/51599

[stuartmorgan-g]

The main thing we'll need in order to land this is wiring up Linux in platform_tests/test_plugin/ and the test suites in tool so that we're running integration tests and have confidence that everything is working end to end. Let me know if you'd like me to do that part.

If you have the time please do work on this.

I started doing the basic wiring up for this (here), but it fails as soon as we try to generate from core_tests.dart because the *List types aren't supported. We'll need enough support that generation can run on at least core_tests.dart (we could temporarily exclude other files) before test setup can go any further. You can repro by running dart tool/generate.dart in that branch.

(I can play with seeing if I can do just enough to make generation pass and compile even if it doesn't actually work yet, but that's the current state.)

[stuartmorgan-g]

I've updated my branch with minimal changes to get generation working. It's definitely not functional though (there's no way to get numerical list lengths from data classes, and numerical list copies aren't implemented at all), and I haven't even tried to compile it yet.

Let me know if you want me to push the changes into this PR now, or wait until I have time to get further on it.

[stuartmorgan-g]

My branch now allows compiling and running the unit tests and integration tests. The test plugin and the unit test are just the template placeholder, so the test plugin still needs to implement all the echo functionality like the other platforms, and we need at least minimal native unit tests (although we're de-emphasizing those for the most part now that we have e2e testing set up for Pigeon).

[stuartmorgan-g]

I'd forgotten to add all the generated code to the makefile, so it wasn't yet being compiled. I've done that now, and it's failing compilation in ways that highlight issues with the generated code. If you merge my branch in, you can repro the failures with:

dart tool/test.dart -t linux_unittests

[robert-ancell]

@stuartmorgan I've finished off the list support and committed it here. I'll have a look at the integration tests next but if you get to them feel free to push into here directly.

[stuartmorgan-g]

Pushed the test harness, as well as (untested) support for Object as a type so that generation runs successfully.

The state isn't changed since my last comments, so it's still the case that the test plugin itself is completely unimplemented, and there are no actual native unit tests either. For some initial pointers:

• This file, which is currently just the boilerplate from flutter create, needs to be a Linux version of, e.g., this. It's almost all just echoing values. This is what the Dart integration tests talk to, to do all of the end-to-end testing that values are being passed around correctly.
  • As you're doing bringup, you may want to skip some integration tests on Linux temporarily, which you can do with the TargetGenerator value.
• This file should be replaced with files that do some actual native unit testing. The Windows tests might be a useful reference here, although as mentioned above these tests are much less necessary now that we have full e2e testing. I would suggest we at least do some simple testing of getters/setters on generated objects though, since those aren't necessarily well-excercised in the integration tests. E.g., I found issues with the usability of making data class objects on the native side (to call FlutterApi methods) in the early version of the Windows generator by writing those tests.

[robert-ancell]

Made some good progress here - there's one fail currently with a field called type - this generates a _get_type() method which conflicts with GObject, so I'll have to add some name mangling there.

[stuartmorgan-g]

Oops, CI can't run the tests currently. I'll fix that in a separate PR so that every push here doesn't run tests for every single package.

[robert-ancell]

@stuartmorgan when I run the integration tests they all seem to fail, but I haven't worked out how to get more logging information out of them - is there an easy way to do that / can you check if you can run them locally? (dart tool/test.dart -t linux_integration_tests). Note they don't work in CI due to not using a Flutter engine that supports custom FlValue types.

[stuartmorgan-g]

I'll try to take a look today or early next week.

Note they don't work in CI due to not using a Flutter engine that supports custom FlValue types.

That should be true for stable, but not for master. We could exclude the tests on stable temporarily, although I need to think about whether we actually want to land this while it produces code that doesn't work on stable; it may be better to have it ready to go, but not actually land it until stable supports it.

[stuartmorgan-g]

Sorry for the delay getting to this. I've pushed a change to disable almost all of the integration tests, next I'll see what I can turn on successfully.

For other platforms, the approach I've taken to debugging the native side is to take a call that's failing and put it into the example app's main.dart, then run that app under a native debugger. I haven't tried that with Linux before, but I would expect it to work just as well.

[stuartmorgan-g]

I've enabled a bunch of tests. Disabled still are:

• The all-type class object tests, which hang (at least some; I haven't tried each one individually yet).
• The Flutter API tests, where the basic void-void test never completed.
• The multi-arity tests, which I'm surprised are failing since they just use simple data types that work in other tests.
• The suffix tests, which aren't implement here yet since it's a new feature.

I'm not sure why things fail to compile in CI. @robert-ancell Is the output relying on newer compiler or library behaviors, maybe?

[stuartmorgan-g]

I put the code from Arguments of multiple types serialize and deserialize correctly into example_app.dart to play with it, and the app to make sure I could repro before I tried in a debugger logged:

** (test_plugin_example:43737): CRITICAL **: 20:03:24.431: FlValue *fl_value_ref(FlValue *): assertion 'self != nullptr' failed
** (test_plugin_example:43737): CRITICAL **: 20:03:24.431: FlValue *fl_value_ref(FlValue *): assertion 'self != nullptr' failed
** (test_plugin_example:43737): CRITICAL **: 20:03:24.431: FlValue *fl_value_ref(FlValue *): assertion 'self != nullptr' failed
** (test_plugin_example:43737): CRITICAL **: 20:03:24.431: FlValue *fl_value_ref(FlValue *): assertion 'self != nullptr' failed
** (test_plugin_example:43737): CRITICAL **: 20:03:24.431: FlValue *fl_value_ref(FlValue *): assertion 'self != nullptr' failed
** (test_plugin_example:43737): CRITICAL **: 20:03:24.431: FlValue *fl_value_ref(FlValue *): assertion 'self != nullptr' failed
(test_plugin_example:43737): GLib-GObject-CRITICAL **: 20:03:24.431: g_object_ref: assertion 'G_IS_OBJECT (object)' failed
** (test_plugin_example:43737): WARNING **: 20:03:24.431: Failed to send response to HostIntegrationCoreApi.sendMultipleNullableTypes: Custom value not implemented
** (test_plugin_example:43737): CRITICAL **: 20:03:24.431: FlBinaryMessengerResponseHandle was not responded to

So even without a native debugger we can get more information outside the context of an integration test.

[stuartmorgan-g]

(Also, the PR will need to be updated when https://github.com/flutter/packages/pull/6600 lands since it changes the way enums and classes are serialized.)

[robert-ancell]

@stuartmorgan I have the tests passing locally, but there's this TestDeviceException(Unable to start the app on the device.) error on the CI - It's not clear to me what might be failing, can you have a look?

[stuartmorgan-g]

It's probably that I forgot to plumb xvfb into the custom Pigeon tests. I'll look at adding that tomorrow.

Exciting news that the integration tests are passing!

[stuartmorgan-g]

Tests are green! 🎉

[robert-ancell]

The windows tests are failing with:

00:02 +372 -4: test\pigeon_lib_test.dart: generator validation
Error: C:\b\s\w\ir\x\t\74db0377/foo.dart: _ValidatorGenerator
00:03 +373 -4: test\pigeon_lib_test.dart: generator validation skipped

I'm not sure what's going wrong here.

[robert-ancell]

And I can't work out what is wrong with the Linux tests, the log says:

The following packages had errors:
  packages/pigeon
See above for full details.

But I can't see any errors above.

[stuartmorgan-g]

This looks like a crash:

[==========] Running 15 tests from 5 test suites.
[----------] Global test environment set-up.
[----------] 1 test from MultipleArity
[ RUN      ] MultipleArity.HostSimple

[packages/pigeon completed in 8m 9s]

It says it's going to run 15 tests, but then has no output after the first.

[stuartmorgan-g]

I though maybe the unit test might need GTK initialization to work (that's been true of other Linux unit tests in the past) so I added an xvfb wrapper. It's still crashing in the same place though (although it does actually say it seg faulted this time, so progress?). Does it actually pass locally?

[robert-ancell]

Ah, I thought they were passing locally but I didn't notice they were actually crashing in the same was as in the CI... Some challenges I've had running the tests:

• The test runner not clearly marking when things fail (e.g. return code or signal that occurred)
• Linker failures on Flutter that don't have the linker errors in the logs (unlike compiler errors).
• Having to manually edit tools/run_tests.dart to disable the Android tests.

[stuartmorgan-g]

• The test runner not clearly marking when things fail (e.g. return code or signal that occurred)

We should probably have the Pigeon test runner log any non-zero return code from gtest for clarity.

• Linker failures on Flutter that don't have the linker errors in the logs (unlike compiler errors).

Even in verbose mode, or do you mean just when the build is run by the Pigeon test scripts?

• Having to manually edit tools/run_tests.dart to disable the Android tests.

run_tests.dart is configured for CI; locally you'll want to use tests.dart with the -t flag to select specific suites. We should add more about that to CONTRIBUTING.md.

[robert-ancell]

For example if I break a function signature like:

$ git diff ./platform_tests/test_plugin/linux/test/utils/fake_host_messenger.cc
diff --git a/packages/pigeon/platform_tests/test_plugin/linux/test/utils/fake_host_messenger.cc b/packages/pigeon/platform_tests/test_plugin/linux/test/utils/fake_host_messenger.cc
index b78301cec..3edee14e6 100644
--- a/packages/pigeon/platform_tests/test_plugin/linux/test/utils/fake_host_messenger.cc
+++ b/packages/pigeon/platform_tests/test_plugin/linux/test/utils/fake_host_messenger.cc
@@ -149,7 +149,7 @@ static void fake_host_messenger_init(FakeHostMessenger* self) {
                                                  g_free, message_handler_free);
 }
 
-FakeHostMessenger* fake_host_messenger_new(FlMessageCodec* codec) {
+FakeHostMessenger* fake_host_messenger_new(FlMessageCodec* codec, bool unused) {
   FakeHostMessenger* self = FAKE_HOST_MESSENGER(
       g_object_new(fake_host_messenger_get_type(), nullptr));
 

The output is (CMake warnings removed for clarity):

$ dart tool/test.dart -t linux_unittests
# Generating platform_test/ output...
Generation complete!
##############################
# Running linux_unittests
clang++: error: linker command failed with exit code 1 (use -v to see invocation)
Building Linux application...                                           
Build process failed

I wasn't able to find any easy means of enabling any verbose flags.

[stuartmorgan-g]

There's currently no way to pass a verbose build flag through the Pigeon tooling (although we could definitely add one), but the build is just a flutter build linux in platform_tests/test_plugin/example, so you can run that directly with verbose Flutter build flags and you should be able to get actual errors that way.