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

deps: Upgrade Flutter SDK and video_player_android #968

Merged
merged 2 commits into from
Sep 27, 2024

Conversation

rajveermalviya
Copy link
Member

@rajveermalviya rajveermalviya commented Sep 27, 2024

@rajveermalviya
Copy link
Member Author

(If Android builds were part of Flutter's Customer test suite (#774), it would have added more friction on flutter/engine#55463.)

@rajveermalviya rajveermalviya added the maintainer review PR ready for review by Zulip maintainers label Sep 27, 2024
@gnprice
Copy link
Member

gnprice commented Sep 27, 2024

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.

@rajveermalviya rajveermalviya force-pushed the pr-update-flutter branch 2 times, most recently from 38d0834 to 6f0c8c6 Compare September 27, 2024 18:30
@rajveermalviya
Copy link
Member Author

Thanks for the review @gnprice, pushed a revision with reordered commits and updated commit message.

Changelog:
  https://pub.dev/packages/video_player_android/changelog

This update addresses a few bug fixes, including the
suppression of deprecation warnings for `onSurfaceCreated`,
which was introduced in an early version of the
Impeller-compatible Texture APIs. For more info, see note of
the migration guide:
  https://docs.flutter.dev/release/breaking-changes/android-surface-plugins#migration-guide

Without this update, Android builds will fail with Flutter SDK
versions after the following change that landed yesterday:
  flutter/engine#55463
  flutter/flutter@7bb9352
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
And update Flutter's supporting libraries to match.
@gnprice
Copy link
Member

gnprice commented Sep 27, 2024

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.)

@gnprice gnprice merged commit 6979b50 into zulip:main Sep 27, 2024
1 check passed
@rajveermalviya rajveermalviya deleted the pr-update-flutter branch September 27, 2024 18:51
@gnprice
Copy link
Member

gnprice commented Sep 27, 2024

(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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintainer review PR ready for review by Zulip maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants