608432 - port sign_android.sh to mozharness

Status: RESOLVED FIXED

(Release Engineering :: Applications: MozharnessCore, defect, P5)

Description

[Aki Sasaki (not active)]

Right now it's not multilocale/en-US friendly and makes various assumptions about paths.

It also makes you rename fennec.apk (shouldn't), and should give you the hash+filesize for the snippet just to be extra special friendly.

Comment 1

[Aki Sasaki (not active)]

We also don't really need the gecko-*.apk files.

Comment 2

[Aki Sasaki (not active)]

Current ugliness is documented, and lsblakk is familiar with it.

Comment 3

[Aki Sasaki (not active)]

sign_android.sh is a lot better currently.

My current beef with it is:

a) you have to enter the passphrases twice, once each for en-US and multi
b) it will happily fail at signing, and upload
c) if you ctrl-c out, it'll happily fail at signing, and upload
d) if you run again, it'll download to apkname.1

the solution here is currently to blow away the signing directory and try again.

Comment 4

[Aki Sasaki (not active)]

Attachment: wrapper sign_android.sh

There's a build/tools/release/signing/sign_android.sh that signs a single gecko_unsigned_unaligned.apk and zipaligns it.

This script is a wrapper that creates the directory, downloads the apks, signs them, and uploads them.

I want to improve it:

a) grab the signing passphrases once instead of twice
b) fail out if signing fails.

Comment 5

[Aki Sasaki (not active)]

Attachment: [untested] run verify-android-signature.sh from sign_android.sh

* move build/tools/release/signing/sign_android.sh to sign_apk.sh
* overwrite sign_android.sh with the wrapper sign_android.sh
* edit sign_android.sh to:
** call sign_apk.sh instead of sign_android.sh
** exit on non-zero status
** run verify-android-signature.sh after signing

Issues:

* untested
* won't give you a big error banner when you need to start over
* have to enter your passphrases 2x2 (twice for 2 passphrases)
* requires editing the script for each new release

I think other than needing testing, this is an improvement.

Comment 6

[Armen [:armenzg]]

Attachment: sign_android.sh changes to take it work on staging-stage and staging keymaster

Comment 7

[Aki Sasaki (not active)]

Comment on attachment 540745 [details] [diff] [review]
sign_android.sh changes to take it work on staging-stage and staging keymaster

Hm, maybe this should default to stage.mozilla.org and we should alter to go to staging-stage in staging.

Comment 8

[Aki Sasaki (not active)]

This is a stopgap for bug 705214, until we can get release signing on demand (bug 705807).

Comment 9

[Aki Sasaki (not active)]

Work is happening on github, on the android-signing branch.
https://github.com/escapewindow/mozharness/commits/android-signing

Comment 12

[Aki Sasaki (not active)]

Attachment: android signing in mozharness

Phew!

Not quite done yet, but here's where I'm at now.

* I have a script
* I have a config file
* The script:
** gets the passphrases and verifies them at the beginning
** clobbers the work_dir
** pulls the repos (build/tools + buildbot-configs currently)
** downloads the unsigned apks
** signs them
** verifies the signatures
** creates betatest, releasetest, and release snippets

I also:

* added BaseErrorList
* added OutputParser, and changed run_command() to use it
** this allowed me to parse password-sensitive output without logging or outputting to the screen
** this will also allow for bug 706887
* added query_exe() to specify command paths that aren't in PATH
* quieted a number of common tasks (rmtree in get_output_from_command(), various operations in copy_to_upload_dir()

All in all, without hg.m.o lag, 2 locales on 1 platform with 3 channels takes < 1min to run.  I have some ideas on how to speed things up if we have a lot of apks to sign and create snippets for (bug 706887)

I still need to:

* add more commandline options
* write the two upload steps and three push-snippets steps
* test on keystage01 or signing1
* add partner repack signing/snippet creation when partner repacks land; I may leave this for later.

How I'm going to be dealing with Android snippets:

* Upload complete snippets to ftp.m.o, as VERSION-candidates/buildNUM/update/PLATFORM/LOCALE/latest-CHANNEL text files
* During the release build, when we normally would be creating complete and partial snippets, create size 0 snippets and softlink the complete.txt to ../../../../../snippets/PLATFORM/LOCALE/latest-CHANNEL
* In the push-CHANNEL-snippets steps, push the latest-CHANNEL snippets to /opt/aus2/incoming/3/snippets/PLATFORM/LOCALE/latest-CHANNEL

I can do this because Android updates:

* never have a partial
* point to the apk, not a mar
* always update to the latest when we say 'go'; none of this MU/throttling/silent stuff.

Posting all of this for open feedback from anyone.
If you're confused why I did something, I put per-checkin comments here:
  https://github.com/escapewindow/mozharness/commits/android-signing
I got this diff via
  git diff -U9 -r master
after checking out the android-signing branch.

Comment 13

[Rail Aliiev [:rail]]

Comment on attachment 578466 [details] [diff] [review]
android signing in mozharness

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

Whooohooo!

::: configs/signing/staging_android_mozilla-beta.py
@@ +6,5 @@
> +VERSION = "9.0b3"
> +OLD_VERSION = "9.0b2"
> +OLD_BUILDNUM = 1
> +BUILDNUM = 1
> +TAG = "FENNEC_9_0b3_RELEASE"

Per TODO comments in sign_android.py it sounds that you are going to pass version/build#/etc without hard coding them. Nice ;)

::: mozharness/base/log.py
@@ +179,5 @@
> +                if 'substr' in error_check:
> +                    if error_check['substr'] in line:
> +                        match = True
> +                elif 'regex' in error_check:
> +                    if re.search(error_check['regex'], line):

Probably compiling regex in advance would be better.

::: scripts/sign_android.py
@@ +218,5 @@
> +        url = base_url % replace_dict
> +        # TODO stop using curl
> +        output = self.get_output_from_command(["curl", "--silent", url])
> +        if output.startswith("buildID="):
> +            return output.replace("buildID=", "")

Don't recall ho get_ouput_command works, but probably it would be safer to add .strip().

Comment 14

[Aki Sasaki (not active)]

Attachment: sign_android_info.log

INFO log output from sign_android.py.

Comment 15

[Aki Sasaki (not active)]

Attachment: sign_android.py with staging beta release config

This script

1. Gets passphrases
2. Verifies passphrases
3. Clobbers
4. Pulls buildbot-configs and build/tools
5. Downloads unsigned android apks
6. Signs android apks
7. Verifies those apks' signatures
8. Uploads signed apks
9. Creates snippets for betatest, releasetest, release
10. Uploads those snippets to aus.  To push them live we'll have to use pushsnip.

It pulls a lot of its config from the release config file (staging_release-fennec-mozilla-beta.py in this case), to avoid too much duplication of configs.  This adds quite a bit of complexity, but it's nice to not have to bump the signing config file along with everything else.

I've verified it works on linux-ix-slave04, and an earlier draft worked on my laptop.  I need to test on the official signing env before I can generate the production signing configs -- is that signing1 (which I can't seem to log into anymore) ?  Since I'm using softlinks now, running on a windows box will require a rewrite of the previous builddir snippet links.

Comment 16

[Rail Aliiev [:rail]]

Comment on attachment 591311 [details] [diff] [review]
sign_android.py with staging beta release config

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

I'm pretty happy to see the progress here!

I haven't run this for reals, I believe you did. :)

Almost all comments are minor except the one in verify_signatures.

I removed r? to remove this from my review queue. Once you're ready for the next round mark it r? and I'll get it in my "special" mail folder. :)

::: configs/signing/staging_android_mozilla-beta.py
@@ +1,3 @@
> +#!/usr/bin/env python
> +
> +import os

This looks a little bit weird in a config file.

@@ +13,5 @@
> +DOWNLOAD_BASE_URL = "http://ftp.mozilla.org/pub/mozilla.org/mobile/candidates/%(version)s-candidates/build%(buildnum)d"
> +APK_BASE_NAME = "fennec-%(version)s.%(locale)s.android-arm.apk"
> +# because sign_android-0.8.sh renamed these wrong :(
> +BUILDID_BASE_URL = DOWNLOAD_BASE_URL + "/%(platform)_info.txt"
> +OLD_STYLE_BUILDID_BASE_URL = DOWNLOAD_BASE_URL + "/linux-android_info.txt"

This is to be removed once we have old version using android_info.txt. Correct?

@@ +14,5 @@
> +APK_BASE_NAME = "fennec-%(version)s.%(locale)s.android-arm.apk"
> +# because sign_android-0.8.sh renamed these wrong :(
> +BUILDID_BASE_URL = DOWNLOAD_BASE_URL + "/%(platform)_info.txt"
> +OLD_STYLE_BUILDID_BASE_URL = DOWNLOAD_BASE_URL + "/linux-android_info.txt"
> +FFXBLD_SSH_KEY = '%s/.ssh/ffxbld_dsa' % os.environ['HOME']

I would use
FFXBLD_SSH_KEY = '~/.ssh/ffxbld_dsa'
and ssh -oIdentityFile instead of -i. -oIdentityFile can expand ~ and more portable. Not a big deal since we use cygwin python on the keysigning machine.

@@ +15,5 @@
> +# because sign_android-0.8.sh renamed these wrong :(
> +BUILDID_BASE_URL = DOWNLOAD_BASE_URL + "/%(platform)_info.txt"
> +OLD_STYLE_BUILDID_BASE_URL = DOWNLOAD_BASE_URL + "/linux-android_info.txt"
> +FFXBLD_SSH_KEY = '%s/.ssh/ffxbld_dsa' % os.environ['HOME']
> +CLTBLD_SSH_KEY = '%s/.ssh/cltbld_dsa' % os.environ['HOME']

Unused.

@@ +89,5 @@
> +
> +    "keystore": KEYSTORE,
> +    "key_alias": KEY_ALIAS,
> +    "env": {
> +        "PATH": "%(PATH)s:" + JAVA_HOME + "/bin",

IMO, this one is safer:
"PATH": JAVA_HOME + "/bin:%(PATH)s",

@@ +97,5 @@
> +        "zipalign": "/tools/android-sdk-r13/tools/zipalign",
> +    },
> +    "signature_verification_script": "tools/release/signing/verify-android-signature.sh",
> +
> +    "user_repo_override": "users/asasaki_mozilla.com",

use stage-ffxbld unless you want your repo messed by others. :P

::: scripts/sign_android.py
@@ +56,5 @@
> +
> +from mozharness.base.config import parse_config_file
> +from mozharness.base.errors import BaseErrorList, SSHErrorList
> +from mozharness.base.log import OutputParser, DEBUG, INFO, WARNING, ERROR, \
> +     CRITICAL, FATAL, IGNORE

CRITICAL, DEBUG, WARNING are unused

@@ +85,5 @@
> +}]
> +TEST_JARSIGNER_ERROR_LIST = [{
> +    "substr": "jarsigner: unable to open jar file:",
> +    "level": IGNORE,
> +}] + JARSIGNER_ERROR_LIST

Does this override the same message in JARSIGNER_ERROR_LIST?

@@ +89,5 @@
> +}] + JARSIGNER_ERROR_LIST
> +
> +# From http://bytes.com/topic/python/answers/26569-finding-file-size,
> +# for query_filesize()
> +class SizedFile(file):

/me wonders what's wrong with os.path.getsize or os.stat?

@@ +250,5 @@
> +            self.release_config['ftp_user'] = c.get('ftp_user', rc['hgUsername'])
> +            self.release_config['ftp_ssh_key'] = c.get('ftp_ssh_key', rc['hgSshKey'])
> +            self.release_config['aus_server'] = rc['stagingServer']
> +            self.release_config['aus_user'] = rc['ausUser']
> +            self.release_config['aus_ssh_key'] = os.path.join(os.environ['HOME'], '.ssh', rc['ausSshKey'])

'~/.ssh/%s' % rc['ausSshKey'] and use -oIdentityFile?

@@ +261,5 @@
> +        self.info("Release config:\n%s" % self.release_config)
> +        return self.release_config
> +
> +    # TODO query_filesize and query_sha512sum probably belong in
> +    # mozharness.base somewhere

Agreed :)
If you prefer, you can file a separate file for this.

@@ +282,5 @@
> +        self.info(" %s" % sha512)
> +        return sha512
> +
> +    def query_buildid(self, platform, base_url, buildnum=None, version=None):
> +        c = self.config

unused

@@ +284,5 @@
> +
> +    def query_buildid(self, platform, base_url, buildnum=None, version=None):
> +        c = self.config
> +        rc = self.query_release_config()
> +        locales = self.query_locales()

unused

@@ +296,5 @@
> +        if version:
> +            replace_dict['version'] = version
> +        url = base_url % replace_dict
> +        # TODO stop using curl
> +        output = self.get_output_from_command(["curl", "--silent", url])

Can you retry if the command fails here?

@@ +331,5 @@
> +    # Actions {{{2
> +    def passphrase(self):
> +        if not self.store_passphrase:
> +            print "(store passphrase): ",
> +            self.store_passphrase = getpass.getpass()

IMO self.store_passphrase = getpass.getpass("(store passphrase): ") is more readable

@@ +334,5 @@
> +            print "(store passphrase): ",
> +            self.store_passphrase = getpass.getpass()
> +        if not self.key_passphrase:
> +            print "(key passphrase): ",
> +            self.key_passphrase = getpass.getpass()

The same here.

@@ +337,5 @@
> +            print "(key passphrase): ",
> +            self.key_passphrase = getpass.getpass()
> +
> +    def verify_passphrases(self):
> +        c = self.config

unused

@@ +449,5 @@
> +        verification_error_list = BaseErrorList + [{
> +            "regex": re.compile(r'''^Invalid$'''),
> +            "level": FATAL,
> +            "explanation": "Signature is invalid!"
> +        }]

Does this work? I know that signature_verification_script should print Valid/Invalid, but when we run it from buildbot it usually prints the following and exits too early to print Valid/Invalid:

caution: filename not matched:  META-INF/RELEASE.RSA
ERROR: Could not unzip ./package.apk
Are you sure this is a release package?
TOOLS DIR: scripts/
Archive:  ./package.apk

See: http://buildbot-master08.build.sjc1.mozilla.com:8001/builders/release-mozilla-beta-android_signature_verification/builds/67/steps/run_script/logs/stdio

Probably the tool should be adjusted or you should add other messages here.

@@ +478,5 @@
> +        ftp_upload_dir = c['ftp_upload_base_dir'] % {
> +            'version': rc['version'],
> +            'buildnum': rc['buildnum'],
> +        }
> +        cmd = [ssh, '-i', rc['ftp_ssh_key'],

As I mentioned in the config file comments you can use the following here:
cmd = [ssh, '-oIdentityFile', rc['ftp_ssh_key'],

@@ +483,5 @@
> +               '%s@%s' % (rc['ftp_user'], rc['ftp_server']),
> +               'mkdir', '-p', ftp_upload_dir]
> +        self.run_command(cmd, cwd=dirs['abs_work_dir'],
> +                         error_list=SSHErrorList)
> +        cmd = [rsync, '-e', 'ssh -i %s' % rc['ftp_ssh_key'], '-azv']

Since you query_exe, maybe it would be safer to use the following?
cmd = [rsync, '-e', '%s -oIdentityFile %s' % (ssh, rc['ftp_ssh_key']), '-azv']

@@ +489,5 @@
> +        cmd += ["%s@%s:%s/" % (rc['ftp_user'], rc['ftp_server'], ftp_upload_dir)]
> +        self.run_command(cmd, cwd=dirs['abs_work_dir'],
> +                         error_list=SSHErrorList)
> +
> +    def create_snippets(self):

If I understand correctly, this function creates snippets only for the previous build plus creates latest-* snippet. Does this mean that fennec queries latest-* channels as well?

@@ +580,5 @@
> +        aus_upload_dir = c['aus_upload_base_dir'] % {
> +            'version': rc['version'],
> +            'buildnum': rc['buildnum'],
> +        }
> +        cmd = [ssh, '-i', rc['aus_ssh_key'],

See comments above:
cmd = [ssh, '-oIdentityFile', rc['aus_ssh_key'],

@@ +586,5 @@
> +               'mkdir', '-p', aus_upload_dir]
> +        self.run_command(cmd, cwd=dirs['abs_work_dir'],
> +                         error_list=SSHErrorList)
> +        cmd = [rsync, '-e', 'ssh -i %s' % rc['aus_ssh_key'], '-azv', '.']
> +        cmd += ["%s@%s:%s/." % (rc['aus_user'], rc['aus_server'], aus_upload_dir)]

See comments above plus "rsync ./ dir/" is easier to parse imho :
cmd = [rsync, '-e', '%s -oIdentityFile %s' % (ssh, rc['aus_ssh_key']), '-azv', './']
 cmd += ["%s@%s:%s/" % (rc['aus_user'], rc['aus_server'], aus_upload_dir)]

Comment 17

[Aki Sasaki (not active)]

Attachment: fixed sign_android.py

(In reply to Rail Aliiev [:rail] from comment #16)
> I haven't run this for reals, I believe you did. :)

Yes.

> ::: configs/signing/staging_android_mozilla-beta.py
> @@ +1,3 @@
> > +#!/usr/bin/env python
> > +
> > +import os
> 
> This looks a little bit weird in a config file.

Yeah.  The getcwd() isn't strictly needed. I preferred the import over hardcoding various paths; comments and a little bit of self-contained logic are one advantage of .py config files.

> @@ +13,5 @@
> > +DOWNLOAD_BASE_URL = "http://ftp.mozilla.org/pub/mozilla.org/mobile/candidates/%(version)s-candidates/build%(buildnum)d"
> > +APK_BASE_NAME = "fennec-%(version)s.%(locale)s.android-arm.apk"
> > +# because sign_android-0.8.sh renamed these wrong :(
> > +BUILDID_BASE_URL = DOWNLOAD_BASE_URL + "/%(platform)_info.txt"
> > +OLD_STYLE_BUILDID_BASE_URL = DOWNLOAD_BASE_URL + "/linux-android_info.txt"
> 
> This is to be removed once we have old version using android_info.txt.
> Correct?

Correct.  That can happen in 11.0b2.

> > +FFXBLD_SSH_KEY = '%s/.ssh/ffxbld_dsa' % os.environ['HOME']
> 
> I would use
> FFXBLD_SSH_KEY = '~/.ssh/ffxbld_dsa'
> and ssh -oIdentityFile instead of -i. -oIdentityFile can expand ~ and more
> portable. Not a big deal since we use cygwin python on the keysigning
> machine.

I replaced all keys and ssh/rsync commands to use -oIdentityFile and ~/.ssh/KEY.

> @@ +15,5 @@
> > +# because sign_android-0.8.sh renamed these wrong :(
> > +BUILDID_BASE_URL = DOWNLOAD_BASE_URL + "/%(platform)_info.txt"
> > +OLD_STYLE_BUILDID_BASE_URL = DOWNLOAD_BASE_URL + "/linux-android_info.txt"
> > +FFXBLD_SSH_KEY = '%s/.ssh/ffxbld_dsa' % os.environ['HOME']
> > +CLTBLD_SSH_KEY = '%s/.ssh/cltbld_dsa' % os.environ['HOME']
> 
> Unused.

Used now! :)

> @@ +89,5 @@
> > +
> > +    "keystore": KEYSTORE,
> > +    "key_alias": KEY_ALIAS,
> > +    "env": {
> > +        "PATH": "%(PATH)s:" + JAVA_HOME + "/bin",
> 
> IMO, this one is safer:
> "PATH": JAVA_HOME + "/bin:%(PATH)s",

Sure. I think I just wanted jarsigner in the path somewhere, didn't care which.  But I generally add stuff in the front, no real reason not to here.

> ::: scripts/sign_android.py
> @@ +56,5 @@
> > +
> > +from mozharness.base.config import parse_config_file
> > +from mozharness.base.errors import BaseErrorList, SSHErrorList
> > +from mozharness.base.log import OutputParser, DEBUG, INFO, WARNING, ERROR, \
> > +     CRITICAL, FATAL, IGNORE
> 
> CRITICAL, DEBUG, WARNING are unused

Yeah. I tend to import all of them because I want to be able to use them easily.

> @@ +85,5 @@
> > +}]
> > +TEST_JARSIGNER_ERROR_LIST = [{
> > +    "substr": "jarsigner: unable to open jar file:",
> > +    "level": IGNORE,
> > +}] + JARSIGNER_ERROR_LIST
> 
> Does this override the same message in JARSIGNER_ERROR_LIST?

Yes, it stops matching after the first match.

> @@ +89,5 @@
> > +}] + JARSIGNER_ERROR_LIST
> > +
> > +# From http://bytes.com/topic/python/answers/26569-finding-file-size,
> > +# for query_filesize()
> > +class SizedFile(file):
> 
> /me wonders what's wrong with os.path.getsize or os.stat?

Uh, evidently nothing.
Using os.path.getsize() now :)

> @@ +261,5 @@
> > +        self.info("Release config:\n%s" % self.release_config)
> > +        return self.release_config
> > +
> > +    # TODO query_filesize and query_sha512sum probably belong in
> > +    # mozharness.base somewhere
> 
> Agreed :)
> If you prefer, you can file a separate file for this.

Sure.

> @@ +296,5 @@
> > +        if version:
> > +            replace_dict['version'] = version
> > +        url = base_url % replace_dict
> > +        # TODO stop using curl
> > +        output = self.get_output_from_command(["curl", "--silent", url])
> 
> Can you retry if the command fails here?

Sure, done.

> > +    def passphrase(self):
> > +        if not self.store_passphrase:
> > +            print "(store passphrase): ",
> > +            self.store_passphrase = getpass.getpass()
> 
> IMO self.store_passphrase = getpass.getpass("(store passphrase): ") is more
> readable

Done.

> @@ +449,5 @@
> > +        verification_error_list = BaseErrorList + [{
> > +            "regex": re.compile(r'''^Invalid$'''),
> > +            "level": FATAL,
> > +            "explanation": "Signature is invalid!"
> > +        }]
> 
> Does this work? I know that signature_verification_script should print
> Valid/Invalid, but when we run it from buildbot it usually prints the
> following and exits too early to print Valid/Invalid:

I expanded the error list.

> @@ +483,5 @@
> > +               '%s@%s' % (rc['ftp_user'], rc['ftp_server']),
> > +               'mkdir', '-p', ftp_upload_dir]
> > +        self.run_command(cmd, cwd=dirs['abs_work_dir'],
> > +                         error_list=SSHErrorList)
> > +        cmd = [rsync, '-e', 'ssh -i %s' % rc['ftp_ssh_key'], '-azv']
> 
> Since you query_exe, maybe it would be safer to use the following?
> cmd = [rsync, '-e', '%s -oIdentityFile %s' % (ssh, rc['ftp_ssh_key']),
> '-azv']

Sure. Done.

> @@ +489,5 @@
> > +        cmd += ["%s@%s:%s/" % (rc['ftp_user'], rc['ftp_server'], ftp_upload_dir)]
> > +        self.run_command(cmd, cwd=dirs['abs_work_dir'],
> > +                         error_list=SSHErrorList)
> > +
> > +    def create_snippets(self):
> 
> If I understand correctly, this function creates snippets only for the
> previous build plus creates latest-* snippet. Does this mean that fennec
> queries latest-* channels as well?

No.

I'm creating a snippets/.../latest-CHANNEL; I'm also creating a VERSION/.../CHANNEL/complete.txt link to the latest-CHANNEL file.  So when AUS looks in VERSION/.../CHANNEL for the snippet, it's softlinked to the latest-CHANNEL file and all Fennecs get that complete snippet.

There's no reason to call it latest-CHANNEL, other than it should always point to the most recent release apk.

(I did notice that I wasn't creating a complete.txt and partial.txt, though since you asked, so there was a bug here. Fixed. :)

> @@ +586,5 @@
> > +               'mkdir', '-p', aus_upload_dir]
> > +        self.run_command(cmd, cwd=dirs['abs_work_dir'],
> > +                         error_list=SSHErrorList)
> > +        cmd = [rsync, '-e', 'ssh -i %s' % rc['aus_ssh_key'], '-azv', '.']
> > +        cmd += ["%s@%s:%s/." % (rc['aus_user'], rc['aus_server'], aus_upload_dir)]
> 
> See comments above plus "rsync ./ dir/" is easier to parse imho :
> cmd = [rsync, '-e', '%s -oIdentityFile %s' % (ssh, rc['aus_ssh_key']),
> '-azv', './']
>  cmd += ["%s@%s:%s/" % (rc['aus_user'], rc['aus_server'], aus_upload_dir)]

I personally really prefer "rsync . dir/.", but I'm open.  Changed.


(In reply to Aki Sasaki [:aki] from comment #15)
> I need to test on the official signing env before I can generate
> the production signing configs -- is that signing1 (which I can't seem to
> log into anymore) ?  Since I'm using softlinks now, running on a windows box
> will require a rewrite of the previous builddir snippet links.

Do you know what signing machine I should be using?

Comment 18

[Aki Sasaki (not active)]

Attachment: sign_android interdiff (patch1 vs patch2)

Comment 19

[Rail Aliiev [:rail]]

(In reply to Aki Sasaki [:aki] from comment #17)
> Do you know what signing machine I should be using?

Hmmmm. Not sure if the linux based singing severs (signing{1,2}.b.scl1.m.c) are ready out of the box for this. They would need JDK (puppet?) and the keys for this (anything else?).

At the same time I think that using the current ones (windows based) is waste of time because they will be gone quite soon.

Comment 20

[Aki Sasaki (not active)]

(In reply to Rail Aliiev [:rail] from comment #19)
> (In reply to Aki Sasaki [:aki] from comment #17)
> > Do you know what signing machine I should be using?
> 
> Hmmmm. Not sure if the linux based singing severs (signing{1,2}.b.scl1.m.c)
> are ready out of the box for this. They would need JDK (puppet?) and the
> keys for this (anything else?).
> 
> At the same time I think that using the current ones (windows based) is
> waste of time because they will be gone quite soon.

I think that's what bug 706243 is for.
I'd poke around but I don't seem to know how to log in to signing1.

Comment 21

[Aki Sasaki (not active)]

Comment on attachment 591637 [details] [diff] [review]
fixed sign_android.py

http://hg.mozilla.org/build/mozharness/rev/9123b2113719

I still need to get this working on signing{1,2} (or other box), which will hopefully only involve config file changes.

Then I need to change the Android signing documentation.

Comment 22

[Aki Sasaki (not active)]

Attachment: production mozilla-beta android signing config

Comment 23

[Aki Sasaki (not active)]

This was tested on signing1, though I used dev-stage01 instead of aus2-staging and stage.m.o.

Comment 24

[Rail Aliiev [:rail]]

Comment on attachment 592316 [details] [diff] [review]
production mozilla-beta android signing config

lgtm

Comment 25

[Aki Sasaki (not active)]

Attachment: add android_mozilla-release.py, change config file from staging_release* to release*

Oops.

I don't need the release config yet, but it's good to get in before 11.0; I do need to point at release-fennec-mozilla-{beta,release}.py instead of staging_*.

I think this is good, although I haven't actually run this against stage.m.o and aus2-staging; I'll close this bug after this and track documentation in bug 705214.

Comment 26

[Aki Sasaki (not active)]

[18:31]	<rail>	meh, no easy signing for repacks: 404 for http://dev-stage01.build.mozilla.org/pub/mozilla.org/mobile/candidates/11.0b1-candidates/build1/unsigned/android/zh-TW/gecko-unsigned-unaligned.apk
[18:31]	<aki>	rail: yuk. i gotta fix that

I also need to get locales from the locales.json + en-US (and a flag for multilocale).

Comment 27

[Aki Sasaki (not active)]

bug 718777 comment 11:
> Still need a mozharness config for android-xul signing (configs/signing/{,staging_}android_mozilla-beta.py)

Comment 28

[Aki Sasaki (not active)]

Attachment: additional signing fixes

* Removed unused CHANGES file. I like this, but I haven't been keeping it up to date :(
* Changed the locales specified in the signing config files to additional_locales, and rely on the locales file for the list of locales.
* Changed the buildid base urls to the standardized name, since we won't be creating snippets for 11.0b1, but will for beta 2 on.
* Changed the log_name and work_dir for each config file, so we can potentially run signing in the same directory (not completely tested yet)
* Downloaded the fennec*.apk rather than gecko-unsigned-unaligned.apk.  Added code to delete META-INF from the apk before signing.
* Ported the actions-in-config-files patch from the talosrunner branch.  Changed the non-staging config files to not create/upload snippets yet.
** This also adds volatile_config in the saved localconfig.json.  I'm undecided about this.
* Fixed the query_locales() way of using ignore_locales and additional_locales.
* Added more helpful? summary messages, like I had in sign_debs.py.
* Skipped known-failure apks (if we can't download an apk, don't try to sign it, etc.)
* Downloaded unsigned apk in a different directory, so we upload only the signed apk, and only if it's successful.

Comment 29

[Rail Aliiev [:rail]]

Comment on attachment 593212 [details] [diff] [review]
additional signing fixes

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

some overall comments:

* _sign() may throw an exception. Do you feel comfortable with this or prefer to catch it?
* config changes should be landed once we done with 11.0b1, correct?

r+ with verify_signatures() messages fixes.

::: scripts/sign_android.py
@@ +294,4 @@
>                  self.warning("Can't get buildID from %s (try %d)" % (url, count))
>          self.critical("Can't get buildID from %s!" % url)
>  
> +    def _sign(self, apk, meta_inf=True, error_list=None):

Maybe remove_meta_inf or something like that? meta_inf sounds like the function expects some meta information.

@@ +326,5 @@
>  
> +    def add_failure(self, platform, locale,
> +                    message="%(platform)s:%(locale)s failed.",
> +                    level=ERROR):
> +        s = "%s_%s" % (platform, locale)

Have you thought about moving these methods to one of the parents and make the signature more generic?
something like 
  def add_failure(self, key,
                  message="%(key)s failed.",
		  level=ERROR):

and query failures by key? I have no firm opinion here though, up to you.

@@ +485,5 @@
>                                            'locale': locale})
> +                if not os.path.exists(os.path.join(dirs['abs_work_dir'],
> +                                                   signed_path)):
> +                    self.add_failure(platform, locale,
> +                                     message="Can't sign nonexistent %s:%s apk!" % (platform, locale))

"Can't verify nonexistent %s:%s apk!" I believe.

@@ +498,5 @@
> +                    error_list=verification_error_list
> +                )
> +                if status:
> +                    self.add_failure(platform, locale,
> +                                     message="Errors signing %s:%s apk!" % (platform, locale))

"Errors verifying %s:%s apk!"?

Comment 30

[Aki Sasaki (not active)]

I've got my changes here:
https://github.com/escapewindow/mozharness/commit/02127a500461fe20932c4678cb59ce18fe520322

(In reply to Rail Aliiev [:rail] from comment #29)
> * _sign() may throw an exception. Do you feel comfortable with this or
> prefer to catch it?

I prefer to catch it; fixed.  dump_exception() works well; thanks!

> * config changes should be landed once we done with 11.0b1, correct?

Actually, I designed this patch for 11.0b1, and we'll need a slightly new config for 11.0b2.  However, if you feel more comfortable, I can leave it alone.

> > +    def _sign(self, apk, meta_inf=True, error_list=None):
> 
> Maybe remove_meta_inf or something like that? meta_inf sounds like the
> function expects some meta information.

Yeah.
Renamed to "remove_signature" here:
https://github.com/escapewindow/mozharness/commit/0e9807ec515a60a7df26328c8ac17228c2031e99

> > +    def add_failure(self, platform, locale,
> > +                    message="%(platform)s:%(locale)s failed.",
> > +                    level=ERROR):
> > +        s = "%s_%s" % (platform, locale)
> 
> Have you thought about moving these methods to one of the parents and make
> the signature more generic?
> something like 
>   def add_failure(self, key,
>                   message="%(key)s failed.",
> 		  level=ERROR):
> 
> and query failures by key? I have no firm opinion here though, up to you.

Yeah.
I added it specifically to avoid typo-ing the key, and to allow myself to change the key easily in two places rather than all over the script.
I'm not sure myself; I'm going to leave it in here until we find we want to use it in another script.

> @@ +485,5 @@
> >                                            'locale': locale})
> > +                if not os.path.exists(os.path.join(dirs['abs_work_dir'],
> > +                                                   signed_path)):
> > +                    self.add_failure(platform, locale,
> > +                                     message="Can't sign nonexistent %s:%s apk!" % (platform, locale))
> 
> "Can't verify nonexistent %s:%s apk!" I believe.
> 
> @@ +498,5 @@
> > +                    error_list=verification_error_list
> > +                )
> > +                if status:
> > +                    self.add_failure(platform, locale,
> > +                                     message="Errors signing %s:%s apk!" % (platform, locale))
> 
> "Errors verifying %s:%s apk!"?

Nice catch, fixed.

Comment 31

[Rail Aliiev [:rail]]

(In reply to Aki Sasaki [:aki] from comment #30)
> I've got my changes here:
> https://github.com/escapewindow/mozharness/commit/
> 02127a500461fe20932c4678cb59ce18fe520322


lgtm

Comment 32

[Aki Sasaki (not active)]

Comment on attachment 593212 [details] [diff] [review]
additional signing fixes

http://hg.mozilla.org/build/mozharness/rev/95410ef239f8