408868 - Teach verify-locales about tar.bz2 and betas

Status: RESOLVED FIXED

(Release Engineering :: General, defect, P2)

Description

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

Comment 1

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

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.

Comment 2

[[e-mail] (:bhearsum)]

Attachment: teach verify-locales about bz2

This patch makes verify-locales.pl support gz and bz2 files for Linux packages.

Comment 3

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

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/

Comment 4

[[e-mail] (:bhearsum)]

Attachment: require --linux-extension as a command-line option

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.

Comment 5

[[e-mail] (:bhearsum)]

Attachment: [checked in] use new verify-locales option

This patch adds the '-l' option to the verify-locales call in Bootstrap::Step::Stage to be compatible with the other patch attached here.

Comment 6

[[e-mail] (:bhearsum)]

Attachment: use tabs instead of spaces in usage statement

Sorry, didn't notice that there was some real tabs in here. Spacing is fixed in this patch.

Comment 7

[[e-mail] (:bhearsum)]

There's something wrong with the verify-locales patch..investigating now..

Comment 8

[[e-mail] (:bhearsum)]

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

Comment 9

[[e-mail] (:bhearsum)]

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

Comment 10

[[e-mail] (:bhearsum)]

Attachment: the patch tested on staging-trunk-automation

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.

Comment 11

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

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.

Comment 12

[[e-mail] (:bhearsum)]

Attachment: support betas on all 3 platforms

Comment 13

[[e-mail] (:bhearsum)]

Attachment: diff against the right file this time

Comment 14

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

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

Comment 15

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

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

Comment 16

[[e-mail] (:bhearsum)]

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

Comment 17

[[e-mail] (:bhearsum)]

Attachment: [checked in] fix regexes

Carrying review forward.

Coop, can you land this now?

Comment 18

[Chris Cooper [:coop] (he/him)]

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

Comment 19

[[e-mail] (:bhearsum)]

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

Comment 20

[[e-mail] (:bhearsum)]

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.

Comment 21

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

I did stage:~/bin too, just to complete the set.