708656 - Use signing on demand for releases

Status: RESOLVED FIXED

(Release Engineering :: General, defect, P2)

Description

[Chris AtLee [:catlee]]

We need this before Firefox 11 goes to beta.

Comment 1

[Rail Aliiev [:rail]]

Attachment: tools

Comment 2

[Rail Aliiev [:rail]]

Attachment: buildbotcustom

Comment 3

[Rail Aliiev [:rail]]

Attachment: configs

Not final. The final patches will be adjusted depending on which release ig going to be singed.

Comment 4

[Rail Aliiev [:rail]]

Attachment: partner repacks

Comment 5

[Rail Aliiev [:rail]]

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

[Chris Cooper [:coop] (he/him)]

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.

Comment 7

[Rail Aliiev [:rail]]

Attachment: tools

Refreshed patch

Comment 8

[Rail Aliiev [:rail]]

Attachment: configs

Refreshed patch with signing_done template fixed

Comment 9

[Rail Aliiev [:rail]]

Attachment: buildbotcustom

* Refreshed patch.
* AV and perm check run after all deliverables are ready (source, builds, repacks, partner-repacks (if any), updates (for manual signing)).

Comment 10

[[e-mail] (: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.

Comment 11

[[e-mail] (:bhearsum)]

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

[[e-mail] (:bhearsum)]

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

[[e-mail] (:bhearsum)]

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?

Comment 14

[Rail Aliiev [:rail]]

Attachment: partner repacks

(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

Comment 15

[Rail Aliiev [:rail]]

Attachment: configs

(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.

Comment 16

[Rail Aliiev [:rail]]

Attachment: buildbotcustom

(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.

Comment 17

[Rail Aliiev [:rail]]

Attachment: tools

(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.

Comment 18

[Rail Aliiev [:rail]]

Attachment: buildbotcustom

* remove env parameters from SingleSourceFactory and BaseRepackFactory
* should have used changeContainsProduct instead of isHgPollerTriggered

interdiff: https://gist.github.com/1627220

Comment 19

[Rail Aliiev [:rail]]

Attachment: tools

* create contrib directories after generating sums files https://gist.github.com/1627417

Comment 20

[Rail Aliiev [:rail]]

Comment on attachment 588038 [details] [diff] [review]
partner repacks

http://hg.mozilla.org/build/partner-repacks/rev/864ccaa580d0

Comment 21

[Rail Aliiev [:rail]]

Comment on attachment 589212 [details] [diff] [review]
tools

http://hg.mozilla.org/build/tools/rev/5017230a50a3

Comment 22

[Rail Aliiev [:rail]]

Comment on attachment 588121 [details] [diff] [review]
configs

http://hg.mozilla.org/build/buildbot-configs/rev/d12a039c9a1a

Comment 23

[Rail Aliiev [:rail]]

Comment on attachment 589202 [details] [diff] [review]
buildbotcustom

http://hg.mozilla.org/build/buildbotcustom/rev/35fd5683abb4

Comment 24

[Rail Aliiev [:rail]]

* 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.

Comment 25

[Rail Aliiev [:rail]]

Attachment: fix env in UnittestBuildFactory

Comment 26

[Rail Aliiev [:rail]]

Attachment: fix env in UnittestPackagedBuildFactory

Comment 27

[Rail Aliiev [:rail]]

Attachment: fix env in UnittestPackagedBuildFactory

more readable one

Comment 28

[Rail Aliiev [:rail]]

Comment on attachment 589520 [details] [diff] [review]
fix env in UnittestBuildFactory

http://hg.mozilla.org/build/buildbotcustom/rev/fd28fda117f3

Comment 29

[Rail Aliiev [:rail]]

Attachment: Fix failures

This actually revert attachment 589520 [details] [diff] [review] and passes env to the parent.

Comment 30

[Rail Aliiev [:rail]]

Comment on attachment 589557 [details] [diff] [review]
Fix failures

http://hg.mozilla.org/build/buildbotcustom/rev/2bf271a746e7

Comment 31

[Rail Aliiev [:rail]]

Comment on attachment 589543 [details] [diff] [review]
fix env in UnittestPackagedBuildFactory

http://hg.mozilla.org/build/buildbotcustom/rev/1ff40491f48a

Comment 32

[Rail Aliiev [:rail]]

Attachment: use AggregatingScheduler for repacks_complete builder

It would be better to trigger repacks_done when some of repacks fail and forced. To be tested.

Comment 33

[Rail Aliiev [:rail]]

Comment on attachment 589657 [details] [diff] [review]
use AggregatingScheduler for repacks_complete builder

Looks fine in staging.

Comment 34

[Rail Aliiev [:rail]]

Comment on attachment 589657 [details] [diff] [review]
use AggregatingScheduler for repacks_complete builder

http://hg.mozilla.org/build/buildbotcustom/rev/2d017c0f2359

Comment 35

[Mike Taylor [:bear]]

landed in production with tonights reconfig

Comment 36

[Rail Aliiev [:rail]]

Available in 11.0b1