Add Linux support to Pigeon #5100

robert-ancell · 2023-10-10T04:07:28Z

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.

flutter/flutter#73740

stuartmorgan

Nice! Thanks for tackling this :)

packages/pigeon/example/app/pigeons/messages.dart

packages/pigeon/example/app/linux/messages.g.h

stuartmorgan · 2024-02-21T14:35:14Z

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 · 2024-02-21T19:56:41Z

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

flutter/flutter#73740

robert-ancell · 2024-03-05T04:04:44Z

@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.

stuartmorgan

Overall this looks good (with the usual caveats about my poor gobject-fu). I mostly just skimmed the generator code; @tarrinneal should review that for structure.

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.

I'll also try this out on url_launcher_linux and see how it works for me in practice, since we often find things that we missed in review once we actually use the generators.

packages/pigeon/CHANGELOG.md

packages/pigeon/example/README.md

stuartmorgan · 2024-03-19T14:36:12Z

packages/pigeon/example/README.md

+    return;
+  }
+
+  g_printerr("Got result from Flutter method: %s\n", return_value);


This is inconsistent with the other examples, but also seems more useful. @tarrinneal what do you think about changing the other examples to show a simple example like this of handling success and failure, instead of just being straight passthroughs that don't do anything with the result?

Seems like a very reasonable idea

packages/pigeon/lib/linux_generator.dart

tarrinneal · 2024-03-19T17:13:18Z

packages/pigeon/example/app/pigeons/messages.dart

@@ -11,6 +11,9 @@ import 'package:pigeon/pigeon.dart';
  cppOptions: CppOptions(namespace: 'pigeon_example'),
  cppHeaderOut: 'windows/runner/messages.g.h',
  cppSourceOut: 'windows/runner/messages.g.cpp',
+  linuxHeaderOut: 'linux/messages.g.h',
+  linuxSourceOut: 'linux/messages.g.cc',
+  linuxOptions: LinuxOptions(),


Seems like module may be doing that work. It is prepended to all class names.

packages/pigeon/lib/linux_generator.dart

stuartmorgan · 2024-06-27T12:01:38Z

I haven't had time to do a full review (hopefully today), but given that you've eliminated all of the integration test skips, and that we've tried to make the integration tests pretty comprehensive in terms of feature support, I would expect that the green tests mean we're on par with the other generators modulo the kind of polish issues we always find as we start to use it.

(I'll also try using this on our native Linux plugins ASAP. Those aren't especially complex plugins so they probably won't find anything that the tests didn't, but it'll be another data point.)

robert-ancell · 2024-07-12T07:07:31Z

@stuartmorgan another review please! Hopefully all issues are resolved.

stuartmorgan · 2024-07-12T20:38:18Z

@stuartmorgan another review please! Hopefully all issues are resolved.

Sorry, I'd been trying to find some time to update my url_launcher_linux rework prototype again, since using the generator often lets me find issues I would miss in review (like the lifetime thing from the last round).

Now I'm running into issues with unit tests. It looks like the response objects generated by pigeon are totally opaque, which means we can't write unit tests directly against the handlers that we use to populate the vtable. Doing that is the pattern we use in most of our plugin tests in other languages, because it means we can do integration-y tests of just the native part, without leaving out any code that's not Pigeon-generated.

Can we put some accessors for the value and error of the response in the generated header?

stuartmorgan

Other than needing the query methods for host API responses, this looks great! I've both reviewed it and successfully used it for url_launcher_linux locally, and don't have any other feedback at this point.

Thank you so much for tackling this! It's awesome that we're going to have complete 1P platform coverage for Pigeon now.

packages/pigeon/platform_tests/test_plugin/linux/test_plugin.cc

stuartmorgan · 2024-07-15T16:11:06Z

packages/pigeon/lib/gobject_generator.dart

+
+    indent.newln();
+    indent.writeln('/**');
+    indent.writeln(' * ${responseMethodPrefix}_is_error:');


It looks like we just need to have these five methods for Host API response classes too.

packages/pigeon/lib/gobject_generator.dart

tarrinneal

The Changelog was merged improperly, but everything is fine besides that. I can do the doc comment stuff later (since it only effects code maintenance). Thanks again, a ton, for putting in all this time and effort to this project! (and I hope I can reach out for help on occasion when I'm working on this in the future :)

tarrinneal · 2024-07-15T23:26:13Z

packages/pigeon/CHANGELOG.md

+## 20.1.0

 * [java] Adds `equals` and `hashCode` support for data classes.
 * [swift] Fully-qualifies types in Equatable extension test.
+* Adds GObject (Linux) support.


The 20.1.0 and Adds GObject lines should be their own entry, not merged with the previous

tarrinneal · 2024-07-15T23:33:49Z

packages/pigeon/lib/gobject_generator.dart

+    indent.writeln('/**');
+    indent.writeln(' * ${methodPrefix}_new:');
+    for (final NamedType field in classDefinition.fields) {
+      final String fieldName = _getFieldName(field.name);
+      indent.writeln(' * $fieldName: field in this object.');
+      if (_isNumericListType(field.type)) {
+        indent.writeln(' * ${fieldName}_length: length of @$fieldName.');
+      }
+    }
+    indent.writeln(' *');
+    indent.writeln(' * Creates a new #${classDefinition.name} object.');
+    indent.writeln(' *');
+    indent.writeln(' * Returns: a new #$className');
+    indent.writeln(' */');
+    indent.writeln(
+        "$className* ${methodPrefix}_new(${constructorArgs.join(', ')});");
+
+    for (final NamedType field in classDefinition.fields) {
+      final String fieldName = _getFieldName(field.name);
+      final String returnType = _getType(module, field.type);
+
+      indent.newln();
+      indent.writeln('/**');
+      indent.writeln(' * ${methodPrefix}_get_$fieldName');
+      indent.writeln(' * @object: a #$className.');
+      if (_isNumericListType(field.type)) {
+        indent
+            .writeln(' * @length: location to write the length of this value.');
+      }
+      indent.writeln(' *');
+      if (field.documentationComments.isNotEmpty) {
+        addDocumentationComments(


Can these documentation comments be added using the addDocumentationComments method? If there's a reason not to, it's ok as is.

This comment applies to any manually added documentation comments.

I can also do it later myself if you don't want to deal with it :)

I pushed a change to adopt addDocumentationComment everywhere; do you want to give it a quick review? (I validated that the output is identical to before, so just the code structure).

stuartmorgan · 2024-07-23T14:52:39Z

Now I'm running into issues with unit tests. It looks like the response objects generated by pigeon are totally opaque, which means we can't write unit tests directly against the handlers that we use to populate the vtable. Doing that is the pattern we use in most of our plugin tests in other languages, because it means we can do integration-y tests of just the native part, without leaving out any code that's not Pigeon-generated.

Can we put some accessors for the value and error of the response in the generated header?

Per offline discussion, we're going to go ahead and land without that since adding them later won't affect any existing code, and we have a short-term work-around. I filed flutter/flutter#152166 to track adding it.

I'll do the final tiny cleanup on this PR, and then land it! 🎉

tarrinneal

another check mark for this

flutter/packages@1c319ac...19daf6f 2024-07-25 [email protected] Modernize the rest of the `index.html` files to support WASM compilation (flutter/packages#7192) 2024-07-25 [email protected] Add Linux support to Pigeon (flutter/packages#5100) 2024-07-24 [email protected] [gis_web] Allow package:web 1.0.0 (flutter/packages#7203) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages-flutter-autoroll Please CC [email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

robert-ancell requested a review from tarrinneal as a code owner October 10, 2023 04:07

robert-ancell marked this pull request as draft October 10, 2023 04:07

github-actions bot added p: pigeon platform-linux labels Oct 10, 2023

robert-ancell requested a review from stuartmorgan October 10, 2023 04:08

stuartmorgan reviewed Oct 10, 2023

View reviewed changes

jwinarske mentioned this pull request Nov 20, 2023

(Will PR) Make native code super thin? bluefireteam/audioplayers#1527

Open

robert-ancell force-pushed the pigeon-linux branch 2 times, most recently from b891227 to 8378c7d Compare December 11, 2023 03:40

robert-ancell force-pushed the pigeon-linux branch 8 times, most recently from 13aeabc to b7cdd5f Compare March 5, 2024 03:41

Add Linux support

4fb2014

flutter/flutter#73740

robert-ancell force-pushed the pigeon-linux branch from b7cdd5f to 4fb2014 Compare March 5, 2024 04:02

robert-ancell marked this pull request as ready for review March 5, 2024 04:02

robert-ancell requested a review from stuartmorgan March 5, 2024 04:02

stuartmorgan reviewed Mar 19, 2024

View reviewed changes

tarrinneal reviewed Mar 19, 2024

View reviewed changes

stuartmorgan reviewed Mar 19, 2024

View reviewed changes

packages/pigeon/lib/linux_generator.dart Outdated Show resolved Hide resolved

robert-ancell added 3 commits March 20, 2024 14:39

Bump minor version as new feature

a6fd0b4

Fix grammar

c019fd7

Merge remote-tracking branch 'origin/main' into pigeon-linux

01855bf

Merge remote-tracking branch 'origin/main' into pigeon-linux

e168930

robert-ancell added 6 commits July 2, 2024 12:54

Stop the Api object from holding a strong reference to the channels.

ce3829d

Fix code snippet

c7b4221

Replace host API object with a _set_method_handlers function

8323c6c

Merge remote-tracking branch 'origin/main' into pigeon-linux

d6047f3

Fix test

fd0fd3b

Fix snippet

d939e58

stuartmorgan reviewed Jul 15, 2024

View reviewed changes

tarrinneal approved these changes Jul 15, 2024

View reviewed changes

stuartmorgan mentioned this pull request Jul 23, 2024

[pigeon] Add APIs for reading Linux Host response objects flutter/flutter#152166

Open

stuartmorgan added 4 commits July 23, 2024 11:06

Merge branch 'main' into pigeon-linux

b9e61c3

Adopt addDocumentationComments everywhere

4c474a8

Clear handlers in test plugin

4c539b5

Use the right version

7458f06

tarrinneal approved these changes Jul 25, 2024

View reviewed changes

tarrinneal requested a review from stuartmorgan July 25, 2024 09:37

stuartmorgan added the autosubmit Merge PR when tree becomes green via auto submit App label Jul 25, 2024

auto-submit bot merged commit 2c1f713 into flutter:main Jul 25, 2024
76 checks passed

robert-ancell deleted the pigeon-linux branch July 25, 2024 10:42

engine-flutter-autoroll mentioned this pull request Jul 25, 2024

Roll Packages from 1c319ac0fad1 to 19daf6f36df7 (3 revisions) flutter/flutter#152311

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 25, 2024

2c1f713 Add Linux support to Pigeon (flutter/packages#5100)

de878ba

engine-flutter-autoroll mentioned this pull request Jul 25, 2024

Roll Packages from 1c319ac0fad1 to 19daf6f36df7 (3 revisions) flutter/flutter#152327

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 25, 2024

2c1f713 Add Linux support to Pigeon (flutter/packages#5100)

9800442

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Linux support to Pigeon #5100

Add Linux support to Pigeon #5100

robert-ancell commented Oct 10, 2023 •

edited

Loading

stuartmorgan left a comment

stuartmorgan commented Feb 21, 2024

robert-ancell commented Feb 21, 2024

robert-ancell commented Mar 5, 2024

stuartmorgan left a comment

stuartmorgan Mar 19, 2024

tarrinneal Mar 19, 2024

tarrinneal Mar 19, 2024 •

edited

Loading

stuartmorgan commented Jun 27, 2024

robert-ancell commented Jul 12, 2024

stuartmorgan commented Jul 12, 2024

stuartmorgan left a comment

stuartmorgan Jul 15, 2024

tarrinneal left a comment

tarrinneal Jul 15, 2024

tarrinneal Jul 15, 2024

tarrinneal Jul 15, 2024

tarrinneal Jul 15, 2024

stuartmorgan Jul 23, 2024

stuartmorgan commented Jul 23, 2024

tarrinneal left a comment

Add Linux support to Pigeon #5100

Add Linux support to Pigeon #5100

Conversation

robert-ancell commented Oct 10, 2023 • edited Loading

stuartmorgan left a comment

Choose a reason for hiding this comment

stuartmorgan commented Feb 21, 2024

robert-ancell commented Feb 21, 2024

robert-ancell commented Mar 5, 2024

stuartmorgan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tarrinneal Mar 19, 2024 • edited Loading

Choose a reason for hiding this comment

stuartmorgan commented Jun 27, 2024

robert-ancell commented Jul 12, 2024

stuartmorgan commented Jul 12, 2024

stuartmorgan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tarrinneal left a 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

Choose a reason for hiding this comment

stuartmorgan commented Jul 23, 2024

tarrinneal left a comment

Choose a reason for hiding this comment

robert-ancell commented Oct 10, 2023 •

edited

Loading

tarrinneal Mar 19, 2024 •

edited

Loading