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

Upgrade Flutter and all packages to latest #621

Merged
merged 6 commits into from
Apr 6, 2024

Conversation

chrisbobbe
Copy link
Collaborator

@chrisbobbe chrisbobbe commented Apr 5, 2024

Done because it's the first step in our plan for #612, the "Missing API declaration" warning from Apple.

Changelogs for deps that are major-version upgraded:
https://pub.dev/packages/device_info_plus/changelog
https://pub.dev/packages/file_picker/changelog
https://pub.dev/packages/flutter_local_notifications/changelog
https://pub.dev/packages/package_info_plus/changelog
https://pub.dev/packages/share_plus/changelog
https://pub.dev/packages/pigeon/changelog

Manual tests done:

  • My iPhone 13 Pro running iOS 17.3.1:
    • App starts up; pages load (including Inbox and message list)
  • Office Android device (Samsung Galaxy S9 running Android 9):
    • App starts up; pages load (including Inbox and message list)
    • DM notification received; navigates to conversation when tapped
    • device_info_plus exercised and seems to work correctly (discussion)

Related: #612

@chrisbobbe chrisbobbe requested a review from gnprice April 5, 2024 22:02
@gnprice
Copy link
Member

gnprice commented Apr 5, 2024

Thanks!

All these changes look good. I won't have time to read those changelogs before heading out on vacation; but if you've read them and nothing jumps out at you, and given that it passes your manual testing, please go ahead and merge. I looked through all these changes other than reading the changelogs.

All of these changelogs except `pigeon` mention adding "privacy
info" or "privacy manifest" in the version we're taking here --
exactly the kind of thing we want to see, with #612 in mind.

Great!

I would
guess that `pigeon` doesn't need one, as it's mainly just a code
generator tool.

Yeah. And more concretely: it's a dev dependency only, so it's not part of the actual app build.

@chrisbobbe
Copy link
Collaborator Author

Hmm, a CI failure:

Error: there were changes to pigeons:
 M android/app/src/main/kotlin/com/zulip/flutter/Notifications.g.kt
 M lib/host/android_notifications.g.dart

But when I run tools/check pigeon or tools/check pigeon --all-files locally, at the tip of this PR branch, it succeeds without error.

@gnprice
Copy link
Member

gnprice commented Apr 5, 2024

Ah, and CI is failing — looks like you just need to update the Pigeon-generated output.

(And then I guess rerun the manual testing, unless the changes are trivial.)

@gnprice
Copy link
Member

gnprice commented Apr 5, 2024

Huh odd.

I guess one thing you can do to debug is to edit tools/check to print the full diff in that case, rather than the status.

@gnprice
Copy link
Member

gnprice commented Apr 5, 2024

Alternatively, try making a meaningless edit to the Pigeon input file and rerunning Pigeon — wonder if this is a caching issue, like the one we've seen with other code generators (using build_runner).

@chrisbobbe
Copy link
Collaborator Author

I guess one thing you can do to debug is to edit tools/check to print the full diff in that case, rather than the status.

Aha, this seems like a useful clue from that output:

-// Autogenerated from Pigeon (v17.2.0), do not edit directly.
+// Autogenerated from Pigeon (v18.0.0), do not edit directly.

@chrisbobbe
Copy link
Collaborator Author

chrisbobbe commented Apr 5, 2024

The issue seems to be that the old Pigeon 17 is running locally despite the upgrade in the headline commit taking it to Pigeon 18. The command tools/check pigeon --all-files kept succeeding.

Then I did something different:

$ dart run pigeon
Resolving dependencies in `/Users/chrisbobbe/dev/zulip-flutter`... 
Downloading packages... 
Got dependencies in `/Users/chrisbobbe/dev/zulip-flutter`.
Building package executable... (5.2s)
Built pigeon:pigeon.
Pigeon is a tool for generating type-safe communication code between Flutter
and the host platform.

usage: pigeon --input <pigeon path> --dart_out <dart path> [option]*
[…]

and thereafter, tools/check pigeon --all-files is failing, which is the expected behavior from Pigeon 18. I think the Building package executable... step is what got it working. Is there a nice way to make sure that step gets run automatically whenever our Pigeon version changes?

@chrisbobbe
Copy link
Collaborator Author

Anyway, revision pushed; tools/check pigeon should be happy now.

@chrisbobbe
Copy link
Collaborator Author

And I've just redone manual tests for Android notifications. (The code did look NFC to me, but just for good measure.)

All these changes look good. I won't have time to read those changelogs before heading out on vacation; but if you've read them and nothing jumps out at you, and given that it passes your manual testing, please go ahead and merge.

Cool; yep. Merging.

@chrisbobbe
Copy link
Collaborator Author

Filed #622 for the Pigeon version issue.

In 2023-09, just 2.2% of our zulip-mobile Android users were below
this version, according to stats Greg posted:
  https://chat.zulip.org/#narrow/stream/48-mobile/topic/platform.20versions/near/1648299

So I think by now we can be comfortable making this the new minumum.

I'm not sure if this is actually strictly required by some
dependency upgrades we'll take later in this series of commits. But
Android 9 is what the office Android device is on, and the core team
doesn't have a physical device running a lower version. So we'll
validate the upcoming dependency upgrades experimentally, by running
on the office Android device. And with this bump to the threshold,
we can leave it at that.

Discussion:
  https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/flutter.3A.20Java.20versions.3F/near/1775991
And update Flutter's supporting libraries to match.
Done because it's the first step in our plan for zulip#612, the "Missing
API declaration" warning from Apple.

This is the result of `tools/upgrade pod-major`. No breaking changes
requiring synchronous code changes were found.

Changelogs:
  https://pub.dev/packages/device_info_plus/changelog
  https://pub.dev/packages/file_picker/changelog
  https://pub.dev/packages/flutter_local_notifications/changelog
  https://pub.dev/packages/package_info_plus/changelog
  https://pub.dev/packages/share_plus/changelog
  https://pub.dev/packages/pigeon/changelog

All of these changelogs except `pigeon` mention adding "privacy
info" or "privacy manifest" in the version we're taking here --
exactly the kind of thing we want to see, with zulip#612 in mind. And
`pigeon` shouldn't need one, as it's a dev dependency only, not part
of the actual build.

Related: zulip#612
We could almost certainly bump it to 15, based on the last
platform-version statistics Greg posted, and that was in 2023-09:
  https://chat.zulip.org/#narrow/stream/48-mobile/topic/platform.20versions/near/1648327

But there isn't specific code we want to write for iOS 15 features
right now, so might as well leave that for later.

In fact, I don't think there's a particularly strong need to bump it
to 14, at least at the moment. But there's no reason it needs to lag
behind zulip-mobile. And in case the lower threshold would be an
obstacle later on, it's nice to have it cleared away proactively
ahead of time.
@chrisbobbe chrisbobbe merged commit a7ed31e into zulip:main Apr 6, 2024
1 check passed
@chrisbobbe chrisbobbe deleted the pr-deps-latest-2024-04-05 branch April 6, 2024 00:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants