Skip to content

Upgrade Flutter, packages and Android build dependencies #1504

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 6 commits into from
May 16, 2025

Conversation

rajveermalviya
Copy link
Member

@rajveermalviya rajveermalviya commented May 8, 2025

@rajveermalviya rajveermalviya requested a review from chrisbobbe May 13, 2025 16:32
@rajveermalviya rajveermalviya added the maintainer review PR ready for review by Zulip maintainers label May 13, 2025
@chrisbobbe
Copy link
Collaborator

chrisbobbe commented May 15, 2025

LGTM, thanks! Marking for Greg's review.

@chrisbobbe chrisbobbe added integration review Added by maintainers when PR may be ready for integration and removed maintainer review PR ready for review by Zulip maintainers labels May 15, 2025
@chrisbobbe chrisbobbe requested a review from gnprice May 15, 2025 23:18
@chrisbobbe chrisbobbe assigned gnprice and unassigned chrisbobbe May 15, 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! Looks good, except one comment below.

Flutter: e0871f40cf51350855a761d2e70bf5af5b9b5de7
Flutter: cabc95a1d2626b1b06e7179b784ebcf0c0cde467
Copy link
Member

Choose a reason for hiding this comment

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

ios build: Take auto updates to Podfile.lock

This commit is a result of running `flutter run`.

Probably a side-effect of the change in
37dc9eaa441f893417e8bf048e989f7f892fae85.

There are two changes in this commit, and probably only one of them is related to that previous commit, right? Namely the "PODFILE CHECKSUM" change. (I believe we discussed this on a recent call.)

This change to the Flutter hash is presumably instead due to the last couple of Flutter upgrades (which I made without running on iOS).

The commit's changes are fine, but let's say a few more words to distinguish which change we're attributing to what cause.

@rajveermalviya
Copy link
Member Author

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

@rajveermalviya rajveermalviya requested a review from gnprice May 16, 2025 03:07
@gnprice
Copy link
Member

gnprice commented May 16, 2025

Thanks! That first commit message looks good now. But it seems like the new revision makes a smaller Flutter upgrade than the previous revision:

$ git range-diff origin pr/1504@{{1,0}}
[…]
2:  a0427063c ! 2:  ed4d55850 deps: Upgrade Flutter to 3.33.0-1.0.pre.44
    @@ Metadata
     Author: Rajesh Malviya <[email protected]>
     
      ## Commit message ##
    -    deps: Upgrade Flutter to 3.33.0-1.0.pre.44
    +    deps: Upgrade Flutter to 3.32.0-1.0.pre.446
     
      ## macos/Podfile.lock ##
     @@ macos/Podfile.lock: SPEC CHECKSUMS:
    @@ pubspec.lock: packages:
      sdks:
     -  dart: ">=3.9.0-63.0.dev <4.0.0"
     -  flutter: ">=3.32.0-1.0.pre.332"
    -+  dart: ">=3.9.0-114.0.dev <4.0.0"
    -+  flutter: ">=3.33.0-1.0.pre.44"
    ++  dart: ">=3.9.0-97.0.dev <4.0.0"
    ++  flutter: ">=3.32.0-1.0.pre.446"
     
      ## pubspec.yaml ##
     @@ pubspec.yaml: environment:
        # We use a recent version of Flutter from its main channel, and
        # the corresponding recent version of the Dart SDK.
        # Feel free to update these regularly; see README.md for instructions.
     -  sdk: '>=3.9.0-63.0.dev <4.0.0'
     -  flutter: '>=3.32.0-1.0.pre.332'  # adae8bbdbaed53ef305726fcfe811b2351d73a1a
    -+  sdk: '>=3.9.0-114.0.dev <4.0.0'
    -+  flutter: '>=3.33.0-1.0.pre.44'  # 358b0726882869cd917e1e2dc07b6c298e6c2992
    ++  sdk: '>=3.9.0-97.0.dev <4.0.0'
    ++  flutter: '>=3.32.0-1.0.pre.446'  # 3a71fefdcaf1ce569dfab580db81e48894696bfb

Was that intended?

@gnprice
Copy link
Member

gnprice commented May 16, 2025

(I'd imagined using git rebase -i to just edit the first commit message, not rerunning tools/upgrade or making any other edits to the later commits.)

@rajveermalviya
Copy link
Member Author

Was that intended?

I'd imagined using git rebase -i to just edit the first commit message, not rerunning tools/upgrade or making any other edits to the later commits.

That was definitely unintended and a bad rebase because I'd done exactly that, only reword-ed the first commit message, but not sure what resulted in all the unintended changes.

Just tried to do it again with the previous revision, and saw this error, even when I did not have any local changes/unstaged files:

error: Your local changes to the following files would be overwritten by merge:
        pubspec.lock
Please commit your changes or stash them before you merge.
Aborting
hint: Could not execute the todo command
hint:
hint:     pick a0427063cc92075a15667c045b54d1fa6b2df6d6 deps: Upgrade Flutter to 3.33.0-1.0.pre.44
hint:
hint: It has been rescheduled; To edit the command before continuing, please
hint: edit the todo list first:
hint:
hint:     git rebase --edit-todo
hint:     git rebase --continue

Previously I think I did git rebase --continue after seeing this message which resulted in the bad rebase, but now I did git rebase --edit-todo then exited the editor without any changes and then ran git rebase --continue again, and now surprisingly the state of the branch seems correct. So pushed that now.

@gnprice
Copy link
Member

gnprice commented May 16, 2025

Hmm interesting. I'm not sure what would have led to that error, or to the result we observed after that.

The main takeaway here is a reminder that when pushing a revision to a PR, it's important to check your work to be sure that the revision is what you intend. A key tool for this is git range-diff. When pushing a revision, the commands I run look like:

$ git diff --stat -p me/pr-upgrade-flutter @
$ git range-diff origin me/pr-upgrade-flutter @
$ git push me +@:pr-upgrade-flutter

Sometimes the git diff output has a lot of intended changes that are just due to rebasing forward, but the git range-diff helps isolate those away.


Less important than the practice of rereading your changes, but in case it's informative: the other thing I notice now is that the revision with that unintended change was based on an older version of main:

$ git reflog show pr/1504@{now}
812bdb541 (HEAD -> main, pr/1504) refs/remotes/pr/1504@{Fri May 16 10:15:58 2025 -0700}: fetch origin +pull/1504/head:refs/remotes/pr/1504: forced-update
c0c8b3769 refs/remotes/pr/1504@{Thu May 15 21:51:19 2025 -0700}: fetch pr: forced-update
7beb65ff1 refs/remotes/pr/1504@{Thu May 15 10:00:15 2025 -0700}: fetch pr: forced-update
b84ea6079 refs/remotes/pr/1504@{Tue May 13 11:51:25 2025 -0700}: fetch pr: forced-update
b6e944410 refs/remotes/pr/1504@{Thu May 8 10:36:02 2025 -0700}: fetch pr: storing ref

$ git log --oneline --graph --boundary pr/1504@{2}...pr/1504@{1}
* c0c8b3769 deps android: Upgrade Android Gradle Plugin to 8.10.0, from 8.9.1
* c8405e6c2 deps android: Upgrade Gradle to 8.14, from 8.13
* ca0b53e06 deps: Upgrade packages within constraints (tools/upgrade pub)
* 291f06c4f deps: Update share_plus to 11.0.0, from 10.1.4
* ed4d55850 deps: Upgrade Flutter to 3.32.0-1.0.pre.446
* 178fb2d56 ios build: Take auto updates to Podfile.lock
| * 7beb65ff1 deps android: Upgrade Android Gradle Plugin to 8.10.0, from 8.9.1
| * 341b9f53a deps android: Upgrade Gradle to 8.14, from 8.13
| * 3cb811bf1 deps: Upgrade packages within constraints (tools/upgrade pub)
| * c7c40315a deps: Update share_plus to 11.0.0, from 10.1.4
| * a0427063c deps: Upgrade Flutter to 3.33.0-1.0.pre.44
| * ae4bd75dc ios build: Take auto updates to Podfile.lock
| * 710ebd9bb (pr/1488, origin/main, origin/HEAD) nixos: Update to use libepoxy, jdk17, nodejs
| * c5e695738 (pr/1512) msglist [nfc]: Handle start markers within widget code, not model
| * 4b69adfae msglist [nfc]: Make start/loading indicators their own widgets
| * a6e0efde2 msglist [nfc]: Simplify a bit by centralizing `model!` null-assertion
| * 86e2397d0 msglist [nfc]: Assert the model is non-null a bit more
| * 0c0300997 (pr/1509) binding [nfc]: Add utcNow
| * fa5dfd568 test [nfc]: Add utcTimestamp
| * b982a781d store [nfc]: Move zulip{FeatureLevel,Version} to PerAccountStoreBase
| * 373e23a69 api [nfc]: Add TopicName.processLikeServer
| * 000cc84e1 (pr/1365) api: Indicate support for handling empty topics
| * 4c198de6e api: Make displayName nullable
| * 070e72062 compose: Change content input hint text statefully
| * b4344c723 compose: Change topic input hint text
| * 25eace6ba compose [nfc]: Convert _TopicInput to a StatefulWidget
| * 5700b4296 compose: Fix empty topics not being shown correctly in hint text
| * 7f203273d store [nfc]: Assert that FL>=334 when accessing realmEmptyTopicName
| * 91699172c test: Bump recentZulipFeatureLevel to FL 382, from FL 278
|/  
o 1e8f2c257 (pr/1475) msglist test [nfc]: Add MessageEvent test group

So the revision c0c8b37 with the unexpectedly less-new Flutter version was based on 1e8f2c2, but the previous revision 7beb65f was based on the current main which is 710ebd9.

@gnprice
Copy link
Member

gnprice commented May 16, 2025

Thanks for the revision!

Speaking of additional changes 🙂 : this one restores the Flutter version to the same as the earlier revision, but also shortens the commit IDs in the first commit message:

         The change to "PODFILE CHECKSUM" is probably a side-effect of the
    -    change in 37dc9eaa4.
    +    change in 37dc9ea.
     
         And the change to "Flutter" entry in "SPEC CHECKSUMS" is probably
    -    a result of the previous Flutter upgrade in 7dfad7c42.
    +    a result of the previous Flutter upgrade in 7dfad7c.

Those 7-hex-digit IDs are too short for use in long-lived text; a good length is 9 hex digits, like in the previous revision. See https://github.com/zulip/zulip-mobile/blob/main/docs/style.md#mentioning-commits .

(Going afk at the moment, or else I would fix that and merge.)

This commit is a result of running `flutter run`.

The change to "PODFILE CHECKSUM" is probably a side-effect of the
change in 37dc9ea.

And the change to "Flutter" entry in "SPEC CHECKSUMS" is probably
a result of the previous Flutter upgrade in 7dfad7c.
Changelog:
  https://pub.dev/packages/share_plus/changelog#1100

Just one change, a fairly large refactor:
  fluttercommunity/plus_plugins@0a19d4601

So, this commit also includes the switch to use the newer API.
Release notes:
  https://developer.android.com/build/releases/past-releases/agp-8-9-0-release-notes#android-gradle-plugin-8.9.2
  https://developer.android.com/build/releases/gradle-plugin (for 8.10.0, for now)

Changes mostly seem to be bug fixes to various components.

One notable change is that AGP version 8.10 now requires
Android Studio Meerkat Feature Drop (2024.3.2).
@gnprice gnprice force-pushed the pr-upgrade-flutter branch from 812bdb5 to 9689dce Compare May 16, 2025 21:29
@gnprice gnprice merged commit 9689dce into zulip:main May 16, 2025
1 check passed
@gnprice
Copy link
Member

gnprice commented May 16, 2025

OK, fixed that and merging. Thanks again @rajveermalviya!

@rajveermalviya rajveermalviya deleted the pr-upgrade-flutter branch May 19, 2025 04:57
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