Closed Bug 1411654 (gradle-3.0) Opened 8 years ago Closed 8 years ago

Upgrade Android-Gradle plugin to 3.0+

Categories

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

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

Tracking

(firefox59 wontfix, firefox60 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla60
Tracking Flags:
Tracking Status
firefox59 --- wontfix
firefox60 --- fixed

People

(Reporter: nalexander, Assigned: nalexander)

References

(Blocks 2 open bugs)

Details

Attachments

(20 files, 1 obsolete file)

Bug 1411654 - Pre: Teach |mach gradle| to take GRADLE_FLAGS from the command line.
59 bytes, text/x-review-board-request
maliu
: review+
Details
Bug 1411654 - Part 1: Upgrade to Android-Gradle 3.0+ and build-tools;26.0.2.
59 bytes, text/x-review-board-request
maliu
: review+
Details
Bug 1411654 - Pre: Don't block Google's maven repository.
59 bytes, text/x-review-board-request
maliu
: review+
Details
Bug 1411654 - Pre: Fix dangling resource.
59 bytes, text/x-review-board-request
mcomella
: review+
Details
Bug 1411654 - Pre: Remove unused PerProfileDatabaseProvider.
59 bytes, text/x-review-board-request
mcomella
: review+
Details
Bug 1411654 - Pre: Clear icon disk storage in test.
59 bytes, text/x-review-board-request
mcomella
: review+
Details
Bug 1411654 - Pre: Gradle is no longer experimental; IntelliJ not supported.
59 bytes, text/x-review-board-request
maliu
: review+
Details
Bug 1411654 - Pre: Update toolchain documents for Android toolchain jobs.
59 bytes, text/x-review-board-request
maliu
: review+
Details
Bug 1411654 - Part 0: Add rudimentary documentation about Gradle integration.
59 bytes, text/x-review-board-request
maliu
: review+
Details
Bug 1411654 - Part 2: Update Robolectric to 3.5.1.
59 bytes, text/x-review-board-request
mcomella
: review+
Details
Bug 1411654 - Part 3: Make each variant handle source from moz.build.
59 bytes, text/x-review-board-request
maliu
: review+
Details
Bug 1411654 - Part 4: Use flavorDimensions to simplify {with,without}GeckoBinaries logic.
59 bytes, text/x-review-board-request
maliu
: review+
Details
Bug 1411654 - Part 5: Work through various new lint issues.
59 bytes, text/x-review-board-request
mcomella
: review+
Details
Bug 1411654 - Part 6: Fix checkstyle.
59 bytes, text/x-review-board-request
mcomella
: review+
Details
Bug 1411654 - Post: Cull unused variables; guard more with MOZ_BUILD_MOBILE_ANDROID_WITH_GRADLE.
59 bytes, text/x-review-board-request
maliu
: review+
Details
Bug 1411654 - Part 5b: Work through WrongConstant lint issues.
59 bytes, text/x-review-board-request
mcomella
: review+
Details
Bug 1411654 - Part 5c: Work through ResourceUnused lint issues.
59 bytes, text/x-review-board-request
mcomella
: review+
Details
Bug 1411654 - Pre: Remove unused HomeExpandableListView.
59 bytes, text/x-review-board-request
mcomella
: review+
Details
Bug 1411654 - Pre: Fix checkstyle whitespace error in GeckoHlsPlayer.
59 bytes, text/x-review-board-request
Details
Bug 1411654 - Pre: Fix CustomTabsSecurityPopup package.
59 bytes, text/x-review-board-request
nalexander
: review+
Details
This is the successor to Bug 1366644. Android-Gradle version 3.0.0 is at rc2; this tracks upgrading. There are lots of structural changes in the new plugin version; I'm filing because I've worked through most of them and don't want somebody to repeat all that work without trying to coordinate. One interesting wrinkle is that the new plugin pins the build-tools version. As an aside, I got interested in this because most of the Java 8 features are gated on the new plugin version.
Android Studio 3.0 is live today: https://android-developers.googleblog.com/2017/10/android-studio-30.html. Also, another wrinkle: the new Android-Gradle plugin parses AndroidManifest.xml at (Gradle) configuration time, not at evaluation time. That means that our current preprocessed_manifest stuff is broken. I think the best thing to do is: - write a stub AndroidManifest.xml, in Gradle, at configuration time - keep the existing preprocessed manifest stuff, but have it be a per-variant overlay on top of the stub manifest It's possible that the tools: XML merging settings could actually make this work pretty well.
Attachment #8922441 - Flags: review?(max)
Attachment #8922442 - Flags: review?(max)
Attachment #8922998 - Flags: review?(max)
Depends on: 1413006
Getting closer: I've got the android-test job updated and green in https://treeherder.mozilla.org/#/jobs?repo=try&revision=5a3e26444568ccc30d629ec0c61817dda16cd12c. Working on lint locally, and then we'll figure out what checkstyle is doing.
Also, it's worth noting that the motivation for working on this has changed. The new "variant aware dependency management" is the best way to simplify the {with,without}GeckoLibraries snarl and tease out the Gradle, moz.build, and Android Studio integration points.
OK, this is ready for the first reviews, although it's not 100% done -- there are still a few more lint issues to address or to ignore. I've flagged maliu for the build bits and mcomella for the lint bits, but that's really not hard and fast. snorp, you've complained about the Gecko binaries logic -- can you take a look at this? nechen, you might be interested in reviewing this as well or instead -- discuss with maliu, please. This builds on Bug 1417232.
Assignee: nobody → nalexander
Status: NEW → ASSIGNED
Depends on: 1417232
Flags: needinfo?(snorp)
Flags: needinfo?(cnevinchen)
Comment on attachment 8922998 [details] Bug 1411654 - Pre: Don't block Google's maven repository. https://reviewboard.mozilla.org/r/194202/#review205884
Attachment #8922998 - Flags: review?(max) → review+
Comment on attachment 8928745 [details] Bug 1411654 - Pre: Gradle is no longer experimental; IntelliJ not supported. https://reviewboard.mozilla.org/r/200010/#review205886
Attachment #8928745 - Flags: review?(max) → review+
Comment on attachment 8928746 [details] Bug 1411654 - Pre: Update toolchain documents for Android toolchain jobs. https://reviewboard.mozilla.org/r/200012/#review205888
Attachment #8928746 - Flags: review?(max) → review+
Comment on attachment 8928747 [details] Bug 1411654 - Part 0: Add rudimentary documentation about Gradle integration. https://reviewboard.mozilla.org/r/200014/#review205890
Attachment #8928747 - Flags: review?(max) → review+
Looks like a step in the right direction to me, though I don't fully understand much of it.
Flags: needinfo?(snorp)
(In reply to Nick Alexander :nalexander from comment #8) > Also, it's worth noting that the motivation for working on this has changed. > The new "variant aware dependency management" is the best way to simplify > the {with,without}GeckoLibraries snarl and tease out the Gradle, moz.build, > and Android Studio integration points. Ah, so the workaround from bug 1385695 will no longer be required, nice.
mcomella: maliu: are these patches in your queues? I got partial review from maliu, but it's been a long time.
Flags: needinfo?(michael.l.comella)
Flags: needinfo?(max)
(In reply to Nick Alexander :nalexander (less responsive until Jan 3, 2018) from comment #35) > mcomella: maliu: are these patches in your queues? I got partial review > from maliu, but it's been a long time. Sorry, I've been PTO - I should be able to look at these today or tomorrow (catching up has been fast and afaict there are no urgent issues for me to address)
Flags: needinfo?(michael.l.comella)
Comment on attachment 8922441 [details] Bug 1411654 - Pre: Teach |mach gradle| to take GRADLE_FLAGS from the command line. https://reviewboard.mozilla.org/r/193494/#review208690
Attachment #8922441 - Flags: review?(max) → review+
Comment on attachment 8928742 [details] Bug 1411654 - Pre: Fix dangling resource. https://reviewboard.mozilla.org/r/200004/#review208692 Didn't test to verify this works correctly but it looks fine to me.
Attachment #8928742 - Flags: review?(michael.l.comella) → review+
Comment on attachment 8928743 [details] Bug 1411654 - Pre: Remove unused PerProfileDatabaseProvider. https://reviewboard.mozilla.org/r/200006/#review208696 ::: commit-message-d2159:4 (Diff revision 1) > +Bug 1411654 - Pre: Remove unused PerProfileDatabaseProvider. r=mcomella > + > +No idea what is going on with this hierarchy, but this isn't used and > +isn't helping anything. You can remove the reference in lint.xml too: https://searchfox.org/mozilla-central/rev/7a8c667bdd2a4a32746c9862356e199627c0896d/mobile/android/app/lint.xml#54
Attachment #8928743 - Flags: review?(michael.l.comella) → review+
Comment on attachment 8928743 [details] Bug 1411654 - Pre: Remove unused PerProfileDatabaseProvider. https://reviewboard.mozilla.org/r/200006/#review208696 fwiw, I briefly tried to figure out when this stopped being used but it's unused since release (57).
Comment on attachment 8928744 [details] Bug 1411654 - Pre: Clear icon disk storage in test. https://reviewboard.mozilla.org/r/200008/#review208700 > Newer versions of Robolectric seem to have different semantics about clearing disk caches, Didn't verify that this is true and thus this change necessary but if so, the change seems reasonable. I wonder what other caches we need to delete?
Attachment #8928744 - Flags: review?(michael.l.comella) → review+
Comment on attachment 8928748 [details] Bug 1411654 - Part 2: Update Robolectric to 3.5.1. https://reviewboard.mozilla.org/r/200016/#review208702 Only skimmed this but seems reasonable: if the tests work for you, works for me! ::: mobile/android/services/src/test/java/org/mozilla/gecko/background/testhelpers/MockGlobalSession.java:31 (Diff revision 1) > this(new SyncConfiguration(username, new BasicAuthHeaderProvider(username, password), new MockSharedPreferences(), keyBundle), callback); > } > > public MockGlobalSession(SyncConfiguration config, GlobalSessionCallback callback) > throws SyncConfigurationException, IllegalArgumentException, IOException, NonObjectJSONException { > - super(config, callback, null, null); > + super(config, callback, RuntimeEnvironment.application,null); nit: add ws after comma
Attachment #8928748 - Flags: review?(michael.l.comella) → review+
Comment on attachment 8928756 [details] Bug 1411654 - Pre: Remove unused HomeExpandableListView. https://reviewboard.mozilla.org/r/200032/#review208706
Attachment #8928756 - Flags: review?(michael.l.comella) → review+
Thanks for the info. I'll discuss with maliu !
Flags: needinfo?(cnevinchen)
Comment on attachment 8928751 [details] Bug 1411654 - Part 5: Work through various new lint issues. https://reviewboard.mozilla.org/r/200022/#review209126 r+ once we can resolve the pathData change and the multi-catch changes. ::: mobile/android/app/src/main/res/drawable/ic_as_trending.xml:8 (Diff revision 1) > android:height="24dp" > android:viewportWidth="12.0" > android:viewportHeight="12.0"> > <path > android:fillColor="@color/activity_stream_icon" > - android:pathData="M4.97.151l-2.819,6.5A.25.25,0,0,0,2.381,7H4.029a.25.25,0,0,1,.225.359L2,12,9.4,5.437A.25.25,0,0,0,9.234,5H6.791a.25.25,0,0,1-.19-.412L10.15.412A.25.25,0,0,0,9.959,0H5.2A.25.25,0,0,0,4.97.151Z"/> > + android:pathData="M4.97,0.151l-2.819,6.5A0.25,0.25,0,0,0,2.381,7H4.029a0.25,0.25,0,0,1,0.225,0.359L2,12,9.4,5.437A0.25,0.25,0,0,0,9.234,5H6.791a0.25,0.25,0,0,1,-0.19,-0.412L10.15,0.412A0.25,0.25,0,0,0,9.959,0H5.2A0.25,0.25,0,0,0,4.97,0.151Z"/> Can you explain this change? The warning mentions the problem is scientific notation, not `M4.97.1511-2.819,` but I'm not sure what that is supposed to mean – is it actually scientific notation? ::: mobile/android/base/AndroidManifest.xml.in:137 (Diff revision 1) > <intent-filter> > <action android:name="org.mozilla.gecko.UPDATE"/> > <category android:name="android.intent.category.DEFAULT" /> > </intent-filter> > > - <intent-filter> > + <intent-filter tools:ignore="AppLinkUrlError"> I can't get this warning to show up locally (perhaps because the file is generated?) and there is [mysteriously very little about this warning on the internet](https://duckduckgo.com/?q=%22applinkurlerror%22&t=ffab&ia=web) so I don't know what it's for. Does it occur when the system thinks you're looking for a filetype it thinks is an error? e.g. xpi is basically only used for add-ons so the Android system won't know about it and think it's an error. If so, it makes sense to me why `mimeType=application/x-xpinstall` & `pathPattern=".*\\.xpi"` would throw an error but I'm not sure why this one would too. But if there's a warning, wfm. ::: mobile/android/base/java/org/mozilla/gecko/CrashReporter.java:51 (Diff revision 1) > import android.widget.CompoundButton; > import android.widget.EditText; > > -@SuppressLint("Registered") // This activity is only registered in the manifest if MOZ_CRASHREPORTER is set > +// Registered: This activity is only registered in the manifest if MOZ_CRASHREPORTER is set. > +// CutPasteId: This lint appears to be broken for this file. > +@SuppressLint("Registered,CutPasteId") I think `CutPasteId` wants us to cache the views after inflation rather than requesting findViewById on ids redundantly. From [the docs](https://sites.google.com/a/android.com/tools/tips/lint-checks): > This lint check looks for cases where you have cut & pasted calls to findViewById but have forgotten to update the R.id field. It's possible that your code is simply (redundantly) looking up the field repeatedly, but lint cannot distinguish that from a case where you for example want to initialize fields prev and next and you cut & pasted findViewById(R.id.prev) and forgot to update the second initialization to R.id.next. That being said, no one ever touches this code so I'm fine use `CutPasteId` if you choose to – just update the comment mentioning that it's not worth fixing because no one touches the code. ::: mobile/android/base/java/org/mozilla/gecko/db/LocalBrowserDB.java:309 (Diff revision 1) > final int faviconID = faviconIDs.get(name); > iconValue.put("_id", faviconID); > bookmarkValue.put(Bookmarks.FAVICON_ID, faviconID); > faviconValues.add(iconValue); > } > - } catch (IllegalAccessException | IllegalArgumentException | NoSuchFieldException e) { > + } catch (IllegalAccessException e) { multi-catch should be supported if we enabled 1_7 source compatibility (iirc, it's desugared into multiple catch statements by the compiler). This SO comment mentions it's supported – https://stackoverflow.com/a/13550632/2219998 – and it worked fine before. Why is this necessary now? ::: mobile/android/base/java/org/mozilla/gecko/db/LocalBrowserDB.java:509 (Diff revision 1) > // Look for a favicon with the id R.raw.bookmarkdefaults_favicon_*. > Field faviconField = drawablesClass.getField(name.replace("_title_", "_favicon_")); > faviconField.setAccessible(true); > > return faviconField.getInt(null); > - } catch (IllegalAccessException | NoSuchFieldException e) { > + } catch (IllegalAccessException e) { Same, multi-catch.
Comment on attachment 8928751 [details] Bug 1411654 - Part 5: Work through various new lint issues. https://reviewboard.mozilla.org/r/200022/#review209136 r- to clear it from my queue – please reflag me after commenting on those issues.
Attachment #8928751 - Flags: review?(michael.l.comella) → review-
Comment on attachment 8928754 [details] Bug 1411654 - Part 5b: Work through WrongConstant lint issues. https://reviewboard.mozilla.org/r/200028/#review209140 ::: mobile/android/base/java/org/mozilla/gecko/activitystream/ActivityStreamTelemetry.java:202 (Diff revision 1) > throw new IllegalStateException("Unknown highlight source: " + source); > } > return this; > } > > + @SuppressLint("WrongConstant") Add a comment explaining this ::: mobile/android/base/java/org/mozilla/gecko/dlc/BaseAction.java:96 (Diff revision 1) > } > } > > public abstract void perform(Context context, DownloadContentCatalog catalog); > > + @SuppressLint("WrongConstant") Add a comment explaining this ::: mobile/android/base/java/org/mozilla/gecko/dlc/DownloadAction.java:51 (Diff revision 1) > > public DownloadAction(Callback callback) { > this.callback = callback; > } > > + @SuppressLint("WrongConstant") Add a comment explaining this
Attachment #8928754 - Flags: review?(michael.l.comella) → review+
Comment on attachment 8928754 [details] Bug 1411654 - Part 5b: Work through WrongConstant lint issues. https://reviewboard.mozilla.org/r/200028/#review209142 These int constants and annotations create weird warnings. :( Actually, maybe we can escalate this lint warning to an error and remove the code that handles default cases for int constants, thus making suppression unnecessary?
Comment on attachment 8928755 [details] Bug 1411654 - Part 5c: Work through ResourceUnused lint issues. https://reviewboard.mozilla.org/r/200030/#review209146
Attachment #8928755 - Flags: review?(michael.l.comella) → review+
Comment on attachment 8928757 [details] Bug 1411654 - Part 5c: More work on UnusedResource lint errors. https://reviewboard.mozilla.org/r/200034/#review209152 ::: mobile/android/base/java/org/mozilla/gecko/util/UnusedResourcesUtil.java:105 (Diff revision 1) > > public static final int[] USED_IN_PAGE_ACTION = { > R.drawable.add_to_homescreen > }; > + > + public static final int[] USED_IN_LEANPLUM_EXPANDABLE_LIST_ACTIVITY = { A plain text search shows this is unused: https://searchfox.org/mozilla-central/search?q=path%3Amobile%2Fandroid+Widget.ExpandableListView&case=false&regexp=false&path= Are you sure keeping this in UnusedResourcesUtil is necessary? But perhaps I'm converting `Widget_ExpandableListView` -> `Widget.ExpandableListView` incorrectly...
Attachment #8928757 - Flags: review?(michael.l.comella) → review+
> As an aside, I got interested in this because most of the Java 8 features are gated on the new plugin version. Nick, I noticed the Java 8 feature set [1] is much more comprehensive than I was expecting (e.g. lambdas, method references) and can a big difference in how we write readable code! I'm sure you were going to do it anyway but just a reminder that it'd be good to notify mobile-firefox-dev@ when these are working! :) [1]: https://developer.android.com/studio/write/java8-support.html#supported_features
Comment on attachment 8922442 [details] Bug 1411654 - Part 1: Upgrade to Android-Gradle 3.0+ and build-tools;26.0.2. https://reviewboard.mozilla.org/r/193500/#review208694 ::: mobile/android/base/Makefile.in:251 (Diff revision 3) > # .gradle.deps: .aapt.deps FORCE > $(eval $(call gradle_command,.gradle.deps,.aapt.deps FORCE)) > > classes.dex: .gradle.deps > $(REPORT_BUILD) > - cp $(gradle_dir)/app/intermediates/transforms/dex/officialPhoton/debug/folders/1000/1f/main/classes.dex $@ > + cp $(gradle_dir)/app/intermediates/transforms/dexMerger/officialPhoton/debug/0/classes.dex classes.dex Will this "classes.dex" be broken if we turn ON multi-dex?
Attachment #8922442 - Flags: review?(max) → review+
Comment on attachment 8928749 [details] Bug 1411654 - Part 3: Make each variant handle source from moz.build. https://reviewboard.mozilla.org/r/200018/#review209316
Attachment #8928749 - Flags: review?(max) → review+
Comment on attachment 8928750 [details] Bug 1411654 - Part 4: Use flavorDimensions to simplify {with,without}GeckoBinaries logic. https://reviewboard.mozilla.org/r/200020/#review209318 ::: mobile/android/base/Makefile.in:251 (Diff revision 1) > # .gradle.deps: .aapt.deps FORCE > $(eval $(call gradle_command,.gradle.deps,.aapt.deps FORCE)) > > classes.dex: .gradle.deps > $(REPORT_BUILD) > - cp $(gradle_dir)/app/intermediates/transforms/dexMerger/officialPhoton/debug/0/classes.dex classes.dex > + cp $(gradle_dir)/app/intermediates/transforms/dexMerger/officialWithoutGeckoBinariesNoMinApiPhoton/debug/0/classes.dex classes.dex Will this be broken when we turn ON the multi-dex flag? ::: mobile/android/base/Makefile.in:511 (Diff revision 1) > $(eval $(call gradle_command,.gradle.nodeps,AndroidManifest.xml $(constants_PP_JAVAFILES) FORCE)) > > .aapt.nodeps: .gradle.nodeps FORCE > @$(TOUCH) $@ > cp $(GRADLE_ANDROID_APP_APK) gecko-nodeps.ap_ > - cp $(gradle_dir)/app/intermediates/transforms/dex/officialPhoton/debug/folders/1000/1f/main/classes.dex classes.dex > + cp $(gradle_dir)/app/intermediates/transforms/dexMerger/officialWithoutGeckoBinariesNoMinApiPhoton/debug/0/classes.dex classes.dex Will this be broken when we turn ON the multi-dex flag?
Attachment #8928750 - Flags: review?(max) → review+
Comment on attachment 8928753 [details] Bug 1411654 - Post: Cull unused variables; guard more with MOZ_BUILD_MOBILE_ANDROID_WITH_GRADLE. https://reviewboard.mozilla.org/r/200026/#review209330
Attachment #8928753 - Flags: review?(max) → review+
Nick it looks like this is all reviewed now, do you know what's blocking the landing?
Flags: needinfo?(nalexander)
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #57) > Nick it looks like this is all reviewed now, do you know what's blocking the > landing? "Just" my time: I need to rebase, address the review comments, and handle the final few lint errors (likely by ignoring them). It's on my list.
Flags: needinfo?(nalexander)
Comment on attachment 8922442 [details] Bug 1411654 - Part 1: Upgrade to Android-Gradle 3.0+ and build-tools;26.0.2. https://reviewboard.mozilla.org/r/193500/#review208694 > Will this "classes.dex" be broken if we turn ON multi-dex? Yes, but no more than the current situation. Turning on multi-dex will require some version of https://bugzilla.mozilla.org/show_bug.cgi?id=1414054; this doesn't address that.
Comment on attachment 8928750 [details] Bug 1411654 - Part 4: Use flavorDimensions to simplify {with,without}GeckoBinaries logic. https://reviewboard.mozilla.org/r/200020/#review209318 > Will this be broken when we turn ON the multi-dex flag? See previous comment: yes, more work will be required to turn on multi-dex.
Comment on attachment 8928751 [details] Bug 1411654 - Part 5: Work through various new lint issues. https://reviewboard.mozilla.org/r/200022/#review209126 > Can you explain this change? The warning mentions the problem is scientific notation, not `M4.97.1511-2.819,` but I'm not sure what that is supposed to mean – is it actually scientific notation? I don't recall everything, but there's some non-standard parsing whereby ".xx.yy" is parsed like "0.xx,0.yy" and lint warns you not to rely on that (presumably since it's not entirely standard). So I manually moved to the more normal (but slightly larger) form, verifying (using the browser) that the resulting SVG results "looked the same". > I can't get this warning to show up locally (perhaps because the file is generated?) and there is [mysteriously very little about this warning on the internet](https://duckduckgo.com/?q=%22applinkurlerror%22&t=ffab&ia=web) so I don't know what it's for. > > Does it occur when the system thinks you're looking for a filetype it thinks is an error? e.g. xpi is basically only used for add-ons so the Android system won't know about it and think it's an error. > > If so, it makes sense to me why `mimeType=application/x-xpinstall` & `pathPattern=".*\\.xpi"` would throw an error but I'm not sure why this one would too. > > But if there's a warning, wfm. Yeah, I couldn't find much that seemed relevant. I didn't want to think through the details of the XPI registration (which seems to be malformed in our manifest), so I elected to silence the warning. I still don't want to think through all of that stuff, so if you think we need more, let's file follow-up. > I think `CutPasteId` wants us to cache the views after inflation rather than requesting findViewById on ids redundantly. From [the docs](https://sites.google.com/a/android.com/tools/tips/lint-checks): > > > This lint check looks for cases where you have cut & pasted calls to > findViewById but have forgotten to update the R.id field. It's possible that > your code is simply (redundantly) looking up the field repeatedly, but lint > cannot distinguish that from a case where you for example want to initialize > fields prev and next and you cut & pasted findViewById(R.id.prev) and forgot > to update the second initialization to R.id.next. > > That being said, no one ever touches this code so I'm fine use `CutPasteId` if you choose to – just update the comment mentioning that it's not worth fixing because no one touches the code. I tried to understand this and didn't twig that lint wants us to cache the lookups ourselves. I'll update the comment. > multi-catch should be supported if we enabled 1_7 source compatibility (iirc, it's desugared into multiple catch statements by the compiler). > > This SO comment mentions it's supported – https://stackoverflow.com/a/13550632/2219998 – and it worked fine before. Why is this necessary now? This is the situation that it doesn't work -- reflection operation related things. See https://gyulajuhasz.com/blog/dangers-of-multi-catch-in-android/.
mcomella: I think this is ready for re-review.
Try is looking healthy: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2f947c15742027ffbdb80f2f8d643cc7f19bf94a has one checkstyle issue, which I have a trivial patch to address and will push at some point. (Not sure why it's not failing now, tbh.)
Comment on attachment 8928751 [details] Bug 1411654 - Part 5: Work through various new lint issues. https://reviewboard.mozilla.org/r/200022/#review216948 Nick's explanations make sense to me - wfm.
Attachment #8928751 - Flags: review?(michael.l.comella) → review+
Attachment #8928757 - Attachment is obsolete: true
Comment on attachment 8941192 [details] Bug 1411654 - Pre: Fix checkstyle whitespace error in GeckoHlsPlayer. This already had mcomella's r+.
Flags: needinfo?(max)
Attachment #8941192 - Flags: review?(michael.l.comella) → review+
Comment on attachment 8928746 [details] Bug 1411654 - Pre: Update toolchain documents for Android toolchain jobs. https://reviewboard.mozilla.org/r/200012/#review217244 This already had mcomella's r+.
Attachment #8928746 - Flags: review+
Attachment #8941192 - Flags: review+ → review?(nalexander)
Comment on attachment 8941192 [details] Bug 1411654 - Pre: Fix checkstyle whitespace error in GeckoHlsPlayer. https://reviewboard.mozilla.org/r/211468/#review217246
Attachment #8941192 - Flags: review?(nalexander) → review+
Comment on attachment 8928751 [details] Bug 1411654 - Part 5: Work through various new lint issues. https://reviewboard.mozilla.org/r/200022/#review217250
Comment on attachment 8928754 [details] Bug 1411654 - Part 5b: Work through WrongConstant lint issues. https://reviewboard.mozilla.org/r/200028/#review217254
Comment on attachment 8942288 [details] Bug 1411654 - Pre: Fix CustomTabsSecurityPopup package. https://reviewboard.mozilla.org/r/212572/#review218274 Cherry picked from https://bugzilla.mozilla.org/show_bug.cgi?id=1426244.
Attachment #8942288 - Flags: review?(nalexander) → review+
Pushed by nalexander@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/78576ff98660 Pre: Teach |mach gradle| to take GRADLE_FLAGS from the command line. r=maliu https://hg.mozilla.org/integration/autoland/rev/d23f467218da Pre: Don't block Google's maven repository. r=maliu https://hg.mozilla.org/integration/autoland/rev/264476c77210 Pre: Fix dangling resource. r=mcomella https://hg.mozilla.org/integration/autoland/rev/fb15e1f54987 Pre: Remove unused PerProfileDatabaseProvider. r=mcomella https://hg.mozilla.org/integration/autoland/rev/108674372ef7 Pre: Clear icon disk storage in test. r=mcomella https://hg.mozilla.org/integration/autoland/rev/9141bbdfd13b Pre: Gradle is no longer experimental; IntelliJ not supported. r=maliu https://hg.mozilla.org/integration/autoland/rev/43787f4089c3 Pre: Update toolchain documents for Android toolchain jobs. r=maliu https://hg.mozilla.org/integration/autoland/rev/bffb6a5b78d1 Pre: Fix checkstyle whitespace error in GeckoHlsPlayer. r=nalexander https://hg.mozilla.org/integration/autoland/rev/5c64eda20f1e Pre: Remove unused HomeExpandableListView. r=mcomella https://hg.mozilla.org/integration/autoland/rev/359fadf9b3e9 Pre: Fix CustomTabsSecurityPopup package. r=nalexander https://hg.mozilla.org/integration/autoland/rev/eda74143389f Part 0: Add rudimentary documentation about Gradle integration. r=maliu https://hg.mozilla.org/integration/autoland/rev/23cbcf427745 Part 1: Upgrade to Android-Gradle 3.0+ and build-tools;26.0.2. r=maliu https://hg.mozilla.org/integration/autoland/rev/0e493bacc5e3 Part 2: Update Robolectric to 3.5.1. r=mcomella https://hg.mozilla.org/integration/autoland/rev/6ff0cdf46a3d Part 3: Make each variant handle source from moz.build. r=maliu https://hg.mozilla.org/integration/autoland/rev/56538ed998cf Part 4: Use flavorDimensions to simplify {with,without}GeckoBinaries logic. r=maliu https://hg.mozilla.org/integration/autoland/rev/db978e230556 Part 5: Work through various new lint issues. r=mcomella https://hg.mozilla.org/integration/autoland/rev/a371f3ef4312 Part 6: Fix checkstyle. r=mcomella https://hg.mozilla.org/integration/autoland/rev/c2e51b70519f Part 5b: Work through WrongConstant lint issues. r=mcomella https://hg.mozilla.org/integration/autoland/rev/649e7aa405ca Part 5c: Work through ResourceUnused lint issues. r=mcomella https://hg.mozilla.org/integration/autoland/rev/113f1c4b9564 Post: Cull unused variables; guard more with MOZ_BUILD_MOBILE_ANDROID_WITH_GRADLE. r=maliu
Backed out changeset 113f1c4b9564 (bug 1411654) for android nightly bustages a=backout https://hg.mozilla.org/mozilla-central/rev/c8e0679f0184f38efac317ec4bef70cff0918ef8 https://hg.mozilla.org/mozilla-central/rev/113f1c4b9564
Flags: needinfo?(nalexander)
Status: RESOLVED → REOPENED
status-firefox59: fixed → ---
Resolution: FIXED → ---
Target Milestone: Firefox 59 → ---
Depends on: 1430417
Pushed by nalexander@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/d276d3deba05 Pre: Teach |mach gradle| to take GRADLE_FLAGS from the command line. r=maliu https://hg.mozilla.org/integration/autoland/rev/0850b319efd4 Pre: Don't block Google's maven repository. r=maliu https://hg.mozilla.org/integration/autoland/rev/22a861db1573 Pre: Fix dangling resource. r=mcomella https://hg.mozilla.org/integration/autoland/rev/3dc3beab95f8 Pre: Remove unused PerProfileDatabaseProvider. r=mcomella https://hg.mozilla.org/integration/autoland/rev/373c9a71d945 Pre: Clear icon disk storage in test. r=mcomella https://hg.mozilla.org/integration/autoland/rev/901b304603d9 Pre: Gradle is no longer experimental; IntelliJ not supported. r=maliu https://hg.mozilla.org/integration/autoland/rev/e7b0cc801cf1 Pre: Update toolchain documents for Android toolchain jobs. r=maliu https://hg.mozilla.org/integration/autoland/rev/d0d513d1c379 Pre: Fix checkstyle whitespace error in GeckoHlsPlayer. r=nalexander https://hg.mozilla.org/integration/autoland/rev/2b37201606f5 Pre: Remove unused HomeExpandableListView. r=mcomella https://hg.mozilla.org/integration/autoland/rev/76085ddd5ac7 Pre: Fix CustomTabsSecurityPopup package. r=nalexander https://hg.mozilla.org/integration/autoland/rev/cec2b8828cc8 Part 0: Add rudimentary documentation about Gradle integration. r=maliu https://hg.mozilla.org/integration/autoland/rev/f918500d9cf5 Part 1: Upgrade to Android-Gradle 3.0+ and build-tools;26.0.2. r=maliu https://hg.mozilla.org/integration/autoland/rev/690e265c684c Part 2: Update Robolectric to 3.5.1. r=mcomella https://hg.mozilla.org/integration/autoland/rev/730a70767743 Part 3: Make each variant handle source from moz.build. r=maliu https://hg.mozilla.org/integration/autoland/rev/01836fd98c63 Part 4: Use flavorDimensions to simplify {with,without}GeckoBinaries logic. r=maliu https://hg.mozilla.org/integration/autoland/rev/fde9bf9c14c3 Part 5: Work through various new lint issues. r=mcomella https://hg.mozilla.org/integration/autoland/rev/c270f97bb0da Part 6: Fix checkstyle. r=mcomella https://hg.mozilla.org/integration/autoland/rev/c5bf85d56fed Part 5b: Work through WrongConstant lint issues. r=mcomella https://hg.mozilla.org/integration/autoland/rev/55776829a744 Part 5c: Work through ResourceUnused lint issues. r=mcomella https://hg.mozilla.org/integration/autoland/rev/cfad693be918 Post: Cull unused variables; guard more with MOZ_BUILD_MOBILE_ANDROID_WITH_GRADLE. r=maliu
Depends on: 1430788
Depends on: 1430791
No longer depends on: 1430791
No longer depends on: 1430788
Just noticed the "Leaks" shortcut on my android phone after some recently Firefox Nightly update. When started it displays "Leaks in org.mozilla.fennec" in the title strip. There is no vsible content of the window. Oh, when I moved the "Leaks" icon to the trashcan the Firefox Nightly went with it. That's pretty inconvenient considering I had web apps with Indexed DB installed.
(In reply to Adrian Aichner [:anaran] from comment #155) > Just noticed the "Leaks" shortcut on my android phone after some recently > Firefox Nightly update. Yes -- that's https://bugzilla.mozilla.org/show_bug.cgi?id=1430417, which I just landed a fix for. > > When started it displays "Leaks in org.mozilla.fennec" in the title strip. > > There is no vsible content of the window. > > Oh, when I moved the "Leaks" icon to the trashcan the Firefox Nightly went > with it. > > That's pretty inconvenient considering I had web apps with Indexed DB > installed. Indeed, and we're sorry -- but that's how Android works: when you trash a launcher icon, you remove the App entirely.
Flags: needinfo?(nalexander)
Depends on: 1431140
Blocks: 1419581
OK, backouts for Bug 1419581, Bug 1430417, and Bug 1411654 (in that order) don't conflict. Try pushes with full builds https://treeherder.mozilla.org/#/jobs?repo=try&revision=01154eb18c96610c202e6eb8bcb84355dacc1fcf and with artifact builds https://treeherder.mozilla.org/#/jobs?repo=try&revision=2761199fdc0bfabf4ed2f4cf9f21f2da850224f1 RyanVM: others: I'll be pretty confident after the artifact build, since this is all on the Java/Gradle side of the house. I'll comment on this ticket after manually verifying that the debuggable state is correct on the try builds.
No longer blocks: 1419581
> RyanVM: others: I'll be pretty confident after the artifact build, since > this is all on the Java/Gradle side of the house. I'll comment on this > ticket after manually verifying that the debuggable state is correct on the > try builds. I checked one of the APKs from one of the try build, and it's definitely not debuggable, so I think this is good to land on m-c. I'll push it myself.
Backed out for causing bug 1431140. https://hg.mozilla.org/mozilla-central/rev/ef04f3ad8475
Status: RESOLVED → REOPENED
status-firefox59: fixed → wontfix
Resolution: FIXED → ---
Target Milestone: Firefox 59 → ---
Status: REOPENED → ASSIGNED
Comment on attachment 8941192 [details] Bug 1411654 - Pre: Fix checkstyle whitespace error in GeckoHlsPlayer. https://reviewboard.mozilla.org/r/211468/#review219476
Attachment #8941192 - Flags: review?(michael.l.comella) → review+
Status update: there's a change to how the Android-Gradle plugin handles the underlying AndroidManifest.xml. The plugin now overrides android:debuggable from the manifest. That much is fine; we can always set the android:debuggable flag in Gradle. (What's not fine: I didn't catch this, and our automated tests don't either.) However, when debuggable is false, we don't have an external library compile dependency. That means that we don't run a particular Gradle build step -- merging DEX files from the App and all the external libraries -- and that means classes.dex is in dex/... and not dexMerger/... and that fails our Makefile.in/classes.dex hackery. The ugly hack is to look in two places for classes.dex. The better approach is to _not monkey with classes.dex directly_ and instead use the APK produced by Gradle. (And still have classes.dex handling for non-Gradle builds, may they rot in a warm place.) This approach is basically implemented in my patches for https://bugzilla.mozilla.org/show_bug.cgi?id=1414054. So my plan is to return to that ticket, extract the classes.dex changes, and try this again. Tricky, tricky.
Depends on: 1433285
Alias: gradle-3.0
status-firefox60: --- → affected
FYI: I made progress upgrading the Android Gradle Plugin for Firefox TV and, while I haven't finished (one test hangs), I didn't have to disable aapt2 like this fix did. fwiw, here are my WIP changes: https://github.com/mozilla-mobile/firefox-tv/pull/426 NI nalexander for info but no action needed.
Flags: needinfo?(nalexander)
OK, now that Bug 1414054 looks good in the wilds of Nightly, and with https://treeherder.mozilla.org/#/jobs?repo=try&revision=3c66d45a187568954d2ec5b4f8fd761c9db24838 looking like it has minimal changes against mozilla-central, I think this is ready to go. Just for the record, I am aware that the sub-packages (geckoview, thirdparty) are now compiled in release mode rather than debug mode, and to the best of my knowledge that's fine -- in fact, better than fine, desirable. Let's hope I'm not wrong!
Flags: needinfo?(nalexander)
Pushed by nalexander@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/c86a29ef0b5b Pre: Teach |mach gradle| to take GRADLE_FLAGS from the command line. r=maliu https://hg.mozilla.org/integration/autoland/rev/24e4f9a8e3cd Pre: Don't block Google's maven repository. r=maliu https://hg.mozilla.org/integration/autoland/rev/f4ea28fdbc3e Pre: Fix dangling resource. r=mcomella https://hg.mozilla.org/integration/autoland/rev/9177a22af8d0 Pre: Remove unused PerProfileDatabaseProvider. r=mcomella https://hg.mozilla.org/integration/autoland/rev/986da6f801e4 Pre: Clear icon disk storage in test. r=mcomella https://hg.mozilla.org/integration/autoland/rev/c9bff6158dd3 Pre: Gradle is no longer experimental; IntelliJ not supported. r=maliu https://hg.mozilla.org/integration/autoland/rev/695ed77978fc Pre: Update toolchain documents for Android toolchain jobs. r=maliu https://hg.mozilla.org/integration/autoland/rev/96c50f164965 Pre: Fix checkstyle whitespace error in GeckoHlsPlayer. r=mcomella https://hg.mozilla.org/integration/autoland/rev/d5a5a3d8e6e5 Pre: Remove unused HomeExpandableListView. r=mcomella https://hg.mozilla.org/integration/autoland/rev/cc3b61efc9b3 Pre: Fix CustomTabsSecurityPopup package. r=nalexander https://hg.mozilla.org/integration/autoland/rev/2d2ce8d0b969 Part 0: Add rudimentary documentation about Gradle integration. r=maliu https://hg.mozilla.org/integration/autoland/rev/bc4796418006 Part 1: Upgrade to Android-Gradle 3.0+ and build-tools;26.0.2. r=maliu https://hg.mozilla.org/integration/autoland/rev/d67b952ecbe4 Part 2: Update Robolectric to 3.5.1. r=mcomella https://hg.mozilla.org/integration/autoland/rev/8f2c1b74bf9a Part 3: Make each variant handle source from moz.build. r=maliu https://hg.mozilla.org/integration/autoland/rev/589194574eff Part 4: Use flavorDimensions to simplify {with,without}GeckoBinaries logic. r=maliu https://hg.mozilla.org/integration/autoland/rev/1eb2f1b4c961 Part 5: Work through various new lint issues. r=mcomella https://hg.mozilla.org/integration/autoland/rev/cbbb957aa470 Part 6: Fix checkstyle. r=mcomella https://hg.mozilla.org/integration/autoland/rev/431cf29e8339 Part 5b: Work through WrongConstant lint issues. r=mcomella https://hg.mozilla.org/integration/autoland/rev/a4acfa51d53c Part 5c: Work through ResourceUnused lint issues. r=mcomella https://hg.mozilla.org/integration/autoland/rev/ba88f9cd2669 Post: Cull unused variables; guard more with MOZ_BUILD_MOBILE_ANDROID_WITH_GRADLE. r=maliu
(In reply to Michael Comella (:mcomella) [please needinfo for a response] from comment #162) > FYI: I made progress upgrading the Android Gradle Plugin for Firefox TV and, > while I haven't finished (one test hangs), I didn't have to disable aapt2 > like this fix did. fwiw, here are my WIP changes: > https://github.com/mozilla-mobile/firefox-tv/pull/426 > > NI nalexander for info but no action needed. Thanks, mcomella! Just for those interested, the aapt2 change was required to placate Robolectric, which just hadn't updated to Android-Gradle 3.0. However, this was done a few months ago -- and we're _already_ at A-G 3.0.1 -- so it's possible that Robolectric has addressed the aapt2 change.
Blocks: 1509539
Product: Firefox for Android → Firefox Build System
Target Milestone: Firefox 60 → mozilla60
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: