Use signing on demand for releases

RESOLVED FIXED

Status

P2
normal
RESOLVED FIXED
7 years ago
5 years ago

People

(Reporter: catlee, Assigned: rail)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(8 attachments, 10 obsolete attachments)

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
(Reporter)

Description

7 years ago
We need this before Firefox 11 goes to beta.
(Assignee)

Updated

7 years ago
Depends on: 710198
(Assignee)

Updated

7 years ago
Depends on: 710199
(Assignee)

Updated

7 years ago
Depends on: 710200
(Assignee)

Updated

7 years ago
Depends on: 710221
Blocks: 710312
Priority: -- → P3
Whiteboard: [release-process-improvement][automation][signing]
(Assignee)

Updated

7 years ago
Depends on: 711309
(Assignee)

Updated

7 years ago
Depends on: 715119
(Assignee)

Updated

7 years ago
Depends on: 715120
(Assignee)

Updated

7 years ago
Depends on: 715586
(Assignee)

Updated

7 years ago
Assignee: nobody → rail
Priority: P3 → P2
(Assignee)

Comment 1

7 years ago
Created attachment 587311 [details] [diff] [review]
tools
Attachment #587311 - Flags: review?(bhearsum)
(Assignee)

Comment 2

7 years ago
Created attachment 587313 [details] [diff] [review]
buildbotcustom
Attachment #587313 - Flags: review?(bhearsum)
(Assignee)

Comment 3

7 years ago
Created attachment 587314 [details] [diff] [review]
configs

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

7 years ago
Created attachment 587315 [details] [diff] [review]
partner repacks
Attachment #587315 - Flags: review?(coop)
Attachment #587315 - Flags: review?(bhearsum)
(Assignee)

Comment 5

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

7 years ago
Created attachment 587648 [details] [diff] [review]
tools

Refreshed patch
Attachment #587311 - Attachment is obsolete: true
Attachment #587311 - Flags: review?(bhearsum)
Attachment #587648 - Flags: review?(bhearsum)
(Assignee)

Comment 8

7 years ago
Created attachment 587649 [details] [diff] [review]
configs

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

7 years ago
Created attachment 587650 [details] [diff] [review]
buildbotcustom

* 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-
(Assignee)

Comment 14

7 years ago
Created attachment 588038 [details] [diff] [review]
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
Attachment #587315 - Attachment is obsolete: true
Attachment #588038 - Flags: review?(bhearsum)
(Assignee)

Comment 15

7 years ago
Created attachment 588121 [details] [diff] [review]
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.
Attachment #587649 - Attachment is obsolete: true
Attachment #587649 - Flags: feedback?(bhearsum)
Attachment #588121 - Flags: review?(bhearsum)
(Assignee)

Comment 16

7 years ago
Created attachment 588128 [details] [diff] [review]
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.
Attachment #587650 - Attachment is obsolete: true
Attachment #587650 - Flags: review?(bhearsum)
Attachment #588128 - Flags: review?(bhearsum)
(Assignee)

Comment 17

7 years ago
Created attachment 588133 [details] [diff] [review]
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.
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+
(Assignee)

Updated

7 years ago
Blocks: 718460
(Assignee)

Comment 18

7 years ago
Created attachment 589202 [details] [diff] [review]
buildbotcustom

* 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

7 years ago
Created attachment 589212 [details] [diff] [review]
tools

* create contrib directories after generating sums files https://gist.github.com/1627417
Attachment #588133 - Attachment is obsolete: true
Attachment #589212 - Flags: review?(bhearsum)
(Assignee)

Updated

7 years ago
Blocks: 718777
(Assignee)

Updated

7 years ago
Depends on: 718804
Attachment #589202 - Flags: review?(bhearsum) → review+
Attachment #589212 - Flags: review?(bhearsum) → review+
(Assignee)

Comment 24

7 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

7 years ago
Created attachment 589520 [details] [diff] [review]
fix env in UnittestBuildFactory
Attachment #589520 - Flags: review?(bhearsum)
Attachment #589520 - Flags: review?(bhearsum) → review+
(Assignee)

Comment 26

7 years ago
Created attachment 589539 [details] [diff] [review]
fix env in UnittestPackagedBuildFactory
Attachment #589539 - Flags: review?(bhearsum)
(Assignee)

Comment 27

7 years ago
Created attachment 589543 [details] [diff] [review]
fix env in UnittestPackagedBuildFactory

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+
(Assignee)

Comment 29

7 years ago
Created attachment 589557 [details] [diff] [review]
Fix failures

This actually revert attachment 589520 [details] [diff] [review] and passes env to the parent.
Attachment #589557 - Flags: review?(catlee)
(Reporter)

Updated

7 years ago
Attachment #589557 - Flags: review?(catlee) → review+
(Assignee)

Comment 31

7 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

7 years ago
Created attachment 589657 [details] [diff] [review]
use AggregatingScheduler for repacks_complete builder

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

Comment 33

7 years ago
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+
(Assignee)

Comment 34

7 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

7 years ago
landed in production with tonights reconfig
(Assignee)

Comment 36

7 years ago
Available in 11.0b1
Status: NEW → RESOLVED
Last Resolved: 7 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.