[camera] Define clang modules in for iOS #2115

jmagman · 2019-09-27T02:32:06Z

Description

Limit the supported podspec platform to iOS so tests don't run (and fail) for macOS.
Define the module by setting DEFINES_MODULE in the podspec. See CocoaPod modular headers docs.
Explicitly set VALID_ARCHS to architectures included in the Flutter universal binary. This is the CocoaPods-suggested workaround to prevent tests from running on the i386 simulator. See Fix linting when armv7 is included but i386 isn't CocoaPods/CocoaPods#8159.

Related Issues

Fixes flutter/flutter#41439.
See also flutter/flutter#41007.

Tests

Add a test_spec to the podspec so the CocoaPods linter will test them. These tests fail before the s.platform and s.pod_target_xcconfig changes.
Add Cirrus script to lint and test plugins with iOS platform code. This uses CocoaPods's built-in test_specs and lints and tests via pod lib lint.
Exclude every iOS plugin except for camera from this script until they can be migrated. Linter errors/warnings for all plugins:

find . -name "*\.podspec" -exec pod lib lint {} --analyze --verbose  \;

Checklist

Breaking Change

Yes, this is a breaking change (please indicate a breaking change in CHANGELOG.md and increment major revision).
No, this is not a breaking change.

jmagman · 2019-09-27T02:45:23Z

Add Cirrus script to lint and test plugins with iOS platform code. This uses CocoaPods's built-in test_specs and lints and tests via pod lib lint.

Here's the first run:
https://cirrus-ci.com/task/5308706226438144?command=main#L5

script/lint_darwin_plugins.sh

stuartmorgan-g · 2019-09-27T03:05:20Z

script/lint_darwin_plugins.sh

@@ -0,0 +1,88 @@
+#!/bin/bash


Is there a reason this script can't be Dart? It seems like anything non-trivial should be Dart if it all possible, as it's easier to maintain.

