[tool] Update tool to set macOS deployment target to 10.15. #6605

IVLIVS-III · 2022-10-23T13:00:16Z

This change is a necessary precondition for #6517.

Adding support for macOS to in_app_purchase requires the macOS apps deployment target to be set to 10.15.
This PR makes the necessary changes to the all-plugins-app command.

Closes flutter/flutter#113913

Pre-launch Checklist

If you need help, consider asking for advice on the #hackers-new channel on Discord.

IVLIVS-III · 2022-10-23T15:01:17Z

As mentioned here by @stuartmorgan, it looks like this PR needs some additional tests.

It needs some review, and also its own test changes

However, I do not know where to start adding such tests and what exactly such additional tests should cover.

flutter-dashboard · 2022-10-23T15:01:38Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!).

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.

a-wallen · 2022-10-23T18:27:23Z

script/tool/lib/src/create_all_plugins_app_command.dart

+    final File podfileFile =
+        app.platformDirectory(FlutterPlatform.macos).childFile('Podfile');
+    if (!podfileFile.existsSync()) {
+      throw ToolExit(64);


I wonder why 64 is the ToolExit code. Could that be documented here?

Initially it was suggested to me, to look at the implementation of _updateAppGradle to see how changes to project configuration files could be made (see this comment).

This would be done in script/tool/lib/src/create_all_plugins_app_command.dart. See _updateAppGradle for an example of making a similar change to some Android values.

There, this (to me arbitrary) exit code 64 was thrown in case the relevant android config file didn't exist.
I don't know the meaning of this value either. Looking back at git history, the value was introduced two years ago in #3544.
The value itself seems to date back even further tho, since the PR mentioned above only moved some files around.

This does look it was arbitrary. The way we're handling error codes for any current code is to define file-level constants starting with 3 (since 1 and 2 are global errors) for each failure, and using those, so that it's self-documenting. Let's do that here; it's fine that it's inconsistent with the older code in the file since those values aren't meaningful anyway.

a-wallen

LGTM

stuartmorgan-g

Thanks, looks great other than some minor nits. This will need a unit test though; it should be straightforward following the pattern of the existing tests for this file: run the command, then read the files that are expected to be modified and make sure they match expectations (e.g., that every line in the project file that contains MACOS_DEPLOYMENT_TARGET also contains 10.15).

stuartmorgan-g · 2022-10-24T14:30:12Z

script/tool/lib/src/create_all_plugins_app_command.dart

+  Future<int> _genNativeBuildFiles() async {
+    // Only run on macOS.
+    // Other platforms don't need generation of additional files.
+    if (!io.Platform.isMacOS) {


We may need this for other platforms in the future, and pub get will be part of building the app anyway, so we can just do this unconditionally and keep the codepaths the same on all platforms.

stuartmorgan-g · 2022-10-24T14:34:16Z

script/tool/lib/src/create_all_plugins_app_command.dart

+    final File podfileFile =
+        app.platformDirectory(FlutterPlatform.macos).childFile('Podfile');
+    if (!podfileFile.existsSync()) {
+      throw ToolExit(64);


This does look it was arbitrary. The way we're handling error codes for any current code is to define file-level constants starting with 3 (since 1 and 2 are global errors) for each failure, and using those, so that it's self-documenting. Let's do that here; it's fine that it's inconsistent with the older code in the file since those values aren't meaningful anyway.

stuartmorgan-g · 2022-10-24T14:37:39Z

script/tool/lib/src/create_all_plugins_app_command.dart

+  }
+
+  Future<void> _updateMacosPbxproj() async {
+    // Only change the macOS deployment target if the host platform is macOS.


Same; this can be unconditional.

script/tool/lib/src/create_all_plugins_app_command.dart

IVLIVS-III · 2022-10-26T15:47:39Z

From all the failing tests, it looks like we need the host platform guards after all, since the macOS configuration files are not found / generated on any platform except for macOS.

stuartmorgan-g · 2022-10-26T16:08:20Z

From all the failing tests, it looks like we need the host platform guards after all, since the macOS configuration files are not found / generated on any platform except for macOS.

Sorry about that; most of the Flutter tool's platform build file generation is unconditional, and I didn't realize Podfile wasn't. The rest of it should be fine.

stuartmorgan-g

Sorry, I missed a few details in the implementation review.

On the test side, it looks like your changes broke the Dart unit tests for this command.

stuartmorgan-g · 2022-10-26T19:26:35Z

script/tool/lib/src/create_all_plugins_app_command.dart

+    /// Run `flutter pub get` to generate all native build files for macOS.
+    final int genNativeBuildFilesExitCode = await _genNativeBuildFiles();
+    if (genNativeBuildFilesExitCode != 0) {
+      throw ToolExit(genNativeBuildFilesExitCode);


This should instead printError a message (e.g., "Failed to generate native build files"), then throw a new defined error like the ones you added above. We don't actually need the underlying exit code.

stuartmorgan-g · 2022-10-26T19:27:11Z

script/tool/lib/src/create_all_plugins_app_command.dart

@@ -61,10 +64,19 @@ class CreateAllPluginsAppCommand extends PackageCommand {
      print('');
    }

+    await _genPubspecWithAllPlugins();
+
+    /// Run `flutter pub get` to generate all native build files for macOS.


Nit: Remove the "for macOS" here since it's all platform (even if we only happen to be using it for macOS right now).

stuartmorgan-g · 2022-10-26T19:31:52Z

script/tool/lib/src/create_all_plugins_app_command.dart

@@ -259,4 +271,64 @@ dev_dependencies:${_pubspecMapString(pubspec.devDependencies)}

    return buffer.toString();
  }
+
+  Future<int> _genNativeBuildFiles() async {


This can be a boolean.

stuartmorgan-g · 2022-10-26T19:32:50Z

script/tool/lib/src/create_all_plugins_app_command.dart

@@ -259,4 +271,64 @@ dev_dependencies:${_pubspecMapString(pubspec.devDependencies)}

    return buffer.toString();
  }
+
+  Future<int> _genNativeBuildFiles() async {
+    final io.ProcessResult result = io.Process.runSync(


You can simplify this method by using processRunner.runAndStream, which will automatically print all of the output.

Oh neat, didn't know this existed.

script/tool/lib/src/create_all_plugins_app_command.dart

IVLIVS-III · 2022-10-26T20:20:40Z

Ohh right, I think I found where the existing tests break:

Fake plugins (e.g. plugina) are generated to test e.g. the exclusion flag. It seems like those fake plugins are not generated with all files included, that are expected by pub get. But pub get is now invoked earlier (i.e. within the all-plugins-app command and not afterwards).

Is it really necessary to edit the code generating fake plugins or is there an easier change that I didn't think of?

stuartmorgan-g · 2022-10-26T20:50:14Z

Ah right, I didn't think about that aspect.

I think the best approach would be:

Add a RecordingProcessRunner to the test setup, so that the pub get call is mocked out (relevant pointers: plumbing it into the command constructor, setting up a test-global fake runner)
- I think this should be enough to fix the existing tests.
Add some tests that it's called as expected, and that failures are handled (validating calls, mocking results to test failure)
In the new tests where you validate the actual file changes, just add code after creating the fake plugins to inject a dummy file with the relevant content. It doesn't have to be an actual instance of the file (i.e., don't paste in a whole real Xcode project), just some relevant bits from a real file. It would look something like this, except with a manually constructed subpath.

This change is necessary for flutter#6517.

The Podfile is only generated on a macOS host.

Before throwing a `ToolExit`, a error message is printed.

…is correct.

IVLIVS-III · 2022-10-27T00:27:56Z

Tests are still failing on Windows, but I'm not sure what causes these failures.
Also, my host platform is macOS, so I'm unable to personally (locally) test on Windows.

stuartmorgan-g · 2022-10-27T13:43:13Z

It's the introduction of the MockProcess, which breaks things because we're actually calling flutter create in the tests. The way we test this command is problematic; I filed flutter/flutter#114153 to track improving it.

I'll push a fix here shortly to fix the Windows tests.

stuartmorgan-g · 2022-10-27T14:21:15Z

The other Windows failures are because pub get is hanging. I can reproduce this locally, but I'm still trying to figure out why (it works fine when run in the same directory outside of the context of the script.)

stuartmorgan-g · 2022-10-27T14:46:47Z

I spent a while looking at this, and I have no idea what's going on. It hangs inside pub get early in the command (almost nothing is printed in verbose mode), it doesn't seem to be a race with some earlier step not having fully finished (adding a long delay just before it doesn't help), but if I comment it out, run the script, then run the same command manually in the directory, everything is fine.

Since I don't have more time to investigate it right now, and I don't want to block the PR chain indefinitely on this, I re-added conditionals to bypass some steps on Windows after all, with TODOs to investigate further and unwind them so we don't have to maintain the conditional logic forever.

stuartmorgan-g · 2022-10-27T14:50:44Z

script/tool/test/create_all_plugins_app_command_test.dart

+    test('macOS deployment target is modified in Podfile', () async {
+      final RepositoryPackage plugin = createFakePlugin('plugina', packagesDir);
+      final File podfileFile =
+          plugin.directory.childDirectory('macos').childFile('Podfile');


This file is being created in the wrong directory; the podfile should be in the created app, not the plugin.

Moved creation of the mock Podfile to the app directory.

script/tool/test/create_all_plugins_app_command_test.dart

Podfile is now generated in the correct location. project.pbxproj uses the real file generated by `flutter create`.

stuartmorgan-g

LGTM! @tarrinneal or @bparrishMines could you do the secondary review here since it's repo tooling?

stuartmorgan-g · 2022-10-27T18:24:41Z

script/tool/lib/src/create_all_plugins_app_command.dart

@@ -179,7 +205,7 @@ class CreateAllPluginsAppCommand extends PackageCommand {
      },
      dependencyOverrides: pluginDeps,
    );
-    app.pubspecFile.writeAsStringSync(_pubspecToString(pubspec));
+    app.pubspecFile.writeAsStringSync(_pubspecToString(pubspec), flush: true);


Oops, I didn't mean to push this; I was trying it locally when I was grasping at straws on the pub get hang issue. You can revert this line.

bparrishMines

LGTM

…flutter/plugins#6605)

…6605)

IVLIVS-III mentioned this pull request Oct 23, 2022

[in_app_purchase_storekit] Add support for macOS #6517

Merged

11 tasks

IVLIVS-III marked this pull request as ready for review October 23, 2022 15:01

flutter-dashboard bot added the needs tests label Oct 23, 2022

a-wallen reviewed Oct 23, 2022

View reviewed changes

a-wallen requested a review from stuartmorgan-g October 23, 2022 18:29

stuartmorgan-g suggested changes Oct 24, 2022

View reviewed changes

a-wallen requested a review from cyanglaz October 24, 2022 16:56

IVLIVS-III force-pushed the iap_macos/tool-change branch from 419bea0 to b8573eb Compare October 26, 2022 15:40

stuartmorgan-g suggested changes Oct 26, 2022

View reviewed changes

IVLIVS-III force-pushed the iap_macos/tool-change branch from f050c8c to 8e06f2c Compare October 26, 2022 20:00

IVLIVS-III added 6 commits October 26, 2022 23:22

Update tool to set macOS deployment target to 10.15.

ee0aebe

This change is necessary for flutter#6517.

Added a unit test for changing the macOS deployment version.

abd3bc4

Skip Podfile change on non-macOS platforms.

5d2c735

The Podfile is only generated on a macOS host.

Minor changes to error handling.

46a360c

Before throwing a `ToolExit`, a error message is printed.

Mocked out pub get while testing 'all-plugins-app'-command.

cc6277f

Add additional tests to ensure the mocked out flutter pub get call …

45ea1cf

…is correct.

IVLIVS-III force-pushed the iap_macos/tool-change branch from 8e06f2c to 45ea1cf Compare October 26, 2022 22:23

stuartmorgan-g mentioned this pull request Oct 27, 2022

Convert plugin tooling all-plugins-app test to mock process flutter/flutter#114153

Closed

Revert mock process to fix Windows unit tests

aef364d

Skip pub get on Windows

0873843

stuartmorgan-g suggested changes Oct 27, 2022

View reviewed changes

Fixed usage of macOS mock files.

c95ea9f

Podfile is now generated in the correct location. project.pbxproj uses the real file generated by `flutter create`.

stuartmorgan-g mentioned this pull request Oct 27, 2022

[tool] Rename all-plugins-app command #6600

Merged

11 tasks

stuartmorgan-g approved these changes Oct 27, 2022

View reviewed changes

IVLIVS-III added 2 commits October 27, 2022 22:37

Revert unnecessary force-flush.

76c2152

Merge branch 'flutter:main' into iap_macos/tool-change

c3d2276

bparrishMines approved these changes Oct 28, 2022

View reviewed changes

stuartmorgan-g added autosubmit Merge PR when tree becomes green via auto submit App and removed needs tests labels Oct 28, 2022

auto-submit bot merged commit 6f91122 into flutter:main Oct 28, 2022

engine-flutter-autoroll mentioned this pull request Oct 28, 2022

Roll Plugins from efef3ab28eb4 to 6f9112227516 (4 revisions) flutter/flutter#114233

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 28, 2022

6f9112227 [tool] Update tool to set macOS deployment target to 10.15. (…

22a066b

…flutter/plugins#6605)

adam-harwood pushed a commit to adam-harwood/flutter_plugins that referenced this pull request Nov 3, 2022

[tool] Update tool to set macOS deployment target to 10.15. (flutter#…

a62de12

…6605)

mauricioluz pushed a commit to mauricioluz/plugins that referenced this pull request Jan 26, 2023

[tool] Update tool to set macOS deployment target to 10.15. (flutter#…

ab39ca1

…6605)

[tool] Update tool to set macOS deployment target to 10.15. #6605

[tool] Update tool to set macOS deployment target to 10.15. #6605

Uh oh!

Conversation

IVLIVS-III commented Oct 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

IVLIVS-III commented Oct 23, 2022

Uh oh!

flutter-dashboard bot commented Oct 23, 2022

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!

a-wallen left a comment

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

IVLIVS-III commented Oct 26, 2022

Uh oh!

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

Uh oh!

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!

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!

Uh oh!

Uh oh!

IVLIVS-III commented Oct 26, 2022

Uh oh!

stuartmorgan-g commented Oct 26, 2022

Uh oh!

IVLIVS-III commented Oct 27, 2022

Uh oh!

stuartmorgan-g commented Oct 27, 2022

Uh oh!

stuartmorgan-g commented Oct 27, 2022

Uh oh!

stuartmorgan-g commented Oct 27, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

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!

bparrishMines left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

IVLIVS-III commented Oct 23, 2022 •

edited

Loading

stuartmorgan-g commented Oct 26, 2022 •

edited

Loading