Closed Bug 1119520 (gradle) Opened 5 years ago Closed 4 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

Tracking

(firefox47 fixed)

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

People

(Reporter: nalexander, Assigned: nalexander)

References

(Depends on 3 open bugs, Blocks 1 open bug)

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
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.
/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.
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
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)
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/
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: 4 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 → ---
https://hg.mozilla.org/mozilla-central/rev/ff393d231e94
Status: REOPENED → RESOLVED
Closed: 4 years ago → 4 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)
Depends on: 1363843
No longer depends on: lint-newapi
Component: Build Config → Build Config & IDE Support
Product: Core → Firefox for Android
Target Milestone: mozilla47 → ---
Product: Firefox for Android → Firefox Build System
You need to log in before you can comment on or make changes to this bug.