Conversation
|
CI is failing because of new deprecations introduced by flutter/flutter@2391bdb, and this PR uses a Flutter version that is couple of commits older than this commit. Will send another revision to use latest Flutter SDK, and fix the deprecations. |
|
Another failure in the Android build is because upstream Flutter SDK broke the |
abd32cd to
e5bbade
Compare
Our CI is broken at the moment because of changes in recent Flutter commits upstream. Usually we'd just upgrade to the latest and make any corresponding updates in our code. That's blocked for a couple of reasons, including a change in the Android embedding: zulip#2129 (comment) While that's still getting resolved, fix CI by having it run on the same version that's in a comment in pubspec.yaml, which is the version we'd use when making a release. Earlier today on a pairing call, Rajesh and I developed a version of this that's more cleanly organized, including some refactoring. He hasn't yet sent that branch as a PR, though, and it's night-time for him now. I'll still be glad to have that refactoring, but the immediate priority is to get the CI fixed.
Our CI is broken at the moment because of changes in recent Flutter commits upstream. Usually we'd just upgrade to the latest and make any corresponding updates in our code. That's blocked for a couple of reasons, including a change in the Android embedding: zulip#2129 (comment) While that's still getting resolved, fix CI by having it run on the same version that's in a comment in pubspec.yaml, which is the version we'd use when making a release. Earlier today on a pairing call, Rajesh and I developed a version of this that's more cleanly organized, including some refactoring. He hasn't yet sent that branch as a PR, though, and it's night-time for him now. I'll still be glad to have that refactoring, but the immediate priority is to get the CI fixed.
Our CI is broken at the moment because of changes in recent Flutter commits upstream. Usually we'd just upgrade to the latest and make any corresponding updates in our code. That's blocked for a couple of reasons, including a change in the Android embedding: zulip#2129 (comment) While that's still getting resolved, fix CI by having it run on the same version that's in a comment in pubspec.yaml, which is the version we'd use when making a release. Earlier today on a pairing call, Rajesh and I developed a version of this that's more cleanly organized, including some refactoring. He hasn't yet sent that branch as a PR, though, and it's night-time for him now. I'll still be glad to have that refactoring, but the immediate priority is to get the CI fixed.
e4c9afc to
656651a
Compare
|
Now that we are using a pinned Flutter version in CI (#2138, #2140), we can update to a Flutter version that is recent-ish and one that doesn't cause us any issues. The new revision updates Flutter version to flutter/flutter@f3a6493 (merged 4 days ago), which is one commit before the Android related change (flutter/flutter@99c5bd4) that is causing the build failures. So, @chrisbobbe this should be ready for review again. |
|
Cool! LGTM, and passes a quick smoke test on the iOS simulator. Marking for Greg's review. |
gnprice
left a comment
There was a problem hiding this comment.
Thanks @rajveermalviya for taking care of this, and @chrisbobbe for the testing! Small comments below, entirely on the commit messages.
| sdk: '>=3.12.0-72.0.dev <4.0.0' | ||
| flutter: '>=3.41.0-1.0.pre-283' # 055f3e5bf58354cb50eb55f7e35d03d8e15a6f07 | ||
| sdk: '>=3.12.0-128.0.dev <4.0.0' | ||
| flutter: '>=3.42.0-1.0.pre-36' # f3a64931b166cb42b822c3f51d89a7f95f277e7d |
There was a problem hiding this comment.
Since this differs from our usual habit of updating to whatever's the latest as of when we make the upgrade, let's include in the commit message a mention of that fact and a link to your reasoning here:
#2129 (comment)
| version: "2.29.0" | ||
| version: "2.31.0" |
There was a problem hiding this comment.
nit in commit message:
Notable packages:drift changes:
should be package:, singular
| version: "2.29.0" | ||
| version: "2.31.0" |
There was a problem hiding this comment.
From the drift_dev changelog:
Support dothand syntax when analyzing Dart sources.
Neat — that sounds like it fixes an issue I ran into in my draft branch for E2EE notifications. I'll have to see if I can go back to dot-shorthand syntax there.
There was a problem hiding this comment.
(Confirmed — this works, which didn't before:
Column<int> get accountId => integer()
- .references(Accounts, #id, onDelete: .cascade)();
+ .references(Accounts, #id, onDelete: KeyAction.cascade)();)
| for (final row in await select(intGlobalSettings).get()) | ||
| if (IntGlobalSetting.byName(row.name) case final setting?) | ||
| setting: row.value | ||
| ?IntGlobalSetting.byName(row.name): row.value |
There was a problem hiding this comment.
Huh neat. This bit especially seems like it gets nicely simplified by using null-aware elements.
| name: lints | ||
| sha256: a5e2b223cb7c9c8efdc663ef484fdd95bb243bff242ef5b13e26883547fce9a0 | ||
| sha256: "12f842a479589fea194fe5c5a3095abc7be0c1f2ddfa9a0e76aed1dbd26a87df" | ||
| url: "https://pub.dev" | ||
| source: hosted | ||
| version: "6.0.0" | ||
| version: "6.1.0" |
There was a problem hiding this comment.
The commit message says:
It now enables two new rules:
- use_null_aware_elements
https://dart.dev/tools/linter-rules/use_null_aware_elements
- no_wildcard_variable_uses
https://dart.dev/tools/linter-rules/no_wildcard_variable_uses
So, fix the code by applying the suggestions in the IDE to make
`flutter analyze` happy.
It looks like all the edits are from just one of those rules, though, namely use_null_aware_elements. Is that right? If so it'd be good to make clear in the commit message.
| message="Invalid package reference in library; not included in Android: `javax.xml.stream`. Referenced from `org.apache.tika.utils.XMLReaderUtils`."> | ||
| <location | ||
| file="$GRADLE_USER_HOME/caches/modules-2/files-2.1/org.apache.tika/tika-core/3.2.0/9232bb3c71f231e8228f570071c0e1ea29d40115/tika-core-3.2.0.jar"/> | ||
| file="$GRADLE_USER_HOME/caches/modules-2/files-2.1/org.apache.tika/tika-core/3.2.3/4b1b82f8cce72c9bd3676532c8b613e24041d96c/tika-core-3.2.3.jar"/> |
There was a problem hiding this comment.
The ignored issue in `android/app/lint-baseline.xml` regarding the
Apache Tika library still occurs with this newer version. Tika
library was updated in the newer version:
https://github.com/miguelpruivo/flutter_file_picker/commit/66fedc4f6
and because of that Android lint was complaining about this error again
but didn't ignore it because the error's version didn't match the
baseline file. So, this commit also updates the `lint-baseline.xml`
by running `./gradlew updateLintBaseline` in the android directory.
Annoying. Thanks for taking care of this.
Generally we would update to the latest Flutter version, but currently latest version has a bug which breaks the Android build: flutter/flutter#182153 So, instead of the latest version this upgrade is to one commit before the Android build failure causing commit, see: zulip#2129 (comment)
656651a to
492807b
Compare
|
Thanks for the review @gnprice! Pushed an update, PTAL. |
Generally we would update to the latest Flutter version, but currently latest version has a bug which breaks the Android build: flutter/flutter#182153 So, instead of the latest version this upgrade is to one commit before the Android build failure causing commit, see: zulip#2129 (comment)
492807b to
0ef0a40
Compare
Generally we would update to the latest Flutter version, but currently latest version has a bug which breaks the Android build: flutter/flutter#182153 So, instead of the latest version this upgrade is to one commit before the Android build failure causing commit, see: zulip#2129 (comment)
This commit is the result of the following commands: flutter pub upgrade --major-versions firebase_messaging firebase_core pod update --project-directory=ios/ pod update --project-directory=macos/ Changelogs: https://pub.dev/packages/firebase_core/changelog#440 https://pub.dev/packages/firebase_messaging/changelog#1611 package:firebase_messaging on iOS, adds support for projects migrated to use SceneDelegate. Another change is a fix for duplicate notifications sent to the onMessage handlers, but we do not use those on iOS so we are unaffected by it. Other notable changes include bump to Firebase Android BoM (34.4.0 to 34.7.0) and Firebase iOS SDK (12.4.0 to 12.8.0), changelog for those are at: https://firebase.google.com/support/release-notes/android https://firebase.google.com/support/release-notes/ios For Android SDK, no notable changes in the FCM component. For iOS SDK, there was one bug fix for a potential crash due to a race condition with Realm DB: firebase/firebase-ios-sdk#14880 (comment) but we do not use Realm DB so it doesn't affect us. But either way the fix involves recovering from a corrupted database rather than a crash so it is a welcome change: firebase/firebase-ios-sdk#15573
This commit is the result of the following commands: flutter pub upgrade --major-versions pigeon tools/check --all-files --fix pigeon Changelog: https://pub.dev/packages/pigeon/changelog#2617 One notable change is a fix for some warnings on Android when using EventChannel API: flutter/flutter#178908 flutter/packages#10632 Other changes are related to ProxyApi, which we do not use.
This commit is the result of the following commands: flutter pub upgrade --major-versions drift drift_dev sqlite3 sqlite3_flutter_libs tools/check --all-files --fix drift pod update --project-directory=ios/ pod update --project-directory=macos/ Changelog: https://pub.dev/packages/drift/changelog#2310 https://pub.dev/packages/drift_dev/changelog#2310 https://pub.dev/packages/sqlite3_flutter_libs/changelog#0541 (package:sqlite3 is pinned to 2.x by package:drift currently.) Notable package:drift changes: - Now supports rightOuterJoin and fullOuterJoin operators: simolus3/drift#3718 simolus3/drift@f3c522225 - Step-by-step migration now throws when attempting a downgrade, where previously it could corrupt the database. It didn't affect us because we reset the database when downgrading anyway: simolus3/drift@918348783
This commit is the result of the following commands: flutter pub upgrade --major-versions app_settings pod update --project-directory=ios/ pod update --project-directory=macos/ Changelog: https://pub.dev/packages/app_settings/changelog#700
This commit is the result of the following commands: flutter pub upgrade path_provider_foundation pod update --project-directory=ios/ pod update --project-directory=macos/ Changelog: https://pub.dev/packages/path_provider_foundation/changelog#260 It now calls to Foundation APIs directly using FFI instead of Pigeon/MethodChannels.
Changelog: https://pub.dev/packages/lints/changelog#610 It now enables two new rules: - use_null_aware_elements https://dart.dev/tools/linter-rules/use_null_aware_elements - no_wildcard_variable_uses https://dart.dev/tools/linter-rules/no_wildcard_variable_uses So, fix the code by applying the suggestions in the IDE, which were all for the `use_null_aware_elements` rule, to make `flutter analyze` happy.
This commit is the result of the following commands: flutter pub upgrade file_picker pod update --project-directory=ios/ pod update --project-directory=macos/ cd android && ./gradlew updateLintBaseline Changelog: https://pub.dev/packages/file_picker/changelog#10310 The ignored issue in `android/app/lint-baseline.xml` regarding the Apache Tika library still occurs with this newer version. Tika library was updated in the newer version: miguelpruivo/flutter_file_picker@66fedc4f6 and because of that Android lint was complaining about this error again but didn't ignore it because the error's version didn't match the baseline file. So, this commit also updates the `lint-baseline.xml` by running `./gradlew updateLintBaseline` in the android directory.
|
Thanks! Looks good; merging. |
0ef0a40 to
43302ac
Compare
This was introduced in aacf52a (zulip#2136), after I rebased that PR and immediately merged. The rebase went past 5705f83 (zulip#2129), which had enabled the `use_null_aware_elements` lint rule, so this was effectively an indirect merge conflict.
Our CI is broken at the moment because of changes in recent Flutter commits upstream. Usually we'd just upgrade to the latest and make any corresponding updates in our code. That's blocked for a couple of reasons, including a change in the Android embedding: zulip#2129 (comment) While that's still getting resolved, fix CI by having it run on the same version that's in a comment in pubspec.yaml, which is the version we'd use when making a release. Earlier today on a pairing call, Rajesh and I developed a version of this that's more cleanly organized, including some refactoring. He hasn't yet sent that branch as a PR, though, and it's night-time for him now. I'll still be glad to have that refactoring, but the immediate priority is to get the CI fixed.
Generally we would update to the latest Flutter version, but currently latest version has a bug which breaks the Android build: flutter/flutter#182153 So, instead of the latest version this upgrade is to one commit before the Android build failure causing commit, see: zulip#2129 (comment)
This was introduced in 7584e9f (zulip#2136), after I rebased that PR and immediately merged. The rebase went past a82d79b (zulip#2129), which had enabled the `use_null_aware_elements` lint rule, so this was effectively an indirect merge conflict.
Flutter notable commits