Closed Bug 708656 Opened 11 years ago Closed 11 years ago

Use signing on demand for releases

Categories

(Release Engineering :: General, defect, P2)

defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: catlee, Assigned: rail)

References

Details

(Whiteboard: [release-process-improvement][automation][signing])

Attachments

(8 files, 10 obsolete files)

10.57 KB, patch
bhearsum
: review+
Details | Diff | Splinter Review
14.06 KB, patch
bhearsum
: review+
Details | Diff | Splinter Review
99.09 KB, patch
bhearsum
: review+
Details | Diff | Splinter Review
42.26 KB, patch
bhearsum
: review+
Details | Diff | Splinter Review
2.06 KB, patch
bhearsum
: review+
Details | Diff | Splinter Review
1.93 KB, patch
bhearsum
: review+
Details | Diff | Splinter Review
2.32 KB, patch
catlee
: review+
Details | Diff | Splinter Review
1.41 KB, patch
bhearsum
: review+
Details | Diff | Splinter Review
We need this before Firefox 11 goes to beta.
Depends on: 710198
Depends on: 710199
Depends on: 710200
Depends on: 710221
Priority: -- → P3
Whiteboard: [release-process-improvement][automation][signing]
Depends on: 711309
Depends on: 715119
Depends on: 715120
Depends on: 715586
Assignee: nobody → rail
Priority: P3 → P2
Attached patch tools (obsolete) — Splinter Review
Attachment #587311 - Flags: review?(bhearsum)
Attached patch buildbotcustom (obsolete) — Splinter Review
Attachment #587313 - Flags: review?(bhearsum)
Attached patch configs (obsolete) — Splinter Review
Not final. The final patches will be adjusted depending on which release ig going to be singed.
Attachment #587314 - Flags: feedback?(bhearsum)
Attached patch partner repacks (obsolete) — Splinter Review
Attachment #587315 - Flags: review?(coop)
Attachment #587315 - Flags: review?(bhearsum)
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 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+
Attached patch tools (obsolete) — Splinter Review
Refreshed patch
Attachment #587311 - Attachment is obsolete: true
Attachment #587311 - Flags: review?(bhearsum)
Attachment #587648 - Flags: review?(bhearsum)
Attached patch configs (obsolete) — Splinter Review
Refreshed patch with signing_done template fixed
Attachment #587314 - Attachment is obsolete: true
Attachment #587314 - Flags: feedback?(bhearsum)
Attachment #587649 - Flags: feedback?(bhearsum)
Attached patch buildbotcustom (obsolete) — Splinter Review
* 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 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 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 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 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-
Attached patch partner repacksSplinter Review
(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)
Attached patch configsSplinter Review
(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)
Attached patch buildbotcustom (obsolete) — Splinter Review
(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)
Attached patch tools (obsolete) — Splinter Review
(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)
Attachment #588038 - Flags: review?(bhearsum) → review+
Attachment #588121 - Flags: review?(bhearsum) → review+
Attachment #588128 - Flags: review?(bhearsum) → review+
Attachment #588133 - Flags: review?(bhearsum) → review+
Blocks: 718460
Attached patch buildbotcustomSplinter Review
* 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)
Attached patch toolsSplinter Review
* create contrib directories after generating sums files https://gist.github.com/1627417
Attachment #588133 - Attachment is obsolete: true
Attachment #589212 - Flags: review?(bhearsum)
Blocks: 718777
Depends on: 718804
Attachment #589202 - Flags: review?(bhearsum) → review+
Attachment #589212 - Flags: review?(bhearsum) → review+
* 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.
Attachment #589520 - Flags: review?(bhearsum)
Attachment #589520 - Flags: review?(bhearsum) → review+
Attachment #589539 - Flags: review?(bhearsum)
more readable one
Attachment #589539 - Attachment is obsolete: true
Attachment #589539 - Flags: review?(bhearsum)
Attachment #589543 - Flags: review?(bhearsum)
Attachment #589543 - Flags: review?(bhearsum) → review+
Attached patch Fix failuresSplinter Review
This actually revert attachment 589520 [details] [diff] [review] and passes env to the parent.
Attachment #589557 - Flags: review?(catlee)
Attachment #589557 - Flags: review?(catlee) → review+
Comment on attachment 589543 [details] [diff] [review]
fix env in UnittestPackagedBuildFactory

http://hg.mozilla.org/build/buildbotcustom/rev/1ff40491f48a
Attachment #589543 - Flags: checked-in+
It would be better to trigger repacks_done when some of repacks fail and forced. To be tested.
Comment on attachment 589657 [details] [diff] [review]
use AggregatingScheduler for repacks_complete builder

Looks fine in staging.
Attachment #589657 - Flags: review?(bhearsum)
Attachment #589657 - Flags: review?(bhearsum) → review+
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+
landed in production with tonights reconfig
Available in 11.0b1
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Duplicate of this bug: 710199
Product: mozilla.org → Release Engineering
You need to log in before you can comment on or make changes to this bug.