Closed rajveermalviya closed 2 months ago
(If Android builds were part of Flutter's Customer test suite (https://github.com/zulip/zulip-flutter/issues/774), it would have added more friction on https://github.com/flutter/engine/pull/55463.)
Thanks for digging into this! I noticed CI fail on #967 last night, but it was right at the end of my day so I didn't investigate.
If I understand the commit messages right, tools/check --all
would always fail at the middle commit, right? That commit makes us start requiring a Flutter version that will always cause that warning.
In that case, can the two commits go in the opposite order instead? I expect that way things work at every step (if you use the minimum required Flutter version), which is our usual approach because it makes things a bit cleaner to understand.
Thanks for the review @gnprice, pushed a revision with reordered commits and updated commit message.
Thanks! Looks good — merging.
I tweaked the commit message a bit:
- Without this update, Android builds will fail with Flutter
- SDK revisions after this commit[0], due to our strict
- warnings as errors config in `app/build.gradle`.
+ Without this update, Android builds will fail with Flutter SDK
+ versions after the following change that landed yesterday:
+ https://github.com/flutter/engine/pull/55463
+ https://github.com/flutter/flutter/commit/7bb9352911d0471568a77fc57b94a646fe9571af
+ due to our strict warnings as errors config in `app/build.gradle`.
The build error would look like this:
/…/video_player_android-2.7.3/android/src/main/java/io/flutter/plugins/videoplayer/VideoPlayer.java:88:
warning: [removal] onSurfaceCreated() in Callback has been deprecated and marked for removal
public void onSurfaceCreated() {
^
error: warnings found and -Werror specified
- [0]: https://github.com/flutter/engine/commit/c2c242d50cf8d78057f347f600460f17c102e927
-
mainly because the reference to "this commit" sounded like it meant this commit, i.e. the one the commit message is on. (I think just putting the link immediately after the reference, instead of down in a footnote, would have been enough to fix that problem. But then once I was editing it, I changed "this" to "the following" to further disambiguate, and added a bit more information too.)
(If Android builds were part of Flutter's Customer test suite (#774), it would have added more friction on flutter/engine#55463.)
This is a good reminder that #774 would be good to do… but I think having our Android build there wouldn't actually have affected this. That's because that repo's policy is that deprecation warnings are to be ignored. It says:
The tests must pass even if there are analysis 'info' level issues in the code. Generally, this means that if the test performs static analysis, it does so ignoring info level items (i.e.,
flutter analyze --no-fatal-infos
).
and the point of that is precisely about deprecation warnings — they want to be able to add deprecations and have those not be breaking changes. (Later when they actually remove the deprecated item, I guess that's a breaking change if people haven't migrated.)
So that's something I'll want to watch for when implementing #774. I'll make a note there.
Fixes the CI failure: https://github.com/zulip/zulip-flutter/actions/runs/11063896720/job/30740794793#step:7:1899