Closed Bug 1119520 (gradle) Opened 10 years ago Closed 9 years ago

Add opt-in Gradle build mode for mobile/android developers

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

(firefox47 fixed)

Status:
RESOLVED FIXED
Tracking Flags:
Tracking Status
firefox47 --- fixed

People

(Reporter: nalexander, Assigned: nalexander)

References

(Depends on 3 open bugs)

Details

Attachments

(1 file, 4 obsolete files)

MozReview Request: Bug 1119520 - Add opt-in Gradle build mode for mobile/android. r?gps
58 bytes, text/x-review-board-request
gps
: review+
Details
Bug 1041395 and friends added Gradle and IDEA integration; Bug 1103121 added |mach gradle|. This ticket tracks adding an opt-in build mode that builds mobile/android with Gradle rather than with the usual moz.build/Makefile.in rules. That will include: * a configure.in flag (--enable-mobile-android-gradle-build or similar); * whatever config/ and tooltool magic is needed to get try builds standing; * invoking gradle from mobile/android/{base/?}Makefile.in as appropriate; * configuring Gradle to Proguard as appropriate; * arranging to export classes.dex to dist/, etc.
I forget that this will need to build robocop.apk as well.
Depends on: 1120032
Depends on: 1121622
Attached file MozReview Request: bz://1119520/nalexander (obsolete) —
Attachment #8549902 - Flags: review?(rnewman)
/r/2561 - Bug 1119520 - Add opt-in Gradle build mode for mobile/android. r=gps /r/2563 - Bug 1120032 - WIP. /r/2565 - Bug 1120032 - WebRTC, work around Make races. /r/2567 - Bug 1120032 - No maxSdkVersion. /r/2569 - Bug 1119061 - Part 1: Add Fennec-specific Sync 1.1 -> Sync 1.5 migration telemetry histograms. r=rnewman Pull down these commits: hg pull review -r 8a0c3ae7ef33c5494d57b0175ca73fb65b773e50
https://reviewboard.mozilla.org/r/2569/#review1721 ::: toolkit/components/telemetry/Histograms.json (Diff revision 1) > + "high": 100, I'd be inclined to make this linear and reduce the high. After all, we expect this to be zero or one, no? ::: toolkit/components/telemetry/Histograms.json (Diff revision 1) > + "n_buckets": 10, Do these need to be exponential if the high is 20? Why not make these simple counters? (kind: "count")
https://reviewboard.mozilla.org/r/2559/#review1725 Some open questions, and I'm not really the best person to review this, but if it works, I'm happy with it. I imagine glandium would have higher standards. ::: configure.in (Diff revision 1) > +dnl AC_SUBST(GRADLE_ANDROID_MAVEN_REPOSITORY) ? ::: mobile/android/config/mozconfigs/common.override (Diff revision 1) > +export MOZ_DISABLE_GECKOVIEW=1 I seem to recall glandium disliking this change. ::: mobile/android/gradle/android.gradle (Diff revision 1) > - if (mozconfig.defines.MOZ_ANDROID_MAX_SDK_VERSION) { > + // if (mozconfig.defines.MOZ_ANDROID_MAX_SDK_VERSION) { Why commented out? Did it work before? ::: toolkit/components/telemetry/Histograms.json (Diff revision 1) > + "FENNEC_SYNC11_MIGRATION_SENTINELS_SEEN": { I think this snuck in.
Depends on: 1123013
Depends on: 1125330
Comment on attachment 8549902 [details] MozReview Request: bz://1119520/nalexander Clearing flag; left comments.
Attachment #8549902 - Flags: review?(rnewman) → feedback+
Assignee: nobody → nalexander
Status: NEW → ASSIGNED
Depends on: 1128026
Depends on: 1128362
Attachment #8549902 - Attachment is obsolete: true
Depends on: 1176133
Depends on: 1190940
Blocks: 1208793
Depends on: 1208795
See Also: → 1212913
Depends on: 1217228
Depends on: 1217898
Depends on: 1218370
Depends on: lint-newapi
Depends on: 1233882
jcenter() now contains both com.squareup.spoon:spoon-runner:1.1.10 and org.simpleframework:simple-http:6.0.1 so we can get rid of some outdated listings. Review commit: https://reviewboard.mozilla.org/r/30449/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/30449/
Opt-in by adding --enable-building-mobile-android-with-gradle. Configure the location of Gradle with --with-gradle. Gradle dependencies (including the Android-Gradle plugin) are fetched from the jcentral repository for local developers, and from the GRADLE_BUILDSCRIPT_MAVEN_REPOSITORY path in automation. Android-specific Maven dependencies are shipped as "extras" with the Android SDK, and should be found automatically by the Android-Gradle plugin. Review commit: https://reviewboard.mozilla.org/r/30451/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/30451/
Attachment #8706710 - Flags: review?(gps)
This is a convenience, and a look ahead to a Gradle-based future. The reason to do this is that front-end builds don't (really) support testing, so we set --disable-tests. However, this means that mobile/android/tests/browser/robocop doesn't get processed at build time, and that means the Robocop manifest doesn't get preprocessed. Rather than hack generating the Robocop manifest in, let's skate to where the puck is going and just use Gradle where appropriate. Review commit: https://reviewboard.mozilla.org/r/30453/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/30453/
Attachment #8706711 - Flags: review?(s.kaspari)
I have no particular attachment to anything here other than using a stripped down manifest that does not include the NDK toolchain. This is grade A cargo cult, with extra cult followers. Review commit: https://reviewboard.mozilla.org/r/30455/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/30455/
Attachment #8706712 - Flags: review?(dustin)
Folks -- these are somewhat experimental commits. sebastian, I think yours is no issue. gps, this is a sequence of hacks to try to opt-in a Gradle build mode before eventually just using Gradle across the board. It's not pretty but it keeps our current shape, which is basically needed for l10n repacks. That is, we need to produce a classes.dex and a fennec.ap_ independently, at any time, which is madness but there you have it. dustin, this is straight cargo. Feel free to set direction (I've been cribbing from the eslint ticket) or redirect as appropriate. I have additional tickets that build on this frontend build; I don't know how to structure them, so I'll just push them at you (tomorrow) and let you guide me.
Comment on attachment 8706711 [details] MozReview Request: Bug 1119520 - Post: Use Gradle's manifest placeholders to produce Robocop manifest. r?sebastian https://reviewboard.mozilla.org/r/30453/#review27215 ::: mobile/android/app/build.gradle:15 (Diff revision 1) > + manifestPlaceholders = [ > + ANDROID_PACKAGE_NAME: mozconfig.substs.ANDROID_PACKAGE_NAME, > + MOZ_ANDROID_MIN_SDK_VERSION: mozconfig.substs.MOZ_ANDROID_MIN_SDK_VERSION, > + MOZ_ANDROID_SHARED_ID: "${mozconfig.substs.ANDROID_PACKAGE_NAME}.sharedID", > + ] Oh, nice. I didn't know about manifestPlaceholders.
Attachment #8706711 - Flags: review?(s.kaspari) → review+
Comment on attachment 8706712 [details] MozReview Request: Bug 1119520 - Add job building Fennec with --disable-compile-environment. r?dustin https://reviewboard.mozilla.org/r/30455/#review27385 I think this is the same patch as https://reviewboard.mozilla.org/r/30445/diff/1#index_header so the same comments apply.
Attachment #8706712 - Flags: review?(dustin)
Attachment #8706710 - Flags: review?(gps)
Attachment #8706710 - Flags: review?(gps)
Comment on attachment 8706710 [details] MozReview Request: Bug 1119520 - Add opt-in Gradle build mode for mobile/android. r?gps Review request updated; see interdiff: https://reviewboard.mozilla.org/r/30451/diff/1-2/
Attachment #8706709 - Attachment is obsolete: true
Attachment #8706711 - Attachment is obsolete: true
Attachment #8706712 - Attachment is obsolete: true
gps: you reviewed this at https://reviewboard.mozilla.org/r/27237/. It got attached to the wrong ticket. I stripped some of the automation stuff (Gradle binary choice, repository stuff). I'll add that back for automation builds eventually; I want to get some local testers onto this and ferret out races, etc. I didn't rewrite the internal variable name (just the external --enable... flag) since the variable will go away eventually.
Comment on attachment 8706710 [details] MozReview Request: Bug 1119520 - Add opt-in Gradle build mode for mobile/android. r?gps https://reviewboard.mozilla.org/r/30451/#review31117 You need to find another Fennec build peer so I don't have to be reminded these make files exist :) ::: mobile/android/base/Makefile.in:7 (Diff revision 2) > +ifdef MOZ_BUILD_MOBILE_ANDROID_WITH_GRADLE > +.NOTPARALLEL: > +endif This is unfortunate. But since this doesn't impact automation builds, I'm OK with it. ::: mobile/android/base/Makefile.in:576 (Diff revision 2) > -libs:: classes.dex jni-stubs.inc GeneratedJNIWrappers.cpp $(CURDIR)/fennec_ids.txt > - $(INSTALL) classes.dex $(FINAL_TARGET) > +ifndef MOZ_BUILD_MOBILE_ANDROID_WITH_GRADLE > +libs:: jni-stubs.inc GeneratedJNIWrappers.cpp Thank you for cleaning this up. ::: python/mozbuild/mozbuild/mozconfig.py:110 (Diff revision 2) > - env_path = env.get('MOZCONFIG', None) > + env_path = env.get('MOZCONFIG', None) or None I'm surprised to see this change here. I grok that MOZCONFIG could be ''. I'm just curious how you managed to tickle this. ::: toolkit/mozapps/installer/upload-files.mk:328 (Diff revision 2) > -ROBOCOP_PATH = $(topobjdir)/mobile/android/tests/browser/robocop > +ifndef MOZ_BUILD_MOBILE_ANDROID_WITH_GRADLE > +robocop_apk := $(topobjdir)/mobile/android/tests/browser/robocop/robocop-debug-unsigned-unaligned.apk > +else > +robocop_apk := $(topobjdir)/gradle/build/mobile/android/app/outputs/apk/app-automation-debug-androidTest-unaligned.apk > +endif That's the 2nd time I've seen this block. If there were 3, I'd insist on moving it to an included .mk file. But I'll allow it.
Attachment #8706710 - Flags: review?(gps) → review+
https://reviewboard.mozilla.org/r/30451/#review31117 > I'm surprised to see this change here. I grok that MOZCONFIG could be ''. I'm just curious how you managed to tickle this. I never figured out why. I tickled this two different ways, both of which involved running mach in strange environments. I see '' intermittently when invoking mach as part of a Gradle build, kicked off from within IntelliJ / Android Studio (under Mac OS X). I also saw it when invoking mach as part of a buck build. I tried quite hard to understand the buck environment and how it would differ, but found nothing -- it appeared to be a Java quirk. > That's the 2nd time I've seen this block. If there were 3, I'd insist on moving it to an included .mk file. But I'll allow it. This signing actually applies to both `moz.build` and Gradle, so I thought I might add some variable like `ANDROID_SIGNED_APKS` to collect these. We should be able to determine the output locations fairly automatically, and do the right thing at package time.
https://hg.mozilla.org/mozilla-central/rev/306cf0271d3e
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Hi, had to back this out since this caused problems with the merge from mozilla-inbound to mozilla-central like warning: conflicts while merging mobile/android/base/Makefile.in! (edit, then use 'hg resolve --mark') warning: conflicts while merging mobile/android/moz.build! (edit, then use 'hg resolve --mark')
Status: RESOLVED → REOPENED
Flags: needinfo?(nalexander)
Resolution: FIXED → ---
Depends on: 1248005
https://hg.mozilla.org/mozilla-central/rev/ff393d231e94
Status: REOPENED → RESOLVED
Closed: 9 years ago → 9 years ago
Resolution: --- → FIXED
(In reply to Carsten Book [:Tomcat] from comment #43) > Hi, had to back this out since this caused problems with the merge from > mozilla-inbound to mozilla-central like > > > warning: conflicts while merging mobile/android/base/Makefile.in! (edit, > then use 'hg resolve --mark') > > warning: conflicts while merging mobile/android/moz.build! (edit, then use > 'hg resolve --mark') Relanded. Thanks Tomcat!
Flags: needinfo?(nalexander)
Blocks: 1255915
Depends on: 1361418
Depends on: 1363843
No longer depends on: lint-newapi
Component: Build Config → Build Config & IDE Support
Product: Core → Firefox for Android
Target Milestone: mozilla47 → ---
Blocks: 1557796
Product: Firefox for Android → Firefox Build System
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: