move BumpPatcherConfig to an external script (in hg.m.o/build/tools)

RESOLVED FIXED

Status

Release Engineering
General
P2
normal
RESOLVED FIXED
9 years ago
4 years ago

People

(Reporter: bhearsum, Assigned: bhearsum)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [hg-automation])

Attachments

(2 attachments, 4 obsolete attachments)

Comment hidden (empty)
(Assignee)

Updated

9 years ago
Priority: -- → P2
Whiteboard: [hg-automation]
(Assignee)

Comment 1

9 years ago
Created attachment 340178 [details] [diff] [review]
move BumpPatcherConfig logic to hg.m.o/build/tools

This is a big patch, but it's not a huge amount of change. Here's an overview:
* Move the bulk of BumpVerifyConfig to an external script. It is the caller's responsibility to checkout patcher config and shipped-locales and commit the config file afterwards. (For CVS builds the caller is Bootstrap, for Mercurial builds it is Buildbot.)
* Use wget instead of scp to get the buildid from stage
* Export LoadLocaleManifest from Bootstrap::Util so patcher-config-bump.pl can call it.
* Move GetBuildIDFromFTP to MozBuild::Util so Bootstrap and patcher-config-bump can both use it. Upon reflection, I could've just moved it to Bootstrap::Util I suppose. I can make that change if you want.

I think that's all the exciting stuff! I ran moz18-branch-patcher2.cfg version 1.17 through patcher-config-bump.pl and came out with _exactly_ the same changes in the Firefox section. <Thunderbird> looked different but that's only because of the tb2.0.0.17 spin. I can post diffs here if it helps you.

I've got a BuildFactory started for this, too, but I'll post that in a big patch when I land production release configs.
Attachment #340178 - Flags: review?(nthomas)
Comment on attachment 340178 [details] [diff] [review]
move BumpPatcherConfig logic to hg.m.o/build/tools

Great start Ben, just need to improve the Bootstrap side of things a bit.

>Index: Bootstrap/Step.pm
>===================================================================
>+    return MozBuild::Util::GetBuildIDFromFTP(os => $os,
>+                                             releaseDir => $releaseDir,
>+                                             stagingServer => $stagingServer);

I don't mind if this in MozBuild::Util or Bootstrap::Util.

>Index: Bootstrap/Step/PatcherConfig.pm
...
>+    $this->Shell(
>+      cmd => 'hg',
>+      cmdArgs => ['clone', $hgToolsRepo],
>+      dir => catfile($buildTagDir)
>+    );

$hgToolsRepo is not defined.

>+    my @args = [catfile($buildTagDir, 'tools', 'release',
>+                     'patcher-config-bump.pl')],
...
>+            '-d', $bouncerServer,
>+            '-l', FIXME];

Looks like you need to checkout the shipped-locales file for the Bootstrap case.

>+    $this->Shell(
>+      cmd => 'perl',
>+      cmdArgs => @args,
>+    );
> }

Do we need to check error status here ? I forget.

>Index: MozBuild/Util.pm
...
>+sub GetBuildIDFromFTP {
...
>+    my ($bh, $buildIDTempFile) = tempfile(UNLINK => 1);
>+    $bh->close();
>+    RunShellCommand(
>+      command => 'wget',
>+      args => ['-O', $buildIDTempFile,
>+               'http://' . $stagingServer . '/' . $releaseDir . '/' .
>+               $os . '_info.txt']
>+    );

Please check that RunShellCommand asserts or otherwise aborts execution if the command has non-zero exit status or there is a timeout. If it doesn't, then please check for that here. I know we don't check with scp, but I'm mean.
----------------------------------------------------------------------------

>diff -r f45852093b61 release/patcher-config-bump.pl
>--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
>+++ b/release/patcher-config-bump.pl	Wed Sep 24 14:38:18 2008 -0400

>+use MozBuild::Util qw(MkdirWithPath GetBuildIDFromFTP);

Can't see MkdirWithPath being used.

>+        print " -u When passed, the Beta channel will be considered the \n";
>+        print "    channel for final release. Specifically, this will \n";
>+        print "    cause the 'beta' channel snippets to point at the \n";
>+        print "    Bouncer server (rather than FTP).\n";

It's the other way around. If -u is given, then we say the version is "$version (build$build)" in the offer and use ftpServer for beta snippets.

>+    if (! defined $config{'patcher-config'} or ! -e $config{'patcher-config'}) {
>+        print "patcher config file must exist.\n";
>+        $error = 1;

Given this file is acquired separately, we should probably test that it's writable rather than just exists. 

>+    if (! defined $config{'useBetaChannel'}) {
>+        $config{'useBetaChannel'} = 0;
>+    }

s/useBetaChannel/use-beta-channel/

>+    my $localeInfo = {};
>+    if (not LoadLocaleManifest(localeHashRef => $localeInfo,
>+                               manifest => $config{'shipped-locales'})) {
>+        die "Could not load locale manifest";
>+    }
>+
>+    $config{'localeInfo'} = $localeInfo;

Pedantic nit: locale-info would match the naming style of other keys in the config hash.

>+sub BumpPatcherConfig {

This function is pretty much gutted when the script moves out, and just does a bunch of lookups that we mostly do in Execute. What do you think to making it vanish by moving the call to the bump script to Execute ?

>+    my $versionedConfigBumpDir = catfile($configBumpDir, 
>+                                          "$product-$version-build$build");

This is never unused by the looks, so it and the $configBumpDir definition can go. Doesn't strict complain about that ? Anything else I missed ?
Attachment #340178 - Flags: review?(nthomas) → review-
(Assignee)

Comment 3

9 years ago
Created attachment 340360 [details] [diff] [review]
take 2 - address nick's review comments, fixes other bugs (and a test!)

OK, I've addressed all of your comments Nick. I added a small test in patcher-config-bump.pl to help us in the future.

I've tested it the following ways:
* Running the included unit test (--run-tests)
* Running patcher-config-bump.pl w/out --run-tests
* Running Bootstrap's PatherConfig step, modified slightly (copy local Bootstrap checkout instead of getting it from CVS; don't try to check the new patcher config in).

I'd like to check this in and put it through a staging run on 1.8 staging automation if you don't have any problems with this patch, Nick. I won't even think about re-tagging Bootstrap until I've tested this stuff in a real run ;-).
Attachment #340178 - Attachment is obsolete: true
Attachment #340360 - Flags: review?(nthomas)
Comment on attachment 340360 [details] [diff] [review]
take 2 - address nick's review comments, fixes other bugs (and a test!)

>diff -r f45852093b61 release/patcher-config-bump.pl
>+    print $useBetaChannel . "\n";

Still need this ?

>+    $bReleases->{$oldVersion}{'platforms'}{'linux-i686'} = '2008070824';
>+    $bReleases->{$oldVersion}{'platforms'}{'win32'} = '2008070824';
>+    $bReleases->{$oldVersion}{'platforms'}{'mac'} = '2008070824';

I'd be tempted to use different buildID's for each of the three releases, just to make it slightly more realistic (and possibly easier to debug problems).

>+    # The patcher config we're bumping does not contain any past-update lines.
>+    # Patcher doesn't deal with this well - it will insert an empty
>+    # past-update line along with the valid one. Rather than fix this bug we'll
>+    # just test patcher's existing behaviour.
>+    $afterConfig->{'app'}->{$uProduct}->{'past-update'} = [
>+        "",
>+        "$oldOldVersion $oldVersion betatest releasetest beta release"
>+    ];

Hmm, you learn something everyday (assuming you mean PatcherConfig for Patcher on the second line). I doubt we've ever really tested this case or hit the bug before.

>+    if (deeply_equal(\%bumpedConfig, $afterConfig)) {
>+        print "PASS\n";
>+        exit(0);
>+    } else {
>+        print "FAIL\n";
>+        exit(1);
>+    }

In the FAIL case, we're going to need a way to debug. Maybe easiest is to write out $afterConfig to disk so we can diff against the bumped one.

>+# Taken from http://www.perlmonks.org/?node_id=121559
>+sub deeply_equal {
>+  my ( $a_ref, $b_ref ) = @_;
>+    
>+  local $Storable::canonical = 1;
>+  return Storable::freeze( $a_ref ) eq Storable::freeze( $b_ref );
>+}

Voodoo! Wanna fix the small stuff on checkin ? r+ if you do.
Attachment #340360 - Flags: review?(nthomas) → review+
(Assignee)

Comment 5

9 years ago
Created attachment 340607 [details] [diff] [review]
[checked in] patch as landed, same as v2 w/ nick's recommended fixes

changeset:   8:b21cbc788829
Checking in Bootstrap/Step.pm;
/cvsroot/mozilla/tools/release/Bootstrap/Step.pm,v  <--  Step.pm
new revision: 1.19; previous revision: 1.18
done
Checking in Bootstrap/Util.pm;
/cvsroot/mozilla/tools/release/Bootstrap/Util.pm,v  <--  Util.pm
new revision: 1.16; previous revision: 1.15
done
Checking in Bootstrap/Step/PatcherConfig.pm;
/cvsroot/mozilla/tools/release/Bootstrap/Step/PatcherConfig.pm,v  <--  PatcherConfig.pm
new revision: 1.22; previous revision: 1.21
done
Checking in MozBuild/Util.pm;
/cvsroot/mozilla/tools/release/MozBuild/Util.pm,v  <--  Util.pm
new revision: 1.21; previous revision: 1.20
done
(Assignee)

Comment 6

9 years ago
I explicitly did NOT re-tag Bootstrap after landing these changes. I intend to do that before starting the 3.1b1 releases, which will be after they've got a better test in staging.
(Assignee)

Updated

9 years ago
Attachment #340360 - Attachment is obsolete: true
(Assignee)

Comment 7

9 years ago
Argh. I just realized that patcher-config-bump.pl needs to support the new candidates directory style, sigh.
(Assignee)

Comment 8

9 years ago
Created attachment 340639 [details] [diff] [review]
bump URLs based on the existing ones in the patcher config

....Rather than hardcoding a format.

I'll admit, I haven't tested this thoroughly yet but it does pass the unit test. If/When I have more time I'd like to write a test that tests a second format...

Sorry to lay another one on you Nick.
Attachment #340639 - Flags: review?(nthomas)
Attachment #340639 - Flags: review?(nthomas) → review-
Comment on attachment 340639 [details] [diff] [review]
bump URLs based on the existing ones in the patcher config

>-    $partialUpdate->{'path'} = catfile($product, 'nightly', $version . 
>-                               '-candidates', $buildStr, $product. '-' . 
>-                               $oldVersion . '-' . $version .
>-                               '.%locale%.%platform%.partial.mar');
>+    my $pPath = $currentUpdateObj->{'partial'}->{'path'};
>+    $partialUpdate->{'path'} = BumpPath(path => $pPath,
>+                                        version => $version,
>+                                        oldVersion => $oldVersion,
>+                                        isPartial => 1);

buildN (buildStr) won't be updated for the respin case in a lot of these.

>-    $completeUpdate->{'path'} = catfile($product, 'nightly', $version . 
>-     '-candidates', $buildStr, $product . '-' . $appVersion .
>-     '.%locale%.%platform%.complete.mar');
>+    my $cPath = $currentUpdateObj->{'complete'}->{'path'};
>+    $completeUpdate->{'path'} = BumpPath(path => $cPath,
>+                                         version => $version,
>+                                         oldVersion => $oldVersion);

This won't update appVersion.

>+    # This won't bump Alphas properly yet since their completemarurl
>+    # normally points to a releases/codename/alphaN style directory
>+    my $completemarurl = $appObj->{'release'}->{$oldVersion}{'completemarurl'};
>+    $releaseObj->{'completemarurl'} = BumpPath(path => $completemarurl,
>+                                               version => $version,
>+                                               oldVersion => $oldVersion);

It'd be cool if we set the oldVersion's completemarurl to app/releases/oldVersion/update/%platform... on build=1, instead of relying that nightly/oldVersion-candidates/ persists.

I'm not sure how I feel about this approach. Hard wiring does seem like something that'll bite us if the layout/filenames ever change, but the regexp's in BumpPath aren't very stringent and may match incorrectly or incompletely anyway. Can you think of a way to extend BumpPath without bloating the script with a lot of long function calls ?
(Assignee)

Comment 10

9 years ago
(In reply to comment #9)

Yeah, you're absolutely right. I realized that if we try to change stagingServer, ftpServer, et. al they won't be updated either. I'll investigate other options here.
(Assignee)

Comment 11

9 years ago
Nick, how do you feel about renaming BumpPath to BumpFilePath and only having it deal with things in or below the buildN dir. The regex's would be the same, but the strings they operate on would exclude everything up to and including buildN. I think this would make them safer - since we have a standard, known format for candidates directories, and it would let us easily construct the various prefix variants in such a way that they will get changed if we pass a new, eg, stageServer.

Does that make sense?
(Assignee)

Comment 12

9 years ago
Created attachment 341119 [details] [diff] [review]
again, with better regexs and the ability to change ftp/stage/bouncer server down the road

This patch is a little more wordy but it's definitely safer.

BumpFilePath() checks explicitly for complete and partial MARs and matches a longer pattern, which should avoid any mistakes down the road.

The rest is pretty straight forward, I think. I'm working an the assumption that we won't be moving away from .../$version-candidates/build$build/ anytime soon - which is why I left it hardcoded outside the BumpFilePath function.

Please don't feel obligated to agree with my methods just because the release is looming - let's get this right.
Attachment #340639 - Attachment is obsolete: true
Attachment #341119 - Flags: review?(nthomas)
Comment on attachment 341119 [details] [diff] [review]
again, with better regexs and the ability to change ftp/stage/bouncer server down the road

Not sure I have this completely straight in my head, but here goes.

>diff -r b21cbc788829 release/patcher-config-bump.pl
>+sub BumpFilePath {
...
>+    my $newPath = $oldFilePath;
>+    # We need to handle partials and complete MARs differently
>+    if ($newPath =~ m/\.partial\.mar$/) {
>+        $newPath =~ s/$product-[\d.ab(rc)]+-$oldVersion\.
>+                     /$product-$oldVersion-$version./x;

I threw some bogus input into this code and it had some problems handling it. If the match for the s// fails then BumpFilePath returns the oldFilePath without error, so we'd need something like an 'or die ...'. That probably hasn't helped you test this code block, eg '(rc)' doesn't work as an optional match in a class like that. You could write '[\d.ab(rc)]+' as '\d(\.\d*)*((a|b|rc)\d+)?' or just do a non-greedy '.*?'. The latter seems fine to me. Also, any . in $oldVersion should really be escaped when used in a regexp, otherwise the likes of 3p3p3 matches just as well as 3.0.3

>+    } elsif ($newPath =~ m/\.complete\.mar$/) {
>+        $newPath =~ s/$product-$oldVersion\.
>+                     /$product-$version./x;

The comments about 'or die' and escaping periods apply here too.

>     $partialUpdate->{'url'} = 'http://' . $bouncerServer . '/?product=' .
>                               $product. '-' . $version . '-partial-' . 
>                               $oldVersion .
>                              '&os=%bouncer-platform%&lang=%locale%';

This is gonna cause grief for 3.1b1 by overwriting the complete-as-partial special casing :-S 
Perhaps we should just fudge it bouncer, but it's not very elegant.

>+    my $pOldPath = $currentUpdateObj->{'partial'}->{'path'};
>+    # strip out everything up to and including 'buildN/'
>+    $pOldPath =~ s/.*\/build\d+\///;
>+    my $pPath = BumpFilePath(oldFilePath => $pOldPath,
>+                             product => $product,
>+                             version => $version,
>+                             oldVersion => $oldVersion);\

We'll modify moz191-branch-patcher2.cfg between 3.1 b1 and b2 to fix the serve-completes-for-partials setup ? Otherwise we'll keep hitting the complete.mar path.

>+    # strip out everything up to and including 'buildN/'
>+    $pOldBetatestPath =~ s/.*\/build\d+\///;
>+    my $pBetatestPath = BumpFilePath(oldFilePath => $pOldBetatestPath,
>+                                     product => $product,
>+                                     version => $version,
>+                                     oldVersion => $oldVersion);

For brevity, you could look up the product, version and oldVersion from the global $config hash at the top of BumpFilePath(). Looks like those three arguments are the same in all the calls. Similarly, the trimming of everything up to buildN could move into the function, if you don't think that's icky style.

Sorry I didn't get at this earlier.
Attachment #341119 - Flags: review?(nthomas) → review-
(Assignee)

Comment 14

9 years ago
(In reply to comment #13)
> This is gonna cause grief for 3.1b1 by overwriting the complete-as-partial
> special casing :-S 
> Perhaps we should just fudge it bouncer, but it's not very elegant.
> 

I think what I'll be doing for 3.1b1 is letting the script bump it, and then I'll check-in a fix for this while it's in --build-tools-hg...I totally agree that fixing it in bouncer isn't great.

> We'll modify moz191-branch-patcher2.cfg between 3.1 b1 and b2 to fix the
> serve-completes-for-partials setup ? Otherwise we'll keep hitting the
> complete.mar path.

Yeah, absolutely.

> For brevity, you could look up the product, version and oldVersion from the
> global $config hash at the top of BumpFilePath(). Looks like those three
> arguments are the same in all the calls. Similarly, the trimming of everything
> up to buildN could move into the function, if you don't think that's icky
> style.

Ended up making these changes - the script looks a little nicer now, hooray.
(Assignee)

Comment 15

9 years ago
Created attachment 341628 [details] [diff] [review]
[checked in] on more time, addressing nick's comments.
Attachment #341119 - Attachment is obsolete: true
Attachment #341628 - Flags: review?(nthomas)
Comment on attachment 341628 [details] [diff] [review]
[checked in] on more time, addressing nick's comments.

>+    if ($newPath =~ m/\.partial\.mar$/) {
>+        $newPath =~ s/$product-.*?-$escapedOldVersion\.

Nit: Can I change my mind and use .+? here ?

>+    my $oldCompleteMarPath =
>+      $appObj->{'release'}->{$oldVersion}->{'completemarurl'};
>+    $oldCompleteMarPath =~ s/.*\/build\d+\///;

The s/// can be culled too, I think.
Attachment #341628 - Flags: review?(nthomas) → review+
(Assignee)

Comment 17

9 years ago
Comment on attachment 341628 [details] [diff] [review]
[checked in] on more time, addressing nick's comments.

with changes to address nick's last comments.

changeset:   11:8d12d74fa329
Attachment #341628 - Attachment description: on more time, addressing nick's comments. → [checked in] on more time, addressing nick's comments.
(Assignee)

Comment 18

9 years ago
I think we're done here.
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Thanks for your patience.
Attachment #340607 - Attachment is patch: true
Attachment #340607 - Attachment mime type: application/octet-stream → text/plain
Product: mozilla.org → Release Engineering
You need to log in before you can comment on or make changes to this bug.