Closed Bug 408868 Opened 12 years ago Closed 12 years ago

Teach verify-locales about tar.bz2 and betas

Categories

(Release Engineering :: General, defect, P2)

defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: nthomas, Assigned: bhearsum)

References

Details

Attachments

(2 files, 6 obsolete files)

From the latest Fx3.0b2 Stage run:

Unknown file in /builds/data/cltbld/firefox-3.0b2/batch1/stage-signed/linux-i686/cs: firefox-3.0b2.tar.bz2
Unknown file in /builds/data/cltbld/firefox-3.0b2/batch1/stage-signed/linux-i686/it: firefox-3.0b2.tar.bz2
Unknown file in /builds/data/cltbld/firefox-3.0b2/batch1/stage-signed/linux-i686/tr: firefox-3.0b2.tar.bz2

It's expecting tar.gz for all but the source file.
Priority: -- → P3
Assignee: nobody → bhearsum
Priority: P3 → P2
Attached patch teach verify-locales about bz2 (obsolete) — Splinter Review
This patch makes verify-locales.pl support gz and bz2 files for Linux packages.
Attachment #293729 - Flags: review?(nrthomas)
Comment on attachment 293729 [details] [diff] [review]
teach verify-locales about bz2

This would certainly do the trick, but is a less specific test. What do you think about adding another commandline argument to verify-locales.pl, specifying the linux extension. Bootstrap could pass that based on useTarGz.

BTW, the file lives in mofo/release/stage/
Here's a version doing as you suggested, cf.

Is verify-locales.pl used anywhere other than automation/releases? If so, this will break things.
Attachment #293729 - Attachment is obsolete: true
Attachment #293851 - Flags: review?(nrthomas)
Attachment #293729 - Flags: review?(nrthomas)
This patch adds the '-l' option to the verify-locales call in Bootstrap::Step::Stage to be compatible with the other patch attached here.
Attachment #293852 - Flags: review?(nrthomas)
Sorry, didn't notice that there was some real tabs in here. Spacing is fixed in this patch.
Attachment #293851 - Attachment is obsolete: true
Attachment #293856 - Flags: review?(nrthomas)
Attachment #293851 - Flags: review?(nrthomas)
There's something wrong with the verify-locales patch..investigating now..
So, it turns out the problem was two things:
1) I forgot to 'cvs up' /data/cltbld/bin. We'll have to do this on all tinderboxen when this is landed (I think). Definently have to do it on the release automation machines.
2) Silly errors in my patch. Updated one coming in a sec...
From IRC:
12:48 <@cf_nthomas> I think we only need to update verify-locales on the consoles, rather than all the tinderboxes
12:48 <@cf_nthomas> wherever Stage runs
OK. This is the version that I tested on staging-trunk-automation (http://staging-trunk-automation.build:8810/stage/builds/36/step-shell_5/0), updated against the latest code in the mofo repo. There's some statements at the bottom of the log:
Missing locale for platform 'linux': uk
Missing locale for platform 'linux': zh-CN
Missing locale for platform 'linux': ja
Missing locale for platform 'linux': gu-IN
Missing locale for platform 'linux': ka
Missing locale for platform 'linux': cs
Missing XPI locale for platform 'linux': uk
Missing XPI locale for platform 'linux': zh-CN
Missing XPI locale for platform 'linux': ja
Missing XPI locale for platform 'linux': gu-IN
Missing XPI locale for platform 'linux': ka
Missing XPI locale for platform 'linux': cs

These are caused by an out-of-date l10n mirror on staging-trunk-automation. There were other errors, but after refreshing my patch against the latest verify-locales.pl they've gone away.
Attachment #293856 - Attachment is obsolete: true
Attachment #293912 - Flags: review?(nrthomas)
Attachment #293856 - Flags: review?(nrthomas)
Attachment #293852 - Flags: review?(nrthomas) → review+
Comment on attachment 293912 [details] [diff] [review]
the patch tested on staging-trunk-automation

>Index: verify-locales.pl
...
>+   my $fileRegex = '^firefox\-[\d\.]+([ab][0-9]+)?\.tar\.' . $extension .
>+    '(\.asc)?$';
>+

I see you're adding support for alphas and betas here. Could you do that for win32 and mac too?

>    $shellReturnVal = (
>-    VerifyLinuxLocales(locales => $locales, dir => "$checkDir/linux-i686") and
>+    VerifyLinuxLocales(locales => $locales, dir => "$checkDir/linux-i686",
>+     extension => $linuxExtension) and
>     VerifyWin32Locales(locales => $locales, dir => "$checkDir/win32") and
>     VerifyMacLocales(locales => $locales, dir => "$checkDir/mac") );

The change here looks fine, but reading this again reminds me that we only do checks until we get an error, so if there's a problem with linux we never check win32 and mac. It would be really great if we could fix this and report all the errors in one go.

I think it makes sense to fix the first issue on this bug, so I'll r- even though everything else is just fine. If you're happy to fix the second issue here that's great, but feel free to punt to a follow-up bug. Please fix up the summary either way.
Attachment #293912 - Flags: review?(nrthomas) → review-
Summary: Teach verify-locales about tar.bz2 files on trunk → Teach verify-locales about tar.bz2 and alphas/betas
Attached patch support betas on all 3 platforms (obsolete) — Splinter Review
Attachment #293912 - Attachment is obsolete: true
Attachment #294059 - Flags: review?(nrthomas)
Attachment #294059 - Attachment is obsolete: true
Attachment #294063 - Flags: review?(nrthomas)
Attachment #294059 - Flags: review?(nrthomas)
Comment on attachment 294063 [details] [diff] [review]
diff against the right file this time

r+ with a couple of nits

>+   my $fileRegex = '^firefox\-[\d\.]+([b][0-9]+)?\.tar\.' . $extension .
>+    '(\.asc)?$';

Please use \d instead of [0-9], to match what's there already, and drop the [] around the b. ie (b\d+)?

>+   my $fileRegex = '^Firefox\sSetup\s[\d\.]+(\ Beta\ \d+)?\.exe(\.asc)?$';
>+

Please change the two \s to "\ ", for consistency across the file
Attachment #294063 - Flags: review?(nrthomas) → review+
Summary: Teach verify-locales about tar.bz2 and alphas/betas → Teach verify-locales about tar.bz2 and betas
Comment on attachment 293852 [details] [diff] [review]
[checked in] use new verify-locales option

Forgot to mention that this code

+    my $useTarGz = $config->Exists(var => 'useTarGz') ?
+     $config->Get(var => 'useTarGz') : 0;
+    my $linuxExtension = ($useTarGz) ? 'gz' : 'bz2';

is used four times in Bootstrap now, and it's probably worth factoring it out into a helper function. In someones copious free time :-)
(In reply to comment #15)
> (From update of attachment 293852 [details] [diff] [review])
> Forgot to mention that this code
> 
> +    my $useTarGz = $config->Exists(var => 'useTarGz') ?
> +     $config->Get(var => 'useTarGz') : 0;
> +    my $linuxExtension = ($useTarGz) ? 'gz' : 'bz2';
> 
> is used four times in Bootstrap now, and it's probably worth factoring it out
> into a helper function. In someones copious free time :-)
> 

Filed 409395 on this. I'll probably get to it over Christmas.
Carrying review forward.

Coop, can you land this now?
Attachment #294063 - Attachment is obsolete: true
Attachment #294209 - Flags: review+
Landed.

Checking in verify-locales.pl;
/mofo/release/stage/verify-locales.pl,v  <--  verify-locales.pl
new revision: 1.7; previous revision: 1.6
done
Checking in Bootstrap/Step/Stage.pm;
/cvsroot/mozilla/tools/release/Bootstrap/Step/Stage.pm,v  <--  Stage.pm
new revision: 1.30; previous revision: 1.29
done
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Attachment #294209 - Attachment description: fix regexes → [checked in] fix regexes
Attachment #293852 - Attachment description: use new verify-locales option → [checked in] use new verify-locales option
I've updated all of the release automation machines' (production-trunk-automation, staging-trunk-automation, staging-build-console, build-console) /data/cltbld/bin directory with the new version.
I did stage:~/bin too, just to complete the set.
Product: mozilla.org → Release Engineering
You need to log in before you can comment on or make changes to this bug.