Force plugins to inherit minimum macOS version from Flutter app by jmagman · Pull Request #72764 · flutter/flutter

jmagman · 2020-12-22T01:05:29Z

Background

macOS version of #66590

Xcode supports specific SDK ranges, and every release they drop the lowest. For example, Xcode 12 dropped support for 10.8.
Xcode emits a warning when build targets support a minimum version lower than the range to indicate it can't validate compilation against the older SDK.

The macOS deployment target 'MACOSX_DEPLOYMENT_TARGET' is set to 10.6, but the range of supported deployment target versions is 10.9 to 10.16.99

This warning is benign: if the app supports a minimum of macOS 10.9, it doesn't cause any problems that a plugin framework embedded in the app supports macOS 10.8. These warnings concern users who think there's something wrong with their app, and there's no way for users to fix it (the plugin author would have to up their minimum supported version).

This warning was seen in flutter_gallery because the Reachability pod supported 10.8.

Description

If the plugin's minimum version is less than macOS 10.9, remove the version build setting. This will cause it to inherit from the minimum version set in the Flutter app (currently 10.11).

If the plugin minimum is higher than macOS 10.9, leave the build setting so if there is a versioning range issue (the plugin supports a minimum macOS 11 but the app needs 10.11) the correct error (not warning) will be emitted.

Related Issues

iOS issue at #40109, there's no macOS equivalent.

Tests

Added macOS to the plugin integration test. Confirm this test fails on master.

flutter-dashboard · 2020-12-22T01:05:32Z

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.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

jmagman · 2020-12-22T01:06:09Z

Leftover debugging log.

christopherfujino · 2020-12-22T02:50:02Z

should these *'s be +'s?

should we validate that there are in fact two capture group matches?

should these *'s be +'s?

No, then "11" is captured as "1" and "1".

should we validate that there are in fact two capture group matches?

If there's only one capture deployment_target_minor will be set to "", which is coerced to 0. So "11" is 11 and 0 which is what we want.

/(\d+).?(\d*)/ might be more precise though; there does need to be a first number.

stuartmorgan-g · 2020-12-29T10:42:34Z

These warnings concern users who think there's something wrong with their app, and there's no way for users to fix it

Aren't we trading a single relatively clear warning for an arbitrary set of deprecated API warnings (also unfixable by the user), which may be confusing if reported to the plugin author? Or do we get those regardless when it's out of range?

jmagman · 2020-12-29T18:40:06Z

Aren't we trading a single relatively clear warning for an arbitrary set of deprecated API warnings (also unfixable by the user), which may be confusing if reported to the plugin author? Or do we get those regardless when it's out of range?

You get the deprecated API warnings regardless of whether it's out of range. I just tested this.

#import <Carbon/Carbon.h>
...
UpTime();

UpTime was deprecated in 10.8. Whether I set my project to 10.8, which is out of range (equivalent to not doing this PR) or 10.9 I still get 'UpTime' is deprecated: first deprecated in macOS 10.8.

jmagman · 2020-12-29T18:46:39Z

UpTime was deprecated in 10.8. Whether I set my project to 10.8, which is out of range (equivalent to not doing this PR) or 10.9 I still get 'UpTime' is deprecated: first deprecated in macOS 10.8.

Actually that experiment didn't make sense, it should have been 10.7 when that API wasn't deprecated. You're right, the deprecation warning does go away. However I'd still trade avoiding a warning for 100% of our users vs causing deprecation warnings for the ones that with pods using APIs deprecated before 10.9.

