Closed Bug 1263390 Opened 8 years ago Closed 8 years ago

Android Studio 2.0: Use Gradle 2.10 and Android Gradle plugin 2.0

Categories

(Firefox Build System :: Android Studio and Gradle Integration, defect)

Product:
Firefox Build System
Component:
Android Studio and Gradle Integration
Platform:
All
Android
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox49 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla49
Tracking Flags:
Tracking Status
firefox49 --- fixed

People

(Reporter: sebastian, Assigned: sebastian)

References

Details

Attachments

(6 files)

MozReview Request: Bug 1263390 - Update releng manifests to point to new dependencies. r?mcomella
58 bytes, text/x-review-board-request
mcomella
: review+
Details
MozReview Request: Bug 1263390 - Use Gradle 2.10 and Android Gradle plugin 2.0. r?mcomella
58 bytes, text/x-review-board-request
mcomella
: review+
Details
MozReview Request: Bug 1263390 - TestRunner: Always use non-library configuration. r?mcomella
58 bytes, text/x-review-board-request
mcomella
: review+
Details
MozReview Request: Bug 1263390 - Post: Fix new "Registered" lint errors. r?mcomella
58 bytes, text/x-review-board-request
mcomella
: review+
Details
MozReview Request: Bug 1263390 - Post: Add new lint check "GoogleAppIndexingUrlError" to warning list. r?mcomella
58 bytes, text/x-review-board-request
mcomella
: review+
Details
MozReview Request: Bug 1263390 - Post: Add new unused resources to UnusedResourcesUtil. r?ahunt
58 bytes, text/x-review-board-request
ahunt
: review+
Details 
Android Studio is now available in the stable channel. To use it we need to use Gradle 2.10 and the new Android gradle plugin (2.0).

I tested this locally and this seems to work without any issues. Even "Instant Run" does work - it's not really "instant" though.
I had to disable "Instant Run" because sometimes it didn't pick up changes. However looking at G+ and Reddit it seems like this is a problem with the Android plugin in general and not related to our setup.
Running unit tests locally with 2.0 is broken:

> java.lang.RuntimeException: /Users/sebastian/Projects/Mozilla/fx-team/objdir-frontend/gradle/build/mobile/android/app/intermediates/bundles/local/debug/AndroidManifest.xml not found or not a file; it should point to your project's AndroidManifest.xml
> 	at org.robolectric.manifest.AndroidManifest.validate(AndroidManifest.java:121)
> 	at org.robolectric.manifest.AndroidManifest.getResourcePath(AndroidManifest.java:469)
> 	at org.robolectric.manifest.AndroidManifest.getIncludedResourcePaths(AndroidManifest.java:475)
> 	at org.robolectric.RobolectricTestRunner.createAppResourceLoader(RobolectricTestRunner.java:491)
> 	at org.robolectric.RobolectricTestRunner.getAppResourceLoader(RobolectricTestRunner.java:483)
> 	at org.robolectric.internal.ParallelUniverse.setUpApplicationState(ParallelUniverse.java:73)
> 	at org.robolectric.RobolectricTestRunner.setUpApplicationState(RobolectricTestRunner.java:433)
> 	at org.robolectric.RobolectricTestRunner$2.evaluate(RobolectricTestRunner.java:240)
> 	at org.robolectric.RobolectricTestRunner.runChild(RobolectricTestRunner.java:188)
> 	at org.robolectric.RobolectricTestRunner.runChild(RobolectricTestRunner.java:54)
> 	at org.junit.runners.ParentRunner$3.run(ParentRunner.java:290)
> 	at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:71)
> 	at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:288)
> 	at org.junit.runners.ParentRunner.access$000(ParentRunner.java:58)
> 	at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:268)
> 	at org.robolectric.RobolectricTestRunner$1.evaluate(RobolectricTestRunner.java:152)
> 	at org.junit.runners.ParentRunner.run(ParentRunner.java:363)
> 	at org.junit.runners.Suite.runChild(Suite.java:128)
> 	at org.junit.runners.Suite.runChild(Suite.java:27)
> 	at org.junit.runners.ParentRunner$3.run(ParentRunner.java:290)
> 	at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:71)
> 	at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:288)
> 	at org.junit.runners.ParentRunner.access$000(ParentRunner.java:58)
> 	at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:268)
> 	at org.junit.runners.ParentRunner.run(ParentRunner.java:363)
> 	at org.junit.runner.JUnitCore.run(JUnitCore.java:137)
> 	at com.intellij.junit4.JUnit4IdeaTestRunner.startRunnerWithArgs(JUnit4IdeaTestRunner.java:69)
> 	at com.intellij.rt.execution.junit.JUnitStarter.prepareStreamsAndStart(JUnitStarter.java:234)
> 	at com.intellij.rt.execution.junit.JUnitStarter.main(JUnitStarter.java:74)
> 	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
> 	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
> 	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
> 	at java.lang.reflect.Method.invoke(Method.java:497)
> 	at com.intellij.rt.execution.application.AppMain.main(AppMain.java:144)

I'll push the patch to try.. Let's see if there's more.
(In reply to Sebastian Kaspari (:sebastian) from comment #2)
> Running unit tests locally with 2.0 is broken:
> 
> > java.lang.RuntimeException: /Users/sebastian/Projects/Mozilla/fx-team/objdir-frontend/gradle/build/mobile/android/app/intermediates/bundles/local/debug/AndroidManifest.xml not found or not a file; it should point to your project's AndroidManifest.xml

This can be fixed here:
https://dxr.mozilla.org/mozilla-central/source/mobile/android/tests/background/junit4/src/org/mozilla/gecko/background/testhelpers/TestRunner.java#74-89

areResourcesFromLibrary() returns true and creates the wrong folders. Forcing areResourcesFromLibrary() to return false creates the correct folders and the tests get executed.
Mh, looking at the deps task it seems like I can download the gradle and jcenter artifacts but need to build/package the SDK myself:
https://tools.taskcluster.net/task-inspector/#c5R9foOWQLujCkDqFPX2Vw/
Everything's now ready to switch, except that I see some new lint errors:
* 6x Registered: Class is not registered in the manifest
* 25x UnusedResources: Unused resources
* 2x GoogleAppIndexingUrlError: URL not supported by app for Google App Indexing

Especially the UnusedResources is interesting. Either something is just wrong or the new tools are able to find more things.
(In reply to Sebastian Kaspari (:sebastian) from comment #11)
> Everything's now ready to switch, except that I see some new lint errors:

I might be able to provide additional context if you pasted in some of the new lint errors.
(In reply to Michael Comella (:mcomella) from comment #12)
> (In reply to Sebastian Kaspari (:sebastian) from comment #11)
> > Everything's now ready to switch, except that I see some new lint errors:
> 
> I might be able to provide additional context if you pasted in some of the
> new lint errors.

https://public-artifacts.taskcluster.net/X2TJ29qAQWCnVUgK66yebQ/0/public/android/lint/lint-results-automationDebug.html

I'll try to prepare patches for the errors.
Review commit: https://reviewboard.mozilla.org/r/47615/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/47615/
Attachment #8743180 - Flags: review?(michael.l.comella)
Attachment #8743181 - Flags: review?(michael.l.comella)
Attachment #8743182 - Flags: review?(michael.l.comella)
Attachment #8743183 - Flags: review?(michael.l.comella)
Attachment #8743184 - Flags: review?(michael.l.comella)
* GeckoActivity, LocaleAwareAppCompatActivity, LocaleAwareFragmentActivity, LocaleAwareActivity:
  Those activities are never instantiated directly. Make them abstract.
* CrashReporter: This activity is only registered if MOZ_CRASHREPORTER is set. Supress warning.

Unfortunately I had to downgrade this lint check from "error" to "warning" because the current
gradle plugin doesn't recognize the SupressLint annotation for the "Registered" check:
https://code.google.com/p/android/issues/detail?id=204846

Review commit: https://reviewboard.mozilla.org/r/47621/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/47621/
This is the current state.

What's left are the UnusedResources errors:
https://public-artifacts.taskcluster.net/a_cZqb-JTSC31Djt9ZpGGA/0/public/android/lint/lint-results-automationDebug.html#UnusedResources

Most of them are about the RemoteTabsPanel. The Panel is not used anymore and I filed bug 1265996 to remove the code. But I wonder why those show up. I can follow some of the styles up to the layout and from there to the code. It almost looks like there's a bug in the Android plugin.
I did a sanity check by running the 2.1.0-beta3 preview version of the plugin. However I still get the same set of UnusedResources errors.
Comment on attachment 8743180 [details]
MozReview Request: Bug 1263390 - Update releng manifests to point to new dependencies. r?mcomella

https://reviewboard.mozilla.org/r/47615/#review44659
Attachment #8743180 - Flags: review?(michael.l.comella) → review+
Comment on attachment 8743181 [details]
MozReview Request: Bug 1263390 - Use Gradle 2.10 and Android Gradle plugin 2.0. r?mcomella

https://reviewboard.mozilla.org/r/47617/#review44661

I don't know all the places these versions need to get updated, but these are all the places I already know about!
Attachment #8743181 - Flags: review?(michael.l.comella) → review+
Comment on attachment 8743182 [details]
MozReview Request: Bug 1263390 - TestRunner: Always use non-library configuration. r?mcomella

https://reviewboard.mozilla.org/r/47619/#review44663

Does what it says it will (so r+) – but why do we want this? Thus, heisitant r+.
Attachment #8743182 - Flags: review?(michael.l.comella) → review+
Comment on attachment 8743183 [details]
MozReview Request: Bug 1263390 - Post: Fix new "Registered" lint errors. r?mcomella

https://reviewboard.mozilla.org/r/47621/#review44665

::: mobile/android/app/lint.xml:38
(Diff revision 1)
>      <issue id="ResourceAsColor" severity="warning" />
>      <issue id="ResourceType" severity="warning" />
>      <issue id="ValidFragment" severity="warning" />
>      <issue id="WrongConstant" severity="warning" />
>  
> +    <!-- We fixed all "Registered" lint errors. However the current gradle plugin has a bug where

Well that's annoying...
Attachment #8743183 - Flags: review?(michael.l.comella) → review+
Comment on attachment 8743184 [details]
MozReview Request: Bug 1263390 - Post: Add new lint check "GoogleAppIndexingUrlError" to warning list. r?mcomella

https://reviewboard.mozilla.org/r/47623/#review44667
Attachment #8743184 - Flags: review?(michael.l.comella) → review+
(In reply to Sebastian Kaspari (:sebastian) from comment #20)
> Most of them are about the RemoteTabsPanel. The Panel is not used anymore
> and I filed bug 1265996 to remove the code.

Maybe it's smart enough to know when classes files are unused and thus when the resources it points to are unused (or it piggie-backs on our Proguard optimizations which would zap the class).
re comment 24...

(In reply to Sebastian Kaspari (:sebastian) from comment #4)
> This can be fixed here:
> https://dxr.mozilla.org/mozilla-central/source/mobile/android/tests/
> background/junit4/src/org/mozilla/gecko/background/testhelpers/TestRunner.
> java#74-89
> 
> areResourcesFromLibrary() returns true and creates the wrong folders.
> Forcing areResourcesFromLibrary() to return false creates the correct
> folders and the tests get executed.

Are we sure we don't ever need the `areResourcesFromLibrary() == true` case?
(In reply to Michael Comella (:mcomella) from comment #28)
> re comment 24...
> 
> (In reply to Sebastian Kaspari (:sebastian) from comment #4)
> > This can be fixed here:
> > https://dxr.mozilla.org/mozilla-central/source/mobile/android/tests/
> > background/junit4/src/org/mozilla/gecko/background/testhelpers/TestRunner.
> > java#74-89
> > 
> > areResourcesFromLibrary() returns true and creates the wrong folders.
> > Forcing areResourcesFromLibrary() to return false creates the correct
> > folders and the tests get executed.
> 
> Are we sure we don't ever need the `areResourcesFromLibrary() == true` case?

Right now yes.

If you look at the top javadoc, you can see that we copied it from here (and modified it):
ttps://github.com/robolectric/robolectric/blob/8676da2daa4c140679fb5903696b8191415cec8f/robolectric/src/main/java/org/robolectric/RobolectricGradleTestRunner.java

It doesn't look like we wrote those lines intentionally.

However there's a new version of the test runner on GitHub:
https://github.com/robolectric/robolectric/blob/master/robolectric/src/main/java/org/robolectric/RobolectricGradleTestRunner.java

We could try to use this version and re-apply our customizations. But I'm hesitant to file a bug. Right now we can't win much more than just having tests running I guess? :)
(In reply to Michael Comella (:mcomella) from comment #27)
> Maybe it's smart enough to know when classes files are unused and thus when
> the resources it points to are unused (or it piggie-backs on our Proguard
> optimizations which would zap the class).

Oh yeah, it might be the interaction with proguard. I briefly tried to remove the resources but this broke the build because they are still referenced in the classes. If lint runs after proguard then for lint the classes might be gone and the resources are actually unused.

I was wondering what's the best path forward here, but then I just saw that bug 1262343 already has some patches to remove the code. Reviewing... :)
(In reply to Sebastian Kaspari (:sebastian) from comment #33)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=c25cad87fe56

Okay, this chase is getting ridiculous. I'm inclined to downgrade the UnusedResources error to a warning and file a follow-up bug. A big problem right now is that we still keep the remote tabs empty view around (it will be re-used for the new panel later) and the new plugin seems to recognize that the ViewStub is never inflated.
@mcomella: What's your opinion? (comment 34).
Flags: needinfo?(michael.l.comella)
(In reply to Sebastian Kaspari (:sebastian) from comment #35)
> @mcomella: What's your opinion? (comment 34).

We could throw the unused resources into UnusedResourcesUtil (in a separate section) so that way we can keep the lint error as an error. Then we can file a follow-up bug to remove it once the empty view is removed, or the resources are used.
Flags: needinfo?(michael.l.comella)
(In reply to Michael Comella (:mcomella) from comment #36)
> (In reply to Sebastian Kaspari (:sebastian) from comment #35)
> > @mcomella: What's your opinion? (comment 34).
> 
> We could throw the unused resources into UnusedResourcesUtil (in a separate
> section) so that way we can keep the lint error as an error. Then we can
> file a follow-up bug to remove it once the empty view is removed, or the
> resources are used.

Aye, that's a good idea and much easier. :)
Depends on: 1268414
This is a temporary fix. The new plugin is able to find more unused resources.
However we are not ready to remove all of them yet. Some of them will be in
use again soon.
This patch will add those files to UnusedResourcesUtil in order to suppress
the lint error.

Review commit: https://reviewboard.mozilla.org/r/49437/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/49437/
Attachment #8746459 - Flags: review?(ahunt)
Comment on attachment 8743180 [details]
MozReview Request: Bug 1263390 - Update releng manifests to point to new dependencies. r?mcomella

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47615/diff/1-2/
Comment on attachment 8743181 [details]
MozReview Request: Bug 1263390 - Use Gradle 2.10 and Android Gradle plugin 2.0. r?mcomella

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47617/diff/1-2/
Comment on attachment 8743182 [details]
MozReview Request: Bug 1263390 - TestRunner: Always use non-library configuration. r?mcomella

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47619/diff/1-2/
Comment on attachment 8743183 [details]
MozReview Request: Bug 1263390 - Post: Fix new "Registered" lint errors. r?mcomella

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47621/diff/1-2/
Comment on attachment 8743184 [details]
MozReview Request: Bug 1263390 - Post: Add new lint check "GoogleAppIndexingUrlError" to warning list. r?mcomella

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47623/diff/1-2/
Comment on attachment 8746459 [details]
MozReview Request: Bug 1263390 - Post: Add new unused resources to UnusedResourcesUtil. r?ahunt

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49437/diff/1-2/
Attachment #8746459 - Flags: review?(ahunt) → review+
Comment on attachment 8746459 [details]
MozReview Request: Bug 1263390 - Post: Add new unused resources to UnusedResourcesUtil. r?ahunt

https://reviewboard.mozilla.org/r/49437/#review46277
https://hg.mozilla.org/integration/fx-team/rev/4b3437e9dd4b56524469b522649eb870459dc2c5
Bug 1263390 - Update releng manifests to point to new dependencies. r=mcomella

https://hg.mozilla.org/integration/fx-team/rev/cd9b6ddfb507fcc922195ad0f35d5e7ff2d20057
Bug 1263390 - Use Gradle 2.10 and Android Gradle plugin 2.0. r=mcomella

https://hg.mozilla.org/integration/fx-team/rev/1ac17eedc75ce78ec2060c0773809e55984e860b
Bug 1263390 - TestRunner: Always use non-library configuration. r=mcomella

https://hg.mozilla.org/integration/fx-team/rev/58da59309c4615f784c827c4227d1b471d3688bb
Bug 1263390 - Post: Fix new "Registered" lint errors. r=mcomella

https://hg.mozilla.org/integration/fx-team/rev/ba848d207bff32315e33e682b61b2a4d82cf2cf2
Bug 1263390 - Post: Add new lint check "GoogleAppIndexingUrlError" to warning list. r=mcomella

https://hg.mozilla.org/integration/fx-team/rev/830f7765555a3740103c31100118bf318cf785b2
Bug 1263390 - Post: Add new unused resources to UnusedResourcesUtil. r=ahunt
Product: Firefox for Android → Firefox Build System
Target Milestone: Firefox 49 → mozilla49
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: