462143 - Move BumpVerifyConfig from Bootstrap into an external script

Status: RESOLVED FIXED

(Release Engineering :: General, defect, P2)

Comment 1

[[e-mail] (:bhearsum)]

Attachment: move updateverify to an external script

This is mostly just a copy out of BumpVerifyConfig. The only added feature to that function is the handling of long filenames for the "to" parameter - which we need for Mercurial based releases.

Unit tests included!

Bootstrap patch upcoming.

Comment 2

[[e-mail] (:bhearsum)]

Attachment: Bootstrap changes to pull update verify bumper from HG tools

This patch is pretty straightforward I think. Before I land it I'll need to make sure Mercurial is on all of our release machines.

Comment 3

[[e-mail] (:bhearsum)]

Comment on attachment 345719 [details] [diff] [review]
Bootstrap changes to pull update verify bumper from HG tools

sigh. I need to update this to pull update verify scripts from Mercurial.

Comment 4

[Nick Thomas [:nthomas] (UTC+12)]

Comment on attachment 345718 [details] [diff] [review]
move updateverify to an external script

Could I see the whole file SVP

Comment 5

[[e-mail] (:bhearsum)]

Attachment: the entire script this time

Sorry about that. Here's the whole script.

Comment 6

[[e-mail] (:bhearsum)]

Attachment: factor hg clone/push into Step.pm

When I started trying to do pushes in Updates.pm I realized that having a single value for hgToolsRepo was proving troublesome for pushes and pulls. I factored this stuff out into Step.pm similar to how CVS is factored out.

The incoming Updates.pm patch depends on this one.

Comment 7

[[e-mail] (:bhearsum)]

Attachment: [checked in] have Bootstrap call the new bumping script and remove its internal bumping logic

This is pretty similar to my PatcherConfig.pm patch from a couple weeks ago. Most of this is just moving $var = $config->Get(var => var) to Verify() and pulling/pushing from hg instead of CVS.

Remember that 'hg push' on win32 problem I had awhile ago? It turns out the reason it was hanging was because the hg.m.o ssh key wasn't accepted - and MSYS was mangling the prompt for it. After I accepted the key this worked fine. I've got to go around and do that on all of our machines before the 3.1b2 release.

Once again, thanks so much for all the reviewing you've been doing lately.

Comment 8

[[e-mail] (:bhearsum)]

Note to self: Make sure Mercurial is installed on all 1.8/1.9 release machines before landing the Bootstrap patch.

Comment 9

[[e-mail] (:bhearsum)]

Comment on attachment 346662 [details] [diff] [review]
the entire script this time

found a couple bugs in this...new version coming soon.

Comment 10

[[e-mail] (:bhearsum)]

Attachment: same as before, fix pretty-candidates-dir on linux

Comment 11

[Nick Thomas [:nthomas] (UTC+12)]

Comment on attachment 346889 [details] [diff] [review]
same as before, fix pretty-candidates-dir on linux

>diff -r 36d3117cd0c4 -r 9b12e7abac88 release/update-verify-bump.pl
...
>+  --version/-v            The current version of the product.
>+  --app-version           The current app version of the product. If not passed,
>+                          is assumed to be the same as --version.

I always think of version as the current release name, whether that's 3.1b2, 3.1rc1 or 3.1.2, rather than the version in the product (oh yes, the naming sucks). Maybe put in examples to clarify the difference between version and app-version. Similarly the old version case.

>+  --pretty-candidates-dir When passed, the "to" field in the verify config will
>+  --help                  This usage message
>+  --run-tests             Run the (very basic) unit tests include with this

Nit: These three don't take arguments while all the others do, but you can't tell that from the help text.

