Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[iOS] specify minimum os version for native asset frameworks #148504

Merged

Conversation

knopp
Copy link
Member

@knopp knopp commented May 16, 2024

Fixes #148501

Pre-launch Checklist

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

@flutter-dashboard
Copy link

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 "@test-exemption-reviewer" in the #hackers channel in Chat (don't just cc them here, they won't see it! Use 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.

@github-actions github-actions bot added tool Affects the "flutter" command-line tool. See also t: labels. a: desktop Running on desktop labels May 16, 2024
@knopp
Copy link
Member Author

knopp commented May 16, 2024

@dcharkes is the plist file tested anywhere? Also the approach here is a bit ugly, the issue is that the plist key is different for macOS and iOS, and also as far as I can tell it is not mandatory.

@knopp knopp marked this pull request as draft May 16, 2024 18:38
@flutter-dashboard
Copy link

This pull request has been changed to a draft. The currently pending flutter-gold status will not be able to resolve until a new commit is pushed or the change is marked ready for review again.

For more guidance, visit Writing a golden file test for package:flutter.

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

@@ -230,7 +230,7 @@ Future<void> _copyNativeAssetsIOS(
}
await lipoDylibs(dylibFile, sources);
await setInstallNameDylib(dylibFile);
await createInfoPlist(targetUri.pathSegments.last, frameworkDir);
await createInfoPlist(targetUri.pathSegments.last, frameworkDir, minimumIOSVersion: '12.0');
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this value should come from somewhere.

(Also this value should be passed into the BuildConfig so that the build hook can fail if it cannot produce a dylib with the right iOS version (dart-lang/native#1133). This can be done in a separate PR though.)

@dcharkes
Copy link
Contributor

Do we have the minimum iOS version that the app we're building is targeting somewhere? See the code that does the same for minimum android version: #148044 (comment)

@dcharkes
Copy link
Contributor

@dcharkes is the plist file tested anywhere?

No. You could add it here:

void expectDylibIsBundledMacOS(Directory appDirectory, String buildMode) {
final Directory appBundle = appDirectory.childDirectory('build/$hostOs/Build/Products/${buildMode.upperCaseFirst()}/$exampleAppName.app');
expect(appBundle, exists);
final Directory frameworksFolder =
appBundle.childDirectory('Contents/Frameworks');
expect(frameworksFolder, exists);
// MyFramework.framework/
// MyFramework -> Versions/Current/MyFramework
// Resources -> Versions/Current/Resources
// Versions/
// A/
// MyFramework
// Resources/
// Info.plist
// Current -> A
const String frameworkName = packageName;
final Directory frameworkDir =
frameworksFolder.childDirectory('$frameworkName.framework');
final Directory versionsDir = frameworkDir.childDirectory('Versions');
final Directory versionADir = versionsDir.childDirectory('A');
final Directory resourcesDir = versionADir.childDirectory('Resources');
expect(resourcesDir, exists);
final File dylibFile = versionADir.childFile(frameworkName);
expect(dylibFile, exists);
final Link currentLink = versionsDir.childLink('Current');
expect(currentLink, exists);
expect(currentLink.resolveSymbolicLinksSync(), versionADir.path);
final Link resourcesLink = frameworkDir.childLink('Resources');
expect(resourcesLink, exists);
expect(resourcesLink.resolveSymbolicLinksSync(), resourcesDir.path);
final Link dylibLink = frameworkDir.childLink(frameworkName);
expect(dylibLink, exists);
expect(dylibLink.resolveSymbolicLinksSync(), dylibFile.path);
}

@knopp
Copy link
Member Author

knopp commented May 16, 2024

Do we have the minimum iOS version that the app we're building is targeting somewhere? See the code that does the same for minimum android version: #148044 (comment)

I think this is right now hardcoded to 12.0 for App.framework inside AppFrameworkInfo.plist. There is also IPHONEOS_DEPLOYMENT_TARGET set by xcode during build that contains project deployment target. But I think the plugin should be able to specify own deployment target, that seems like the cleanest way.

@dcharkes
Copy link
Contributor

I think this is right now hardcoded to 12.0 for App.framework inside AppFrameworkInfo.plist. There is also IPHONEOS_DEPLOYMENT_TARGET set by xcode during build that contains project deployment target. But I think the plugin should be able to specify own deployment target, that seems like the cleanest way.

cc @vashworth and @jmagman What is the interplay between XCode and a Flutter app on the minimal iOS version?

@jmagman
Copy link
Member

jmagman commented May 16, 2024

What is the interplay between XCode and a Flutter app on the minimal iOS version?

The target version is the minimum support by latest Xcode. https://flutter.dev/go/match-xcode-deployment-range

This document proposes that Flutter’s supported deployment platform versions be updated when Xcode is released annually to match its deployment versions. Versions lower than the Xcode range will become unsupported.

Right now it's set in the tool in a bunch of places, see #140823 for the last time we had to bump it (note the changes in dev and example weren't manually updated, those are automatically migrated example/test apps).

@vashworth filed #145104 to make this settable in fewer places.

@dcharkes
Copy link
Contributor

Thanks @jmagman!

Then I'm fine landing as is once we added a test. And maybe leave a comment on #145104 so that it doesn't get forgotten.

@knopp
Copy link
Member Author

knopp commented May 20, 2024

@dcharkes, I added test and a TODO to wire things once hook can specify deployment target. There is another issue - the bundle identifier must not contain underscores, we should replace them by - probably. Should that be part of this PR or should I make separate PR for it?

@knopp
Copy link
Member Author

knopp commented May 20, 2024

Sanitizing the bundle identifier seems like a tiny change, I added it to this PR.

@knopp knopp marked this pull request as ready for review May 20, 2024 10:07
@knopp knopp force-pushed the native_assets_specify_min_ios_version branch from e33408b to 2237e92 Compare May 20, 2024 14:46
@knopp
Copy link
Member Author

knopp commented May 20, 2024

@dcharkes, just a heads up, with this PR we have been able to get an app uploaded through AppStore connect.

Copy link
Contributor

@dcharkes dcharkes left a comment

Choose a reason for hiding this comment

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

Thanks so much for triaging and supplying a PR @knopp! ❤️ LGTM! 🚀

@knopp knopp added the autosubmit Merge PR when tree becomes green via auto submit App label May 21, 2024
@auto-submit auto-submit bot merged commit 454dd7e into flutter:master May 21, 2024
130 checks passed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 22, 2024
auto-submit bot pushed a commit to flutter/packages that referenced this pull request May 23, 2024
flutter/flutter@d02292d...73bf206

2024-05-22 102401667+Dimilkalathiya@users.noreply.github.com `CupertinoDialogRoute` leak fix (flutter/flutter#148774)
2024-05-22 fluttergithubbot@gmail.com Marks Windows plugin_test to be flaky (flutter/flutter#148835)
2024-05-22 sokolovskyi.konstantin@gmail.com Add tests for actions.0.dart API example. (flutter/flutter#148678)
2024-05-22 tessertaha@gmail.com Introduce `WidgetStateBorderSide.lerp` (flutter/flutter#148122)
2024-05-22 holzgeist@users.noreply.github.com add `default-flavor` field to flutter pubspec, which will be used as the flavor in `flutter build/run` if `--flavor` is not provided (flutter/flutter#147968)
2024-05-22 katelovett@google.com [wiki migration] Pages under docs/postmortems/ (flutter/flutter#148798)
2024-05-22 engine-flutter-autoroll@skia.org Roll Flutter Engine from e5a73e520e89 to c89defa55801 (2 revisions) (flutter/flutter#148812)
2024-05-22 victorsanniay@gmail.com Make hover tests functional and cleanup mouse pointers in Material toggleables (flutter/flutter#148808)
2024-05-21 amirpanahandeh@yahoo.com Fix two dimensional viewport unexpected null exception when no child is laid out (flutter/flutter#148256)
2024-05-21 engine-flutter-autoroll@skia.org Roll Flutter Engine from bc1345b6b50a to e5a73e520e89 (3 revisions) (flutter/flutter#148807)
2024-05-21 32538273+ValentinVignal@users.noreply.github.com Add test for undo_history_controller.0.dart (flutter/flutter#148205)
2024-05-21 engine-flutter-autoroll@skia.org Roll Flutter Engine from a8872c8915a2 to bc1345b6b50a (6 revisions) (flutter/flutter#148802)
2024-05-21 polinach@google.com Fix test that leaks images. (flutter/flutter#148494)
2024-05-21 34871572+gmackall@users.noreply.github.com Fix warnings in `dependency_version_checker.gradle.kts` (flutter/flutter#148699)
2024-05-21 katelovett@google.com [wiki migration] Android team pages (flutter/flutter#148585)
2024-05-21 polinach@google.com Fix leaky test. (flutter/flutter#148788)
2024-05-21 bruno.leroux@gmail.com Add DropdownButton.menuWidth (flutter/flutter#148125)
2024-05-21 82763757+NobodyForNothing@users.noreply.github.com Add test for focus example 2 (flutter/flutter#147624)
2024-05-21 34871572+gmackall@users.noreply.github.com Add a migrator to remove `FlutterMultiDexApplication.java` (flutter/flutter#148515)
2024-05-21 katelovett@google.com [wiki migration] Infra team pages (flutter/flutter#148718)
2024-05-21 engine-flutter-autoroll@skia.org Roll Flutter Engine from 8a352f01e503 to a8872c8915a2 (1 revision) (flutter/flutter#148776)
2024-05-21 jacksongardner@google.com Fix the output of the CDN test. (flutter/flutter#148730)
2024-05-21 katelovett@google.com [wiki migration] Release team pages (flutter/flutter#148723)
2024-05-21 ian@hixie.ch Remove hidden dependencies on LocalProcessManager (flutter/flutter#148096)
2024-05-21 pateltirth454@gmail.com Adds Missing `onHover` & `onFocusChange` for `OutlinedButton.icon` (flutter/flutter#144374)
2024-05-21 82763757+NobodyForNothing@users.noreply.github.com Adds tests to NestedScrollView examples (flutter/flutter#148170)
2024-05-21 engine-flutter-autoroll@skia.org Roll Flutter Engine from c2ef01f6f1ab to 8a352f01e503 (18 revisions) (flutter/flutter#148766)
2024-05-21 nate.w5687@gmail.com `switch` expressions: finale (flutter/flutter#148711)
2024-05-21 matej.knopp@gmail.com [iOS] specify minimum os version for native asset frameworks (flutter/flutter#148504)
2024-05-21 59215665+davidhicks980@users.noreply.github.com Removed brand references from MenuAnchor.dart (flutter/flutter#148760)
2024-05-21 zanderso@users.noreply.github.com Skip flaky test in expression_evaluation_test.dart (flutter/flutter#148737)

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
Please CC rmistry@google.com,stuartmorgan@google.com,tarrinneal@google.com on the revert to ensure that a human
is aware of the problem.

To file a bug in Packages: 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a: desktop Running on desktop autosubmit Merge PR when tree becomes green via auto submit App tool Affects the "flutter" command-line tool. See also t: labels.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[iOS] native asset framework is missing MinimumOSVersion
3 participants