I really wish Xcode (it's not clang) didn't generate this warnings, or it was possible to turn it off. It's not very useful.

stuartmorgan-g · 2020-12-29T19:17:44Z

/(\d+).?(\d*)/ might be more precise though; there does need to be a first number.

stuartmorgan-g · 2020-12-29T19:22:00Z

To make the source of these numbers as clear as possible to future readers/maintainers, how about "Check if this is less than the minimum OS version supported by the latest stable version of Xcode (currently 10.9)"

The details are in the See MACOSX_DEPLOYMENT_TARGET note below. larger comment. I'll move it up.

stuartmorgan-g · 2020-12-29T19:22:53Z

This logic isn't correct; it treats 11.[0-8] as older. Would be good to add an 11.0 test case that would have caught that.

Well I failed the interview.

stuartmorgan-g · 2020-12-29T19:26:20Z

However I'd still trade avoiding a warning for 100% of our users vs causing deprecation warnings for the ones that with pods using APIs deprecated before 10.9.

I guess that makes sense, given that a module developer who really wants to keep supporting old versions could do it with SDK ifdefs (although that's painful enough I'm not sure how many actually would). I do worry a bit about module developers getting reports of warnings they can't reproduce in their own builds, but hopefully it'll be clear enough.

christopherfujino

lgtm

stuartmorgan-g · 2021-01-06T01:04:56Z

Was this ready for re-review? I thought it was waiting for a test update for the version comparison bug.

jmagman · 2021-01-06T02:08:42Z

Was this ready for re-review? I thought it was waiting for a test update for the version comparison bug.

I wasn't going to set up Ruby testing infrastructure for just this. The integration test covers the positive test but I don't have a negative test (which would have caught my silly mistake). Any suggestions?

stuartmorgan-g · 2021-01-06T14:30:09Z

Agreed Ruby testing would be overkill; I was assuming that the effect of that Ruby code would be to cause macos/Pods/Pods.xcodeproj/project.pbxproj not to contain a MACOSX_DEPLOYMENT_TARGET, in which case couldn't a test create a pod with a minimum version of 11, run a build, and validate that the file still has it?

jmagman · 2021-01-08T01:58:43Z

+        // it's not a regression in the IPHONEOS_DEPLOYMENT_TARGET override logic.
+        // The plugintest target should not have IPHONEOS_DEPLOYMENT_TARGET set.
+        // See _reduceDarwinPluginMinimumVersion for details.
+        if (target == 'ios' && 'IPHONEOS_DEPLOYMENT_TARGET'.allMatches(podsProjectContent).length != 6) {


@stuartmorgan I started out actually parsing the Xcode project for the plugintest target's build settings but that got pretty confusing. This count check is a bit of a cheat but it would definitely fail if the behavior was wrong or regressed...

Makes sense; I was thinking it wouldn't be in the project at all, so I didn't realize it would be non-trivial. Agreed that a full parser would be overkill here.

stuartmorgan-g

LGTM!

stuartmorgan-g · 2021-01-11T15:57:52Z

+        // it's not a regression in the IPHONEOS_DEPLOYMENT_TARGET override logic.
+        // The plugintest target should not have IPHONEOS_DEPLOYMENT_TARGET set.
+        // See _reduceDarwinPluginMinimumVersion for details.
+        if (target == 'ios' && 'IPHONEOS_DEPLOYMENT_TARGET'.allMatches(podsProjectContent).length != 6) {


Makes sense; I was thinking it wouldn't be in the project at all, so I didn't realize it would be non-trivial. Agreed that a full parser would be overkill here.

jmagman self-assigned this Dec 22, 2020

flutter-dashboard Bot added c: contributor-productivity Team-specific productivity, code health, technical debt. tool Affects the "flutter" command-line tool. See also t: labels. labels Dec 22, 2020

google-cla Bot added the cla: yes label Dec 22, 2020

jmagman added the platform-macos Building on or for macOS specifically label Dec 22, 2020

jmagman force-pushed the macos-min-version branch from 006ff3f to b158718 Compare December 22, 2020 01:07

jmagman commented Dec 22, 2020

View reviewed changes

Comment thread packages/flutter_tools/bin/podhelper.rb Outdated

jmagman Dec 22, 2020

Copy link
Copy Markdown

Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leftover debugging log.

jmagman requested review from christopherfujino and xster December 22, 2020 01:41

christopherfujino reviewed Dec 22, 2020

View reviewed changes

jmagman requested a review from stuartmorgan-g December 29, 2020 01:16

stuartmorgan-g requested changes Dec 29, 2020

View reviewed changes

jmagman force-pushed the macos-min-version branch from b158718 to ae837c6 Compare December 29, 2020 22:30

christopherfujino approved these changes Jan 6, 2021

View reviewed changes

jmagman added 3 commits January 7, 2021 17:56

Force plugins to inherit minimum macOS version from Flutter app

36342c7

Fix deployment ranges

0b7b2d3

Add cheese test

f3231bd

jmagman force-pushed the macos-min-version branch from ae837c6 to f3231bd Compare January 8, 2021 01:56

jmagman commented Jan 8, 2021

View reviewed changes

stuartmorgan-g approved these changes Jan 11, 2021

View reviewed changes

jmagman merged commit 66145fe into flutter:master Jan 11, 2021

jmagman deleted the macos-min-version branch January 11, 2021 19:16

jmagman mentioned this pull request Oct 7, 2021

Update number of IPHONEOS_DEPLOYMENT_TARGET in plugin_test_ios #91324

Merged

Conversation

jmagman commented Dec 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Background

Description

Related Issues

Tests

Uh oh!

flutter-dashboard Bot commented Dec 22, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

christopherfujino Dec 22, 2020 • 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!

jmagman Dec 22, 2020 • 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!

Choose a reason for hiding this comment

Uh oh!

stuartmorgan-g commented Dec 29, 2020

Uh oh!

jmagman commented Dec 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jmagman commented Dec 29, 2020

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 commented Dec 29, 2020

Uh oh!

christopherfujino left a comment

Choose a reason for hiding this comment

Uh oh!

stuartmorgan-g commented Jan 6, 2021

Uh oh!

jmagman commented Jan 6, 2021

Uh oh!

stuartmorgan-g commented Jan 6, 2021

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jmagman commented Dec 22, 2020 •

edited

Loading

christopherfujino Dec 22, 2020 •

edited

Loading

jmagman Dec 22, 2020 •

edited

Loading

jmagman commented Dec 29, 2020 •

edited

Loading