929298 - Move ANDROID_APK to moz.build

Status: RESOLVED WONTFIX

(Firefox Build System :: General, defect)

Description

[Nick Alexander :nalexander [he/him]]

Bug 919563 moved ad-hoc APK generation to rules.mk (but did not move mobile/android/base to use that APK generation).  We should 1) finish that job (which includes updating |make package| and 2) move APK generation to moz.build.

This could be part of writing IDE project files for all the APKs defined in the build system (Bug 853045).

Comment 1

[Nick Alexander :nalexander [he/him]]

Attachment: 1-Bug_929298___Part_0__Allow_Android_background_tests_to_compile_with__Xlint_all__r_me.patch

Comment 2

[Nick Alexander :nalexander [he/him]]

Attachment: 2-Bug_929298___Part_1__Touch_up_java_build_mk__r_gps.patch

Comment 3

[Nick Alexander :nalexander [he/him]]

Attachment: 3-Bug_929298___Part_2__Add_add_android_apk_to_moz_build__r_gps.patch

Comment 4

[Nick Alexander :nalexander [he/him]]

Attachment: 4-Bug_929298___Part_3__Use_add_android_apk_in_easy_cases__r_gps.patch

Comment 5

[Nick Alexander :nalexander [he/him]]

http://tbpl.mozilla.org/?tree=Try&rev=3762a21cdcf6

Comment 6

[Nick Alexander :nalexander [he/him]]

Here's some work in progress that improves the APK building situation and gets rid of some Makefile.ins.  I'll get back to this for build/mobile/robocop's APK after Bug 905703 lands, and of course there's still mobile/android/base's APK.

Comment 7

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 824218 [details] [diff] [review]
3-Bug_929298___Part_2__Add_add_android_apk_to_moz_build__r_gps.patch

Review of attachment 824218 [details] [diff] [review]:
-----------------------------------------------------------------

::: python/mozbuild/mozbuild/backend/recursivemake.py
@@ +1019,5 @@
>              path = ''
>          backend_file.write('LOCAL_INCLUDES += -I%s%s\n' % (path, generated_include))
>  
> +    def _process_android_apk_data(self, apk, backend_file):
> +        backend_file.write('ANDROID_APK_NAME := %s\n' % apk.name)

Hrm, don't we have some module to write make syntax?

Comment 8

[Nick Alexander :nalexander [he/him]]

Sigh.  When can syncing dirs die?

http://tbpl.mozilla.org/?tree=Try&rev=e465b1cbd0a2

Comment 9

[Gregory Szorc [:gps]]

Comment on attachment 824218 [details] [diff] [review]
3-Bug_929298___Part_2__Add_add_android_apk_to_moz_build__r_gps.patch

Review of attachment 824218 [details] [diff] [review]:
-----------------------------------------------------------------

::: python/mozbuild/mozbuild/frontend/data.py
@@ +456,5 @@
> +          include on the javac compiler bootclasspath
> +        * extra_jars - list of JAR file dependencies to include on the
> +          javac compiler classpath
> +        * javac_flags - string containing extra flags passed to the
> +          javac compiler

Should this be a list instead of a string?

::: python/mozbuild/mozbuild/frontend/reader.py
@@ +235,5 @@
> +        if '/' in name or '\\' in name or '.apk' in name:
> +            raise Exception('Android APK names must not include slashes or'
> +                ' .apk: %s' % name)
> +
> +        if self['ANDROID_APKS'].has_key(name):

if name in self['ANDROID_APKS']: ?

@@ +238,5 @@
> +
> +        if self['ANDROID_APKS'].has_key(name):
> +            raise Exception('Android APK has already been registered: %s' % name)
> +
> +        if len(self['ANDROID_APKS'].keys()) > 0:

You don't need the len() > 0 since len(empty) is falsey.

Comment 10

[Gregory Szorc [:gps]]

Comment on attachment 824219 [details] [diff] [review]
4-Bug_929298___Part_3__Use_add_android_apk_in_easy_cases__r_gps.patch

Review of attachment 824219 [details] [diff] [review]:
-----------------------------------------------------------------

I /think/ the make rules limit us to one APK per directory, correct? Should we enforce that anywhere or do you just want to rely on badness not getting past review? I'm fine with letting it slide.

::: build/mobile/sutagent/android/moz.build
@@ +20,5 @@
> +apk.res = 'res'
> +apk.extra_jars += [
> +    SRCDIR + '/network-libs/commons-net-2.0.jar',
> +    SRCDIR + '/network-libs/jmdns.jar',
> +]

Perhaps extra_jars should just assume srcdir for all relative paths and topsrcdir for all absolute paths? Kinda like how LOCAL_INCLUDES works.

::: config/makefiles/java-build.mk
@@ +81,5 @@
>  
>  $(ANDROID_APK_NAME).apk: $(ANDROID_APK_NAME)-unaligned.apk
>  	$(ZIPALIGN) -f -v 4 $< $@
>  
> +libs:: $(ANDROID_APK_NAME).apk

You changed this from tools to libs. Is this intentional?

::: mobile/android/tests/background/junit3/moz.build
@@ +24,5 @@
> +    'gecko-mozglue.jar',
> +    'gecko-util.jar',
> +    'sync-thirdparty.jar',
> +    'websockets.jar',
> +] ]

Nit: Cuddle braces in Python.

Comment 11

[Mike Hommey [:glandium]]

(In reply to Gregory Szorc [:gps] from comment #10)
> > +libs:: $(ANDROID_APK_NAME).apk
> 
> You changed this from tools to libs. Is this intentional?

Moreover, the corresponding value given in sandbox_symbols.py is 'binaries', which is wrong in both cases.

Comment 12

[Nick Alexander :nalexander [he/him]]

http://tbpl.mozilla.org/?tree=Try&rev=be3cbfd3f5df

Comment 13

[Mike Hommey [:glandium]]

Nick, when do you plan to fix this up and land?

Comment 14

[Nick Alexander :nalexander [he/him]]

(In reply to Mike Hommey [:glandium] from comment #13)
> Nick, when do you plan to fix this up and land?

I'm rebasing as we speak and about to push a try build.  It's very late here so if it's green I'll land tomorrow first thing PST.

Comment 15

[Nick Alexander :nalexander [he/him]]

(In reply to Nick Alexander :nalexander from comment #14)
> (In reply to Mike Hommey [:glandium] from comment #13)
> > Nick, when do you plan to fix this up and land?
> 
> I'm rebasing as we speak and about to push a try build.  It's very late here
> so if it's green I'll land tomorrow first thing PST.

Sorry, wrong bug.  This is going to be a few days; hopefully before I PTO starting next week.  There's lots of additional stuff that I have partially done in my local tree.

Comment 16

[Mike Hommey [:glandium]]

Is this still relevant?

Comment 17

[Nick Alexander :nalexander [he/him]]

(In reply to Mike Hommey [:glandium] from comment #16)
> Is this still relevant?

I imagine the patches have rotted completely, but moving all Android stuff to moz.build is still A Good Thing.

Comment 18

[Nick Alexander :nalexander [he/him]]

We moved to Gradle long ago.