Closed Bug 1143280 Opened 6 years ago Closed 6 years ago

Android build fails with updated support library (22) and/or new API level (22)

Categories

(Firefox for Android :: General, defect)

All
Android
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 39
Tracking Status
firefox38 --- fixed
firefox39 --- fixed

People

(Reporter: sebastian, Assigned: sebastian)

References

(Blocks 1 open bug)

Details

Attachments

(4 files, 6 obsolete files)

I updated the support library using the 'android' tool (21 -> 22). Now the build fails (see errors in build_error_supportlibrary22.txt). Unfortunately there's no way to downgrade the support library (at least using the 'android' tool again) so there might be more people affected by this soon.

Looking at the build output the new support library requires you to build with at least Android API 22. So I tried to build with "android-22" set in .mozconfig. But then the build fails because of some now removed constants in android.text.format.DateFormat used by mobile/android/base/widget/DateTimePicker.java (see build_error_android22.txt).

Possible fixes:

* (1) The support library is always pulled from *ANDROID-SDK*/extras/android/support/v4/android-support-v4.jar. This version changes whenever the 'android' tool updates it. If the support library would be pulled from the local m2repository a specific version could be used.

* (2) Refactor DateTimePicker.java to not need the now missing constants.
Attached patch 1143290_SearchBar.patch (obsolete) — Splinter Review
I attached two patches to fix building Fennec using Android API 22. In addition to DateTimePicker the SearchBar class had to be fixed as well.
Attachment #8577592 - Flags: review+
Comment on attachment 8577593 [details] [diff] [review]
1143290_SearchBar.patch

Review of attachment 8577593 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/search/java/org/mozilla/search/autocomplete/SearchBar.java
@@ +110,5 @@
>          });
>          engineIcon = (ImageView) findViewById(R.id.engine_icon);
>  
> +        focusedBackground = getResources().getDrawable(R.drawable.edit_text_focused, null);
> +        defaultBackground = getResources().getDrawable(R.drawable.edit_text_default, null);

I think this should really be:

    final Resources.Theme theme = context.getTheme();
    final Resources res = getResources();
    focusedBackground = res.getDrawable(…, theme);
    defaultBackground = res.getDrawable(…, theme);

It *probably* doesn't make a difference for bitmap drawables, but I think there's a link between the Theme and screen density, so better safe than sorry.
Attachment #8577593 - Flags: feedback+
Moving this to `General`, 'cos we're fixing it in code, not in the build.

Pinning build artifacts is good, but we shouldn't require an older version of an SDK if we can help it.
Assignee: nobody → s.kaspari
Status: NEW → ASSIGNED
Component: Build Config & IDE Support → General
OS: Mac OS X → Android
Hardware: x86 → All
(In reply to Richard Newman [:rnewman] from comment #6)
> I think this should really be:
> 
>     final Resources.Theme theme = context.getTheme();
>     final Resources res = getResources();
>     focusedBackground = res.getDrawable(…, theme);
>     defaultBackground = res.getDrawable(…, theme);
> 
> It *probably* doesn't make a difference for bitmap drawables, but I think
> there's a link between the Theme and screen density, so better safe than
> sorry.

Good point! I wanted to resemble the previous functionality very closely and was a bit afraid that adding a theme could change the behavior.  But this shouldn't happen.

However I just realized that getDrawable(id, theme) was introduced with API level 21. So this won't run on pre-Android-5 devices. Instead we probably should use ResourcesCompat.getDrawable(id, theme) from the support library.

I'll update the patches accordingly.

[1] https://developer.android.com/reference/android/support/v4/content/res/ResourcesCompat.html#getDrawable%28android.content.res.Resources,%20int,%20android.content.res.Resources.Theme%29
Attached patch 1143290_DateTimePicker_v2.patch (obsolete) — Splinter Review
Attachment #8577592 - Attachment is obsolete: true
Attachment #8577636 - Flags: review?(rnewman)
Attached patch 1143290_SearchBar_v2.patch (obsolete) — Splinter Review
Attachment #8577593 - Attachment is obsolete: true
Attachment #8577638 - Flags: review?(rnewman)
Duplicate of this bug: 1142783
Comment on attachment 8577636 [details] [diff] [review]
1143290_DateTimePicker_v2.patch

Review of attachment 8577636 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/widget/DateTimePicker.java
@@ +409,5 @@
>                      break;
>                  default:
>                      throw new IllegalArgumentException();
>              }
> +

Nit: you can kill this blank line.
Attachment #8577636 - Flags: review?(rnewman) → review+
Comment on attachment 8577638 [details] [diff] [review]
1143290_SearchBar_v2.patch

Review of attachment 8577638 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good, assuming that Try doesn't complain.

I have both of these patches queued up locally with my nits addressed; if Try is green, I'll land these tomorrow.

Thanks for the report and the quick patches, Sebastian!

::: mobile/android/search/java/org/mozilla/search/autocomplete/SearchBar.java
@@ +111,5 @@
>              }
>          });
>          engineIcon = (ImageView) findViewById(R.id.engine_icon);
>  
> +        Resources.Theme theme = context.getTheme();

Nit: final.

@@ +114,5 @@
>  
> +        Resources.Theme theme = context.getTheme();
> +
> +        focusedBackground = ResourcesCompat.getDrawable(getResources(), R.drawable.edit_text_focused, theme);
> +        defaultBackground = ResourcesCompat.getDrawable(getResources(), R.drawable.edit_text_default, theme);

Nit: extract resources to a shared var.
Attachment #8577638 - Flags: review?(rnewman) → review+
Updated DateTimePicker patch to address the nits.
Attachment #8577636 - Attachment is obsolete: true
Attachment #8577715 - Flags: review+
Attached patch 1143290_SearchBar_v3.patch (obsolete) — Splinter Review
Updated SearchBar patch to address the nits.
Attachment #8577638 - Attachment is obsolete: true
Attachment #8577716 - Flags: review+
(In reply to Richard Newman [:rnewman] from comment #12)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=7e937ce665c0

The build probably failed because the build server doesn't know about API level 22 and the new support library yet. I'm not sure yet how to adjust the build and get the servers to pull the new support library.
Attached patch 1143290_api22.patch (obsolete) — Splinter Review
I found the configuration file to use SDK Version 22 for building. However the SDK 22 and the support library 22 need to be installed on the build servers in order to build successfully.
Attachment #8577735 - Flags: review?(rnewman)
Bug 1084498 has some hints, but I don't know how to make that happen. (Nick, chime in if you do; wesj is out.)

Still, if this now *requires* a newer SDK or support library, then this is more of a change, and we need to bump the requirements in configure (see Bug 1122024) as well as doing all the deployment work.

Let's think about landing this without using ResourceCompat.
(In reply to Richard Newman [:rnewman] from comment #19)
> Let's think about landing this without using ResourceCompat.

I can only come up with these three solutions right now (and I don't like all of them):

* Suppress the warning and do not change the code. -> I don't like that because it is not really handling the warning/deprecation.

* Do what the support library does: Based on the API level at runtime either calling the new method or the old one. This would remove the dependency to use the new support library but still require the new SDK (as long as no reflection is used). -> I don't like that because it's basically duplicating the functionality of the support library.

* In the previous version of the support library the method is available in a non-static way: new ResourcesCompat().getDrawable(resources, id, theme); - Not really a solution because it would compile with version 21 of the library but not 22.

I'd still advertise for using the new SDK and support library because:

* It's recommended to always build against the latest SDK and currently version 21 is used according to android/config/mozconfigs/common. I don't see an advantage in using 21 over 22 (or using 21 as minimum).

* Someone who sets up a build system right now will only have support library 22 (as long as the build fetches it from *ANDROID-SDK*/extras/android/support/v4/android-support-v4.jar). There will be more confusion when local builds use a different library version than the build/try servers.

What do you think? Maybe there's an easy way to fix this that I did not think of. I just saw that one method in SearchBar.java already suppresses deprecation warnings - that's at least the easiest way out.
(In reply to :Sebastian Kaspari from comment #20)
> I just saw that one method in SearchBar.java already suppresses
> deprecation warnings - that's at least the easiest way out.

If suppressing deprecation warnings gets us a landable patch today, that's the avenue I'd take. It unblocks people who are trying to build with 22, without forcing hundreds of people to upgrade or causing a firedrill to upgrade infra.

We also don't know for sure whether SDK 22 produces a working APK, so there's risk involved.

We can file a follow-up with a 22-only patch, and kick off the process to move to 22 with appropriate testing.

Sound good?
(In reply to Richard Newman [:rnewman] from comment #21)
> If suppressing deprecation warnings gets us a landable patch today, that's
> the avenue I'd take. It unblocks people who are trying to build with 22,
> without forcing hundreds of people to upgrade or causing a firedrill to
> upgrade infra.

Aww man, I'm already wearing my firefighter's helmet. :)

You are right and this is totally reasonable. I updated the patch to suppress the deprecation warning. I can build locally with API level 22 and I pushed both patches to try here: 
https://treeherder.mozilla.org/#/jobs?repo=try&revision=056943f746b2
Attachment #8577716 - Attachment is obsolete: true
Attachment #8578068 - Flags: review?(rnewman)
Attachment #8577735 - Attachment is obsolete: true
Attachment #8577735 - Flags: review?(rnewman)
Comment on attachment 8578068 [details] [diff] [review]
1143290_SearchBar_v4.patch

Review of attachment 8578068 [details] [diff] [review]:
-----------------------------------------------------------------

Assuming this builds locally for you with 22, LGTM.
Attachment #8578068 - Flags: review?(rnewman) → review+
Local and try server builds have been successful. However there are some Robocop jobs hanging (Android 2.3). I tried to restart them but they are again "running" for > 200 minutes. But I don't think that's related to my patches..
Okay, the Robocop tests finally passed on Android 2.3.
https://hg.mozilla.org/mozilla-central/rev/e051205c0d95
https://hg.mozilla.org/mozilla-central/rev/57ea191ebfe0
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 39
Comment on attachment 8577715 [details] [diff] [review]
1143290_DateTimePicker_v3.patch

Approval Request Comment
[Feature/regressing bug #]:
  Android SDK 22.

[User impact if declined]:
  Developer impact: developers testing uplifts will need to import these two patches in order to compile Fennec.

[Describe test coverage new/current, TreeHerder]:
  Builds complete!

[Risks and why]: 
  Ultra low risk: deprecation warning and inlining removed constants as recommended by the Android docs.

[String/UUID change made/needed]:
  None.
Attachment #8577715 - Flags: approval-mozilla-beta?
Attachment #8577715 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.