>+sub ExecuteTest {
...
>+    # Create a verify config

Nit: for readability, you could append " to bump" here

>+    # BumpVerifyConfig removes everything after 'from' for the previous release
>+    # So must we in the reference file.

Nit: For readability, you could add something like 
  # Prepare the reference config
before this.

Comment 12

[Nick Thomas [:nthomas] (UTC+12)]

Comment on attachment 346665 [details] [diff] [review]
[checked in] have Bootstrap call the new bumping script and remove its internal bumping logic

>Index: Bootstrap/Step/Updates.pm

>+    $this->CvsCo(
>+      cvsroot => $mozillaCvsroot,
>+      modules => [CvsCatfile('mozilla', $appName, 'locales',
>+                             'shipped-locales')],
>+      tag => $branchTag,
>+      workDir => $verifyDirVersion,
>+      checkoutDir => 'locales'
>+    );

Need to check out the shipped-locales for the previous release, so not $branchTag but
  uc($product).'_'.$oldVersion.'_RELEASE';
Otherwise we'll get false positives/negatives when the locale list changes. r+ with that fixed.

Have we tried this stuff in 1.8 staging ? Hopefully we can start using it for 2.0.0.19/3.0.5.

Comment 13

[Nick Thomas [:nthomas] (UTC+12)]

Comment on attachment 346663 [details] [diff] [review]
factor hg clone/push into Step.pm

>Index: Bootstrap/Step.pm
>===================================================================
>+use File::Basename;
>+use File::Copy qw(copy);

No longer needed ?

>+sub HgClone {
>+    # we always pull over https://
>+    $repo = 'https://' . $repo;

Does the python install on the 1.8 and 1.9 machines support this ? Thought we had trouble in buildbot land.

>+    $this->Shell(
>+      cmd => 'hg',
>+      cmdArgs => ['clone', $repo],
>+      dir => $workDir
>+    );

Is this going to require we delete the pull before rerunning (eg) an update verify ? Perhaps we can do a dir existence test for filename($repo) and use rmtree.

>+sub HgPush {

One fine day we should handle other commits between our pull and push. hg incoming seems to have exit status of 1 when there's nothing to pull, 0 otherwise.

>+    # Required arguments
>+    die "ASSERT: Bootstrap::Util::HgClone(): null repo" if
>+     (!exists($args{'repo'}));

s/HgClone/HgPush/ all thru this function.

>+    die "ASSERT: Bootstrap::Util::HgClone(): null hgSshKey" if
>+     (! $config->Exists(sysvar => 'hgSshKey'));

Nit: Add something like "# required config variables" before this to differentiate from arguments.

>Index: configs/fx-moz18-bootstrap.cfg
>===================================================================
>-hgToolsRepo     = https://hg.mozilla.org/build/tools
>+hgToolsRepo     = hg.mozilla.org/build/tools

I'd prefer to handle the protocol the same way in both Bootstrap and Hg/Buildbot, so that we don't have to remember different rules for the two configs. Is that reasonable, at least in cases where variables have the same meaning ? If so, could we have a getPushRepo (or equivalent) in Bootstrap.

>+hgUsername      = ffxbld
>+win32_hgSshKey  = /c/Documents and Settings/cltbld/.ssh/ffxbld_dsa
>+linux_hgSshKey  = /home/cltbld/.ssh/ffxbld_dsa
>+macosx_hgSshKey = /Users/cltbld/.ssh/ffxbld_dsa

Repeating the definition for the symbol server, but I can't think of anything better in a hurry.

>Index: configs/fx-moz18-staging-bootstrap.cfg
>===================================================================
>+hgUsername      = stage-ffxbld
>+win32_hgSshKey  = /c/Documents and Settings/cltbld/.ssh/ffxbld_dsa
>+linux_hgSshKey  = /home/cltbld/.ssh/ffxbld_dsa
>+macosx_hgSshKey = /Users/cltbld/.ssh/ffxbld_dsa

Hmm, is this the "staging key named the same as the production key" trick again ?

Comment 14

[[e-mail] (:bhearsum)]

Attachment: [checked in] update-verify-bump.pl w/ comment fixes

Nick, I checked this in but I wouldn't mind a post-facto review of the --version/--app-version comments.

changeset:   18:4fac774899bd

Comment 15

[[e-mail] (:bhearsum)]

(In reply to comment #13)
> >+sub HgClone {
> >+    # we always pull over https://
> >+    $repo = 'https://' . $repo;
> 
> Does the python install on the 1.8 and 1.9 machines support this ? Thought we
> had trouble in buildbot land.
> 

The problem we had was with HgPoller failing with https:// - which use Twisted getPage to poll. Mercurial itself did not have any problems.

> >+    $this->Shell(
> >+      cmd => 'hg',
> >+      cmdArgs => ['clone', $repo],
> >+      dir => $workDir
> >+    );
> 
> Is this going to require we delete the pull before rerunning (eg) an update
> verify ? Perhaps we can do a dir existence test for filename($repo) and use
> rmtree.
> 

Yeah, will do.

> >+sub HgPush {
> 
> One fine day we should handle other commits between our pull and push. hg
> incoming seems to have exit status of 1 when there's nothing to pull, 0
> otherwise.
> 

Yeah =\. I want to sit down and figure that out someday. This one worries me a bit because update verify has a higher chance than other things of hitting the race condition.

> >Index: configs/fx-moz18-bootstrap.cfg
> >===================================================================
> >-hgToolsRepo     = https://hg.mozilla.org/build/tools
> >+hgToolsRepo     = hg.mozilla.org/build/tools
> 
> I'd prefer to handle the protocol the same way in both Bootstrap and
> Hg/Buildbot, so that we don't have to remember different rules for the two
> configs. Is that reasonable, at least in cases where variables have the same
> meaning ? If so, could we have a getPushRepo (or equivalent) in Bootstrap.
> 

Sure, will do.

> >Index: configs/fx-moz18-staging-bootstrap.cfg
> >===================================================================
> >+hgUsername      = stage-ffxbld
> >+win32_hgSshKey  = /c/Documents and Settings/cltbld/.ssh/ffxbld_dsa
> >+linux_hgSshKey  = /home/cltbld/.ssh/ffxbld_dsa
> >+macosx_hgSshKey = /Users/cltbld/.ssh/ffxbld_dsa
> 
> Hmm, is this the "staging key named the same as the production key" trick again
> ?

Yeah, it's like that on all of our machines, isn't it?

Comment 16

[[e-mail] (:bhearsum)]

Attachment: HgClone/HgPush update

Alright, I think I've addressed all of the comments:
* HgClone will rmtree() if the directory exists, noting that in the log.
* hgToolsRepo specified the same way as in release_config.py (by adding GetPushRepo to Util.pm)
* Fix comments

Comment 17

[[e-mail] (:bhearsum)]

(In reply to comment #12)
> (From update of attachment 346665 [details] [diff] [review])
> >Index: Bootstrap/Step/Updates.pm
> 
> >+    $this->CvsCo(
> >+      cvsroot => $mozillaCvsroot,
> >+      modules => [CvsCatfile('mozilla', $appName, 'locales',
> >+                             'shipped-locales')],
> >+      tag => $branchTag,
> >+      workDir => $verifyDirVersion,
> >+      checkoutDir => 'locales'
> >+    );
> 
> Need to check out the shipped-locales for the previous release, so not
> $branchTag but
>   uc($product).'_'.$oldVersion.'_RELEASE';
> Otherwise we'll get false positives/negatives when the locale list changes. r+
> with that fixed.
> 

Yeah, good point. I'll fix this on checkin.

> Have we tried this stuff in 1.8 staging ? Hopefully we can start using it for
> 2.0.0.19/3.0.5.

I haven't run it through a full run, but I have run perl ./release -v -o Updates on each platform with different config files.

With all the changes I've been landing to Bootstrap we should probably do a full staging run, though.

Comment 18

[Nick Thomas [:nthomas] (UTC+12)]

Comment on attachment 347292 [details] [diff] [review]
HgClone/HgPush update

>Index: Bootstrap/Step.pm
>===================================================================
>+sub HgClone {
...
>+    my $repoDir = catfile($workDir, $repo);
>+    if (-e $repoDir) {
>+        $this->Log(msg => $repoDir . ' exists, removing it.');
>+        rmtree($repoDir);

Don't you want a basename($repo) when setting up $repoDir ? And pull in File::Basename at the top of the file.

>+sub HgPush {
...
>+    # On Windows, ssh keys are stored in /c/Documents and Settings/...
>+    # Mercurial doesn't handle spaces in an identity filename properly
>+    # So we need to copy the ssh key to our working directory before
>+    # executing the command, and reference that one when doing the push

(I could have sworn I already comment on this, but apparently not). This is left over from the key issue you noted in comment #7 ? No longer needed ?

>Index: Bootstrap/Util.pm
>===================================================================
>+sub GetPushRepo {
nice!

r+ with the fixes above.

Comment 19

[Nick Thomas [:nthomas] (UTC+12)]

Comment on attachment 347288 [details] [diff] [review]
[checked in] update-verify-bump.pl w/ comment fixes

Looks good.

Comment 20

[[e-mail] (:bhearsum)]

Comment on attachment 346665 [details] [diff] [review]
[checked in] have Bootstrap call the new bumping script and remove its internal bumping logic

Checking in Bootstrap/Step/Updates.pm;
/cvsroot/mozilla/tools/release/Bootstrap/Step/Updates.pm,v  <--  Updates.pm
new revision: 1.46; previous revision: 1.45
done

Comment 21

[[e-mail] (:bhearsum)]

Attachment: [checked in] HgClone/HgPush - as landed

Nick, I've fixed up the $repoDir removal, and removed the extraneous comment. Going to carry review forward to get this finished up.

Comment 22

[[e-mail] (:bhearsum)]

Now, going through the release machines to make sure HG is on them.

Comment 23

[[e-mail] (:bhearsum)]

OK, Mercurial was already on all of the necessary slaves:
production-prometheus-vm, production-pacifica-vm, bm-xserve05
staging-prometheus-vm, staging-pacifica-vm, bm-xserve03
fx-{linux,mac,win32}-1.9-slave{1,2}

Note that the Thunderbird slaves do not need Mercurial because they're only used for Builds and Repacks - they use the other 1.8 slaves for the rest of automation.

Comment 24

[Nick Thomas [:nthomas] (UTC+12)]

Landed a followup to change
 -win32_hgSshKey  = /c/Documents and Settings/cltbld/.ssh/ffxbld_dsa
 +win32_hgSshKey  = /home/cltbld/.ssh/ffxbld_dsa
in {fx,tb}-moz18-bootstrap.cfg and matching staging configs, as these boxes use Cygwin rather than MSYS.