919563 - Factor APK generation out of Makefile.in files

Status: RESOLVED FIXED

(Android Background Services Graveyard :: Build & Test, defect)

Description

[Nick Alexander :nalexander [he/him]]

This is a blocker for Bug 903534.  We currently have similar (but subtly different) rules for generating debug APKs throughout build/mobile.  This ticket tracks extracting that logic into rules.mk (or possibly java-build.mk).

Comment 1

[Nick Alexander :nalexander [he/him]]

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

Comment 2

[Nick Alexander :nalexander [he/him]]

Attachment: Part 1: Always generate R.java. r=glandium

Comment 3

[Nick Alexander :nalexander [he/him]]

Attachment: Part 2: Standardize APK generation. r=glandium

This only handles the simple cases: no mobile/android/base (yet).

Comment 4

[Nick Alexander :nalexander [he/him]]

Comment on attachment 808679 [details] [diff] [review]
Part 1: Always generate R.java. r=glandium

This standardizes part of how we build these smaller packages.

Comment 5

[Mike Hommey [:glandium]]

Comment on attachment 808679 [details] [diff] [review]
Part 1: Always generate R.java. r=glandium

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

While you're here, you might as well generate the .ap_ files at the same time as R.java. I had done this locally a while ago with something like:

R.java gecko.ap_: aapt_package
aapt_package: $(ANDROID_PACKAGE_RESOURCES) res/values/strings.xml AndroidManifest.xml
       $(AAPT) package -f -M AndroidManifest.xml -I $(ANDROID_SDK)/android.jar -S res -J . --custom-package org.mozilla.gecko -F gecko.ap_

.PHONY: aapt_package

::: build/mobile/robocop/Makefile.in
@@ -71,5 @@
>    $(wildcard $(TESTPATH)/*.xml) \
>    $(NULL)
>  
>  GARBAGE += \
> -  AndroidManifest.xml \

AndroidManifest.xml should stay here.

@@ -93,5 @@
>  
>  # Override rules.mk java flags with the android specific ones
>  include $(topsrcdir)/config/android-common.mk
>  
> -GENERATED_DIRS_tools = classes $(dir-tests)

This most probably needs to stay.

Comment 6

[Mike Hommey [:glandium]]

Comment on attachment 808680 [details] [diff] [review]
Part 2: Standardize APK generation. r=glandium

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

I'll review after part 1 is updated :)

Comment 7

[Nick Alexander :nalexander [he/him]]

(In reply to Mike Hommey [:glandium] from comment #5)
> Comment on attachment 808679 [details] [diff] [review]
> Part 1: Always generate R.java. r=glandium
> 
> Review of attachment 808679 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> While you're here, you might as well generate the .ap_ files at the same
> time as R.java. I had done this locally a while ago with something like:

I think Part 2 hits this.

> R.java gecko.ap_: aapt_package
> aapt_package: $(ANDROID_PACKAGE_RESOURCES) res/values/strings.xml
> AndroidManifest.xml
>        $(AAPT) package -f -M AndroidManifest.xml -I
> $(ANDROID_SDK)/android.jar -S res -J . --custom-package org.mozilla.gecko -F
> gecko.ap_
> 
> .PHONY: aapt_package
> 
> ::: build/mobile/robocop/Makefile.in
> @@ -71,5 @@
> >    $(wildcard $(TESTPATH)/*.xml) \
> >    $(NULL)
> >  
> >  GARBAGE += \
> > -  AndroidManifest.xml \
> 
> AndroidManifest.xml should stay here.

Again, Part 2.  Sorry this patch was not minimal, but I got tired of fixing Makefile's only to fix them for real.

> @@ -93,5 @@
> >  
> >  # Override rules.mk java flags with the android specific ones
> >  include $(topsrcdir)/config/android-common.mk
> >  
> > -GENERATED_DIRS_tools = classes $(dir-tests)
> 
> This most probably needs to stay.

I see nothing that suggest that GENERATED_DIRS_* does *anything*.  GENERATED_DIRS does (and I use it in Part 2).  It's possible that I removed $(dir-tests) by accident.

Comment 8

[Nick Alexander :nalexander [he/him]]

Attachment: Standardize APK generation. r=glandium

This only handles the simple cases: no mobile/android/base (yet).

This version folds the previous two, adds back the GENERATED_DIRS and
GARBAGE removed from robocop/Makefile.in, and only invokes aapt once
(with non-awesome but no-worse-than-before deps on .aapt.deps).

Comment 9

[Nick Alexander :nalexander [he/him]]

(In reply to Nick Alexander :nalexander from comment #8)
> Created attachment 809428 [details] [diff] [review]
> Standardize APK generation. r=glandium
> 
> This only handles the simple cases: no mobile/android/base (yet).
> 
> This version folds the previous two, adds back the GENERATED_DIRS and
> GARBAGE removed from robocop/Makefile.in, and only invokes aapt once
> (with non-awesome but no-worse-than-before deps on .aapt.deps).

ETA on this, glandium?  It's blocking Bug 903534.

Comment 10

[Mike Hommey [:glandium]]

Comment on attachment 809428 [details] [diff] [review]
Standardize APK generation. r=glandium

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

::: build/mobile/robocop/Makefile.in
@@ +78,5 @@
>    $(robocop-deps) \
>    $(NULL)
>  
> +JAVAFILES += \
> +	$(robocop-deps) \

fix indentation.

::: js/src/config/makefiles/java-build.mk
@@ +39,5 @@
> +ifdef ANDROID_APK_NAME
> +ifdef ANDROID_RES_DIR
> +_ANDROID_RES_FLAG := -S $(ANDROID_RES_DIR)
> +else
> +_ANDROID_RES_FLAG := -S res

_ANDROID_RES_FLAG := -S $(or $(ANDROID_RES_DIR),res)

@@ +40,5 @@
> +ifdef ANDROID_RES_DIR
> +_ANDROID_RES_FLAG := -S $(ANDROID_RES_DIR)
> +else
> +_ANDROID_RES_FLAG := -S res
> +endif #{ ANDROID_RES_DIR

#} ; there should be a #{ near the ifdef, if you put those.

@@ +44,5 @@
> +endif #{ ANDROID_RES_DIR
> +
> +ifdef ANDROID_ASSETS_DIR
> +_ANDROID_ASSETS_FLAG := -A $(ANDROID_ASSETS_DIR)
> +endif #{ ANDROID_ASSETS_DIR

likewise, although you could just do:
_ANDROID_ASSERTS_FLAG := $(addprefix -A ,$(ANDROID_ASSETS_DIR)
without an ifdef (addprefix returns nothing if the second argument is empty)

@@ +80,5 @@
> +	cp $< $@
> +	$(DEBUG_JARSIGNER) $@
> +
> +$(ANDROID_APK_NAME).apk: $(ANDROID_APK_NAME)-unaligned.apk
> +	$(ZIPALIGN) -f -v 4 $< $@

Note, for a followup, if we don't need to keep all those intermediate files, we may want to merge the rules and remove them.

@@ +93,5 @@
> +  $(NULL)
> +
> +JAVA_CLASSPATH := $(ANDROID_SDK)/android.jar
> +ifdef ANDROID_EXTRA_JARS
> +JAVA_CLASSPATH := $(JAVA_CLASSPATH):$(subst $(eval) ,:,$(strip $(ANDROID_EXTRA_JARS)))

replace $(eval) with $(NULL)

@@ +94,5 @@
> +
> +JAVA_CLASSPATH := $(ANDROID_SDK)/android.jar
> +ifdef ANDROID_EXTRA_JARS
> +JAVA_CLASSPATH := $(JAVA_CLASSPATH):$(subst $(eval) ,:,$(strip $(ANDROID_EXTRA_JARS)))
> +endif #{ ANDROID_EXTRA_JARS

#}

Comment 11

[Nick Alexander :nalexander [he/him]]

https://hg.mozilla.org/integration/fx-team/rev/a1e6919bff63

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/a1e6919bff63