[pigeon] Enable example app build in CI #8119

stuartmorgan · 2024-11-18T19:36:03Z

Removes example/pubspec.yaml, which is no longer needed since the excerpt system was rewritten not to use build_runner, so that the repo tooling recognizes example/app/ as the actual example, enabling all the standard CI steps (build example for each platform, run integration test, etc.) to run for it.

Pre-launch Checklist

Removes example/pubspec.yaml, which is no longer needed since the excerpt system was rewritten not to use `build_runner`, so that the repo tooling recognizes `example/app/` as the actual example, enabling all the standard CI steps (build example for each platform, run integration test, etc.) to run for it.

flutter-dashboard · 2024-11-18T19:36:06Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!).

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. The test exemption team is a small volunteer group, so all reviewers should feel empowered to ask for tests, without delegating that responsibility entirely to the test exemption group.

stuartmorgan · 2024-11-18T19:37:01Z

...igeon/example/app/android/app/src/main/kotlin/dev/flutter/pigeon_example_app/MainActivity.kt

@@ -37,22 +36,21 @@ private class PigeonApiImplementation : ExampleHostApi {
 // #enddocregion kotlin-class

 // #docregion kotlin-class-flutter
-private class PigeonFlutterApi {
-
+private class PigeonFlutterApi(binding: FlutterPlugin.FlutterPluginBinding) {


This and the constructor->init change were an Android Studio-suggested refactor; I figured I might as well update it to follow recommendations while I was there, but I can back this part out if we don't want it.

stuartmorgan · 2024-11-18T19:37:15Z

...igeon/example/app/android/app/src/main/kotlin/dev/flutter/pigeon_example_app/MainActivity.kt

-  constructor(binding: FlutterPlugin.FlutterPluginBinding) {
-    flutterApi = MessageFlutterApi(binding.getBinaryMessenger())
+  init {
+    flutterApi = MessageFlutterApi(binding.binaryMessenger)


This was a Kotlin style suggestion from AS.

stuartmorgan · 2024-11-18T19:37:49Z

...igeon/example/app/android/app/src/main/kotlin/dev/flutter/pigeon_example_app/MainActivity.kt

  }

  fun callFlutterMethod(aString: String, callback: (Result<String>) -> Unit) {
-    flutterApi!!.flutterMethod(aString) { echo -> callback(Result.success(echo)) }
+    flutterApi!!.flutterMethod(aString) { echo -> callback(echo) }


Per discussion in another PR, this code is wrong, and we just hadn't noticed because it wasn't building.

stuartmorgan · 2024-11-18T19:38:05Z

...igeon/example/app/android/app/src/main/kotlin/dev/flutter/pigeon_example_app/MainActivity.kt

  }
 }
 // #enddocregion kotlin-class-flutter

 class MainActivity : FlutterActivity() {
-  override fun configureFlutterEngine(@NonNull flutterEngine: FlutterEngine) {
+  override fun configureFlutterEngine(flutterEngine: FlutterEngine) {


AS flagged that we shouldn't be using nullability annotations in Kotlin.

stuartmorgan · 2024-11-18T19:38:26Z

packages/pigeon/example/app/ios/Runner/AppDelegate.swift

    }
  }
 }
 // #enddocregion swift-class-flutter

-@UIApplicationMain
+@main


This was a Flutter tool auto-migration; I didn't see any reason to undo it.

stuartmorgan · 2024-11-18T19:38:43Z

packages/pigeon/example/app/ios/Runner/AppDelegate.swift

  ) {
    flutterAPI.flutterMethod(aString: aStringArg) {
-      completion(.success($0))
+      completion($0)


As with the Kotlin file, this was wrong and didn't compile.

stuartmorgan · 2024-11-18T19:39:17Z

test-exempt: this change enables existing CI tests for this package

tarrinneal

These changes are all in my current pr, so we can merge them here or there. I'm not sure you actually enabled anything though, was that done somewhere else?

stuartmorgan · 2024-11-19T03:32:19Z

Converting to Draft while I fix the Windows build, as that will require more extensive changes.

I'm not sure you actually enabled anything though, was that done somewhere else?

Removing example/pubspec.yaml enables everything that was supposed to be running.

stuartmorgan · 2024-11-20T15:45:07Z

packages/pigeon/example/app/windows/runner/flutter_window.cpp

@@ -49,15 +75,6 @@ bool FlutterWindow::OnCreate() {
    return false;
  }

-  // #docregion cpp-method-flutter
-  void TestPlugin::CallFlutterMethod(


This code was not fully C++, and was in the wrong place (as in, this isn't a valid place for this definition to exist in C++). I pulled it out into a class to match Swift and Kotlin, and updated it to compile.

)

flutter/packages@e95f6d8...913b99e 2024-11-21 [email protected] [vector_graphics] handle errors from bytes loader (flutter/packages#8080) 2024-11-21 [email protected] [flutter_svg] Fix SvgNetworkLoader not closing internal http client (flutter/packages#8126) 2024-11-20 [email protected] [video_player_avfoundation] send video load failure even when eventsink was initialized late (flutter/packages#7194) 2024-11-20 [email protected] [flutter_markdown] enable Wasm support (flutter/packages#8120) 2024-11-20 98614782+auto-submit[bot]@users.noreply.github.com Reverts "[url_launcher] Add Swift Package Manager integration to example app (#8128)" (flutter/packages#8136) 2024-11-20 [email protected] [url_launcher] Add Swift Package Manager integration to example app (flutter/packages#8128) 2024-11-20 [email protected] [pigeon] Enable example app build in CI (flutter/packages#8119) 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] 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

* main: (64 commits) [quick_actions_plaform_interface] add localizedSubtitle (flutter#8112) [tools] Don't check license of generated Swift package (flutter#8137) Roll Flutter from 8536b96ebb3e to 93d772c5cdd8 (37 revisions) (flutter#8147) [go_router] Fix: Consistent PopScope Handling on Root Routes issue #140869 (flutter#8045) [in_app_purchase_storekit] fix price displayed with wrong precision (flutter#8127) [pigeon] Removes the `@protected` annotation from the InstanceManager field of the `PigeonInternalProxyApiBaseClass` (flutter#8125) [google_maps_flutter] Use structured Pigeon data on iOS (flutter#8142) [vector_graphics] handle errors from bytes loader (flutter#8080) [flutter_svg] Fix SvgNetworkLoader not closing internal http client (flutter#8126) [video_player_avfoundation] send video load failure even when eventsink was initialized late (flutter#7194) [flutter_markdown] enable Wasm support (flutter#8120) Reverts "[url_launcher] Add Swift Package Manager integration to example app (flutter#8128)" (flutter#8136) [url_launcher] Add Swift Package Manager integration to example app (flutter#8128) [pigeon] Enable example app build in CI (flutter#8119) [in_app_purchase_storekit] disallow ios versions lower than supported from enabling storekit (flutter#8110) [interactive_media_ads]: Bump com.google.ads.interactivemedia.v3:interactivemedia from 3.35.1 to 3.36.0 in /packages/interactive_media_ads/android (flutter#8046) [interactive_media_ads]: Bump androidx.annotation:annotation from 1.8.2 to 1.9.1 in /packages/interactive_media_ads/android (flutter#7980) [webview_flutter_android] Updates plugin to use `ProxyApis`s (flutter#7794) [interactive_media_ads] Adds support to define parameters that control the rendering of ads (flutter#8057) Roll Flutter from b3818f6b5979 to 8536b96ebb3e (22 revisions) (flutter#8124) ... # Conflicts: # packages/quick_actions/quick_actions_platform_interface/CHANGELOG.md

stuartmorgan added the override: no changelog needed Override the check requiring CHANGELOG updates for most changes label Nov 18, 2024

stuartmorgan requested review from hellohuanlin, LouiseHsu and tarrinneal as code owners November 18, 2024 19:36

github-actions bot added p: pigeon platform-android platform-ios labels Nov 18, 2024

stuartmorgan commented Nov 18, 2024

View reviewed changes

stuartmorgan added 2 commits November 18, 2024 14:44

excerpts

b03965c

macos

f965c8b

tarrinneal approved these changes Nov 19, 2024

View reviewed changes

stuartmorgan marked this pull request as draft November 19, 2024 03:31

github-actions bot added platform-macos platform-windows labels Nov 20, 2024

windows

8cf4ca9

stuartmorgan force-pushed the pigeon-example-build-fix branch from ee080ce to 8cf4ca9 Compare November 20, 2024 15:41

windows-excerpt

5c74f0f

stuartmorgan marked this pull request as ready for review November 20, 2024 15:44

stuartmorgan commented Nov 20, 2024

View reviewed changes

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

auto-submit bot merged commit 0d92794 into flutter:main Nov 20, 2024
77 checks passed

engine-flutter-autoroll mentioned this pull request Nov 21, 2024

Roll Packages from e95f6d8fce58 to 913b99ed8542 (7 revisions) flutter/flutter#159268

Merged

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

0d92794ce [pigeon] Enable example app build in CI (flutter/packages#8119

95b7577

)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[pigeon] Enable example app build in CI #8119

[pigeon] Enable example app build in CI #8119

stuartmorgan commented Nov 18, 2024

flutter-dashboard bot commented Nov 18, 2024

stuartmorgan Nov 18, 2024

stuartmorgan Nov 18, 2024

stuartmorgan Nov 18, 2024

stuartmorgan Nov 18, 2024

stuartmorgan Nov 18, 2024

stuartmorgan Nov 18, 2024

stuartmorgan commented Nov 18, 2024

tarrinneal left a comment

stuartmorgan commented Nov 19, 2024

stuartmorgan Nov 20, 2024

[pigeon] Enable example app build in CI #8119

[pigeon] Enable example app build in CI #8119

Conversation

stuartmorgan commented Nov 18, 2024

Pre-launch Checklist

flutter-dashboard bot commented Nov 18, 2024

stuartmorgan Nov 18, 2024

Choose a reason for hiding this comment

stuartmorgan Nov 18, 2024

Choose a reason for hiding this comment

stuartmorgan Nov 18, 2024

Choose a reason for hiding this comment

stuartmorgan Nov 18, 2024

Choose a reason for hiding this comment

stuartmorgan Nov 18, 2024

Choose a reason for hiding this comment

stuartmorgan Nov 18, 2024

Choose a reason for hiding this comment

stuartmorgan commented Nov 18, 2024

tarrinneal left a comment

Choose a reason for hiding this comment

stuartmorgan commented Nov 19, 2024

stuartmorgan Nov 20, 2024

Choose a reason for hiding this comment