Closed Bug 1228418 Opened 4 years ago Closed 2 years ago

Upgrade Fennec Gradle configuration to Gradle 2.8 and com.android.tools.build:gradle to 2.0.0

Categories

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

defect
Not set

Tracking

(Not tracked)

RESOLVED DUPLICATE of bug 1366644

People

(Reporter: nalexander, Unassigned)

References

Details

Attachments

(5 files)

The new Android Studio release uses

com.android.tools.build:gradle:2.0.0-alpha1

That's probably a little bleeding edge for us, but we want to upgrade ASAP, since we really want to use the new Android Studio Instant Run feature.

There's a reasonable likelihood this would deprecate IntelliJ for us, even 15, since IJ lags behind AS in Gradle support.  Unfortunate but probably worth doing.

Locally, this is easy to arrange.  I see slightly faster build times -- 50-55s compared to perhaps 65s.
Depends on: 1229435
Bug 1228418 - Set multiDexEnabled in Gradle builds rooted in the topsrcdir. r?sebastian

The default configuration is now localDebug for API 21+, and
localOldDebug for <API 21.  This actually paves the way nicely for
automation builds; previously, I was intending to use debug and
release to for local and automation builds, respectively, but this is
more clear.
Attachment #8695986 - Flags: review?(s.kaspari)
Bug 1228418 - Upgrade Fennec Gradle configuration to Gradle 2.8 and com.android.tools.build:gradle to 2.0.0-alpha2. r?sebastian
Attachment #8695987 - Flags: review?(s.kaspari)
Comment on attachment 8695986 [details]
MozReview Request: Bug 1228418 - Set multiDexEnabled in Gradle builds rooted in the topsrcdir. r=sebastian

https://reviewboard.mozilla.org/r/27233/#review24645

::: mobile/android/app/build.gradle:34
(Diff revision 1)
> +    productFlavors {

Nice idea to use product flavors for the different local builds!

::: mobile/android/app/build.gradle:80
(Diff revision 1)
> +    compile 'com.android.support:multidex:1.0.0'

I'm confused: According to this official guide[1] the multidex support library is only needed prior to Android 5.0. Above you only enable multidex for API >= 21. So is this really needed here?

The same guide mentions that the Application object has to call MultiDex.install(this) or has to be extended from MultiDexApplication. That's also how I remember it from the last time I used it. However the MultiDexApplication javadoc[2] says that this is only needed if the "legacy multidex library" is used. I assume this means that this only needed if we are actually using the support library..?

[1] http://developer.android.com/tools/building/multidex.html#mdex-pre-l
[2] http://developer.android.com/reference/android/support/multidex/MultiDexApplication.html
Attachment #8695986 - Flags: review?(s.kaspari) → review+
Comment on attachment 8695987 [details]
MozReview Request: Bug 1119520 - Pre: Remove no longer required Maven repository; bump test-only dependency. r=me

https://reviewboard.mozilla.org/r/27235/#review24649

::: gradle/wrapper/gradle-wrapper.properties:1
(Diff revision 1)
>  #Mon Nov 02 13:44:39 GMT 2015

Wrong timestamp! j/k :)
Attachment #8695987 - Flags: review?(s.kaspari) → review+
https://reviewboard.mozilla.org/r/27233/#review24653

::: mobile/android/app/build.gradle:80
(Diff revision 1)
> +    compile 'com.android.support:multidex:1.0.0'

Also: If we need this line: Couldn't we use localCompile '..' to add this dependency only to the local flavor? (untested)
Comment on attachment 8695986 [details]
MozReview Request: Bug 1228418 - Set multiDexEnabled in Gradle builds rooted in the topsrcdir. r=sebastian

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/27233/diff/1-2/
Attachment #8695986 - Attachment description: MozReview Request: Bug 1228418 - Set multiDexEnabled in Gradle builds rooted in the topsrcdir. r?sebastian → MozReview Request: Bug 1228418 - Set multiDexEnabled in Gradle builds rooted in the topsrcdir. r=sebastian
Comment on attachment 8695987 [details]
MozReview Request: Bug 1119520 - Pre: Remove no longer required Maven repository; bump test-only dependency. r=me

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/27235/diff/1-2/
Attachment #8695987 - Attachment description: MozReview Request: Bug 1228418 - Upgrade Fennec Gradle configuration to Gradle 2.8 and com.android.tools.build:gradle to 2.0.0-alpha2. r?sebastian → MozReview Request: Bug 1119520 - Pre: Remove no longer required Maven repository; bump test-only dependency. r=me
Comment on attachment 8695988 [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/27237/diff/1-2/
Attachment #8695988 - Attachment description: MozReview Request: Bug 1228418 - Exclude all but mobile/. r=me → MozReview Request: Bug 1119520 - Add opt-in Gradle build mode for mobile/android. r?gps
Attachment #8695988 - 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/30443/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/30443/
Attachment #8706707 - 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/30445/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/30445/
Attachment #8706708 - Flags: review?(dustin)
Attachment #8695988 - Flags: review?(gps)
Attachment #8706707 - Flags: review?(s.kaspari)
Attachment #8706708 - Flags: review?(dustin)
https://reviewboard.mozilla.org/r/27237/#review27363

::: configure.in:5381
(Diff revision 2)
> +MOZ_ARG_ENABLE_BOOL(building-mobile-android-with-gradle,
> +[  --enable-building-mobile-android-with-gradle      Enable building mobile/android with Gradle],

This is a bit verbose. How about `--enable-gradle-mobile-android-builds`?

::: mobile/android/base/Makefile.in:168
(Diff revision 2)
> +gradle_dir := $(DEPTH)/gradle/build/mobile/android

We added a `$(topobjdir)`, which you are now encouraged to use.

::: mobile/android/config/mozconfigs/common:70
(Diff revision 2)
> +ac_add_options --with-gradle="$topsrcdir/gradle-2.7/bin/gradle"
> +ac_add_options --with-gradle-maven-repository="file://$topsrcdir/central"

If we're not activating the gradle build mode, why do you add these to automation?

::: python/mozbuild/mozbuild/mozconfig.py:110
(Diff revision 2)
>          env_path = env.get('MOZCONFIG', None)
> +        if env_path == '':
> +            env_path = None

env_path = env.get('MOZCONFIG', None) or None

::: testing/mochitest/Makefile.in:108
(Diff revision 2)
> +ifndef MOZ_BUILD_MOBILE_ANDROID_WITH_GRADLE
>  	$(call RELEASE_SIGN_ANDROID_APK,\
>  		$(DEPTH)/mobile/android/tests/browser/robocop/robocop-debug-unsigned-unaligned.apk,\
>  		$(_DEST_DIR)/robocop.apk)
> +else
> +	$(call RELEASE_SIGN_ANDROID_APK,\
> +		$(DEPTH)/gradle/build/mobile/android/app/outputs/apk/app-automation-debug-androidTest-unaligned.apk,\
> +		$(_DEST_DIR)/robocop.apk)
> +endif

Since the only difference is the path, you should assign the path to a variable and move the `if` out of the make rule.

::: toolkit/mozapps/installer/upload-files.mk:344
(Diff revision 2)
> +else
> +INNER_ROBOCOP_PACKAGE= \
> +  cp $(GECKO_APP_AP_PATH)/fennec_ids.txt $(ABS_DIST) && \
> +  $(call RELEASE_SIGN_ANDROID_APK,$(abspath $(DEPTH)/gradle/build/mobile/android/app/outputs/apk/app-automation-debug-androidTest-unaligned.apk),$(ABS_DIST)/robocop.apk)
> +endif

This could also be variable-ized.

::: toolkit/mozapps/installer/upload-files.mk:473
(Diff revision 2)
> +  ((test ! -f $(GECKO_APP_AP_PATH)/R.txt && echo "*** Warning: The R.txt that is being packaged might not agree with the R.txt that was built. This is normal during l10n repacks.") || \

This feels like something that should error if we're not doing an l10n repack. I realize you're just moving code here. So feel free to drop.
https://reviewboard.mozilla.org/r/30445/#review27375

This is looking good.  I think you could pertty easily build a `tasks/builds/android_base.yml` that inherits from `tasks/builds/mobile_base.yml` and defines the image, features, some of the env, and the extra.  That may reduce the duplication in the task definitions, if you'll be adding more of these in the future.

::: mobile/android/config/mozconfigs/android-api-11-frontend/nightly:9
(Diff revision 1)
> +MOZ_AUTOMATION_UPLOAD_SYMBOLS=0

I'm no expert on mozconfigs, but I notice that these definitions are not in `mobile/android/config/mozconfigs/android-api-11/nightly`.  Are they appropriate here?

::: mobile/android/config/mozconfigs/android-api-11-frontend/nightly:16
(Diff revision 1)
> +ac_add_options --disable-tests 

nit: trailing whitespace

::: mobile/android/config/tooltool-manifests/android-frontend/releng.manifest:39
(Diff revision 1)
> +"filename": "maven-central.tar.xz",

Maven scares me a little -- is it going to be doing a lot of downloading of dependencies or anything like that?

Also, nit: lots of trailing whitespace in this file (tooltool.py does that for some reason)

::: testing/taskcluster/tasks/branches/base_job_flags.yml:101
(Diff revision 1)
> +    - android-api-frontend

Bearing in mind that I know ABSOLUTELY nothing about building Android: are "11" and "frontend" parallel here?  I'm just wondering if this naming is confusing to someone who knows more than me, particularly since the task yml is named `android_api_11_frontend`.

::: testing/taskcluster/tasks/branches/try/job_flags.yml:55
(Diff revision 1)
> +        task: tasks/builds/android_api_11_frontend.yml

I think this is redundant to the same change in `tasks/branches/base_jobs.yml`.  I think the things in this file are for try only, which is why they're not in the base jobs definition.

::: testing/taskcluster/tasks/builds/android_api_11_frontend.yml:26
(Diff revision 1)
> +      build-{{project}}-android-api-11-c6-workspace: '/home/worker/workspace'

This will end up using the same workspace as the Android API 11 builds -- will those get along nicely and benefit from shared caches?  If not, they should use different cache names.

::: testing/taskcluster/tasks/builds/android_api_11_frontend.yml:59
(Diff revision 1)
> +        platform: android-4-0-armv7-api11

I think this will make these builds appear on the same line in treeherder as the existing API-11 builds.  Is that what you want?
(In reply to Gregory Szorc [:gps] from comment #12)
> https://reviewboard.mozilla.org/r/27237/#review27363
> 
> ::: configure.in:5381
> (Diff revision 2)
> > +MOZ_ARG_ENABLE_BOOL(building-mobile-android-with-gradle,
> > +[  --enable-building-mobile-android-with-gradle      Enable building mobile/android with Gradle],
> 
> This is a bit verbose. How about `--enable-gradle-mobile-android-builds`?

If there's a --with-gradle, why need this option at all?
https://reviewboard.mozilla.org/r/30445/#review27383

::: testing/taskcluster/tasks/branches/base_job_flags.yml:101
(Diff revision 1)
> +    - android-api-frontend

Actually now that I look, I see that this is just wrong, and should include `-11` to link to the build definition below.
Depends on: 1240579
Depends on: 1242284
This is superseded by https://bugzilla.mozilla.org/show_bug.cgi?id=1366644.
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1366644
Component: Build Config → Build Config & IDE Support
Product: Core → Firefox for Android
Product: Firefox for Android → Firefox Build System
You need to log in before you can comment on or make changes to this bug.