All the Cirrus tests are run from shell scripts, and this was closely cribbed from check_publish.sh. I will defer to the plugins team as to whatever they find most maintainable (I'd prefer to not to be the sole owner of this effort).

I'm fine deferring final decision to the plugins team. I would strongly encourage them to view adding a new non-trivial script as a good opportunity to begin a transition to Dart though :)

(Also worth considering is that at some point we're going to have Windows plugins in this repo, and probably will want to run scripts for them, and I cannot stress enough how much we want to those scripts to be in Dart rather than Batch.)

So it's kind of scattered. A lot of our CI scripts start off in bash here, but then ultimately end up deferred to a completely separate Dart tool maintained in the flutter/plugin_tools repo. It's kind of hard to follow but the flow is that CI calls a bash script, and the bash script downloads and runs random commands from that repo by calling the package via flutter_plugin_tools, see these lines.

I think it probably makes the most sense for this functionality to live as a command in that tool instead of in bash given how much is going on here. The meat of this is easier to do in shell since it needs to run pod lint, but I'm pretty sure that repo does expose process run commands that could be called instead. That said the bash is relatively straightforward here so I wouldn't block the PR on moving it either, I don't think maintaining it in this form would be a huge burden.

I created flutter/flutter#41511 and assigned it to myself. I'll work on it once I convert a few more plugins to modules and get the plugin template done in flutter tools.

script/lint_darwin_plugins.sh

stuartmorgan-g · 2019-09-27T03:15:20Z

script/lint_darwin_plugins.sh

+function lint_package() {
+  local package_name=$1
+  local platform=$2
+  local podspec_dir="$REPO_DIR/packages/$package_name/$platform/$package_name.podspec"


This structure is about to start changing, per https://docs.google.com/document/d/1PeS-QSAFydHXhjZrqkub-GRPMs83roh8VwhbUScWuGQ/edit#

It would be good to support the new structure as well (the plan is option 1, as I understand it) from the start so we don't accidentally start losing coverage as plugins migrate.

I changed it to find all podspecs in each top-level package directory.

mklim · 2019-09-27T03:39:08Z

script/build_all_plugins_app.sh

 # So that users can run this script from anywhere and it will work as expected.
-SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" > /dev/null && pwd)"
-REPO_DIR="$(dirname "$SCRIPT_DIR")"
+readonly SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" > /dev/null && pwd)"


Neat, I didn't know bash had this.

mklim · 2019-09-27T03:53:57Z

script/lint_darwin_plugins.sh

@@ -0,0 +1,88 @@
+#!/bin/bash


So it's kind of scattered. A lot of our CI scripts start off in bash here, but then ultimately end up deferred to a completely separate Dart tool maintained in the flutter/plugin_tools repo. It's kind of hard to follow but the flow is that CI calls a bash script, and the bash script downloads and runs random commands from that repo by calling the package via flutter_plugin_tools, see these lines.

I think it probably makes the most sense for this functionality to live as a command in that tool instead of in bash given how much is going on here. The meat of this is easier to do in shell since it needs to run pod lint, but I'm pretty sure that repo does expose process run commands that could be called instead. That said the bash is relatively straightforward here so I wouldn't block the PR on moving it either, I don't think maintaining it in this form would be a huge burden.

mklim

LGTM

mklim · 2019-09-27T21:25:32Z

script/lint_darwin_plugins.sh

+    # Build as frameworks.
+    # This will also run any tests set up as a test_spec. See https://blog.cocoapods.org/CocoaPods-1.3.0.
+    pod lib lint "$podspec_dir" --allow-warnings --fail-fast --silent
+    if [[ $? -ne 0 ]]; then


nit: Please quote the variables here and throughout.

https://google.github.io/styleguide/shell.xml?showone=Quoting#Quoting

Thanks, I forgot to check that. Hopefully I got them all.

- Limit the supported podspec platform to iOS so tests don't run (and fail) for macOS. - Define the module by setting `DEFINES_MODULE` in the podspec. See [CocoaPod modular headers docs](http://blog.cocoapods.org/CocoaPods-1.5.0/). - Explicitly set `VALID_ARCHS` to architectures included in the Flutter universal binary. This is the CocoaPods-suggested workaround to prevent tests from running on the i386 simulator. See CocoaPods/CocoaPods#8159.

WIP [camera] Define clang modules in for iOS

1f79430

jmagman self-assigned this Sep 27, 2019

googlebot added the cla: yes label Sep 27, 2019

jmagman requested review from mklim and stuartmorgan-g September 27, 2019 02:45

jmagman changed the title ~~WIP [camera] Define clang modules in for iOS~~ [camera] Define clang modules in for iOS Sep 27, 2019

jmagman marked this pull request as ready for review September 27, 2019 02:46

jmagman requested a review from bparrishMines as a code owner September 27, 2019 02:46

jmagman commented Sep 27, 2019

View reviewed changes

script/lint_darwin_plugins.sh Outdated Show resolved Hide resolved

stuartmorgan-g reviewed Sep 27, 2019

View reviewed changes

This was referenced Sep 27, 2019

Add --analyze flag to pod lib lint pre-submit plugin tests flutter/flutter#41443

Closed

Remove --allow-warnings flag from pod lib lint pre-submit plugin tests flutter/flutter#41444

Closed

readonly, rename podspec_dir, add flag TODOs

f14c551

mklim reviewed Sep 27, 2019

View reviewed changes

Find podspecs instead of checking a particular path

a7369c8

jmagman mentioned this pull request Sep 27, 2019

Move pod lib lint pre-submit plugin tests to flutter_plugin_tools flutter/flutter#41511

Closed

mklim approved these changes Sep 27, 2019

View reviewed changes

Quotes

6dab76b

stuartmorgan-g approved these changes Sep 30, 2019

View reviewed changes

mklim merged commit 15b8029 into flutter:master Sep 30, 2019

jmagman deleted the module-camera branch September 30, 2019 19:24

jmagman mentioned this pull request Sep 30, 2019

[instrumentation_adapter] Report Flutter test result via XCTest #2117

Closed

13 tasks

jmagman mentioned this pull request Nov 14, 2019

camera/CameraPlugin.h' file not found flutter/flutter#32910

Closed

jmagman mentioned this pull request Jul 1, 2021

When plugins do not support ARM iOS simulators, the app fails to build flutter/flutter#85713

Closed

jmagman mentioned this pull request Sep 13, 2022

[pigeon] simplify apple platform tests with pod lib lint flutter/flutter#111399

Closed

[camera] Define clang modules in for iOS #2115

[camera] Define clang modules in for iOS #2115

Uh oh!

Conversation

jmagman commented Sep 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Tests

Checklist

Breaking Change

Uh oh!

jmagman commented Sep 27, 2019

Uh oh!

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!

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!

mklim 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jmagman commented Sep 27, 2019 •

edited

Loading