Skip to content

Upgrade Flutter, packages and Android build dependencies #1474

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

Merged
merged 11 commits into from
Apr 18, 2025

Conversation

rajveermalviya
Copy link
Member

No description provided.

Comment on lines -215 to +219
app_settings: 58017cd26b604ae98c3e65acbdd8ba173703cc82
device_info_plus: bf2e3232933866d73fe290f2942f2156cdd10342
app_settings: 5127ae0678de1dcc19f2293271c51d37c89428b2
device_info_plus: 21fcca2080fbcd348be798aa36c3e5ed849eefbe
Copy link
Member

Choose a reason for hiding this comment

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

These Podfile.lock changes appear to revert the changes made in 16e4d88. That's puzzling.

Copy link
Member Author

Choose a reason for hiding this comment

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

This commit was result of running tools/upgrade flutter-local and then running the app on both iOS and macOS.

I tried flutter run after flutter clean on this commit again, to make sure there is nothing wrong with build cache, but it still ends up making these changes.

Copy link
Member

Choose a reason for hiding this comment

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

What happens if you run the app before this commit (and with Flutter at the previous version)? If that makes the same change, then that change should happen in its own commit.

These are third-party packages, not dependencies of Flutter, so it'd be surprising if the versions we get for them in CocoaPods are affected by the Flutter version.

It'd also be good to sort out what the difference is between the versions before and after this change. Is device_info_plus of 21fcca2080fbcd348be798aa36c3e5ed849eefbe a newer or older version than bf2e3232933866d73fe290f2942f2156cdd10342?

It looks like the current pattern may be that when you run the app on your machine, you get the version from after this change (which is also the version from before 16e4d88), but when Chris runs the app on his machine he gets the version that's currently in main. We'd like to instead get to a recipe that everyone can run (maybe running pod repo update and then running the app?) that reproduces the same results on each machine, and for those results to represent an up-to-date version of each dependency.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added a separate base commit that introduces these Podfile.lock changes.

Copy link
Member

Choose a reason for hiding this comment

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

Cool.

The commit message should mention the fact that it's reverting a previous commit — that's quite relevant information for understanding what we're thinking with this change.

@gnprice gnprice self-assigned this Apr 17, 2025
@gnprice gnprice added the integration review Added by maintainers when PR may be ready for integration label Apr 17, 2025
Copy link
Member

@gnprice gnprice left a comment

Choose a reason for hiding this comment

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

Thanks @rajveermalviya for taking care of this! I've now read through all the changes. A few comments below (plus the subthread above).

file_picker: ^9.0.2
file_picker: ^10.1.2
Copy link
Member

Choose a reason for hiding this comment

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

Changelog:
  https://pub.dev/packages/file_picker/changelog#1012

There's one breaking change for an option we didn't use,
and some bug fixes.

So the changelog says:

BREAKING CHANGE: The compressionQuality property in the pickFiles method now defaults to 0.
BREAKING CHANGE: The allowCompression property has been deprecated in favor of compressionQuality, and now defaults to false.

We don't use either allowCompression or compressionQuality. I'm not sure what the meaning of compressionQuality 0 is, though — that's a change to the default, so the fact that we don't pass the option means we are affected.

Would you confirm what that value means?

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, the docs don't say much, but looking at the implementation:

  • On Android it will manually compress any image picked by the file picker, if compressionQuality > 0, then provide the compressed result.
  • On iOS, it will set presets for image and video files if compressionQuality is non-zero, but ignoring the actual compressionQuality value, where the OS will do the compression instead of plugin doing it manually.

Previously, the defaults were allowCompression = true and compressionQuality = 30. I doubt we needed that, so I guess this breaking change is for the better.

Copy link
Member

Choose a reason for hiding this comment

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

Great. OK, so the net effect is:

  • It used to compress the images.
  • Now it doesn't.

Looks like the PR that made the change is this: miguelpruivo/flutter_file_picker#1744

