Closed Bug 919563 Opened 11 years ago Closed 11 years ago

Factor APK generation out of Makefile.in files

Categories

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

Product:
Android Background Services Graveyard
Component:
Build & Test
Platform:
All
Android
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
Firefox 27

People

(Reporter: nalexander, Assigned: nalexander)

References

Details

Attachments

(1 file, 2 obsolete files)

Standardize APK generation. r=glandium
21.09 KB, patch
glandium
: review+
Details | Diff | Splinter Review 
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).
This only handles the simple cases: no mobile/android/base (yet).
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.
Attachment #808679 - Flags: review?(mh+mozilla)
Attachment #808680 - Flags: review?(mh+mozilla)
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.
Attachment #808679 - Flags: review?(mh+mozilla) → feedback+
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 :)
Attachment #808680 - Flags: review?(mh+mozilla)
(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.
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).
Attachment #808679 - Attachment is obsolete: true
Attachment #808680 - Attachment is obsolete: true
Attachment #809428 - Flags: review?(mh+mozilla)
(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 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

#}
Attachment #809428 - Flags: review?(mh+mozilla) → review+
https://hg.mozilla.org/integration/fx-team/rev/a1e6919bff63
Assignee: nobody → nalexander
Status: NEW → ASSIGNED
Target Milestone: --- → Firefox 27
https://hg.mozilla.org/mozilla-central/rev/a1e6919bff63
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Blocks: 929298
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: