[pigeon] Use a shared generation step for platform_tests/ by stuartmorgan-g · Pull Request #2823 · flutter/packages

stuartmorgan-g · 2022-11-18T17:32:03Z

In preparation for adding integration tests to test_plugin and alternate_language_test_plugin, which will involve multiple different tests running code based on the same generated output, this restructures the Pigeon test scripts to do generation of the *_test_plugin output files in a single shared step at the start of the test run, rather than having it be part of each test. This has several advantages:

Reduces overall time for a full test run; there is non-trivial overhead for each invocation of pigeon, so doing it # files times instead of # files x # languages times is faster.
Avoids duplication of generation that would happen otherwise (once for unit tests, then once for integration tests, for each language).
Makes it easy to add a new script (which this PR does) to run just the generation step, which:
- is useful for manually checking generation output (I've wanted it quite a few times) without running all tests.
- will work better if we check in the generated output (since it separates generation from running tests), which per a number of previous discussions is something we are very likely to do once we've done some more restructuring, as it will dramatically improve the PR review experience for generator changes.
Migrates more code from Bash to Dart (since it's a command we are exposing for command-line use anyway, we can migrate it without having to migrate the rest of the Bash code right now).

It does mean that running just individual tests that don't use the test plugins are currently slower; if that ends up being a common enough case before we switch to checking in generated code, we can potentially add logic to make this conditional. (Since that will be easier once everything is Dart, and my hope is that we move to checking in relatively soon, I didn't invest effort in that here.)

Opportunistic refactoring/cleanup done as part of this PR:

Extracted runPigeon to a separate file (unchanged).
Extracted runProcess to a separate file, and adjusted it slightly so that code that was currently using Process.start directly can share runProcess instead.
Added convenience wrappers for running Flutter, Xcode, and Gradle commands to make the main script slightly higher level. (These are essentially light-weight versions of wrappers that exist in flutter_plugin_tools, where they have been useful abstractions.)
ObjC output now uses camel-case files, as is the standard convention.
ObjC prefixes are no longer used, since collisions were eliminated from the input pigeons during the Swift generator bringup.
Generated files in the shared code have standardized on .gen.* instead of some using that and some using .g.*.

Prep for flutter/flutter#111505
Part of flutter/flutter#115169
Part of flutter/flutter#85068

Pre-launch Checklist

stuartmorgan-g · 2022-11-18T17:32:48Z

Adding overrides as this is a dev-only change, the scripts just don't know about pigeon's test structure. (Eventually we should fix that; it wouldn't be that big a deal to special-case it in the tooling.)

stuartmorgan-g

This seemed like a reasonably sized cut-point for this prep work, but let me know if you want me to split anything out.

stuartmorgan-g · 2022-11-18T17:33:41Z

packages/pigeon/platform_tests/ios_unit_tests/ios/Runner.xcodeproj/project.pbxproj

 	objects = {

 /* Begin PBXBuildFile section */
-		0D02163D27BC7B48009BD76F /* nullable_returns.gen.m in Sources */ = {isa = PBXBuildFile; fileRef = 0D02163C27BC7B48009BD76F /* nullable_returns.gen.m */; };


These project changes are all mechanical renames due to switching over to camel-casing for ObjC generation.

stuartmorgan-g · 2022-11-18T17:34:24Z

packages/pigeon/platform_tests/ios_unit_tests/ios/RunnerTests/AllDatatypesTest.m

 #import <XCTest/XCTest.h>
 #import "EchoMessenger.h"
-#import "all_datatypes.gen.h"
+#import "AllDatatypes.gen.h"


The ObjC test changes are all:

import renames for CamelCase files, or

removal of prefixes.

stuartmorgan-g · 2022-11-18T17:38:10Z

packages/pigeon/tool/shared/generation.dart

+}
+
+// A map of pigeons/ files to the languages that they can't yet be generated
+// for due to limitations of that generator.


This is a behavioral change; we used to generate an opted-in list of files for each language, whereas now we generate everything that doesn't actively fail. That means we're generating files that we aren't using yet, but

that should be harmless

it can be useful for manual generator change checking

it'll be one less thing to change when backfilling missing platform tests

[pigeon] Reorganize test pigeons flutter#115168 will render the difference moot anyway

stuartmorgan-g · 2022-11-22T18:00:21Z

Hm, I'm not sure what's going on with Windows; it passes for me locally now. I suspect we're missing some log output, as we've seen with other Windows LUCI hangs.

tarrinneal · 2022-11-23T00:00:51Z

Still hanging :/

stuartmorgan-g · 2022-11-23T03:53:43Z

With verbose build logging it looks suspiciously like it's hanging in the package resolution failure stage, which generally means a resolution conflict. But I didn't change anything about the Dart dependencies, so now I'm even more confused.

…differences" This reverts commit ddfbf8b.

This reverts commit e4e867c.

…hang" This reverts commit a124e08.

stuartmorgan-g · 2022-11-23T15:58:33Z

It turns out I forgot to update the header includes in the unit tests, so it may be that this was actually just the tests failing to compile (and passing for me locally because I apparently forgot to clean the output directory, so had the old files?). Why even just failing to compile would hang the CI, I have no idea; I'll file an issue with a repro case if that's really all it was.

gaaclarke · 2022-11-23T17:20:39Z

packages/pigeon/platform_tests/ios_unit_tests/ios/RunnerTests/EnumTest.m

 - (void)testEcho {
-  ACDataWithEnum *data = [[ACDataWithEnum alloc] init];
-  data.state = ACEnumStateError;
+  DataWithEnum *data = [[DataWithEnum alloc] init];


How come we lost all the prefixes to class names?

From the PR description:

ObjC prefixes are no longer used, since collisions were eliminated from the input pigeons during the Swift generator bringup.

gaaclarke

This looks good with a few recommendations:

Fill in some of the docstrings, nothing exhaustive needed, just a simple sentence that explains what public procedures are doing
I'd keep the class prefix on objc generation. Since there is no namespaces in objc its easy to get collisions and we don't want people coming up with goofy names just to avoid collisions.

gaaclarke · 2022-11-23T17:27:11Z

packages/pigeon/tool/shared/generation.dart

+  final String iosObjCBase =
+      p.join(baseDir, 'platform_tests', 'ios_unit_tests');
+
+  for (final String input in inputs) {


It worth trying parallelizing this: https://pub.dev/packages/pmap The input size is small and the execution time is large.

I forgot to move your existing TODO about this; I've added it. (But also, flutter/flutter#115168 will largely make this much less of an issue.)

gaaclarke · 2022-11-23T17:28:54Z

packages/pigeon/tool/shared/process_utils.dart

+  return process;
+}
+
+Future<int> runProcess(String command, List<String> arguments,


docstring please

gaaclarke · 2022-11-23T17:29:19Z

packages/pigeon/tool/shared/native_project_runners.dart

+
+import 'process_utils.dart';
+
+Future<int> runFlutterCommand(


docstrings would help here too

gaaclarke · 2022-11-23T17:29:41Z

packages/pigeon/tool/shared/generation.dart

+  return specialCases[inputName] ?? _snakeToPascalCase(inputName);
+}
+
+Future<int> generatePigeons({required String baseDir}) async {


Thanks, I completely forgot to go back and add them! Done everywhere.

stuartmorgan-g

I'd keep the class prefix on objc generation. Since there is no namespaces in objc its easy to get collisions and we don't want people coming up with goofy names just to avoid collisions.

We already need different names to make things work in Swift (and maybe Kotlin?). And it'll be moot when we do flutter/flutter#115168 anyway.

stuartmorgan-g · 2022-11-23T22:06:32Z

packages/pigeon/tool/shared/generation.dart

+  final String iosObjCBase =
+      p.join(baseDir, 'platform_tests', 'ios_unit_tests');
+
+  for (final String input in inputs) {


I forgot to move your existing TODO about this; I've added it. (But also, flutter/flutter#115168 will largely make this much less of an issue.)

stuartmorgan-g · 2022-11-23T22:22:14Z

packages/pigeon/tool/shared/generation.dart

+  return specialCases[inputName] ?? _snakeToPascalCase(inputName);
+}
+
+Future<int> generatePigeons({required String baseDir}) async {


Thanks, I completely forgot to go back and add them! Done everywhere.

gaaclarke

👍

stuartmorgan-g added 7 commits November 16, 2022 15:58

Reorganize process helpers to reduce duplicate code

43a2ed9

Add wrappers for common process commands

58cadcf

WIP generation consolidation

c247598

Re-add the Java special casing that the bash version did

1a1866a

Fix gradlew check

f985c95

Add ignores for Dart for now

c57d068

Update README

1d1f802

stuartmorgan-g requested review from gaaclarke and tarrinneal as code owners November 18, 2022 17:32

stuartmorgan-g added override: no versioning needed Override the check requiring version bumps for most changes override: no changelog needed Override the check requiring CHANGELOG updates for most changes labels Nov 18, 2022

github-actions bot added the p: pigeon label Nov 18, 2022

Typo fix

a6a1ca7

stuartmorgan-g commented Nov 18, 2022

View reviewed changes

stuartmorgan-g added 3 commits November 18, 2022 12:41

Autoformat

86a4342

Fix Windows

33e57e9

Windows command fix

3b31169

tarrinneal approved these changes Nov 21, 2022

View reviewed changes

stuartmorgan-g added 2 commits November 22, 2022 16:33

Try returning to runInShell:true to see if that fixes the CI hang

a124e08

Merge branch 'main' into pigeon-integration-prep-shared-generation

7cc9dea

stuartmorgan-g added 4 commits November 22, 2022 22:00

Revert more of _runWindowsUnitTests for CI debugging

e4e867c

Add missed early return for failure

538b88b

Verbose build, and disable Dart generation to further reduce differences

ddfbf8b

Merge branch 'main' into pigeon-integration-prep-shared-generation

c57e269

stuartmorgan-g mentioned this pull request Nov 23, 2022

[pigeon] CI debugging; not for landing #2843

Closed

Revert "Verbose build, and disable Dart generation to further reduce …

f0541a5

…differences" This reverts commit ddfbf8b.

stuartmorgan-g added 3 commits November 23, 2022 10:49

Revert "Revert more of _runWindowsUnitTests for CI debugging"

21c3c5c

This reverts commit e4e867c.

Revert "Try returning to runInShell:true to see if that fixes the CI …

3104630

…hang" This reverts commit a124e08.

Fix Windows includes

d7b4b8c

stuartmorgan-g mentioned this pull request Nov 23, 2022

Windows compilation failures in flutter/packages tests hang LUCI flutter/flutter#115937

Open

gaaclarke reviewed Nov 23, 2022

View reviewed changes

stuartmorgan-g commented Nov 23, 2022

View reviewed changes

gaaclarke approved these changes Nov 23, 2022

View reviewed changes

tarrinneal added the autosubmit Merge PR when tree becomes green via auto submit App label Nov 23, 2022

auto-submit bot merged commit 9134c12 into flutter:main Nov 23, 2022

Conversation

stuartmorgan-g commented Nov 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

stuartmorgan-g commented Nov 18, 2022

Uh oh!

stuartmorgan-g left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stuartmorgan-g Nov 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stuartmorgan-g commented Nov 22, 2022

Uh oh!

tarrinneal commented Nov 23, 2022

Uh oh!

stuartmorgan-g commented Nov 23, 2022

Uh oh!

stuartmorgan-g commented Nov 23, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gaaclarke left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stuartmorgan-g left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gaaclarke left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

stuartmorgan-g commented Nov 18, 2022 •

edited

Loading

stuartmorgan-g Nov 18, 2022 •

edited

Loading