567945 - Android Agent needs to build in our Makesystem

Status: RESOLVED FIXED

(Testing :: General, defect)

Description

[cmtalbert]

Attachment: Makefile changes for android agent.

The Android agent needs to build in our makesystem so that it is signed with the same key that the Android builds are signed with.  This will enable the agent and fennec to be able to read each others files etc.

Note that to build this, you'll need to have an android build environment set up and you'll need to have pulled the android2 branch from Vlad's repo: http://hg.mozilla.org/users/vladimir_mozilla.com/mozilla-droid.

Attaching two patches - first one is the makefile changes for this support. (needs review)
Second patch is the entire android sutagent codebase.

Comment 1

[cmtalbert]

Well, the sutagent codebase is slightly more than 2Mb.  You can get to a diff for it here: http://people.mozilla.org/~ctalbert/android/sutagentcode.diff

Comment 2

[Joel Maher ( :jmaher ) (UTC -8)]

not sure if this would be of use, but we have /build/mobile/ directory: http://mxr.mozilla.org/mozilla-central/source/build/mobile/.

That is where devicemanager.py and the c++ proxy tests that blassey worked on live.  Would it make sense to put this there instead of testing/sutagent?

Comment 3

[cmtalbert]

(In reply to comment #2)
> not sure if this would be of use, but we have /build/mobile/ directory:
> http://mxr.mozilla.org/mozilla-central/source/build/mobile/.
> 
> That is where devicemanager.py and the c++ proxy tests that blassey worked on
> live.  Would it make sense to put this there instead of testing/sutagent?
That'd be fine.  It doesn't really matter much to me.  Ted, thoughts?  Also, Ted, ping for review.   Gracias.

Comment 4

[(not currently active) Ted Mielczarek]

Comment on attachment 447264 [details] [diff] [review]
Makefile changes for android agent.

>diff --git a/Makefile.in b/Makefile.in
>--- a/Makefile.in
>+++ b/Makefile.in
>@@ -78,7 +78,9 @@
> 
> # test harnesses
> ifdef ENABLE_TESTS
>-tier_testharness_dirs += testing/xpcshell
>+tier_testharness_dirs += testing/xpcshell \
>+												 testing/sutagent/android \
>+												 $(NULL)
> endif

jhammel's patch in bug 516984 moves this whole block to toolkit/toolkit-tiers.mk (alongside testing/mochitest), although I think Joel is right and you should just put this in build/mobile anyway.

>diff --git a/testing/sutagent/android/Makefile.in b/testing/sutagent/android/Makefile.in
>new file mode 100644
>--- /dev/null
>+++ b/testing/sutagent/android/Makefile.in
>+JAVA=java
>+JAVAC=javac

I had mwu change this in his patch on bug 564327, so please follow his lead there.

>+
>+DX=$(ANDROID_SDK)/tools/dx
>+AAPT=$(ANDROID_SDK)/tools/aapt
>+APKBUILDER=$(ANDROID_SDK)/../../tools/apkbuilder
>+ZIPALIGN=$(ANDROID_SDK)/../../tools/zipalign
>+
>+JAVAFILES = \
>+	AlertLooperThread.java \
>+	ASMozStub.java \
>+	CmdWorkerThread.java \
>+	DataWorkerThread.java \
>+	DoAlert.java \
>+	DoCommand.java \
>+	Power.java \
>+	RedirOutputThread.java \
>+	RunCmdThread.java \
>+	RunDataThread.java \
>+	SUTAgentAndroid.java \
>+	SUTStartupIntentReceiver.java \
>+	WifiConfiguration.java \
>+	R.java \
>+	$(NULL)

Please use a two-space indent for line continuation in makefiles.

>+# rules.mk has some java stuff, but we're going to ignore it
>+JAVAC_FLAGS = \

I didn't re-read mwu's entire patch, but if this stuff is still there then please move it to a common .mk file and include it in both Makefiles instead of duplicating it.

Comment 5

[cmtalbert]

(In reply to comment #4)

> Please use a two-space indent for line continuation in makefiles.
> 
> >+# rules.mk has some java stuff, but we're going to ignore it
> >+JAVAC_FLAGS = \
> 
> I didn't re-read mwu's entire patch, but if this stuff is still there then
> please move it to a common .mk file and include it in both Makefiles instead of
> duplicating it.

Hey Ted, thanks for the review.  I've got everything done except this bit. I ran into a problem with the crystax ndk (it's needed for --enable-tests on android) when trying to build against mozilla central.  I'm running a build now against mozilla-central without the ndk to verify that the make changes themselves will work, and if so, I should have a new patch here.

One more question, in build/Makefile.in I'm tying this in right after this line:
http://mxr.mozilla.org/mozilla-central/source/build/Makefile.in#72
with the following code:
ifdef ANDROID
  DIRS += mobile/sutagent/android
endif

I originally was going to put this inside the large ifdef ENABLE_TESTS block at line 103 of that file, but each time I did that, it would fail to generate the directory tree for mobile/sutagent/android and would then fail to call my makefile for the agent.  

Is that expected?  In the short term, the agent-tie-in will have to live after the PGO line because we can't build with --enable-tests on mozilla central (for android).  But in the long term, it feels like it should live inside the ifdef ENABLE_TESTS block, but I don't understand why it doesn't allow me to change the DIRS attribute at that point in the makefile.  Am I missing something?

Comment 6

[(not currently active) Ted Mielczarek]

You're not supposed to change DIRS after you include rules.mk. Also, I think we got rid of the ANDROID makefile variable, didn't we? You want to use ifeq (Android,$(OS_TARGET))

Comment 7

[cmtalbert]

Attachment: Addresses comments

This patch addresses your comments.  I looked through the rules.mk Java stuff but it doesn't really set the things we need for Android.  So, I made an config/android-common.mk.  All the variables I could feasibly see us overwriting I made overwritable.  That said, I'm no Java whiz, so make sure it's right.  The one difference between the agent Makefile and the fennec Makefile was the classpath setting so I require that to be set prior to including the file.

Let me know what you think.  This builds atop m-c using the patch queue here: https://wiki.mozilla.org/Mobile/Fennec/Android/PatchQueue.  You also need the agent code which I've uploaded to: http://people.mozilla.org/~ctalbert/android/agentcode.diff

Comment 8

[(not currently active) Ted Mielczarek]

Comment on attachment 450808 [details] [diff] [review]
Addresses comments

>diff --git a/build/mobile/sutagent/android/Makefile.in b/build/mobile/sutagent/android/Makefile.in
>new file mode 100644
>--- /dev/null
>+++ b/build/mobile/sutagent/android/Makefile.in

>+DX=$(ANDROID_SDK)/tools/dx
>+AAPT=$(ANDROID_SDK)/tools/aapt
>+APKBUILDER=$(ANDROID_SDK)/../../tools/apkbuilder
>+ZIPALIGN=$(ANDROID_SDK)/../../tools/zipalign

This stuff can all go in android-common.mk.

>+ifdef JARSIGNER
>+    APKBUILDER_FLAGS += -u
>+endif

This too.

>+JAVA_CLASSPATH = $(ANDROID_SDK)/android.jar:$(srcdir)/network-libs/commons-net-2.0.jar

>+tools:: sutAgentAndroid.apk
>+
>+# Note that we're going to set up a dependency directly between embed_android.dex and the java files
>+# Instead of on the .class files, since more than one .class file might be produced per .java file
>+classes.dex: $(JAVAFILES)
>+	$(NSINSTALL) -D classes
>+	$(JAVAC) $(JAVAC_FLAGS) -d classes  $(addprefix $(srcdir)/,$(JAVAFILES))
+	$(DX) --dex --output=$@ classes

Feels like we could probably factor some of these build rules out into android-common.mk as well, but I'm not going to make you go crazy if you don't want to. If I see a third makefile creep into the tree with these same rules copy/pasted, though, I'm going to make someone fix it.

>diff --git a/config/android-common.mk b/config/android-common.mk
>new file mode 100644
>--- /dev/null
>+++ b/config/android-common.mk
>@@ -0,0 +1,65 @@
>+# ***** BEGIN LICENSE BLOCK *****
>+# Version: MPL 1.1/GPL 2.0/LGPL 2.1
>+#
>+# The contents of this file are subject to the Mozilla Public License Version
>+# 1.1 (the "License"); you may not use this file except in compliance with
>+# the License. You may obtain a copy of the License at
>+# http://www.mozilla.org/MPL/
>+#
>+# Software distributed under the License is distributed on an "AS IS" basis,
>+# WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
>+# for the specific language governing rights and limitations under the
>+# License.
>+#
>+# The Original Code is Android Java Make Settings.
>+#
>+# The Initial Developer of the Original Code is
>+# Mozilla.org.

I think this is supposed to be "The Mozilla Foundation".

>+# Ensure JAVA_CLASSPATH and ANDROID_SDK are defined before including this file.
>+# We use common android defaults for boot class path and java version.
>+ifndef ANDROID_SDK
>+  $(error ANDROID_SDK must be defined before including android-common.mk)
>+endif
>+
>+ifndef JAVA_CLASSPATH
>+  $(error JAVA_CLASSPATH must be defined before including android-common.mk)
>+endif

You could also write something like:
ANDROID_SDK ?= $(error ...)

which would have the effect that if ANDROID_SDK is unset, the first time it's referenced in a rule make will error. Your way will error immediately upon including the makefile though, which may be what you want.

>+JAVAC_FLAGS = \
>+	-target $(JAVA_VERSION) \
>+	-classpath $(JAVA_CLASSPATH) \
>+	-bootclasspath $(JAVA_BOOTCLASSPATH) \
>+	-encoding ascii \
>+	-g \
>+	$(NULL)

Two-space indent, not tabs.

Thanks for factoring those common bits out.

r=me with those fixes.

Comment 9

[cmtalbert]

Attachment: Final Review Comments

(In reply to comment #8)
> >+classes.dex: $(JAVAFILES)
> >+	$(NSINSTALL) -D classes
> >+	$(JAVAC) $(JAVAC_FLAGS) -d classes  $(addprefix $(srcdir)/,$(JAVAFILES))
> +    $(DX) --dex --output=$@ classes
> 
> Feels like we could probably factor some of these build rules out into
> android-common.mk as well, but I'm not going to make you go crazy if you don't
> want to. If I see a third makefile creep into the tree with these same rules
> copy/pasted, though, I'm going to make someone fix it.
I tried this, but the embedding/android makefile has two interpolated java files that are specified here which are built by other rules and placed in the objdir and this rule just picks them up.  I tried, factoring this out but I couldn't get the dependencies to generate those files properly in a "factored out" version.  If you have an idea on a way to do this, then I'm happy to factor this into android-common.mk in a follow on patch.
> 
> >+# Ensure JAVA_CLASSPATH and ANDROID_SDK are defined before including this file.
> >+# We use common android defaults for boot class path and java version.
> >+ifndef ANDROID_SDK
> >+  $(error ANDROID_SDK must be defined before including android-common.mk)
> >+endif
> >+
> >+ifndef JAVA_CLASSPATH
> >+  $(error JAVA_CLASSPATH must be defined before including android-common.mk)
> >+endif
> 
> You could also write something like:
> ANDROID_SDK ?= $(error ...)
> 
> which would have the effect that if ANDROID_SDK is unset, the first time it's
> referenced in a rule make will error. Your way will error immediately upon
> including the makefile though, which may be what you want.
> 
Actually, that is exactly the behavior I wanted (took a fair bit of testing to figure this out).  I wanted the failure to be as explicit as possible and localized as possible during the make process.  That said, if it is more preferable to do this the way you suggest, I'm happy to change it in a follow-on patch.

I made all the rest of the suggested fixes.  I'll land this as soon as the tree reopens (or tomorrow, whichever happens first).

Comment 10

[cmtalbert]

Pushed Agent code as: changeset f8e71992be6b
Pushed make system changes as changeset da2d8dc257d5

--> FIXED

Comment 11

[cmtalbert]

Attachment: Fix bustage on android

Bustage fix for android.  The problem is that the buildstep that handles the embedding/android build sets the location of the jdk on the path.  The buildstep for the main build of m-c does not set this path string, and therefore the build breaks.  However, no amount of attempting to explicitly setting the path in the mozconfig makes this work.

I've tried:
export JAVA_HOME=<pathtojdk>
export PATH=$JAVA_HOME/bin:$PATH
and it didn't work

I also tried:
export JAVA_HOME=<pathtojdk>
export PATH=<pathtojdk>/bin:$PATH
And it still didn't work.

So, I'm out of ideas and we need to get this to quit burning, so I'll check this in as a bustage fix on Android.  Bear is going to set the path explicitly in his buildstep for the main android build and we'll be good to go tomorrow, when I'll re-enable this code.

Also, I'm no configure expert, but it looks like this code:
http://mxr.mozilla.org/mozilla-central/source/configure.in#6093
should handle this case and ensure that the java stuff is properly set.

However, I think we pass that code because our *BUILD SYSTEM* understands where all the Java bits are.  But the thing that is failing here is the call to the android SDK which is operating its own executable.  It may be that using android-sdk/tools/dx simply requires us to set it on the system path.  (I also wonder if make and dx are using two different shell environments (so, while the path is actually being set properly in make due to my mozconfig changes above, dx doesn't see that because it is running in its own shell environment where that isn't set).  If they are indeed running in two shell environments the only way forward here will be to set the system path variable from buildbot before we call make.

Comment 12

[cmtalbert]

Bear added the java path to the system path in buildbot for the main compile step on android.  I've now re-enabled this patch, and we'll see how it goes, but I think it will pass this time.

Changeset: 17fcd8fa7e2f