Closed Bug 607389 Opened 14 years ago Closed 13 years ago

generate partial updates at build time for releases

Categories

(Release Engineering :: General, defect, P2)

Product:
Release Engineering
Component:
General
Platform:
All
macOS
Type:
defect

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: bhearsum, Assigned: rail)

References

Details

(Whiteboard: [automation][updates][releases])

Attachments

(1 file, 1 obsolete file)

patcher2.pl: use checksums instead of real files to generate snippets
24.69 KB, patch
nthomas
: review+
rail
: checked-in+
Details | Diff | Splinter Review
We started doing this for nightly builds a long time ago, and we should do it for releases, too. For Linux and Mac this is pretty easy. For Windows, we can't generate the partial until the complete MAR is signed. Because of that, this bug is blocked on bug 509158.
Depends on: 607978
No longer blocks: 509158
Depends on: 509158
For release builds we use ReleaseBuildFactory. ReleaseBuildFactory need to know the URL of the previous MAR and use the same automation as we use for nightlies. For release l10n repacks we need to add the similar code. The updates builder should be changed so it (actually patcher2.pl) uses checksums files instead of real files to populate the cache (%SNIPPET_CHECKSUM_HASH_CACHE) or wrap HashFile function. This will be obsoleted by Balrog!
Assignee: nobody → rail
Priority: P4 → P2
Whiteboard: [automation][updates][releases]
Depends on: 714542
Comment on attachment 587287 [details] [diff] [review] patcher2.pl: use checksums instead of real files to generate snippets Review of attachment 587287 [details] [diff] [review]: ----------------------------------------------------------------- ::: patcher2.pl @@ +130,5 @@ > my $deliverableDir = EnsureDeliverablesDir(config => $config); > > #printf("PRE-REMOVE-BROKEN-UPDATES:\n\n%s", Data::Dumper::Dumper($config)); > + if (!$config->useChecksums()){ > + # TODO: touch mars and use this? ignore the line above, to be removed
the patch implies that config-bumper uses the following patch: --- a/lib/perl/Release/Patcher/Config.pm +++ b/lib/perl/Release/Patcher/Config.pm @@ -72,16 +72,20 @@ sub GetReleaseBlock { } $releaseBlock->{'locales'} = join(' ', sort (keys(%{$localeInfo}))); $releaseBlock->{'completemarurl'} = 'http://' . $stagingServer . '/pub/mozilla.org/' . $product . '/nightly/' . $version . '-candidates/' . $buildStr . '/update/%platform%/%locale%/' . $product . '-' . $version . '.complete.mar'; + $releaseBlock->{'checksumsurl'} = 'http://' . $stagingServer . + '/pub/mozilla.org/' . $product . '/nightly/' . $version . '-candidates/' . + $buildStr . '/%platform%/%locale%/' . $product . '-' . $version . + '.checksums'; $releaseBlock->{'exceptions'} = {}; my %platformMap = GetBouncerToPatcherPlatformMap(); my %FTPplatformMap = GetFTPToBuildbotPlatformMap(); foreach my $locale (keys(%{$localeInfo})) { my $allPlatformsHash = {}; foreach my $platform (GetBouncerPlatforms()) {
Comment on attachment 587287 [details] [diff] [review] patcher2.pl: use checksums instead of real files to generate snippets >diff --git a/MozAUSLib.pm b/MozAUSLib.pm >+ PreopulateHashCache PrepopulateHashCache, as bhearsum pointed out. >+sub PreopulateHashCache { ... >+ if ($fname =~ m/partial.mar$/){ >+ $file = $partial_mar_path; >+ } elsif ($fname =~ m/complete.mar$/){ >+ $file = $complete_mar_path; >+ } From a data integrity point of view, it may be worth checking that fname matches basename(partial_mar_path). >+ if (defined($file)){ >+ my @files = (catfile("$from-$to", "ftp", $file), >+ catfile($to, "ftp", $complete_mar_local_path)); >+ for my $f (@files){ Could you explain complete_mar_local_path here ? I'm not sure what it means to set that if the current checksum line is a partial. Perhaps you're implicitly relying on completes following partials in the checksums file to set this correctly ? That doesn't seem to be the case in you 10.0 staging release on dev-stage01, so I must be missing something. >diff --git a/patcher2.pl b/patcher2.pl >- $config->RemoveBrokenUpdates(); >+ if (!$config->useChecksums()){ >+ # TODO: touch mars and use this? >+ $config->RemoveBrokenUpdates(); >+ } Ignoring the TODO... I'm not sure what this is supposed to do. In our existing code, to call RemoveBrokenUpdates() before a downloading mars seems odd to me. On face value it seems like it would not find any mar files, and prune the entire graph. Does it only prune the previous releases or something ? Could you comment on why the handling need to change for checksums mode ? >- if ($config->RequestedStep('create-patches')) { >+ if (! $config->useChecksums() && $config->RequestedStep('create-patches')) { > CreatePartialPatches(config => $config); > CreateCompletePatches(config => $config); > } Something in this control flow keeps drawing me back here with a nagging feeling of unease. One thing that would help is swapping the conditions for the && over, to change the emphasis when reading. > if ($config->RequestedStep('(create-patches|create-patchinfo)')) { >+ if ($config->useChecksums()){ >+ PreopulateHashCache(config => $config); >+ chdir($startdir); Why not do the chdir at the end of PrepopulateHashCache() ? >+ $config->RemoveBrokenUpdates(); Just to be paranoid, if we fail to download a checksum file then it fails obviously rather than hiding it ? We should avoid finding out at update verify that a locale hasn't got snippets. >+--use-checksums Try to use checksums files instead of complete >+ MAR files. Not just completes, eh ? > my $output_filename = SubstitutePath( > path => $MozAUSConfig::DEFAULT_MAR_NAME, > platform => $p, > locale => $l, > version => $r, > app => lc($config->GetApp())); >+ if ($config->useChecksums()){ >+ $output_filename = SubstitutePath( >+ path => $MozAUSConfig::DEFAULT_CHECKSUMS_NAME, >+ platform => $p, >+ locale => $l, >+ version => $r, >+ app => lc($config->GetApp())); >+ } It would be cleaner to have a my $output_filename = $MozAUSConfig::DEFAULT_MAR_NAME; if ($config->useChecksums()){ $output_filename = $MozAUSConfig::DEFAULT_CHECKSUMS_NAME; } then have a single call SubstitutePath once (like $download_url). >@@ -798,17 +820,18 @@ sub CreateCompletePatchinfo { > # Go to next iteration if this partial patch already exists. >- next if ( -e $complete_patch->{'info_path'} or ! -e $complete_pathname ); >+ next if ( -e $complete_patch->{'info_path'} or >+ (! $config->useChecksums() && ! -e $complete_pathname ) ); > $i++; Looks like the 'partial' in the comment is bogus.
(In reply to Nick Thomas [:nthomas] from comment #6) > PrepopulateHashCache, as bhearsum pointed out. Done. > From a data integrity point of view, it may be worth checking that fname > matches basename(partial_mar_path). Done. > >+ if (defined($file)){ > >+ my @files = (catfile("$from-$to", "ftp", $file), > >+ catfile($to, "ftp", $complete_mar_local_path)); > >+ for my $f (@files){ > > Could you explain complete_mar_local_path here ? I'm not sure what it means > to set that if the current checksum line is a partial. Perhaps you're > implicitly relying on completes following partials in the checksums file to > set this correctly ? That doesn't seem to be the case in you 10.0 staging > release on dev-stage01, so I must be missing something. For partial MARs patcher2.pl uses catfile("$from-$to", "ftp", $file) ("temp/9.0.1-10.0/ftp/.../af/") as an index. For completes it (CachedHashFile actually) may use catfile("$from-$to", "ftp", $file) or catfile($to, "ftp", $complete_mar_local_path) ("temp/10.0/ftp/%app%-%version%.%locale%.%platform%.complete.mar") depending on its mood. :) I could make it use just one prefix, but it would require more debugging. In any case the hash indexes are unique and the data is valid. > >diff --git a/patcher2.pl b/patcher2.pl > >- $config->RemoveBrokenUpdates(); > >+ if (!$config->useChecksums()){ > >+ # TODO: touch mars and use this? > >+ $config->RemoveBrokenUpdates(); > >+ } > > Ignoring the TODO... I'm not sure what this is supposed to do. In our > existing code, to call RemoveBrokenUpdates() before a downloading mars seems > odd to me. On face value it seems like it would not find any mar files, and > prune the entire graph. Does it only prune the previous releases or > something ? Could you comment on why the handling need to change for > checksums mode ? Yeah... The logic looks broken to me too. We haven't hit problems here before because we run patcher in 2 times: download mode, then patcher mode. I simplified it a bit. > >- if ($config->RequestedStep('create-patches')) { > >+ if (! $config->useChecksums() && $config->RequestedStep('create-patches')) { > > CreatePartialPatches(config => $config); > > CreateCompletePatches(config => $config); > > } > > Something in this control flow keeps drawing me back here with a nagging > feeling of unease. One thing that would help is swapping the conditions for > the && over, to change the emphasis when reading. I simplified this as well. > > if ($config->RequestedStep('(create-patches|create-patchinfo)')) { > >+ if ($config->useChecksums()){ > >+ PreopulateHashCache(config => $config); > >+ chdir($startdir); > > Why not do the chdir at the end of PrepopulateHashCache() ? Right... > >+ $config->RemoveBrokenUpdates(); > > Just to be paranoid, if we fail to download a checksum file then it fails > obviously rather than hiding it ? We should avoid finding out at update > verify that a locale hasn't got snippets. DownloadFile dies, so we're safe here. > >+--use-checksums Try to use checksums files instead of complete > >+ MAR files. > > Not just completes, eh ? Right. And not "Try to...". My first attempt was trying to use checksums and have a fallback to mars. > > my $output_filename = SubstitutePath( > > path => $MozAUSConfig::DEFAULT_MAR_NAME, > > platform => $p, > > locale => $l, > > version => $r, > > app => lc($config->GetApp())); > >+ if ($config->useChecksums()){ > >+ $output_filename = SubstitutePath( > >+ path => $MozAUSConfig::DEFAULT_CHECKSUMS_NAME, > >+ platform => $p, > >+ locale => $l, > >+ version => $r, > >+ app => lc($config->GetApp())); > >+ } > > It would be cleaner to have a > my $output_filename = $MozAUSConfig::DEFAULT_MAR_NAME; > if ($config->useChecksums()){ > $output_filename = $MozAUSConfig::DEFAULT_CHECKSUMS_NAME; > } > then have a single call SubstitutePath once (like $download_url). +1 > >@@ -798,17 +820,18 @@ sub CreateCompletePatchinfo { > > # Go to next iteration if this partial patch already exists. > >- next if ( -e $complete_patch->{'info_path'} or ! -e $complete_pathname ); > >+ next if ( -e $complete_patch->{'info_path'} or > >+ (! $config->useChecksums() && ! -e $complete_pathname ) ); > > $i++; > > Looks like the 'partial' in the comment is bogus. I just removed those. Interdiff: https://gist.github.com/1602986
Attachment #587287 - Attachment is obsolete: true
Attachment #588164 - Flags: review?(nrthomas)
Attachment #587287 - Flags: review?(nrthomas)
Comment on attachment 588164 [details] [diff] [review] patcher2.pl: use checksums instead of real files to generate snippets >diff --git a/MozAUSLib.pm b/MozAUSLib.pm >+sub PrepopulateHashCache { I'd suggest doing this on top of your patch: diff -rU3 patcher.r2/MozAUSLib.pm patcher.r2me/MozAUSLib.pm --- patcher.r2/MozAUSLib.pm 2012-01-18 09:27:38.000000000 +1300 +++ patcher.r2me/MozAUSLib.pm 2012-01-18 14:23:49.000000000 +1300 @@ -177,12 +177,17 @@ if ($fname =~ m/partial.mar$/ && basename($fname) eq basename($partial_mar_path)){ $file = $partial_mar_path; - } elsif ($fname =~ m/complete.mar$/){ + } elsif ($fname =~ m/complete.mar$/ && + basename($fname) eq basename($complete_mar_path)){ $file = $complete_mar_path; } if (defined($file)){ - my @files = (catfile("$from-$to", "ftp", $file), - catfile($to, "ftp", $complete_mar_local_path)); + my @files = (catfile("$from-$to", "ftp", $file)); + # work around snippet creation functions using + # different places for complete mars + if ($file =~ m/complete.mar$/){ + push(@files, catfile($to, "ftp", $complete_mar_local_path)); + } for my $f (@files){ if (! exists($SNIPPET_CHECKSUM_HASH_CACHE->{$f})) { $SNIPPET_CHECKSUM_HASH_CACHE->{$f} = {}; * check the basename for completes too * only add the $to location for completes, since you're relying completes to be ahead of the partials in the checksums file I verified it didn't change anything in the 9.0.1 -> 10.0 you have in staging. >diff --git a/patcher2.pl b/patcher2.pl > run_download_complete_patches(config => $config) if $config->RequestedStep('download'); >+ if ($config->useChecksums()) { >+ PrepopulateHashCache(config => $config); >+ } >+ #printf("PRE-REMOVE-BROKEN-UPDATES:\n\n%s", Data::Dumper::Dumper($config)); >+ $config->RemoveBrokenUpdates(); >+ #printf("POST-REMOVE-BROKEN:\n\n%s", Data::Dumper::Dumper($config)); I wanted to understand this better so I did some experimenting around this function. With the code prior to this bug, we do trim the graph in the build-tools and download calls. It pretty much empties mRawConfig->app->Firefox->update_data in particular setting <old_ver>-<new_ver>->platforms-><platform>->locales to {} instead of having cooked info. However, neither of those steps cares - download looks up info in mRawConfig->app->Firefox->release-><old_ver> and <new_ver> directly, having got the versions from current-update. So assuming the download worked OK, when we call with create-patches the RemoveBrokenUpdates() is a no-op. So with the old or new code it's fine to move this down as you have. r+ with the diff I added above. This is worthy of creating UPDATE_PACKAGING_R16 but you won't need to drop that on mozilla-central/aurora etc. Please update https://wiki.mozilla.org/ReleaseEngineering/PatcherTags when this lands.
Attachment #588164 - Flags: review?(nrthomas) → review+
Comment on attachment 588164 [details] [diff] [review] patcher2.pl: use checksums instead of real files to generate snippets $ cvs commit -m "Bug 607389 - generate partial updates at build time for releases. p=rail,r=nthomas" cvs commit: Examining . Checking in MozAUSConfig.pm; /cvsroot/mozilla/tools/patcher/MozAUSConfig.pm,v <-- MozAUSConfig.pm new revision: 1.21; previous revision: 1.20 done Checking in MozAUSLib.pm; /cvsroot/mozilla/tools/patcher/MozAUSLib.pm,v <-- MozAUSLib.pm new revision: 1.20; previous revision: 1.19 done Checking in patcher2.pl; /cvsroot/mozilla/tools/patcher/patcher2.pl,v <-- patcher2.pl new revision: 1.45; previous revision: 1.44 done $ cvs tag UPDATE_PACKAGING_R16 cvs tag: Tagging . T MozAUSConfig.pm T MozAUSLib.pm T README T patcher2.cfg T patcher2.pl
Attachment #588164 - Flags: checked-in+
$ cd Bootstrap $ cvs tag UPDATE_PACKAGING_R16 cvs tag: Tagging . T Util.pm $ cd ../MozBuild $ cvs tag UPDATE_PACKAGING_R16 cvs tag: Tagging . T TinderLogParse.pm T Util.pm
Still to be tagged: mozilla repos.
(In reply to Rail Aliiev [:rail] from comment #11) > Still to be tagged: mozilla repos. Do the slaves need this ? Looks like the updates builder doesn't any more.
(In reply to Nick Thomas [:nthomas] from comment #13) > (In reply to Rail Aliiev [:rail] from comment #11) > > Still to be tagged: mozilla repos. > > Do the slaves need this ? Looks like the updates builder doesn't any more. Just for the case when you decide to use old style update generation and don't want to use old tag for it. One of the use cases comes to my mind is custom updates for releases like 9.0.1 (partials for version=N-2). Otherwise, it's not necessary.
Available in 11.0b1
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Product: mozilla.org → Release Engineering
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: