Closed
Bug 558464
Opened 14 years ago
Closed 14 years ago
Automate signing
Categories
(Release Engineering :: General, defect, P4)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: coop, Assigned: salbiz)
References
Details
(Whiteboard: [signing][automation])
Attachments
(3 files, 9 obsolete files)
2.61 KB,
patch
|
bhearsum
:
review+
bhearsum
:
checked-in+
|
Details | Diff | Splinter Review |
21.01 KB,
patch
|
bhearsum
:
review+
bhearsum
:
checked-in+
|
Details | Diff | Splinter Review |
1.22 KB,
patch
|
bhearsum
:
review+
catlee
:
checked-in+
|
Details | Diff | Splinter Review |
Signing is one of the few places in release automation where we still require lots of manual intervention. We should fix this.
Comment 1•14 years ago
|
||
One approach that might be easy to do right now: After builds have started, log onto keymaster, set up your signing-work directory, launch signcodepwd and run 'make autosign' 'make autosign' will ask for and verify your gpg and authenticode passphrases, and then enter a loop that periodically runs 'make download' and then verifies that it has all the files it needs before continuing.
Assignee | ||
Comment 2•14 years ago
|
||
Implements the approach of continuously downloading stuff from stage/staging-stage until it determines that all the builds are ready to go. It's a bit tricky to get the platform name mappings to align without introducing a dependency on tools/lib/python, so I've just duplicated those maps in this patch. If we're ok with relying on lib/python/platforms, then I can probably remove some of that code.
Attachment #495582 -
Flags: feedback?(coop)
Attachment #495582 -
Flags: feedback?(catlee)
Comment 4•14 years ago
|
||
Comment on attachment 495582 [details] [diff] [review] run downloads continuously until builds are ready to go. nits: * call the script download_build*s*.py, and put a note in the docstring what it does * you've duplicated the docstring in main() and at the toplevel. * I think you should pass __doc__ to OptionParser()? should we be using 'make download' instead of duplicating the logic inside this script?
Attachment #495582 -
Flags: feedback?(catlee) → feedback+
Reporter | ||
Comment 5•14 years ago
|
||
Comment on attachment 495582 [details] [diff] [review] run downloads continuously until builds are ready to go. (In reply to comment #4) > nits: > * call the script download_build*s*.py, and put a note in the docstring what it > does ++ > should we be using 'make download' instead of duplicating the logic inside this > script? I agree with updating the existing download target. The script will (presumably) perform the same actions as the current rsync if run once all builds are ready.
Attachment #495582 -
Flags: feedback?(coop) → feedback+
Assignee | ||
Comment 6•14 years ago
|
||
Done, this required a small change to the Makefile to prevent the GPG Passphrase prompt coming up every time. Also changed logging as a personal pref, I found it annoying to wade through log messages without time/error contexts.
Attachment #495582 -
Attachment is obsolete: true
Attachment #495825 -
Flags: review?(coop)
Attachment #495825 -
Flags: review?(catlee)
Comment 7•14 years ago
|
||
Comment on attachment 495825 [details] [diff] [review] autosigning-r2 We need to read in the release config to find which platforms are signed instead of hardcoding unsigned/win32 in the platform map. We also need to be checking that the MARs exist, not just the .exe/.tar.bz2/.dmg A few style nits: - Your use of _ or CamelCase is inconsistent for functions in download_builds.py - I think you can check for the partner repack directories outside the locale loop
Attachment #495825 -
Flags: review?(catlee) → review-
Assignee | ||
Comment 8•14 years ago
|
||
Done, additionally I also implemented the checks required to close off bug 614044. Tested on the staging signing machine.
Assignee: nobody → salbiz
Attachment #495825 -
Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #496288 -
Flags: review?(coop)
Attachment #496288 -
Flags: review?(catlee)
Attachment #495825 -
Flags: review?(coop)
Comment 9•14 years ago
|
||
Comment on attachment 496288 [details] [diff] [review] autosign-r3 >+def directory_contains(directory, suffix): >+ """ Return true if the given directory contains the provided wildcard >+ suffix, similar to `ls foo/*bar` """ >+ hit = len([f for f in os.listdir(directory) if >+ f.endswith(suffix)]) > 0 You could probably do 'return any([f.endswith(suffix) for f in os.listdir(directory)])' >+def poll_unsigned_dir(unsigned_dir, locales, allplatforms, signed_platforms): >+ """ Check to see if we've finished downloading all locales and partners >+ for all platforms to the unsigned dir >+ Also check for: >+ - *_info.txt for for all platforms >+ - partner-repacks for all platforms >+ - source bundle/tarball >+ """ >+ if not check_source_bundle(unsigned_dir): >+ log.error("Missing source bundles") >+ return False >+ # check partner repacks and info.txt files >+ for platform in allplatforms: >+ unsigned = 'unsigned' if platform in signed_platforms else '' >+ if not os.path.exists(os.path.join( >+ unsigned_dir, unsigned, >+ '%s_info.txt' % platform)): >+ log.error("Missing info files") >+ return False >+ if not os.path.exists(os.path.join( >+ unsigned_dir, >+ 'unsigned/partner-repacks', Should this be unsigned, 'partner-repacks' instead? Also, do we need to wait for all the platforms to be present, or just the one we're signing right now? >+def main(): >+ """ Parse options and run download loop """ >+ parser = OptionParser(__doc__) >+ parser.set_defaults( >+ product="firefox", >+ hgurl="http://hg.mozilla.org", >+ staging=False, >+ ) >+ parser.add_option("-p", "--product", dest="product", help="product name") >+ parser.add_option("-l", "--hgurl", dest="hgurl", help="hg URL prefix") >+ parser.add_option("-s", "--staging", dest="staging", action="store_true", >+ help="run in staging environment") >+ options, args = parser.parse_args() >+ if len(args) < 7: >+ parser.error("") >+ branch = args[0].lstrip("releases/").rstrip("/") >+ version = args[1] >+ unsigned_dir = args[2] >+ ssh_key = args[3] >+ stage_username = args[4] >+ stage_host = args[5] >+ build = args[6] >+ if not options.staging: >+ prefix = "%s/build/buildbot-configs/raw-file/%s_%s_RELEASE/mozilla/release-firefox-%s.py" >+ else: >+ prefix = "%s/build/buildbot-configs/raw-file/%s_%s_RELEASE/mozilla/staging_release-firefox-%s.py" >+ release_config_url = prefix % ( >+ options.hgurl, >+ options.product.upper(), >+ version.replace(".", "_"), >+ branch, >+ ) >+ shipped_locales_url, allplatforms, signed_platforms = extract_config_info(release_config_url, >+ options.hgurl) >+ shipped_locales = read_shipped_locales(urllib2.urlopen(shipped_locales_url)) >+ while not poll_unsigned_dir(unsigned_dir, shipped_locales, allplatforms, >+ signed_platforms): >+ download_builds(ssh_key, stage_username, stage_host, options.product, >+ version, build) Maybe a small sleep of 10 seconds or so would be handy here. If download_builds is generating errors, it gives you a chance to read it before it scrolls off the top of the terminal.
Assignee | ||
Comment 10•14 years ago
|
||
Done, as discussed, the partner-repacks all live under unsigned/ so, separating per platform wouldn't work. I've added support for limiting downloads to specific platforms, but since atm we want to download all platforms to windows for gpg detached signing, it doesn't change the functionality. Something we may want to investigate in future would be looking for EXEs and MARs and starting code-signing on those, with the linux/macosx downloads running in tandem with win32 codesigning, but that is probably out of scope for this, especially with the new signing web-service on the horizon. Also added a verify-download target that just runs the script through once to check if the deliverables are all there.
Attachment #496288 -
Attachment is obsolete: true
Attachment #496536 -
Flags: review?(coop)
Attachment #496536 -
Flags: review?(catlee)
Attachment #496288 -
Flags: review?(coop)
Attachment #496288 -
Flags: review?(catlee)
Updated•14 years ago
|
Attachment #496536 -
Flags: review?(catlee) → review+
Reporter | ||
Comment 11•14 years ago
|
||
Comment on attachment 496536 [details] [diff] [review] autosign + add verify-download target for 614044 Nothing to add to catlee's comments. It looks like this will take the sting out of bug 618043 for me, enough so that I'm willing to WONTFIX it.
Attachment #496536 -
Flags: review?(coop) → review+
Assignee | ||
Comment 12•14 years ago
|
||
Ran into a couple of problems during my staging run that this patch fixes: - for ffx4 releases, using release-firefox-mozilla-$(branch)s.py doesn't work. Since it gets built off mozilla-central. I think the best way to solve this problem is to just let the user set explicitly where to grab the config - for some reason the builds I was testing with had an en-US.xpi hanging around, which doesn't normally get produced. first-locale special-case, at least until the bug to make one gets fixed. - in the future when mac signing happens we'll need a way to verify deliverables on more than one platform at a time, this required some redesign into how download_builds.py handles platform-specific files.
Attachment #496536 -
Attachment is obsolete: true
Attachment #499422 -
Flags: review?(bhearsum)
Comment 13•14 years ago
|
||
Comment on attachment 499422 [details] [diff] [review] allow user to set release_config to use, skip checking en-Us xpi, look at platforms more intelligently Overall this patch looks good, most of the comments below are about code re-use, the logic is pretty much spot-on: I don't see any reason to use "if staging" anywhere in this patch. The only difference in the Makefile targets is not using $(HGROOT) in staging, which is wrong anyways. Please unify all of these parts. We need to do something better than hardcoding the platforms to download, it already differs per-branch. Since you're reading the release config in download_builds.py, I suggest looking at enUSPlatform and l10nPlatforms in it. Please use the platform maps from build/tools, they're centralized there for a reason! This script lives in build/tools anyways, so you've already got the required files on disk. You should be able to set PYTHONPATH in the Makefile. Some of the download script functions should move to lib/python: directory_contains (somewhere in util/) expected_files (somewhere in release/) read_shipped_locales can be replaced with one of the functions from release/l10n.py. There's also a function in there to generate the URL. Similar goes for the release config, use the method from release/info.py to parse it. Don't give en-US a pass when it comes to missing the XPI, we actually *are* supposed to be generating it, and will be again soon, but only on trunk. I'd recommend looking for it, printing a warning if it's missing, but not failing. That should work across all branches, and be easy to remove once we're at point where we're shipping it everywhere. A couple of nits: - "poll_unsigned_directory" sounds like it's going to poll a remote server, can you call use "check" or something like that instead? - Check for '.complete.mar', may as well be as precise as we can.
Attachment #499422 -
Flags: review?(bhearsum) → review-
Assignee | ||
Comment 14•14 years ago
|
||
Should address comments above. Additionally, noticed during testing that using the deferred method of setting GPGPASS doesn't work properly (it re-asks for it at GPG signing time), so I've implemented a different approach. The makefile now checks for the existence of the environment variable 'SKIP_PASS', and skips the prompt if it's set. To use it manually, you need to export it so that it's inherited by the bash command spawned to read the password. (i.e. export SKIP_PASS=1; make foo, just 'SKIP_PASS=1; make foo' still prompts for the passphrase).
Attachment #499422 -
Attachment is obsolete: true
Attachment #500111 -
Flags: review?(bhearsum)
Comment 15•14 years ago
|
||
Comment on attachment 500111 [details] [diff] [review] refactor to integrate with lib/python >+installer_ext_map = { >+ 'win32' : ".exe", >+ 'win64' : ".exe", >+ 'macosx' : ".dmg", >+ 'macosx64' : ".dmg", >+ 'linux' : ".tar.bz2", >+ 'linux64' : ".tar.bz2", >+} I really like the style that's used in release/platforms.py, please have a getter function for this, so consumers don't use it directly. >+PYTHONPATH := ../../lib/python:${PYTHONPATH} Please copy in this entire directory as part of set-up. We should only rely on things inside the work directory to avoid racing with another signing, as well as being able to dig back in time when debugging. >+ if 'win32' in allplatforms: >+ if not check_source_bundle(unsigned_dir): >+ log.error("Missing source bundles") >+ return False I don't understand the purpose of the "if 'win32'" part of this. Can you explain? >+ if options.platform: >+ # only look at platforms that are also in the release configs >+ allplatforms = [p for p in download_platform_map[options.platform] >+ if p in release_config['enUSPlatforms']] >+ log.info("Looking for builds on : %s" % allplatforms) I don't understand the purpose of download_platform_map, either. Why not just use exactly what's in the release config? Did you see this part of my previous comment? > Don't give en-US a pass when it comes to missing the XPI, we actually *are* supposed to be generating it, and will be again soon, but only on trunk. I'd recommend looking for it, printing a warning if it's missing, but not failing. That should work across all branches, and be easy to remove once we're at point where we're shipping it everywhere. I'm going to give this is a try in staging today. Great work Syed, you're almost done here!
Attachment #500111 -
Flags: review?(bhearsum) → review-
Assignee | ||
Comment 16•14 years ago
|
||
(In reply to comment #15) > Comment on attachment 500111 [details] [diff] [review] > refactor to integrate with lib/python > > >+ if 'win32' in allplatforms: > >+ if not check_source_bundle(unsigned_dir): > >+ log.error("Missing source bundles") > >+ return False > > I don't understand the purpose of the "if 'win32'" part of this. Can you > explain? > The rsync rules only download the source bundle on win32. This is necessary so that it doesn't break when we eventually have more than one signing platform. (Only download/sign/check the source bundle once). > >+ if options.platform: > >+ # only look at platforms that are also in the release configs > >+ allplatforms = [p for p in download_platform_map[options.platform] > >+ if p in release_config['enUSPlatforms']] > >+ log.info("Looking for builds on : %s" % allplatforms) > > I don't understand the purpose of download_platform_map, either. Why not just > use exactly what's in the release config? > Again, this is to future-proof it against multiple signing platforms. If we specifically define the signing platform, we only look for the deliverables in download_platform_map. It's not the cleanest way, but I don't see any other place where this information would be better kept. (signedPlatforms just lists the platforms being codesigned, not which signing platform downloads which set of deliverables). > > Did you see this part of my previous comment? > > Don't give en-US a pass when it comes to missing the XPI, we actually *are* > supposed to be generating it, and will be again soon, but only on trunk. I'd > recommend looking for it, printing a warning if it's missing, but not failing. > That should work across all branches, and be easy to remove once we're at point > where we're shipping it everywhere. > Yep, the call to directoryContains will print a log message warning that en-US is not found, the or clause will prevent it from failing. So to switch that off, we can just remove the 'or' clause when we're shipping it everywhere. This was actually in the previous patch, hence no code-change was required. Patch coming up to address the other issues.
Assignee | ||
Comment 17•14 years ago
|
||
Done, the assumption in the Makefile is that the authoritative version of tools/lib/python lives under signing-utils/tools/lib/python. I will update the signing docs to reflect this.
Attachment #500111 -
Attachment is obsolete: true
Attachment #500202 -
Flags: review?(bhearsum)
Assignee | ||
Comment 18•14 years ago
|
||
Based on IRC conversation, we'll add grabbing lib/python to the manual setup steps to avoid race conditions with two people signing at once.
Attachment #500202 -
Attachment is obsolete: true
Attachment #500234 -
Flags: review?(bhearsum)
Attachment #500202 -
Flags: review?(bhearsum)
Assignee | ||
Comment 19•14 years ago
|
||
Forgot to add this patch as well, it uploads the partner_build_%(platform)s file that the automation looks for at the end of the partner repack.
Attachment #500251 -
Flags: review?(bhearsum)
Comment 20•14 years ago
|
||
Comment on attachment 500251 [details] [diff] [review] upload a file for partner repacks to satisfy download_builds Seems ok to me
Attachment #500251 -
Flags: review?(bhearsum) → review+
Updated•14 years ago
|
Attachment #500234 -
Flags: review?(bhearsum) → review+
Comment 21•14 years ago
|
||
Comment on attachment 500251 [details] [diff] [review] upload a file for partner repacks to satisfy download_builds Landed this on default, will reconfig later.
Assignee | ||
Comment 22•14 years ago
|
||
Attachment #500234 -
Attachment is obsolete: true
Attachment #500346 -
Flags: review?(bhearsum)
Assignee | ||
Comment 23•14 years ago
|
||
d'oh, forgot to include this fix in the last patch.
Attachment #500346 -
Attachment is obsolete: true
Attachment #500354 -
Flags: review?(bhearsum)
Attachment #500346 -
Flags: review?(bhearsum)
Updated•14 years ago
|
Attachment #500354 -
Flags: review?(bhearsum) → review+
Comment 24•14 years ago
|
||
Comment on attachment 500251 [details] [diff] [review] upload a file for partner repacks to satisfy download_builds This made it to the masters earlier today.
Attachment #500251 -
Flags: checked-in+
Comment 25•14 years ago
|
||
Comment on attachment 500354 [details] [diff] [review] exclude partner_build_ files from verify-asc Landed this: changeset: 1965:39f32d0437cf Syed has updated the signing docs with the usage changes.
Attachment #500354 -
Flags: checked-in+
Comment 26•14 years ago
|
||
I believe we're all done here now. Great work, Syed!
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Comment 27•14 years ago
|
||
Attachment #502841 -
Flags: review?(bhearsum)
Updated•14 years ago
|
Attachment #502841 -
Flags: review?(bhearsum) → review+
Comment 28•14 years ago
|
||
Comment on attachment 502841 [details] [diff] [review] bustage fix for default signedPlatforms changeset: 1122:cadf51e8291a
Attachment #502841 -
Flags: checked-in+
Updated•11 years ago
|
Product: mozilla.org → Release Engineering
You need to log in
before you can comment on or make changes to this bug.
Description
•