Closed
Bug 1228418
Opened 9 years ago
Closed 7 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)
Firefox Build System
Android Studio and Gradle Integration
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.
Reporter | ||
Comment 1•9 years ago
|
||
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)
Reporter | ||
Comment 2•9 years ago
|
||
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)
Reporter | ||
Comment 3•9 years ago
|
||
Bug 1228418 - Exclude all but mobile/. r=me
DONTBUILD NPOTB
Comment 4•9 years ago
|
||
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 5•9 years ago
|
||
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+
Comment 6•9 years ago
|
||
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)
Reporter | ||
Comment 7•9 years ago
|
||
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
Reporter | ||
Comment 8•9 years ago
|
||
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
Reporter | ||
Comment 9•9 years ago
|
||
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)
Reporter | ||
Comment 10•9 years ago
|
||
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)
Reporter | ||
Comment 11•9 years ago
|
||
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)
Reporter | ||
Updated•9 years ago
|
Attachment #8695988 -
Flags: review?(gps)
Reporter | ||
Updated•9 years ago
|
Attachment #8706707 -
Flags: review?(s.kaspari)
Reporter | ||
Updated•9 years ago
|
Attachment #8706708 -
Flags: review?(dustin)
Comment 12•9 years ago
|
||
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.
Comment 13•9 years ago
|
||
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?
Comment 14•9 years ago
|
||
(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?
Comment 15•9 years ago
|
||
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.
Reporter | ||
Comment 16•7 years ago
|
||
This is superseded by https://bugzilla.mozilla.org/show_bug.cgi?id=1366644.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → DUPLICATE
Reporter | ||
Updated•7 years ago
|
Component: Build Config → Build Config & IDE Support
Product: Core → Firefox for Android
Updated•5 years ago
|
Product: Firefox for Android → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•