Closed
Bug 708656
Opened 13 years ago
Closed 13 years ago
Use signing on demand for releases
Categories
(Release Engineering :: General, defect, P2)
Release Engineering
General
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: catlee, Assigned: rail)
References
Details
(Whiteboard: [release-process-improvement][automation][signing])
Attachments
(8 files, 10 obsolete files)
(deleted),
patch
|
bhearsum
:
review+
rail
:
checked-in+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bhearsum
:
review+
rail
:
checked-in+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bhearsum
:
review+
rail
:
checked-in+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bhearsum
:
review+
rail
:
checked-in+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bhearsum
:
review+
rail
:
checked-in+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bhearsum
:
review+
rail
:
checked-in+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
catlee
:
review+
rail
:
checked-in+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bhearsum
:
review+
rail
:
checked-in+
|
Details | Diff | Splinter Review |
We need this before Firefox 11 goes to beta.
Updated•13 years ago
|
Priority: -- → P3
Whiteboard: [release-process-improvement][automation][signing]
Assignee | ||
Updated•13 years ago
|
Assignee: nobody → rail
Priority: P3 → P2
Assignee | ||
Comment 1•13 years ago
|
||
Attachment #587311 -
Flags: review?(bhearsum)
Assignee | ||
Comment 2•13 years ago
|
||
Attachment #587313 -
Flags: review?(bhearsum)
Assignee | ||
Comment 3•13 years ago
|
||
Not final. The final patches will be adjusted depending on which release ig going to be singed.
Attachment #587314 -
Flags: feedback?(bhearsum)
Assignee | ||
Comment 4•13 years ago
|
||
Attachment #587315 -
Flags: review?(coop)
Attachment #587315 -
Flags: review?(bhearsum)
Assignee | ||
Comment 5•13 years ago
|
||
Test results in staging:
* 10.0 release with signing and partial mars enabled
* Still need to gpg sign xulrunner, but this is not a showstopper
* everything else passed well
* 10.0b4 using old process (manual signing). Everything looks good
Still to be tested:
* 10.0 release with disabled singing and partial mars, mostly to test partner repacks
* 3.6.x release
Comment 6•13 years ago
|
||
Comment on attachment 587315 [details] [diff] [review]
partner repacks
Review of attachment 587315 [details] [diff] [review]:
-----------------------------------------------------------------
Looks good to me. Yay for signing-on-demand.
Attachment #587315 -
Flags: review?(coop) → review+
Assignee | ||
Comment 7•13 years ago
|
||
Refreshed patch
Attachment #587311 -
Attachment is obsolete: true
Attachment #587311 -
Flags: review?(bhearsum)
Attachment #587648 -
Flags: review?(bhearsum)
Assignee | ||
Comment 8•13 years ago
|
||
Refreshed patch with signing_done template fixed
Attachment #587314 -
Attachment is obsolete: true
Attachment #587314 -
Flags: feedback?(bhearsum)
Attachment #587649 -
Flags: feedback?(bhearsum)
Assignee | ||
Comment 9•13 years ago
|
||
* Refreshed patch.
* AV and perm check run after all deliverables are ready (source, builds, repacks, partner-repacks (if any), updates (for manual signing)).
Attachment #587313 -
Attachment is obsolete: true
Attachment #587313 -
Flags: review?(bhearsum)
Attachment #587650 -
Flags: review?(bhearsum)
Comment 10•13 years ago
|
||
Comment on attachment 587315 [details] [diff] [review]
partner repacks
Review of attachment 587315 [details] [diff] [review]:
-----------------------------------------------------------------
::: scripts/partner-repacks.py
@@ +366,5 @@
> pass
>
> + def signBuild(self):
> + assert isinstance(self.signing_formats, (list, tuple))
> + signing_cmd = os.environ['MOZ_SIGN_CMD']
I'd like to see this put somewhere near the option parsing code, and passed in. It's a bit weird to have the worker class looking at the environment. Everything here looks fine otherwise.
Attachment #587315 -
Flags: review?(bhearsum) → review-
Comment 11•13 years ago
|
||
Comment on attachment 587649 [details] [diff] [review]
configs
Review of attachment 587649 [details] [diff] [review]:
-----------------------------------------------------------------
::: mozilla/release-firefox-mozilla-beta.py
@@ +131,5 @@
>
> # Misc configuration
> releaseConfig['enable_repo_setup'] = False
> releaseConfig['enableAutomaticPushToMirrors'] = True
> +releaseConfig['enableSigning'] = True
This flag actually means "enable signing on demand" or "enable signing at build time", so we should try to find a name that reflects this.
::: mozilla/staging_release-firefox-mozilla-release.py
@@ +136,5 @@
> # Misc configuration
> releaseConfig['enable_repo_setup'] = False
> releaseConfig['build_tools_repo_path'] = "users/stage-ffxbld/tools"
> +releaseConfig['enableSigning'] = True
> +releaseConfig['generatePartials'] = True
Similar thing with generatePartials.
What if we did something like enableSigningAtBuildTime at enablePartialMarsAtBuildTime, and default them to True? That way we have explicitly named things, but we don't have to clutter the majority of the configs with them.
Comment 12•13 years ago
|
||
Comment on attachment 587650 [details] [diff] [review]
buildbotcustom
Review of attachment 587650 [details] [diff] [review]:
-----------------------------------------------------------------
::: process/factory.py
@@ +398,5 @@
> self.baseMirrorUrls = baseMirrorUrls
> self.baseBundleUrls = baseBundleUrls
> + self.signingServers = signingServers
> + self.enableSigning = enableSigning
> + self.env = env.copy()
Have you run the standard builders (leak test/opt/etc) after making this change? I know it seems simple, but it touches a lot of things, so who knows what could break! I also think you need to adjust BaseRepackFactory so that it doesn't have env in its args, too.
@@ +889,5 @@
> ( self.stageServer, self.stageProduct, self.logUploadDir)
>
> + if self.signingServers:
> + self.env['MOZ_SIGN_CMD'] = WithProperties(get_signing_cmd(
> + self.signingServers, self.env.get('PYTHON26')))
While it's pretty trivial, it's probably good to factor this out to MozillaBuildFactory, as its repeated a few different times.
@@ +2661,5 @@
> fileType=None, maxDepth=1, haltOnFailure=False):
> # We don't need to do this for release builds.
> pass
>
> + def addCreatePartialUpdateSteps(self):
As mentioned in-person, I'd really like to make the build system capable of creating partial MARs with MOZ_PKG_PRETTYNAMES=1, which would simplify this a bunch. That's not a core part of this, of course, but can you file a bug on it?
::: process/release.py
@@ +82,5 @@
> all_slaves = [x for x in set(all_slaves)]
>
> + signedPlatforms = ()
> + if not releaseConfig.get('enableSigning'):
> + signedPlatforms = releaseConfig.get('signedPlatforms', ('win32',))
The fact that we have to keep the signedPlatforms variable still kindof sucks, but given that it goes away with 3.6, I'm willing to live with it.
@@ +1108,5 @@
> + 'script_repo_revision': releaseTag,
> + 'release_config': releaseConfigFile,
> + }
> + })
> + post_signing_builders.append(
Shouldn't these be a post_deliverables builder?
@@ +1522,5 @@
> reallyShort(builderPrefix('android_verify_sig')),
> },
> })
>
> + # AggregatingSchedulers
How do you feel about moving the other Schedulers / ChangeSources down here, too? If possible, I think it makes sense to keep them all together.
Comment 13•13 years ago
|
||
Comment on attachment 587648 [details] [diff] [review]
tools
Review of attachment 587648 [details] [diff] [review]:
-----------------------------------------------------------------
r=me with the mar downloading changed and standalone_repacks.sh change.
::: lib/python/build/l10n.py
@@ +76,5 @@
> if sys.platform.startswith('darwin'):
> env["MOZ_PKG_PLATFORM"] = "mac"
> run_cmd(["make", "installers-%s" % locale], cwd=localeSrcDir, env=env)
> + UPLOAD_EXTRA_FILES = []
> + if prevMar:
As with the buildbotcustom patch, I would prefer that this be calling build system targets rather than doing all this work itself, but it can be addressed in a follow-up bug.
::: lib/python/release/download.py
@@ +69,5 @@
> + remote_f = urlopen(url)
> + local_f = open(destFileName, "wb")
> + local_f.write(remote_f.read())
> + local_f.close()
> + except HTTPError, e:
Given that this is a generic "download a MAR" function I don't think you should be handling the 404 error here. Move that out to the calling function, so that this function is useful in situations where we don't want this exception, too.
::: scripts/l10n/standalone_repacks.sh
@@ +17,5 @@
> platform=$1
> branchConfig=$2
> +generatePartials=$3
> +if [ "x$generatePartials" != "x" ]; then
> + generatePartials="--generate-partials"
This is a bit weird...can you make it require a specific argument instead of anything at all?
Attachment #587648 -
Flags: review?(bhearsum) → review-
Assignee | ||
Comment 14•13 years ago
|
||
(In reply to Ben Hearsum [:bhearsum] from comment #10)
> Comment on attachment 587315 [details] [diff] [review]
> partner repacks
>
> Review of attachment 587315 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> ::: scripts/partner-repacks.py
> @@ +366,5 @@
> > pass
> >
> > + def signBuild(self):
> > + assert isinstance(self.signing_formats, (list, tuple))
> > + signing_cmd = os.environ['MOZ_SIGN_CMD']
>
> I'd like to see this put somewhere near the option parsing code, and passed
> in. It's a bit weird to have the worker class looking at the environment.
> Everything here looks fine otherwise.
Sure. Interdiff is here: https://gist.github.com/1601395
Attachment #587315 -
Attachment is obsolete: true
Attachment #588038 -
Flags: review?(bhearsum)
Assignee | ||
Comment 15•13 years ago
|
||
(In reply to Ben Hearsum [:bhearsum] from comment #11)
> What if we did something like enableSigningAtBuildTime at
> enablePartialMarsAtBuildTime, and default them to True? That way we have
> explicitly named things, but we don't have to clutter the majority of the
> configs with them.
Done.
Attachment #587649 -
Attachment is obsolete: true
Attachment #587649 -
Flags: feedback?(bhearsum)
Attachment #588121 -
Flags: review?(bhearsum)
Assignee | ||
Comment 16•13 years ago
|
||
(In reply to Ben Hearsum [:bhearsum] from comment #12)
> Have you run the standard builders (leak test/opt/etc) after making this
> change? I know it seems simple, but it touches a lot of things, so who knows
> what could break! I also think you need to adjust BaseRepackFactory so that
> it doesn't have env in its args, too.
That's in my TODO list. I wanted to polish the release process first then to test the rest.
> While it's pretty trivial, it's probably good to factor this out to
> MozillaBuildFactory, as its repeated a few different times.
Optimization time! Done.
> As mentioned in-person, I'd really like to make the build system capable of
> creating partial MARs with MOZ_PKG_PRETTYNAMES=1, which would simplify this
> a bunch. That's not a core part of this, of course, but can you file a bug
> on it?
Yeah. :/ Bug 717668 filed. In any case the outcome will be replacing one ShellCommand with another one since in the current implementation make partial-patch is just a wrapper around tools/update-packaging/make_incremental_update.sh and expects that we downloaded the prev mar files and unpacked both mars before running that target. :/
> The fact that we have to keep the signedPlatforms variable still kindof
> sucks, but given that it goes away with 3.6, I'm willing to live with it.
My TODO says that I should fix this once we EOL 3.6. :)
> > + post_signing_builders.append(
>
> Shouldn't these be a post_deliverables builder?
Oooops. Right.
> How do you feel about moving the other Schedulers / ChangeSources down here,
> too? If possible, I think it makes sense to keep them all together.
Sure!
interdiff is here: https://gist.github.com/1602483. I explicitly disabled signing for xulrunner for now. However, I have that in my TODO, but I don't think that this should be a stopper for now.
Attachment #587650 -
Attachment is obsolete: true
Attachment #587650 -
Flags: review?(bhearsum)
Attachment #588128 -
Flags: review?(bhearsum)
Assignee | ||
Comment 17•13 years ago
|
||
(In reply to Ben Hearsum [:bhearsum] from comment #13)
> Given that this is a generic "download a MAR" function I don't think you
> should be handling the 404 error here. Move that out to the calling
> function, so that this function is useful in situations where we don't want
> this exception, too.
I added a wrapper function downloadUpdateIgnore404 to simplify its usage from retry.
> > +if [ "x$generatePartials" != "x" ]; then
> > + generatePartials="--generate-partials"
>
> This is a bit weird...can you make it require a specific argument instead of
> anything at all?
OK.
interdiff is https://gist.github.com/1602475. Additionally I made downloadChecksums and uploadChecksums functions more generic and renamed them.
So far everything passes reconfig, but I still need to run a couple of releases to make sure.
Attachment #587648 -
Attachment is obsolete: true
Attachment #588133 -
Flags: review?(bhearsum)
Updated•13 years ago
|
Attachment #588038 -
Flags: review?(bhearsum) → review+
Updated•13 years ago
|
Attachment #588121 -
Flags: review?(bhearsum) → review+
Updated•13 years ago
|
Attachment #588128 -
Flags: review?(bhearsum) → review+
Updated•13 years ago
|
Attachment #588133 -
Flags: review?(bhearsum) → review+
Assignee | ||
Comment 18•13 years ago
|
||
* remove env parameters from SingleSourceFactory and BaseRepackFactory
* should have used changeContainsProduct instead of isHgPollerTriggered
interdiff: https://gist.github.com/1627220
Attachment #588128 -
Attachment is obsolete: true
Attachment #589202 -
Flags: review?(bhearsum)
Assignee | ||
Comment 19•13 years ago
|
||
* create contrib directories after generating sums files https://gist.github.com/1627417
Attachment #588133 -
Attachment is obsolete: true
Attachment #589212 -
Flags: review?(bhearsum)
Updated•13 years ago
|
Attachment #589202 -
Flags: review?(bhearsum) → review+
Updated•13 years ago
|
Attachment #589212 -
Flags: review?(bhearsum) → review+
Assignee | ||
Comment 20•13 years ago
|
||
Comment on attachment 588038 [details] [diff] [review]
partner repacks
http://hg.mozilla.org/build/partner-repacks/rev/864ccaa580d0
Attachment #588038 -
Flags: checked-in+
Assignee | ||
Comment 21•13 years ago
|
||
Comment on attachment 589212 [details] [diff] [review]
tools
http://hg.mozilla.org/build/tools/rev/5017230a50a3
Attachment #589212 -
Flags: checked-in+
Assignee | ||
Comment 22•13 years ago
|
||
Comment on attachment 588121 [details] [diff] [review]
configs
http://hg.mozilla.org/build/buildbot-configs/rev/d12a039c9a1a
Attachment #588121 -
Flags: checked-in+
Assignee | ||
Comment 23•13 years ago
|
||
Comment on attachment 589202 [details] [diff] [review]
buildbotcustom
http://hg.mozilla.org/build/buildbotcustom/rev/35fd5683abb4
Attachment #589202 -
Flags: checked-in+
Assignee | ||
Comment 24•13 years ago
|
||
* Merged to production.
* Had to fix mysql related bustage in AggregatingScheduler: bug 719010
* landed post_upload.py on stage.m.o. pp and dev stage machines picked the change automatically.
Assignee | ||
Comment 25•13 years ago
|
||
Attachment #589520 -
Flags: review?(bhearsum)
Updated•13 years ago
|
Attachment #589520 -
Flags: review?(bhearsum) → review+
Assignee | ||
Comment 26•13 years ago
|
||
Attachment #589539 -
Flags: review?(bhearsum)
Assignee | ||
Comment 27•13 years ago
|
||
more readable one
Attachment #589539 -
Attachment is obsolete: true
Attachment #589539 -
Flags: review?(bhearsum)
Attachment #589543 -
Flags: review?(bhearsum)
Assignee | ||
Comment 28•13 years ago
|
||
Comment on attachment 589520 [details] [diff] [review]
fix env in UnittestBuildFactory
http://hg.mozilla.org/build/buildbotcustom/rev/fd28fda117f3
Attachment #589520 -
Flags: checked-in+
Updated•13 years ago
|
Attachment #589543 -
Flags: review?(bhearsum) → review+
Assignee | ||
Comment 29•13 years ago
|
||
This actually revert attachment 589520 [details] [diff] [review] and passes env to the parent.
Attachment #589557 -
Flags: review?(catlee)
Reporter | ||
Updated•13 years ago
|
Attachment #589557 -
Flags: review?(catlee) → review+
Assignee | ||
Comment 30•13 years ago
|
||
Comment on attachment 589557 [details] [diff] [review]
Fix failures
http://hg.mozilla.org/build/buildbotcustom/rev/2bf271a746e7
Attachment #589557 -
Flags: checked-in+
Assignee | ||
Comment 31•13 years ago
|
||
Comment on attachment 589543 [details] [diff] [review]
fix env in UnittestPackagedBuildFactory
http://hg.mozilla.org/build/buildbotcustom/rev/1ff40491f48a
Attachment #589543 -
Flags: checked-in+
Assignee | ||
Comment 32•13 years ago
|
||
It would be better to trigger repacks_done when some of repacks fail and forced. To be tested.
Assignee | ||
Comment 33•13 years ago
|
||
Comment on attachment 589657 [details] [diff] [review]
use AggregatingScheduler for repacks_complete builder
Looks fine in staging.
Attachment #589657 -
Flags: review?(bhearsum)
Updated•13 years ago
|
Attachment #589657 -
Flags: review?(bhearsum) → review+
Assignee | ||
Comment 34•13 years ago
|
||
Comment on attachment 589657 [details] [diff] [review]
use AggregatingScheduler for repacks_complete builder
http://hg.mozilla.org/build/buildbotcustom/rev/2d017c0f2359
Attachment #589657 -
Flags: checked-in+
Comment 35•13 years ago
|
||
landed in production with tonights reconfig
Assignee | ||
Comment 36•13 years ago
|
||
Available in 11.0b1
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
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
•