And it was described as fixing two issues, which are both about how the manual compression it did on Android caused problems.

The iOS implementation looks like it should work nice and smoothly, though, so that there it comes down to a product question of whether we want the images (and videos) compressed. Compressing uploaded images is a feature request we've gotten a few times, and considered on our own before that:
#1419
zulip/zulip-mobile#4340 (comment)
zulip/zulip-mobile#3499
zulip/zulip-mobile#2749

(That latest issue was filed in this repo, but I now suspect it's about zulip-mobile — it says "The current zulip android client" and doesn't give any indication of being aware of the distinction.)

Anyway, let's go ahead with the upgrade and take the new defaults. We'll see if we get feedback one way or another. We'll revise this commit message to be clear about the change, though.

Copy link
Member

Choose a reason for hiding this comment

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

(And as Chris pointed out below, we have #855 to move off that library — then we'll get whatever behavior upstream's package:image_picker offers.)

Comment on lines +4 to +8
<issue
id="InvalidPackage"
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.1.0/6ba44a9ddf8f6f2b4bc88e8bc64269aea0424607/tika-core-3.1.0.jar"/>
Copy link
Member

Choose a reason for hiding this comment

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

Additionally, add a suppression of a lint error caused by
a dependency of this package [i.e., file_picker].

I see, annoying.

Seems like the impact of this, if there is one, would be that it crashes when trying to read some XML file. Is there a file_picker upstream issue for this? If we're lucky, someone has already investigated and has a convincing reason it won't matter for our use (or perhaps even for file_picker's use in general).

Copy link
Member

Choose a reason for hiding this comment

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

I think you said in today's call that you looked in the package's tracker and didn't find an issue for this.

That's fine and we can leave it there — this probably doesn't affect our app's behavior, and we're planning to switch off this library in the future anyway. The commit message should just mention that, though: the fact that you looked and didn't find an upstream issue.

url: "https://pub.dev"
source: hosted
version: "2.21.1"
version: "2.22.0"
firebase_messaging:
Copy link
Member

Choose a reason for hiding this comment

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

This commit is result of 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#3130
  https://pub.dev/packages/firebase_messaging/changelog#1525

Notable changes include bump to Firebase Android BoM (33.9.0 to 33.11.0)
and Firebase iOS SDK (11.8.0 to 11.10.0), changelog for those are at:
  https://firebase.google.com/support/release-notes/android
  https://firebase.google.com/support/release-notes/ios

One bug fix on Android:
  https://firebase.google.com/support/release-notes/android#messaging_v24-1-1

And one migration on iOS to use a newer API:
  https://github.com/firebase/firebase-ios-sdk/pull/14457

Thanks for tracking all these down!

Comment on lines -6 to -8
extension ColorSwatchChecks<T> on Subject<ColorSwatch<T>> {
/// package:checks-style wrapper for [flutter_matcher.isSameColorSwatchAs].
void isSameColorSwatchAs(ColorSwatch<T> colorSwatch) {
Copy link
Member

Choose a reason for hiding this comment

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

Additionally, use the newly available check for `isSameColorSwatchAs`,
replacing the local copy.

🎉

Comment on lines 11 to 17
<issue
id="SyntheticAccessor"
message="Access to `private` method `deepEqualsNotifications` of class `Notifications_gKt` requires synthetic accessor"
errorLine1=" return deepEqualsNotifications(toList(), other.toList()) }"
errorLine2=" ~~~~~~~~~~~~~~~~~~~~~~~">
<location
file="src/main/kotlin/com/zulip/flutter/Notifications.g.kt"
Copy link
Member

Choose a reason for hiding this comment

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

Hmm. Is there an upstream issue in Pigeon tracking this?

Copy link
Member

Choose a reason for hiding this comment

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

Since there's a lot of copies of this, it seems likely that they'd churn frequently when we make changes to our pigeons and/or when unrelated Pigeon upgrades cause the line numbers to change.

So it'd be good to express this lint-ignore in a way that's more stable. Can we say e.g. that this lint rule should be ignored for this file? (Perhaps Pigeon should be emitting such a suppression in the generated file?)

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, turns out this just got fixed in flutter/packages#9034. Updated past that now.

@chrisbobbe
Copy link
Collaborator

(I see a few comments about file_picker; here's a small reminder that #855 is for moving off of that. 🙂)

rajveermalviya added a commit to rajveermalviya/zulip-flutter that referenced this pull request Apr 17, 2025
Changelog:
  https://pub.dev/packages/[file_picker/changelog#1012](https://github.com/file_picker/changelog/issues/1012)

There are two breaking changes, one about deprecating an option
`allowCompression` and changing the default to false (was true),
and another about changing default for `compressionQuality` to 0
(was 30). This means previously it used to compress media by
default and now it doesn't. See discussion for why we take in the
new defaults:
  zulip#1474 (comment)

Additionally, add a suppression of a lint error caused by
a dependency of package:file_picker.
@rajveermalviya rajveermalviya force-pushed the upgrade-flutter branch 2 times, most recently from f71a341 to f86ed2e Compare April 17, 2025 21:45
@rajveermalviya
Copy link
Member Author

Thanks for the review @gnprice! PTAL.

@rajveermalviya rajveermalviya requested a review from gnprice April 17, 2025 21:57
Copy link
Member

@gnprice gnprice left a comment

Choose a reason for hiding this comment

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

Thanks for the revision!

These changes all look good. Just a couple more things to mention in the commit messages at this point.

Comment on lines -215 to +219
app_settings: 58017cd26b604ae98c3e65acbdd8ba173703cc82
device_info_plus: bf2e3232933866d73fe290f2942f2156cdd10342
app_settings: 5127ae0678de1dcc19f2293271c51d37c89428b2
device_info_plus: 21fcca2080fbcd348be798aa36c3e5ed849eefbe
Copy link
Member

Choose a reason for hiding this comment

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

Cool.

The commit message should mention the fact that it's reverting a previous commit — that's quite relevant information for understanding what we're thinking with this change.

file_picker: ^9.0.2
file_picker: ^10.1.2
Copy link
Member

Choose a reason for hiding this comment

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

Changelog:
  https://pub.dev/packages/[file_picker/changelog#1012](https://github.com/file_picker/changelog/issues/1012)

This link seems to have gotten corrupted 🙂

Comment on lines +4 to +8
<issue
id="InvalidPackage"
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.1.0/6ba44a9ddf8f6f2b4bc88e8bc64269aea0424607/tika-core-3.1.0.jar"/>
Copy link
Member

Choose a reason for hiding this comment

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

I think you said in today's call that you looked in the package's tracker and didn't find an issue for this.

That's fine and we can leave it there — this probably doesn't affect our app's behavior, and we're planning to switch off this library in the future anyway. The commit message should just mention that, though: the fact that you looked and didn't find an upstream issue.

rajveermalviya added a commit to rajveermalviya/zulip-flutter that referenced this pull request Apr 18, 2025
Changelog:
  https://pub.dev/packages/file_picker/changelog#1012

There are two breaking changes, one about deprecating an option
`allowCompression` and changing the default to false (was true),
and another about changing default for `compressionQuality` to 0
(was 30). This means previously it used to compress media by
default and now it doesn't. See discussion for why we take in the
new defaults:
  zulip#1474 (comment)

There is also a lint error that is caused by a dependency of
package:file_picker. I didn't find any reports about it upstream,
and we probably aren't affected by it (confirmed by running simple
upload tests of a PDF, an XML file and an image, and didn't see any
errors/crashes), so for now added a suppression for that error.
Also we're planning to switch away from this library anyway (zulip#855).
@rajveermalviya
Copy link
Member Author

Thanks for the review @gnprice! Pushed an update, PTAL.

@rajveermalviya rajveermalviya requested a review from gnprice April 18, 2025 15:32
@gnprice
Copy link
Member

gnprice commented Apr 18, 2025

Thanks for the revision! All looks good now — merging.

This reverts 16e4d88.

These are made automatically when running `flutter run`, probably
because of the updated Ruby version on my machine, than what Chris
had when he authored that commit.

See discussion:
  https://chat.zulip.org/#narrow/channel/243-mobile-team/topic/Podfile.2Elock.20clash.20.23F92/near/2156970
Changelog:
  https://pub.dev/packages/file_picker/changelog#1012

There are two breaking changes, one about deprecating an option
`allowCompression` and changing the default to false (was true),
and another about changing default for `compressionQuality` to 0
(was 30). This means previously it used to compress media by
default and now it doesn't. See discussion for why we take in the
new defaults:
  zulip#1474 (comment)

There is also a lint error that is caused by a dependency of
package:file_picker. I didn't find any reports about it upstream,
and we probably aren't affected by it (confirmed by running simple
upload tests of a PDF, an XML file and an image, and didn't see any
errors/crashes), so for now added a suppression for that error.
Also we're planning to switch away from this library anyway (zulip#855).
This commit is result of 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#3130
  https://pub.dev/packages/firebase_messaging/changelog#1525

Notable changes include bump to Firebase Android BoM (33.9.0 to 33.11.0)
and Firebase iOS SDK (11.8.0 to 11.10.0), changelog for those are at:
  https://firebase.google.com/support/release-notes/android
  https://firebase.google.com/support/release-notes/ios

One bug fix on Android:
  https://firebase.google.com/support/release-notes/android#messaging_v24-1-1

And one migration on iOS to use a newer API:
  firebase/firebase-ios-sdk#14457
Changelog:
  https://pub.dev/packages/flutter_checks/changelog#012---2025-04-11

Additionally, use the newly available check for `isSameColorSwatchAs`,
replacing the local copy.
Changelog:
  https://pub.dev/packages/pigeon/changelog#2531

Updates to the generator to generate equality methods for
the classes, and some bug fixes.
Changelogs:
  https://docs.gradle.org/8.10/release-notes.html
  https://docs.gradle.org/8.11/release-notes.html
  https://docs.gradle.org/8.12/release-notes.html
  https://docs.gradle.org/8.13/release-notes.html

Some highlights:
  * 8.10 "Configuration cache improvements"
  * 8.11 More "Configuration Cache improvements"
    - Specifically, the new flag `org.gradle.configuration-cache.parallel=true`
  * 8.12 "Error and warning reporting improvements"
  * 8.13 "Toolchain support"
Release notes:
  https://kotlinlang.org/docs/whatsnew2020.html
  https://kotlinlang.org/docs/whatsnew21.html
  https://kotlinlang.org/docs/whatsnew2120.html

Highlights:

* 2.0.20
  - Standard library; Support for UUIDs.
  - Otherwise mostly related to Wasm, Native and Kotlin
    Multiplatform.

* 2.1.0
  - K2; Extra compiler checks.
  - Otherwise mostly related to Wasm, Native and Kotlin
    Multiplatform.

* 2.1.20
  - Mostly new experimental features.
Release notes:
  https://developer.android.com/build/releases/past-releases/agp-8-6-0-release-notes
  https://developer.android.com/build/releases/past-releases/agp-8-7-0-release-notes
  https://developer.android.com/build/releases/past-releases/agp-8-8-0-release-notes
  https://developer.android.com/build/releases/gradle-plugin (for 8.9, for now)

Changes mostly seem to be bug fixes to various components.

One notable change is that AGP version 8.9 now requires
Android Studio Meerkat (2024.3.1 Patch 1).

Additionally, use the new extension `toUri` to make the linter
happy.
@gnprice gnprice merged commit b431d8d into zulip:main Apr 18, 2025
@rajveermalviya rajveermalviya deleted the upgrade-flutter branch May 5, 2025 16:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integration review Added by maintainers when PR may be ready for integration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants