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)
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 27
People
(Reporter: nalexander, Assigned: nalexander)
References
Details
Attachments
(1 file, 2 obsolete files)
(deleted),
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).
Assignee | ||
Comment 1•11 years ago
|
||
Assignee | ||
Comment 2•11 years ago
|
||
Assignee | ||
Comment 3•11 years ago
|
||
This only handles the simple cases: no mobile/android/base (yet).
Assignee | ||
Comment 4•11 years ago
|
||
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)
Assignee | ||
Updated•11 years ago
|
Attachment #808680 -
Flags: review?(mh+mozilla)
Comment 5•11 years ago
|
||
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 6•11 years ago
|
||
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)
Assignee | ||
Comment 7•11 years ago
|
||
(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.
Assignee | ||
Comment 8•11 years ago
|
||
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)
Assignee | ||
Comment 9•11 years ago
|
||
(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•11 years ago
|
||
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+
Assignee | ||
Comment 11•11 years ago
|
||
Assignee: nobody → nalexander
Status: NEW → ASSIGNED
Assignee | ||
Updated•11 years ago
|
Target Milestone: --- → Firefox 27
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•