Closed Bug 874132 Opened 11 years ago Closed 11 years ago

Build fails using Android SDK r22

Categories

(Firefox for Android Graveyard :: General, defect)

Product:
Firefox for Android Graveyard
Component:
General
Platform:
All
Android
Type:
defect
Priority:
Not set
Severity:
major

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
Firefox 24

People

(Reporter: mcomella, Assigned: nalexander)

References

Details

Attachments

(4 files, 16 obsolete files)

handle sdk layout differences in the Android r22 SDK
5.26 KB, patch
gps
: review+
nalexander
: review+
Details | Diff | Splinter Review
Part 2: Add custom debug signing tool. r=jmaher
5.06 KB, patch
jmaher
: review+
gps
: feedback+
Details | Diff | Splinter Review
Part 3: Replace uses of apkbuilder with zip and custom debug signing tool. r=jmaher
17.55 KB, patch
jmaher
: review+
Details | Diff | Splinter Review
Bug 874132 - Follow-up: Rename debug key; checking if alias exists; set $(AIDL). r=jmaher
8.40 KB, patch
mfinkle
: review+
rillian
: feedback+
Details | Diff | Splinter Review 
Building Firefox for Android with Android SDK r22 fails. So far I've found the following reasons:
  * aapt and dx were moved to <android-sdk>/build-tools/17.0.0/
  * apkbuilder appears to have been removed

A workaround for the former is as follows:
  cd <android-sdk>/platform-tools
  ln -s ../build-tools/17.0.0/aapt aapt
  ln -s ../build-tools/17.0.0/dx dx

I've been unable to workaround the latter to see if there are more issues.
The paths to dx, aapt, and apkbuilder appear to be set within ./config/android-common.mk (https://mxr.mozilla.org/mozilla-central/source/config/android-common.mk#15).

The DX and AAPT environment variables can (probably) be adjusted here.

I believe all references to APKBUILDER would need to be replaced with an equivalent process. Running apkbuilder on a previous SDK revision gives the following output:

>THIS TOOL IS DEPRECATED and may stop working at any time!
>
>If you wish to use apkbuilder for a custom build system, please look at the
>com.android.sdklib.build.ApkBuilder which provides support for
>recent build improvements including library projects.

However, googleing for this library does not come up with much so I wonder if that hasn't been deprecated as well.

Some APKBUILDER references from mxr:
https://mxr.mozilla.org/mozilla-central/search?string=apkbuilder&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central
Severity: normal → major
Assignee: nobody → stully
Sorry, I should have filed when I had this conversation with mshal in #mobile.  I think that working around apkbuilder should not be a big deal -- it's basically zip.  I am happy to mentor or review.
The short term workaround is to download http://dl-ssl.google.com/android/repository/tools_r21-linux.zip or http://dl-ssl.google.com/android/repository/tools_r21-macosx.zip for the 
apkbuilder and make the symlinks as suggested in comment 0. I've built with this setup.
>The short term workaround is to download ... for the apkbuilder

It seems that using tools/android to update "Android SDK Tools" will remove apkbuilder. So this workaround will work only if the "Android SDK Tools" are not updated. I updated the Wiki accordingly: https://wiki.mozilla.org/Mobile/Fennec/Android#Install_Android_SDK

An alternative to not updating the tools is to download both an old SDK and r22 and symlink the apkbuilder from the old one into r22 (which should be specified to be used for everything else) - this should be easier for developers who have already set up the build environment.
Attached patch 01: WIP - apkbuilder (obsolete) — — Splinter Review 
(Note: I symlinked dx and aapt for the time being as per comment 0)

In the attached patch, I replaced apkbuilder with zip, where I get the following issue when trying to install:

> Failure [INSTALL_PARSE_FAILED_NO_CERTIFICATES]

Googleing led me to http://stackoverflow.com/a/10083280/2219998 I currently have Java 7 installed so I tried signing by hand:

> jarsigner -sigalg MD5withRSA -digestalg SHA1 obj-android/dist/fennec-24.0a1.en-US.android-arm.apk -keystore ~/.android/debug.keystore androiddebugkey -storepass android

> python2 ./mach install
> Failure [INSTALL_FAILED_DEXOPT]

A slightly different error. Logcat output:

>I/PackageManager( 5229): Running dexopt on: org.mozilla.fennec_mcomella         
>W/dalvikvm(22239): DexOptZ: zip archive '/data/app/org.mozilla.fennec_mcomella-1.apk' does not include classes.dex
>W/installd(  703): DexInv: --- END '/data/app/org.mozilla.fennec_mcomella-1.apk' --- status=0xff00, process failed
>E/installd(  703): dexopt failed on '/data/dalvik-cache/data@app@org.mozilla.fennec_mcomella-1.apk@classes.dex' res = 65280
>W/PackageManager( 5229): Package couldn't be installed in /data/app/org.mozilla.fennec_mcomella-1.apk

I could be packaging via zip incorrectly, but I couldn't spot any errors.

I also tried using Java 6 (installing side-by-side with 7) on a partial build (./mach build) which did not work, but I'm not sure if the build used the appropriate paths.

My next approach would be to find where the final org.mozilla.fennec_*.apk package is built and see if I missed a classes.dex step there.
Attached patch xx: TMP - explicit jarsigner (obsolete) — — Splinter Review 
To workaround manually signing the apk as mentioned in comment 5, I attempted to replace all references of "$(JARSIGNER)" with the explicit form, however, I get the cert error again. Notably, the final apk does not appear to be built using jarsigner.
MOZ_SIGN_CMD conditionally replaces JARSIGNER at one point (https://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/installer/packager.mk#250). Perhaps this can be related to comment 6?
> I also tried using Java 6 (installing side-by-side with 7) on a partial build (./mach build)
> which did not work, but I'm not sure if the build used the appropriate paths.

I did a clean build using Java 6 and verified that the appropriate paths were used via <objdir>/config.status. This did not fix the certs and manually signing does not fix the dexopt issue.
(In reply to Michael Comella (:mcomella) from comment #8)
> > I also tried using Java 6 (installing side-by-side with 7) on a partial build (./mach build)
> > which did not work, but I'm not sure if the build used the appropriate paths.
> 
> I did a clean build using Java 6 and verified that the appropriate paths
> were used via <objdir>/config.status. This did not fix the certs and
> manually signing does not fix the dexopt issue.

Thanks for investigating this.  The signing issue is due to MOZ_SIGN_CMD; it is defined only on infrastructure.  We had been using |apkbuilder -u| to get an "unsigned" (really, a debug-signed) APK.

I will try your patch and look into the dexopt issue.  Looking at your traceback, the issue is definitely that classes.dex is missing, but perhaps there is more.

Finally: you can test a good number of these things in build/mobile/robocop/Makefile.in, which is much quicker to test and deploy.
(In reply to Nick Alexander :nalexander from comment #9)
> I will try your patch and look into the dexopt issue.  Looking at your
> traceback, the issue is definitely that classes.dex is missing, but perhaps
> there is more.

I don't know why I didn't think to do this sooner...

$ unzip -l obj-android/dist/fennec*.apk | grep classes
    2451456  2013-05-21 10:37   fennec/classes.dex

Perhaps it's at the incorrect path? In any case, I'm not sure if this helps but I'm assuming you've got it from here.
(In reply to Michael Comella (:mcomella) from comment #10)
> (In reply to Nick Alexander :nalexander from comment #9)
> > I will try your patch and look into the dexopt issue.  Looking at your
> > traceback, the issue is definitely that classes.dex is missing, but perhaps
> > there is more.
> 
> I don't know why I didn't think to do this sooner...
> 
> $ unzip -l obj-android/dist/fennec*.apk | grep classes
>     2451456  2013-05-21 10:37   fennec/classes.dex
> 
> Perhaps it's at the incorrect path? In any case, I'm not sure if this helps
> but I'm assuming you've got it from here.

You are correct: classes.dex needs to be at the root of the APK.  Please continue without me -- happy to give feedback but I shouldn't review since I'm not a peer.
Attached patch 01: apkbuilder (obsolete) — — Splinter Review 
I fixed the classes.dex issue - the zip utility maintains the file hierarchy if not given the "-j" flag. Still remaining:

* Not having to sign jars by hand
* Use <sdk>/build-tools/ if it exists, otherwise platform-tools/.
Attachment #752147 - Attachment is obsolete: true
(In reply to Michael Comella (:mcomella) from comment #12)
> Created attachment 752254 [details] [diff] [review]
> 01: apkbuilder
> 
> I fixed the classes.dex issue - the zip utility maintains the file hierarchy
> if not given the "-j" flag. Still remaining:

Huzzah!
 
> * Not having to sign jars by hand

I suggest you look at https://github.com/mozilla-services/android-sync/blob/develop/tools/fennec_repackager.py#L64 for an example of generating an Android debug key and signing with it.

Now I wonder if there is an opportunity to unify our signing across developer machines and buildbots: should we be using releng's signing command for this?  glandium, you might have insight into this, or could you suggest someone who would have an opinion here?

> * Use <sdk>/build-tools/ if it exists, otherwise platform-tools/.
Flags: needinfo?(mh+mozilla)
Attached patch 02: build-tools (obsolete) — — Splinter Review 
This patch to <sdk>/build-tools appears to work provided the directory exists. I'm going to try without the directory by moving it and symlinking aapt and dx into platform-tools, then report back.
> android_build_tools=android_platform_tools # SDK Tools < r22

Should be "android_build_tools="$android_platform_tools" # ..." I'll fix it in the next patch.
Comment on attachment 752312 [details] [diff] [review]
02: build-tools

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

::: build/autoconf/android.m4
@@ +304,5 @@
>      android_platform_tools="$android_sdk"/../../platform-tools
>      if test ! -d "$android_platform_tools" ; then
>          android_platform_tools="$android_sdk"/tools # SDK Tools < r8
>      fi
> +    android_build_tools="$android_sdk"/../../build-tools/17.0.0

Rather than hardcoding the 17.0.0, the following will grab the most recent revision and use it:

android_build_tools="$android_sdk"/../../build-tools
android_build_tools="$android_build_tools"/$(ls $android_build_tools | sort | tail -n 1)
Assignee: stully → michael.l.comella
(In reply to Shane Tully (:stully: from comment #16)
> Rather than hardcoding the 17.0.0, the following will grab the most recent
> revision and use it:

I agree that hardcoding is a generally a bad idea but are you sure they will always put the full set of tools in the latest revision directory? I can imagine a schema where a new tool is introduced (let's call it "andy") and put into a new directory, while leaving the old tools in the old directory. So...

<sdk>/build-tools/23.0.0/andy
<sdk>/build-tools/17.0.0/aapt
<sdk>/build-tools/17.0.0/dx

There is also the possibility where aapt gets updated and removed from the old directory but not dx (or vice versa). I was unable to find any documentation about this unfortunately.

Alternatively, we can make this more complicated ("AAPT=`find <sdk>/build-tools/ -type f -name "aapt" | xargs sort -n | tail -n 1`"), but that seems like a bad idea without knowing the schema.

Side note: With the change I mentioned, the apk built successfully.
This patch works both with and without the <sdk>/build-tools dir, though with a hardcoded <sdk>/build-tools/17.0.0/ that should be figured out.
Attachment #752312 - Attachment is obsolete: true
Without knowing how the directory structure will be in the future, it's hard to make a decision on how to handle the path currently. At least for now though, using the highest revision number is better than hardcoding revision 17.
Attached patch 02b: build-tools (obsolete) — — Splinter Review 
(In reply to Shane Tully (:stully: from comment #19)
> ... At least for now
> though, using the highest revision number is better than hardcoding revision
> 17.

I agree. I made and tested your change.
Attachment #752364 - Attachment is obsolete: true
Attached patch 02c: build-tools (obsolete) — — Splinter Review 
Update: Added quotes around "$android_build_tools".

I also realized that ls-ing a potentially non-existent directory:

> android_build_tools="$android_build_tools"/$(ls $android_build_tools | sort | tail -n 1)

isn't the cleanest approach, but shouldn't cause any problems as "ls: cannot access aoeu: No such file or directory" (hopefully) isn't a directory name. However, is a:

if test -d "$android_build_tools" ; then
  android_build_tools=...

approach preferable?
Attachment #752395 - Attachment is obsolete: true
Attached patch 02d: build-tools (obsolete) — — Splinter Review 
Update (again): Add -n to sort for numerical sort.
Attachment #752402 - Attachment is obsolete: true
(In reply to Michael Comella (:mcomella) from comment #22)
> Created attachment 752409 [details] [diff] [review]
> 02d: build-tools
> 
> Update (again): Add -n to sort for numerical sort.

Exactly because we don't know what's going to happen in the future, I think hardcoding 17.0.0 is better until we see what happens next.
Flags: needinfo?(mh+mozilla)
(In reply to Nick Alexander :nalexander from comment #13)
> Now I wonder if there is an opportunity to unify our signing across
> developer machines and buildbots: should we be using releng's signing
> command for this?  glandium, you might have insight into this, or could you
> suggest someone who would have an opinion here?

I think Ben would know. I'd definitely be in favor, but i don't know what releng scripts are actually doing with that.
Flags: needinfo?(bhearsum)
Comment on attachment 752364 [details] [diff] [review]
02a: build-tools (hardcoded "17.0.0")

(In reply to Mike Hommey [:glandium] from comment #23)
> Exactly because we don't know what's going to happen in the future, I think
> hardcoding 17.0.0 is better until we see what happens next.

I'll leave both patches up until we get a consensus.
Attachment #752364 - Attachment description: 02a: build-tools → 02a: build-tools (hardcoded "17.0.0")
Attachment #752364 - Attachment is obsolete: false
(In reply to Mike Hommey [:glandium] from comment #24)
> (In reply to Nick Alexander :nalexander from comment #13)
> > Now I wonder if there is an opportunity to unify our signing across
> > developer machines and buildbots: should we be using releng's signing
> > command for this?  glandium, you might have insight into this, or could you
> > suggest someone who would have an opinion here?
> 
> I think Ben would know. I'd definitely be in favor, but i don't know what
> releng scripts are actually doing with that.

Just to make sure I understand correctly: the idea here is that developers would sign their own, local Android builds using RelEng's signing servers. And the advantage is that developers don't need to screw around generating their own keys/learning signing tools, etc. -- Is that right?
Flags: needinfo?(bhearsum)
(In reply to Ben Hearsum [:bhearsum] from comment #26)
> (In reply to Mike Hommey [:glandium] from comment #24)
> > (In reply to Nick Alexander :nalexander from comment #13)
> > > Now I wonder if there is an opportunity to unify our signing across
> > > developer machines and buildbots: should we be using releng's signing
> > > command for this?  glandium, you might have insight into this, or could you
> > > suggest someone who would have an opinion here?
> > 
> > I think Ben would know. I'd definitely be in favor, but i don't know what
> > releng scripts are actually doing with that.
> 
> Just to make sure I understand correctly: the idea here is that developers
> would sign their own, local Android builds using RelEng's signing servers.
> And the advantage is that developers don't need to screw around generating
> their own keys/learning signing tools, etc. -- Is that right?

Thanks for clarifying, because I was not clear.

I don't think standing up a RelEng service to debug sign Android packages is a good idea.  It's easy to do offline locally.

I was thinking of something much simpler: developers would have some version of MOZ_SIGN_CMD defined locally, exporting the same API as RelEng's signing command, but one that manages a local Android debug key.  My motivation is that while I was debugging a different issue, I experienced (broke!) an entire layer of RelEng procedure that isn't visible to day-to-day development.  A developer MOZ_SIGN_CMD would have developers signing builds "RelEng style", so there is one less difference between developer boxes and RelEng practices.
Attached patch 03: WIP - Jarsigner (obsolete) — — Splinter Review 
Assuming we don't merge MOZ_SIGN_CMD, here is a patch to sign the debug apks. However, it only works provided ~/.android/debug.keystore exists.

However, in the case that it does not, it needs to be created but I'm not quite sure how to do that outside of a target in make. I attempted hackishly executing the command in an evaluated variable (via http://stackoverflow.com/a/9065225/2219998), however, I could not get that working properly. Any assistance would be appreciated.
Attachment #752152 - Attachment is obsolete: true
Attached patch 03: jarsigner (obsolete) — — Splinter Review 
I got the environment variable hack functionally working and it appears there is already a precedent for it within the file (ex: https://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/installer/packager.mk#137), so I assume it's okay.

Also, if a new key is generated for the user, installing the apk to the device without first uninstalling it will result in the following error:

Failure [INSTALL_PARSE_FAILED_INCONSISTENT_CERTIFICATES]

We should notify the devs about this, probably in the build section on the Wiki.
Attachment #753505 - Attachment is obsolete: true
Comment on attachment 752254 [details] [diff] [review]
01: apkbuilder

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

I'd like to keep $(APKBUILDER) and $(APKBUILDER_FLAGS).  There's value in names that match functionality: least surprise, easier to grep for, easier to selectively override.  I suggest *defining* APKBUILDER to be $(ZIP) with whatever options are necessary.  It is possible RelEng overrides these settings, too.  joey, any other thoughts?
Attachment #752254 - Flags: feedback+
Attachment #752254 - Flags: review?(joey)
Comment on attachment 752409 [details] [diff] [review]
02d: build-tools

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

::: js/src/build/autoconf/android.m4
@@ +304,5 @@
>      android_platform_tools="$android_sdk"/../../platform-tools
>      if test ! -d "$android_platform_tools" ; then
>          android_platform_tools="$android_sdk"/tools # SDK Tools < r8
>      fi
> +    android_build_tools="$android_sdk"/../../build-tools

This really needs some explanation.  Something like:

Bug 874132: the location of dx and aapt changed between r21 and r22.  This tries to find the correct location...
Attachment #752409 - Flags: feedback+
Comment on attachment 752409 [details] [diff] [review]
02d: build-tools

ted: wanted to get a build peer and someone familiar with RelEng infra to look at this.  Please reassign as appropriate.
Attachment #752409 - Flags: review?(ted)
Comment on attachment 753792 [details] [diff] [review]
03: jarsigner

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

::: toolkit/mozapps/installer/packager.mk
@@ +248,5 @@
>  include $(MOZILLA_DIR)/config/android-common.mk
>  
>  ifdef MOZ_SIGN_CMD
>  JARSIGNER := $(MOZ_SIGN_CMD) -f jar
> +JARSIGNER_ALIAS = ""

This is not great.  I understand that you want to keep flexibility, but this is at the wrong level.  I suggest that if MOZ_SIGN_CMD is *not* set you set 

JARSIGNER := $(MOZILLA_DIR)/mobile/android/debug_sign_tool.py

and implement this functionality as a Python script.  That will be easier to test and modify, and much less fragile, than this hardcoding.
Attachment #753792 - Flags: feedback-
(In reply to Nick Alexander :nalexander from comment #32)
> Comment on attachment 752409 [details] [diff] [review]
> 02d: build-tools
> 
> ted: wanted to get a build peer and someone familiar with RelEng infra to
> look at this.  Please reassign as appropriate.

ted: Please also look into whether hard-coding the build-tools path or sorting by version number, which are patch 02a and 02d respectively, is preferable. Brief discussion between comment 16 and comment 23.
Unfortunately, something came up and I won't have time to continue working on this for several weeks, so I'm unassigning it from myself.

stully: You were previously assigned. Would you like to continue working on this?

Note: I was working on a Python script for the jarsigner (03) which was about halfway done but I don't currently have access to the machine I was developing it on. If I get a chance, I will upload it.
Assignee: michael.l.comella → nobody
Flags: needinfo?(stully)
Attached patch 03a: WIP - jarsigner (py) (obsolete) — — Splinter Review 
This code is largely mimicked from sync's fennec repackager (see comment 13), which will also need to be updated to use explicit signing algorithms on both the jarsigner and the keytool commands. To prevent duplication, these methods can be imported from this script (import android_debug_tool; android_debug_tool.create_keystore_if_not_existent, etc.), but I'm not sure this is possible due to the separation of the repos and the path issues this causes. Thus, explicitly adding the changes to the sync script is probably the most efficient.

The script looks mostly complete, sans documentation, but I don't remember testing it very well, particularly edge cases. The output is probably also insufficient.

See comment 33 for required changes to the make files.

I'm leaving patch "03: jarsigner" up for reference but note that this is a replacement for it.
Comment on attachment 752409 [details] [diff] [review]
02d: build-tools

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

This mostly looks fine, I just have one issue.

::: js/src/build/autoconf/android.m4
@@ +304,5 @@
>      android_platform_tools="$android_sdk"/../../platform-tools
>      if test ! -d "$android_platform_tools" ; then
>          android_platform_tools="$android_sdk"/tools # SDK Tools < r8
>      fi
> +    android_build_tools="$android_sdk"/../../build-tools

FWIW, you don't need to put bug numbers in comments like that, unless you are indicating an open TODO. hg blame is for finding bug numbers. A comment to indicate *why* we're doing this is fine (although we don't do that elsewhere, and the comment in the test below gives an indication).

@@ +305,5 @@
>      if test ! -d "$android_platform_tools" ; then
>          android_platform_tools="$android_sdk"/tools # SDK Tools < r8
>      fi
> +    android_build_tools="$android_sdk"/../../build-tools
> +    android_build_tools="$android_build_tools"/$(ls "$android_build_tools" | sort -n | tail -n 1)

I don't understand this, it's weird. What does this offer vs. just doing a "test ! -d" check like we do elsewhere?
Attachment #752409 - Flags: review?(ted) → review-
Assignee: nobody → michael.l.comella
Michael said that it would be several weeks before he would be free in comment 36. stully is the next most likely person to fix this.
Assignee: michael.l.comella → nobody
I started looking at this when I thought it was just a path issue. I'm not sure I know enough about the build system to fix this in a reasonable amount of time.
Flags: needinfo?(stully)
I tripped over this today. Unfortunately the symlink+install apkbuilder doesn't resolve all the issues for me:

/home/giles/data/android/android-sdk-linux_x86/platforms/android-16/../../tools/apkbuilder robocop-debug-signed-unaligned.apk -v  -z robocop.ap_ -f classes.dex
apkbuilder: can't find sdklib.jar

There is a android-sdk-linux_x86/tools/lib/sdklib.jar

Is this a symptom of the java 1.7 issue?
I ran into this yesterday and it seemed worthwhile to try and push this bug
forward.  This patch handles detecting the build tools for r22.  I'm not
entirely sure why the tools are located in build-tools/android-4.2.2 instead
of build-tools/17.0.0 as the bug indicates thus far, but I tried to write the
code so both of those possibilities are handled and we can easily add more
possibilities later.

Asking for build peer review and for somebody who has more experience with
the SDK than I do.
Attachment #752364 - Attachment is obsolete: true
Attachment #752409 - Attachment is obsolete: true
Attachment #760904 - Flags: review?(nalexander)
Attachment #760904 - Flags: review?(gps)
Attached patch replace apkbuilder with zip (obsolete) — — Splinter Review 
This patch just updates the previous apkbuilder patch to a base much closer to
m-c tip.

Again, asking for build peer and mobile review on this one.  With this and the
build-tools configury patches, I can at least build fennec on my Mac with the
unmodified r22 SDK.
Attachment #752254 - Attachment is obsolete: true
Attachment #752254 - Flags: review?(joey)
Attachment #760907 - Flags: review?(nalexander)
Attachment #760907 - Flags: review?(gps)
This patch adds mobile/android/debug_sign_tool.py and instructs packager.mk
to call it if MOZ_SIGN_CMD is not set.
Attachment #753792 - Attachment is obsolete: true
Attachment #755918 - Attachment is obsolete: true
Attachment #761099 - Flags: review?(gps)
I should note that if all these patches can get approved, they'll be landing with Michael's name on them, not mine.  I'm just trying to move the process forward. :)
Comment on attachment 760904 [details] [diff] [review]
handle sdk layout differences in the Android r22 SDK

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

I don't really have my experience with the Android SDK. But if the tools moved, the tools moved.

::: build/autoconf/android.m4
@@ +315,5 @@
> +            break
> +        fi
> +    done
> +    if test -z "$android_build_tools" ; then
> +        android_build_tools="$android_platform_tools" # SDK Tools < r22

Shouldn't we also |test -d $android_build_tools| just to be sure?
Attachment #760904 - Flags: review?(gps) → review+
Comment on attachment 761099 [details] [diff] [review]
add tool for signing developer versions of APKs

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

This is largely a rubber stamp. I see from the comments this code was apparently copied or based off of Android Sync code. I don't like copying code, so let's get a follow-up bug to unify the code, if possible.

::: mobile/android/debug_sign_tool.py
@@ +15,5 @@
> +import sys
> +import subprocess
> +
> +ANDROID_HOME = os.path.expanduser('~/.android/')
> +KEYSTORE_PATH = ANDROID_HOME + 'debug.keystore'

This seems like the kind of thing that should be configurable. I think it should be overwritable via environment variable and/or command argument.

::: toolkit/mozapps/installer/packager.mk
@@ +249,5 @@
>  
>  ifdef MOZ_SIGN_CMD
>  JARSIGNER := $(MOZ_SIGN_CMD) -f jar
>  else
> +JARSIGNER ?= $(PYTHON) $(call core_abspath,$(topsrcdir)/mobile/android/debug_sign_tool.py)

You probably don't need the $(call core_abspath).
Attachment #761099 - Flags: review?(gps) → review+
Comment on attachment 760907 [details] [diff] [review]
replace apkbuilder with zip

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

This looks sane. I didn't fully verify that replacing apk builder with zip + signing is fully proper. But, I trust someone knows what he or she is doing. Furthermore, I suspect that if we regress something it will be immediately obvious.
Attachment #760907 - Flags: review?(gps) → review+
Would it make sense to get a Releng Linux build image for Android and upgrade it to Android SDK r22 to make sure that we don't break builds when RelEng does upgrade?
Flags: needinfo?(aki)
That might make sense.
If an SDK r22 upgrade is imminent, one thing we've done is

a) roll out the new SDK/NDK to the build slaves, in parallel with the existing one
b) push to try, pointing at the new SDK/NDK rather than the old one

That might not make a lot of sense if we're not planning on upgrading to SDK r22 soon, though.
Flags: needinfo?(aki)
Attachment #760904 - Flags: review?(nalexander) → review+
Comment on attachment 760907 [details] [diff] [review]
replace apkbuilder with zip

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

We'll need to sign all the APKs built outside of |packager.mk| using the Python tool as well.  That means landing all 3 patches as one commit.

::: build/mobile/robocop/Makefile.in
@@ +111,5 @@
>  robocop.ap_: AndroidManifest.xml $(TESTPATH)/assets/*
>  	$(AAPT) package -f -M $< -I $(ANDROID_SDK)/android.jar -I . -S res -A $(TESTPATH)/assets -F $@ -J ./
>  
>  robocop-debug-signed-unaligned.apk: robocop.ap_ classes.dex
> +	$(ZIP) -vj0 $@ robocop.ap_ classes.dex

We also need to sign here (and in the other replacements).
Attachment #760907 - Flags: review?(nalexander) → review-
(In reply to Nick Alexander :nalexander from comment #50)
> ::: build/mobile/robocop/Makefile.in
> @@ +111,5 @@
> >  robocop.ap_: AndroidManifest.xml $(TESTPATH)/assets/*
> >  	$(AAPT) package -f -M $< -I $(ANDROID_SDK)/android.jar -I . -S res -A $(TESTPATH)/assets -F $@ -J ./
> >  
> >  robocop-debug-signed-unaligned.apk: robocop.ap_ classes.dex
> > +	$(ZIP) -vj0 $@ robocop.ap_ classes.dex
> 
> We also need to sign here (and in the other replacements).

For my own edification: why do we need to do this?  Did apkbuilder take care of signing with debug keys before?

Also, if we need to debug-sign when using zip now, we'd want to make sure that we're always using the debug signer and not JARSIGNER (which would sign with Mozilla's keys in RelEng builds), correct?
Flags: needinfo?(nalexander)
(In reply to Nathan Froyd (:froydnj) from comment #51)
> (In reply to Nick Alexander :nalexander from comment #50)
> > ::: build/mobile/robocop/Makefile.in
> > @@ +111,5 @@
> > >  robocop.ap_: AndroidManifest.xml $(TESTPATH)/assets/*
> > >  	$(AAPT) package -f -M $< -I $(ANDROID_SDK)/android.jar -I . -S res -A $(TESTPATH)/assets -F $@ -J ./
> > >  
> > >  robocop-debug-signed-unaligned.apk: robocop.ap_ classes.dex
> > > +	$(ZIP) -vj0 $@ robocop.ap_ classes.dex
> > 
> > We also need to sign here (and in the other replacements).
> 
> For my own edification: why do we need to do this?  Did apkbuilder take care
> of signing with debug keys before?

Yes, |apkbuilder -u| did an "unsigned" -- really "debug signed" -- packaging.

> Also, if we need to debug-sign when using zip now, we'd want to make sure
> that we're always using the debug signer and not JARSIGNER (which would sign
> with Mozilla's keys in RelEng builds), correct?

I think we should be defining JARSIGNER to be our debug signer throughout, which is then overridden in |packager.mk| if MOZ_SIGN_CMD is defined (and left alone if not).  All the APKs save Robocop should work with this configuration.
Flags: needinfo?(nalexander)
> All the APKs save Robocop should work with this configuration.

This was unclear: I mean, the existing Makefile's should already work with this configuration, and the Robocop Makefile can be trivially updated.
Squashing the s/apkbuilder/zip/g and debug-sign-tool.py patches into one and
addressing Nick's comments.  AFAICS, the robocop makefile was the only one
that needed signing after zip.  All the other makefiles use zip to produce an
explicitly-named $APK-unsigned-unaligned.apk and then have further rules to
do the signing using JARSIGNER, so I think everything works as intended.

Asking Nick for re-review to verify.

I kept the core_abspath call because there are some build steps that fail
without it; I can track down the particular build directories if anybody's
really curious.
Attachment #760907 - Attachment is obsolete: true
Attachment #761099 - Attachment is obsolete: true
Attachment #762664 - Flags: review?(nalexander)
After thinking about it just a tiny bit more, let's try this patch.  I think
it will define JARSIGNER in many more places that expect it to be defined now.

Re-requesting build peer review.
Attachment #762664 - Attachment is obsolete: true
Attachment #762664 - Flags: review?(nalexander)
Attachment #762668 - Flags: review?(nalexander)
Attachment #762668 - Flags: review?(gps)
Comment on attachment 762668 [details] [diff] [review]
replace uses of apkbuilder with zip + custom signing tool

OK, so this patch blew up robocop:

https://tbpl.mozilla.org/?rev=da4d885f51f8&tree=Try

Moving JARSIGNER definitions into android-common.mk doesn't seem to be helping, either:

https://tbpl.mozilla.org/?tree=Try&rev=8bc748e0e44d

So I'm not sure how robocop bits ought to be signed at this point.
(In reply to Nathan Froyd (:froydnj) from comment #56)
> Comment on attachment 762668 [details] [diff] [review]
> replace uses of apkbuilder with zip + custom signing tool
> 
> OK, so this patch blew up robocop:
> 
> https://tbpl.mozilla.org/?rev=da4d885f51f8&tree=Try
> 
> Moving JARSIGNER definitions into android-common.mk doesn't seem to be
> helping, either:
> 
> https://tbpl.mozilla.org/?tree=Try&rev=8bc748e0e44d
> 
> So I'm not sure how robocop bits ought to be signed at this point.

This is on my list; I'll should get to it around lunch time.  Thanks for driving this, Nathan.
Callek wanted to be CC'd on whatever bug was causing the robocop bustage.
(In reply to Nick Alexander :nalexander from comment #57)
> (In reply to Nathan Froyd (:froydnj) from comment #56)
> > Comment on attachment 762668 [details] [diff] [review]
> > replace uses of apkbuilder with zip + custom signing tool
> > 
> > OK, so this patch blew up robocop:
> > 
.....

> > So I'm not sure how robocop bits ought to be signed at this point.
> 
> This is on my list; I'll should get to it around lunch time.  Thanks for
> driving this, Nathan.

For sanity of myself/releng/sheriffs can you at least list here try tbpl urls when you test this there, for now.

Since the "blew up" wasn't just red it was "infinite retry" which takes each device it runs on out of the pool for ~an hour... and that can add up fast. A manual cancel tends to stop it when that happens but I just love extra sanity-potential here ;-)
> For sanity of myself/releng/sheriffs can you at least list here try tbpl
> urls when you test this there, for now.

Absolutely.  I filed https://bugzilla.mozilla.org/show_bug.cgi?id=863504 about doing this automatically.
Joel, can you verify that the various testing APKs (sutagent, watcher, fencp, etc) are only ever debug-signed?  I'm confident this is the case, since signing keys are only available at |make package| time and that's much later than anything involving the testing APKs, but want to be certain.

Sub-question: this is possible because the devices are rooted and you can give these processes permissions to do whatever is needed, yes?
Flags: needinfo?(jmaher)
sutagent/watcher are debug signed.  I am not sure about fencp/ffxcp- I believe those are signed with both keys, we just don't use it.

robocop.apk is signed with the same key as the fennec.apk.  

our devices in automation (tegra/panda) are rooted, but that doesn't mean we can set permissions to do anything.  Most likely we can, but there are limitation.  For example, sutagent can't write to files in the org.mozilla.fennec directory without fencp.
Flags: needinfo?(jmaher)
Comment on attachment 762668 [details] [diff] [review]
replace uses of apkbuilder with zip + custom signing tool

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

The make file foo looks good on the technical front. I stopped following the discussions about signing semantics because I don't think they concern me.

::: mobile/android/debug_sign_tool.py
@@ +1,4 @@
> +#!/usr/bin/env python
> +
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this file,

Nit: The proper MPL header has "file," on the 3rd line.
Attachment #762668 - Flags: review?(gps) → review+
Here's a try push with updated signing logic.  Let's save the next person some pain and have a global android-common.mk defined DEBUG_JARSIGNER and a packager.mk defined RELEASE_JARSIGNER.

http://tbpl.mozilla.org/?tree=Try&rev=973bdf8c48b6
So, the original turkey was never going to fly; most notable errors were replacing 0 with O in zip options, and incorrectly including *ap_ files in final APKs rather than merging them in.

http://tbpl.mozilla.org/?tree=Try&rev=1e0660b4d987
Comment on attachment 763981 [details] [diff] [review]
Part 2: Add custom debug signing tool. r=jmaher

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

Review to jmaher for signing correctness.  (You can check the latest try build, too, which seems to be working well.)  Feedback to gps, since I heavily modified the signing tool this iteration.
Attachment #763981 - Flags: review?(jmaher)
Attachment #763981 - Flags: feedback?(gps)
Comment on attachment 763982 [details] [diff] [review]
Part 3: Replace uses of apkbuilder with zip and custom debug signing tool. r=jmaher

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

There were lots of issues with older versions.  This updates the Android build/ makefiles, uniformly (in preparation for future moz.build conversion) and also updates |make package| correctly.
Attachment #763982 - Flags: review?(jmaher)
Comment on attachment 762668 [details] [diff] [review]
replace uses of apkbuilder with zip + custom signing tool

The $(ZIP) commands are incorrect (robocop.ap_ needs to be merged) and there are places where an O is inserted where a 0 was desired.
Attachment #762668 - Attachment is obsolete: true
Attachment #762668 - Flags: review?(nalexander)
Comment on attachment 763981 [details] [diff] [review]
Part 2: Add custom debug signing tool. r=jmaher

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

just a few nits.

::: mobile/android/debug_sign_tool.py
@@ +17,5 @@
> +import subprocess
> +
> +SCRIPT_NAME = os.path.basename(__file__)
> +
> +log = logging.getLogger(SCRIPT_NAME)

we only use script_name here, seems overkill to define the variable and use it.

@@ +98,5 @@
> +    parser.add_argument('-v', '--verbose',
> +                        dest='verbose',
> +                        default=True,
> +                        action='store_true',
> +                        help='verbose output')

so this will always be true unless -q is passed in?  do we need a --verbose option then?

@@ +147,5 @@
> +            log.error(e)
> +
> +
> +if __name__ == '__main__':
> +    main()

there are no return codes here.  I believe our buildbot scripts depend on return codes.
Attachment #763981 - Flags: review?(jmaher) → review+
Comment on attachment 763982 [details] [diff] [review]
Part 3: Replace uses of apkbuilder with zip and custom debug signing tool. r=jmaher

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

I had to double take this patch, but I couldn't find anything out of sorts.
Attachment #763982 - Flags: review?(jmaher) → review+
Thanks for quick review, Joel.  The last patch is definitely funky; it would be better if we had an APK type in rules.mk, but I think we'll get there via moz.build instead.

https://hg.mozilla.org/integration/mozilla-inbound/rev/ceefba074d86
https://hg.mozilla.org/integration/mozilla-inbound/rev/c0b8d909db83
https://hg.mozilla.org/integration/mozilla-inbound/rev/3a4c1a113154

mcomella, can you verify this works for you (with r22, no symlinks) in the next few days?  Thanks for all your work on this patch series.
Flags: needinfo?(michael.l.comella)
Assignee: nobody → nalexander
Target Milestone: --- → Firefox 24
Status: NEW → ASSIGNED
I get the following build error with these patches applied:

make[6]: Entering directory `objdir-droid/build/mobile/sutagent/android'
cp sutAgentAndroid-unsigned-unaligned.apk sutAgentAndroid-unaligned.apk
objdir-droid/_virtualenv/bin/python mobile/android/debug_sign_tool.py sutAgentAndroid-unaligned.apk
Please specify alias name

Please type jarsigner -help for usage
debug_sign_tool.py: Failed to sign sutAgentAndroid-unaligned.apk
debug_sign_tool.py: Command '['jarsigner', '-digestalg', 'SHA1', '-sigalg', 'MD5withRSA', '-keystore', '~/.android/debug.keystore', '-storepass', 'android', 'sutAgentAndroid-unaligned.apk', 'debug']' returned non-zero exit status 1
make[6]: *** [sutAgentAndroid-unaligned.apk] Error 1
make[6]: *** Deleting file `sutAgentAndroid-unaligned.apk'

Reverting part 3 of the patches allows me to build again. Do I have a configuration error or outdated toolset?
(In reply to Chris Double (:doublec) from comment #75)
> I get the following build error with these patches applied:
> 
> make[6]: Entering directory `objdir-droid/build/mobile/sutagent/android'
> cp sutAgentAndroid-unsigned-unaligned.apk sutAgentAndroid-unaligned.apk
> objdir-droid/_virtualenv/bin/python mobile/android/debug_sign_tool.py
> sutAgentAndroid-unaligned.apk
> Please specify alias name
> 
> Please type jarsigner -help for usage
> debug_sign_tool.py: Failed to sign sutAgentAndroid-unaligned.apk
> debug_sign_tool.py: Command '['jarsigner', '-digestalg', 'SHA1', '-sigalg',
> 'MD5withRSA', '-keystore', '~/.android/debug.keystore', '-storepass',
> 'android', 'sutAgentAndroid-unaligned.apk', 'debug']' returned non-zero exit
> status 1
> make[6]: *** [sutAgentAndroid-unaligned.apk] Error 1
> make[6]: *** Deleting file `sutAgentAndroid-unaligned.apk'

My colleague :rillian saw the same thing.  Can you let me know your JDK vendor and version, and also the output of `jarsigner -h`?  Current theory is that this is an issue with OpenJDK 1.7; see http://stackoverflow.com/questions/8409944/using-jarsigner-verify-on-an-apk-asks-me-to-specify-an-alias.

> Reverting part 3 of the patches allows me to build again. Do I have a
> configuration error or outdated toolset?

This doesn't surprise me -- that's back to using apkbuilder.
(In reply to Nick Alexander :nalexander from comment #76)
> My colleague :rillian saw the same thing.  Can you let me know your JDK
> vendor and version, and also the output of `jarsigner -h`?  Current theory
> is that this is an issue with OpenJDK 1.7; see
> http://stackoverflow.com/questions/8409944/using-jarsigner-verify-on-an-apk-
> asks-me-to-specify-an-alias.

$ java -version
java version "1.6.0_45"
Java(TM) SE Runtime Environment (build 1.6.0_45-b06)
Java HotSpot(TM) 64-Bit Server VM (build 20.45-b01, mixed mode)

$ jarsigner -h
Usage: jarsigner [options] jar-file alias
       jarsigner -verify [options] jar-file [alias...]

[-keystore <url>]           keystore location

[-storepass <password>]     password for keystore integrity

[-storetype <type>]         keystore type

[-keypass <password>]       password for private key (if different)

[-certchain <file>]         name of alternative certchain file

[-sigfile <file>]           name of .SF/.DSA file

[-signedjar <file>]         name of signed JAR file

[-digestalg <algorithm>]    name of digest algorithm

[-sigalg <algorithm>]       name of signature algorithm

[-verify]                   verify a signed JAR file

[-verbose[:suboptions]]     verbose output when signing/verifying.
                            suboptions can be all, grouped or summary

[-certs]                    display certificates when verbose and verifying

[-tsa <url>]                location of the Timestamping Authority

[-tsacert <alias>]          public key certificate for Timestamping Authority

[-altsigner <class>]        class name of an alternative signing mechanism

[-altsignerpath <pathlist>] location of an alternative signing mechanism

[-internalsf]               include the .SF file inside the signature block

[-sectionsonly]             don't compute hash of entire manifest

[-protected]                keystore has protected authentication path

[-providerName <name>]      provider name

[-providerClass <class>     name of cryptographic service provider's
  [-providerArg <arg>]] ... master class file and constructor argument

[-strict]                   treat warnings as errors
>Current theory is that this is an issue with OpenJDK 1.7

Nope:

morbo@ubox:~/hg/mozilla-central$ java -version
java version "1.6.0_26"
Java(TM) SE Runtime Environment (build 1.6.0_26-b03)
Java HotSpot(TM) 64-Bit Server VM (build 20.1-b02, mixed mode)

This is a blocker for development now.
Btw, my error is slightly different:

jarsigner: Certificate chain not found for: debug.  debug must reference a valid KeyStore key entry containin
I ran into the same failure as Chris (comment 75); in my case I -do- have java 1.7:

  $ java -version
  java version "1.7.0_21"
  Java(TM) SE Runtime Environment (build 1.7.0_21-b12)
  Java HotSpot(TM) 64-Bit Server VM (build 23.21-b01, mixed mode)

but clearly the problem isn't limited to this version.
Final part backed out for jarsigner errors:

https://hg.mozilla.org/integration/mozilla-inbound/rev/f44ac4a65b1a

Thanks for prompt reporting, chaps.  Any help investigating so we can re-land would be great.
Status: RESOLVED → REOPENED
Flags: needinfo?(michael.l.comella)
Resolution: FIXED → ---
(In reply to Gian-Carlo Pascutto (:gcp) from comment #79)
> Btw, my error is slightly different:
> 
> jarsigner: Certificate chain not found for: debug.  debug must reference a
> valid KeyStore key entry containin

I'm fairly sure that this error means that ~/.android/debug.keystore previously existed but doesn't contain the 'debug' key alias.  This should be a one time fix to either update your key-store (using `debug_sign_tool.py --force test.apk`) or to move your debug.keystore away and create a new one.  I'll investigate doing this more thoroughly.
I tried removing my debug.keystore at Nick's suggestion yesterday. jarsigner created a new one, but I still get the 'Please specify alias name' error.
So the problem is jarsigner uses java.text.Collator to compare arguments against switch names. Collator appears to strip the leading '-' before comparison, so when we pass 'debug' as the alias name, it's interpreted as '-debug' instead.

The work-around is to use an alias name which doesn't happen to correspond to a jarsigner switch.

Nick is filing an upstream bug against jarsigner, which can be fixed by setting the appropriate strength on the Collator to respect the '-' character, or by just using string.equals.
Bug 874132 - Rename debug signing key and set $(ADB) and $(AIDL). r=jmaher

This works around an un-confirmed JDK bug in jarsigner, where '-debug'
and 'debug' compare the same.

There's a little more work to test for an existing keystore without a valid key that I'll do as another follow up; just want this for :rillian to test.
Bug 874132 - Follow-up: Rename debug key; check if alias exists; set $(AIDL). r=jmaher

Renaming the debug key works around an un-confirmed JDK bug in
jarsigner, where '-debug' and 'debug' compare the same.

I'll fold this into Part 3 for landing.  I hope to re-land today, so that the fix is in for the m-c merge tonight.
Attachment #764994 - Attachment is obsolete: true
Attachment #765044 - Flags: review?(jmaher)
Comment on attachment 765044 [details] [diff] [review]
Bug 874132 - Follow-up: Rename debug key; checking if alias exists; set $(AIDL). r=jmaher

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

With this patch I can build fennec with the r22 sdk on my Fedora linux system.
Attachment #765044 - Flags: feedback+
Comment on attachment 765044 [details] [diff] [review]
Bug 874132 - Follow-up: Rename debug key; checking if alias exists; set $(AIDL). r=jmaher

The non-python part looks OK. The python part seems to make sense to me.
Attachment #765044 - Flags: review+
Attachment #765044 - Flags: review?(jmaher)
Folded into one reland, let's hope it sticks.  Thanks to everyone who helped get this relanded, especially giles.

https://hg.mozilla.org/integration/mozilla-inbound/rev/833ca9a17792
Comment on attachment 763981 [details] [diff] [review]
Part 2: Add custom debug signing tool. r=jmaher

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

What jmaher said.
Attachment #763981 - Flags: feedback?(gps) → feedback+
Sigh.  Not my best ticket.  Let's spare myself further embarrassment and

https://tbpl.mozilla.org/?tree=Try&rev=133f3ec87ddc
The fix against backout is trivial: I forgot to create the ~/.android directory.  Not going to ask for re-review on that, it's re-instating old code.

The previous try build was based on m-i and failed due to an unrelated patch (that was immediately backed out).  Signing the tools APKs is earlier, but I'd like to see some green in |make package| before landing.  Here's a try against m-c that works well locally:

http://tbpl.mozilla.org/?tree=Try&rev=7f2e28b2825e
(In reply to Nick Alexander :nalexander from comment #93)

> http://tbpl.mozilla.org/?tree=Try&rev=7f2e28b2825e

This patch works on my system. ./mach build && ./mach package && ./mach install
https://hg.mozilla.org/mozilla-central/rev/e51c7d3d5b9d
Status: REOPENED → RESOLVED
Closed: 11 years ago → 11 years ago
Resolution: --- → FIXED
This page need to be updated to reflect this change:

https://wiki.mozilla.org/Mobile/Fennec/Android#Install_Android_SDK
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(In reply to Bill Gianopoulos [:WG9s] from comment #97)
> This page need to be updated to reflect this change:
> 
> https://wiki.mozilla.org/Mobile/Fennec/Android#Install_Android_SDK

Updated two sections (Linux and Mac OS X).
Status: REOPENED → RESOLVED
Closed: 11 years ago → 11 years ago
Resolution: --- → FIXED
(In reply to Nick Alexander :nalexander from comment #98)
> (In reply to Bill Gianopoulos [:WG9s] from comment #97)
> > This page need to be updated to reflect this change:
> > 
> > https://wiki.mozilla.org/Mobile/Fennec/Android#Install_Android_SDK
> 
> Updated two sections (Linux and Mac OS X).

Thanks.  I will try a build now.
Depends on: 885951
Depends on: 886172
No longer depends on: 886172
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: