[multiple] Java 8 target for all plugins with -Werror compiler arg #3216

brunobowden · 2020-10-28T06:50:24Z

Description

Upgrading to Java 12 will cause the following innocuous warnings
to become build failures due to the "-Werror" flag:

> Task :connectivity:compileReleaseJavaWithJavac FAILED
warning: [options] source value 7 is obsolete and will be removed in a future release
warning: [options] target value 7 is obsolete and will be removed in a future release
warning: [options] To suppress warnings about obsolete options, use -Xlint:-options.
error: warnings found and -Werror specified
FAILURE: Build failed with an exception.

Example: https://github.com/WorldHealthOrganization/app/runs/1318596280

Fix

Target Java 8 for all plugins that use the -Werror compiler argument:

android {
    ...
    compileOptions {
        sourceCompatibility JavaVersion.VERSION_1_8
        targetCompatibility JavaVersion.VERSION_1_8
    }

The plugins were all found with:

$ find . -name build.gradle | xargs grep Werror
./packages/connectivity/connectivity/android/build.gradle:def args = ["-Xlint:deprecation","-Xlint:unchecked","-Werror"]
./packages/camera/android/build.gradle:def args = ["-Xlint:deprecation","-Xlint:unchecked","-Werror"]
./packages/video_player/video_player/android/build.gradle:def args = ["-Xlint:deprecation","-Xlint:unchecked","-Werror"]
./packages/android_alarm_manager/android/build.gradle:def args = ["-Xlint:deprecation","-Xlint:unchecked","-Werror"]
./packages/android_intent/android/build.gradle:def args = ["-Xlint:deprecation","-Xlint:unchecked","-Werror"]

I think the config was broken for video_player. Also updated a few example apps too.

Workaround

As a temporary workaround, developers can downgrade to Java 11 (e.g. 11.0.9) or lower
which doesn't warn that Java 7 is obsolete. Example:
https://github.com/WorldHealthOrganization/app/pull/1669/files#diff-710a4407aba47ffddf61ee9be9490428d5d0883759b18e490858db28e1ace4afR86

Regression

@hamdikahloun - thanks for your work on the Connectivity package. Adding -Werror
flags is a positive thing but I think is best combined with Java 8 compile to ensure that
the warnings don't become errors. Your PR: #3051

FYI @mehmetf, @Salakar, @bkonyi, @ened, @dnfield as you've all edited Java 8 targets

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

I read the Contributor Guide and followed the process outlined there for submitting PRs.
My PR includes unit or integration tests for all changed/updated/fixed behaviors (See Contributor Guide).
All existing and new tests are passing.

# The following tests fail in master and are unchanged by this PR:
$ pub global run flutter_plugin_tools test --plugins android_alarm_manager,android_intent,camera,connectivity,path_provider,video_player
...
Tests for the following packages are failing (see above):
 * connectivity/connectivity_for_web
 * path_provider/path_provider_linux/example

I updated/added relevant documentation (doc comments with ///).
The analyzer (flutter analyze) does not report any problems on my PR.

# This requires a very long command to run
$ pub global run flutter_plugin_tools analyze --custom-analysis camera,google_maps_flutter,in_app_purchase,url_launcher,video_player --plugins android_alarm_manager,android_intent,camera,connectivity,path_provider,video_player

I read and followed the Flutter Style Guide.
The title of the PR starts with the name of the plugin surrounded by square brackets, e.g. [shared_preferences]

Used [multiple]

I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy.
I updated CHANGELOG.md to add a description of the change.

Need to discuss if it's appropriate to update pubspec.yaml and the CHANGELOG

I signed the CLA.
I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require plugin users to manually update their apps to accommodate your change?

Yes, this is a breaking change (please indicate a breaking change in CHANGELOG.md and increment major revision).
No, this is not a breaking change.... though possibly an "Unbreaking" one

@hamdikahloun

PROBLEM: Upgrading to Java 12 will cause the following innocuous warnings to become build failures due to the "-Werror" flag: ``` > Task :connectivity:compileReleaseJavaWithJavac FAILED warning: [options] source value 7 is obsolete and will be removed in a future release warning: [options] target value 7 is obsolete and will be removed in a future release warning: [options] To suppress warnings about obsolete options, use -Xlint:-options. error: warnings found and -Werror specified FAILURE: Build failed with an exception. ``` Example: https://github.com/WorldHealthOrganization/app/runs/1318596280 FIX: Target Java 8 for all plugins that use the `-Werror` compiler argument: ``` android { ... compileOptions { sourceCompatibility JavaVersion.VERSION_1_8 targetCompatibility JavaVersion.VERSION_1_8 } ``` The plugins were all found with: ``` $ find . -name build.gradle | xargs grep Werror ./packages/connectivity/connectivity/android/build.gradle:def args = ["-Xlint:deprecation","-Xlint:unchecked","-Werror"] ./packages/camera/android/build.gradle:def args = ["-Xlint:deprecation","-Xlint:unchecked","-Werror"] ./packages/video_player/video_player/android/build.gradle:def args = ["-Xlint:deprecation","-Xlint:unchecked","-Werror"] ./packages/android_alarm_manager/android/build.gradle:def args = ["-Xlint:deprecation","-Xlint:unchecked","-Werror"] ./packages/android_intent/android/build.gradle:def args = ["-Xlint:deprecation","-Xlint:unchecked","-Werror"] ``` I think the config was broken for video_player. Also updated a few example apps too. WORKAROUND: As a temporary workaround, developers can downgrade to Java 11 which doesn't warn that Java 7 is obsolete. Example: https://github.com/WorldHealthOrganization/app/pull/1669/files#diff-710a4407aba47ffddf61ee9be9490428d5d0883759b18e490858db28e1ace4afR86 REGRESSION: @hamdikahloun - thanks for your work on the Connectivity package. Adding `-Werror` flags is a positive thing but I think is best combined with Java 8 to ensure that the warning don't become error. Your PR: flutter#3051 FYI @mehmetf, @Salakar, @bkonyi, @ened, @dnfield as you've all edited Java 8 targets

ened · 2020-10-28T10:31:27Z

@brunobowden Essentially, the target will be the same (Java 1.8), but it's important to use the constant, right? Therefore I would not see functional changes for most users.

Under which circumstance would you use Java 12 for Android though? I thought Android kind of lived in the past, toolchain wise. Is this introduced in the latest AGP versions?

Would you recommend adding the language level to generated plugin code by default?

IMO you will definitely need changelog & version bump (+ should be enough).

brunobowden · 2020-10-28T17:51:02Z

@ened - I was wary about making change too quickly but I essentially agree with everything you've said:

Under which circumstance would you use Java 12 for Android though? I thought Android kind of lived in the past, toolchain wise. Is this introduced in the latest AGP versions?

AGP 3.0.0 and above has limited support for Java 8 through desugaring (see links below). So use of some of the more advance features might cause issues.
https://developer.android.com/studio/write/java8-support
https://developer.android.com/studio/write/java8-support-table

Would you recommend adding the language level to generated plugin code by default?

I expect this is the right thing to do but I'd like to have someone with more experience weigh in on this. Since it should only be minimal wrapper files, they shouldn't need advanced Java capabilities... and using Java 8 LTS seems the most appropriate choice. Fundamentally pinning versions does improve stability and consistency.

IMO you will definitely need changelog & version bump (+ should be enough).
Essentially, the target will be the same (Java 1.8), but it's important to use the constant, right? Therefore I would not see functional changes for most users.

Correct for some of the plugins.... there's no change and no version bump is needed. Of the rest, 2 plugins have a clear change, the other 2 plugins I'm unsure about due to the nested "android" config in the build.gradle.

# Version Bump Needed
android_intent
connectivity

# Unsure - these two plugins has nested "android" within "android" settings
# and I'm unclear if this results in a change or not.
# build.gradle:
# android {
#     android {
#         compileOptions {
#         ....
path_provider
video_player

# No Change and no version bump
android_alarm_manager
camera

brunobowden · 2020-10-28T17:52:41Z

@ened - as for Java 12, that was mainly a random choice for the CI. We downgraded to Java 11 as it didn't consider Java 7 as obsolete. It took me quite a long time to track down the problem though and it's nice to solve it for others.... particularly the standard Flutter libraries.

brunobowden · 2020-11-14T18:30:58Z

@ened - please excuse me but I've been snowed under with the World Health Organization App. I will come back to this when I can or you're free to take it and finish it yourself.

stuartmorgan · 2021-04-27T20:32:43Z

@blasten Are you familiar with the settings here to review this? Is the outer android wrapper a previous format, or just a bug?

blasten

LGTM

blasten · 2021-04-27T20:53:34Z

@stuartmorgan that is a bug

stuartmorgan · 2021-04-28T15:51:27Z

@brunobowden If you can merge in latest master to resolve the conflict (that should also fix the Windows Plugins CI failure), and add the version bumps (erring on the side of doing it where it may not be strictly necessary is fine), we can get this landed.

brunobowden · 2021-04-28T16:00:19Z

I'll give it a go this morning...

…

On Wed, Apr 28, 2021 at 8:51 AM stuartmorgan ***@***.***> wrote: @brunobowden <https://github.com/brunobowden> If you can merge in latest master to resolve the conflict (that should also fix the Windows Plugins CI failure), and add the version bumps (erring on the side of doing it where it may not be strictly necessary is fine), we can get this landed. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3216 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAFF3K7VFEWNDM3FYUCPV2LTLAVJBANCNFSM4TB4VPEA> .

stuartmorgan · 2021-08-10T17:31:37Z

@blasten Do you have cycles to get this PR over the finish line?

brunobowden · 2021-08-11T13:57:37Z

I can probably get to it in a week or so's time.... but feel free to take it over and do it yourself as that may be easier. Bruno

…

On Tue, Aug 10, 2021 at 6:31 PM stuartmorgan ***@***.***> wrote: @blasten <https://github.com/blasten> Do you have cycles to get this PR over the finish line? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3216 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAFF3K6C3JUAFATTYAY46Z3T4FPALANCNFSM4TB4VPEA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&utm_campaign=notification-email> .

stuartmorgan · 2021-09-15T15:44:57Z

Merged in master, and added metadata updates (version bumps for the changed plugins that aren't deprecated, just CHANGELOG entries for those that are). This should land once tests cycle green.

…r arg (flutter/plugins#3216)

…lutter#3216)

brunobowden requested review from bkonyi, bparrishMines, cyanglaz, iskakaushik, matthew-carroll and mklim as code owners October 28, 2020 06:50

google-cla bot added the cla: yes label Oct 28, 2020

brunobowden mentioned this pull request Oct 28, 2020

simple_connectivity v0.0.1 => connectivity v2.0.0 WorldHealthOrganization/app#1669

Merged

7 tasks

brunobowden changed the title ~~[multiple] Java 8 compile for all plugins with -Werror compiler arg~~ [multiple] Java 8 target for all plugins with -Werror compiler arg Oct 28, 2020

blasten approved these changes Apr 27, 2021

View reviewed changes

stuartmorgan added the last mile label Apr 28, 2021

stuartmorgan added 2 commits May 25, 2021 10:48

Merge branch 'master' into target-7-obsolete-fix

6f63b48

Add version bump to video_player

463de7b

github-actions bot added p: android_alarm_manager p: android_intent p: camera p: connectivity p: path_provider p: video_player platform-android labels May 25, 2021

stuartmorgan added 2 commits September 15, 2021 11:37

Merge branch 'master' into target-7-obsolete-fix

1dfd455

Add version bumps/changelogs

d092d0d

stuartmorgan added the waiting for tree to go green (Use "autosubmit") This PR is approved and tested, but waiting for the tree to be green to land. label Sep 15, 2021

fluttergithubbot merged commit bb3ebb2 into flutter:master Sep 15, 2021

stuartmorgan removed the last mile label Sep 15, 2021

engine-flutter-autoroll mentioned this pull request Sep 18, 2021

Roll Plugins from 5f1c83e3933e to 1fe19f1d4946 (22 revisions) flutter/flutter#90301

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 18, 2021

bb3ebb2 [multiple] Java 8 target for all plugins with -Werror compile…

1d1dbb0

…r arg (flutter/plugins#3216)

amantoux pushed a commit to amantoux/plugins that referenced this pull request Sep 27, 2021

[multiple] Java 8 target for all plugins with -Werror compiler arg (f…

52c8541

…lutter#3216)

KyleFin pushed a commit to KyleFin/plugins that referenced this pull request Dec 21, 2021

[multiple] Java 8 target for all plugins with -Werror compiler arg (f…

ccd1a5b

…lutter#3216)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[multiple] Java 8 target for all plugins with -Werror compiler arg #3216

[multiple] Java 8 target for all plugins with -Werror compiler arg #3216

brunobowden commented Oct 28, 2020 •

edited

Loading

ened commented Oct 28, 2020

brunobowden commented Oct 28, 2020 •

edited

Loading

brunobowden commented Oct 28, 2020 •

edited

Loading

brunobowden commented Nov 14, 2020

stuartmorgan commented Apr 27, 2021

blasten left a comment

blasten commented Apr 27, 2021

stuartmorgan commented Apr 28, 2021

brunobowden commented Apr 28, 2021 via email

stuartmorgan commented Aug 10, 2021

brunobowden commented Aug 11, 2021 via email

stuartmorgan commented Sep 15, 2021

[multiple] Java 8 target for all plugins with -Werror compiler arg #3216

[multiple] Java 8 target for all plugins with -Werror compiler arg #3216

Conversation

brunobowden commented Oct 28, 2020 • edited Loading

Description

Fix

Workaround

Regression

Checklist

Breaking Change

ened commented Oct 28, 2020

brunobowden commented Oct 28, 2020 • edited Loading

brunobowden commented Oct 28, 2020 • edited Loading

brunobowden commented Nov 14, 2020

stuartmorgan commented Apr 27, 2021

blasten left a comment

Choose a reason for hiding this comment

blasten commented Apr 27, 2021

stuartmorgan commented Apr 28, 2021

brunobowden commented Apr 28, 2021 via email

stuartmorgan commented Aug 10, 2021

brunobowden commented Aug 11, 2021 via email

stuartmorgan commented Sep 15, 2021

brunobowden commented Oct 28, 2020 •

edited

Loading

brunobowden commented Oct 28, 2020 •

edited

Loading

brunobowden commented Oct 28, 2020 •

edited

Loading