-
Notifications
You must be signed in to change notification settings - Fork 29.8k
[iOS] specify minimum os version for native asset frameworks #148504
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
Conversation
|
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. |
|
@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. |
|
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 Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
There was a problem hiding this comment.
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.)
|
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) |
No. You could add it here: flutter/packages/flutter_tools/test/integration.shard/isolated/native_assets_test.dart Lines 314 to 348 in 6b38361
|
I think this is right now hardcoded to 12.0 for |
cc @vashworth and @jmagman 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
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 @vashworth filed #145104 to make this settable in fewer places. |
|
@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? |
|
Sanitizing the bundle identifier seems like a tiny change, I added it to this PR. |
e33408b to
2237e92
Compare
|
@dcharkes, just a heads up, with this PR we have been able to get an app uploaded through AppStore connect. |
dcharkes
left a comment
There was a problem hiding this 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! 🚀
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
|
I've been trying to build and release via TestFlight an app using Thermion (which requires iOS 13.0). This commit adds the capability to provide the version from the script and the only thing missing (if I understood the conversation) is ensuring a value passed via configuration is used here. I think that part was pending on dart-lang/native#1133 ? correct, @knopp ? It seems closed now. Is there something I could do to help? |
|
@tnarik This works the other way around, the end user specifies the minimum iOS version in their app. Then this version is passed into the build hook and is available as in https://pub.dev/documentation/code_assets/latest/code_assets/IOSCodeConfig/targetVersion.html. You should then throw an |
|
I see, so this would be for the package to support that hook? should I let the developer of Thermion know? apologies if I am asking nonsensical questions, I've been playing aroudn with Flutter, but I don't understand fully the build process. I only noticed that the native assets (Thermion ones at least) are set to 12.0, while everything else I have seen as a result of 'flutter build ios' is 13.0, and the minimum iOS version for the default App is 13.0 (but I still see that 12.0 in flutter_tools, which is confusing me). edit: or, let's see if I get it: what this and dart-lang/native#1133 add is to allow the hook to fail if the package would not be compatible with that target version, BUT it has nothing to do with the fact that the generated , because that is what the native_assets script uses in order to generate the Info.plist? |
This is not true. (I would like it to be true, but we don't read the cocoapods specs to fish out the number.) It's a hardcoded number right now. Bumping it from 12 to 13 to align with the Flutter supported lowest version: @tnarik, since Flutter only supports 13 and up, it should probably be fine as long as the Flutter SDK lower bound on your package is the that moved the lowest supported version up to 13. |
We don't have a single place to update the minimum iOS version in the code base: * #145104 So, when the minimum version was bumped, one place was missed. Thanks for reporting @tnarik! (#148504 (comment))
We don't have a single place to update the minimum iOS version in the code base: * flutter#145104 So, when the minimum version was bumped, one place was missed. Thanks for reporting @tnarik! (flutter#148504 (comment))
We don't have a single place to update the minimum iOS version in the code base: * flutter#145104 So, when the minimum version was bumped, one place was missed. Thanks for reporting @tnarik! (flutter#148504 (comment))
Fixes #148501
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.