If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Teach verify-locales about tar.bz2 and betas

RESOLVED FIXED

Status

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

People

(Reporter: nthomas, Assigned: bhearsum)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 6 obsolete attachments)

(Reporter)

Description

10 years ago
 
(Reporter)

Comment 1

10 years ago
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)

Updated

10 years ago
Assignee: nobody → bhearsum
Priority: P3 → P2
(Assignee)

Comment 2

10 years ago
Created attachment 293729 [details] [diff] [review]
teach verify-locales about bz2

This patch makes verify-locales.pl support gz and bz2 files for Linux packages.
Attachment #293729 - Flags: review?(nrthomas)
(Reporter)

Comment 3

10 years ago
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/
(Assignee)

Comment 4

10 years ago
Created attachment 293851 [details] [diff] [review]
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.
Attachment #293729 - Attachment is obsolete: true
Attachment #293851 - Flags: review?(nrthomas)
Attachment #293729 - Flags: review?(nrthomas)
(Assignee)

Comment 5

10 years ago
Created attachment 293852 [details] [diff] [review]
[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.
Attachment #293852 - Flags: review?(nrthomas)
(Assignee)

Comment 6

10 years ago
Created attachment 293856 [details] [diff] [review]
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.
Attachment #293851 - Attachment is obsolete: true
Attachment #293856 - Flags: review?(nrthomas)
Attachment #293851 - Flags: review?(nrthomas)
(Assignee)

Comment 7

10 years ago
There's something wrong with the verify-locales patch..investigating now..
(Assignee)

Comment 8

10 years ago
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...
(Assignee)

Comment 9

10 years ago
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
(Assignee)

Comment 10

10 years ago
Created attachment 293912 [details] [diff] [review]
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.
Attachment #293856 - Attachment is obsolete: true
Attachment #293912 - Flags: review?(nrthomas)
Attachment #293856 - Flags: review?(nrthomas)
(Reporter)

Updated

10 years ago
Attachment #293852 - Flags: review?(nrthomas) → review+
(Reporter)

Comment 11

10 years ago
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-
(Assignee)

Updated

10 years ago
Summary: Teach verify-locales about tar.bz2 files on trunk → Teach verify-locales about tar.bz2 and alphas/betas
(Assignee)

Comment 12

10 years ago
Created attachment 294059 [details] [diff] [review]
support betas on all 3 platforms
Attachment #293912 - Attachment is obsolete: true
Attachment #294059 - Flags: review?(nrthomas)
(Assignee)

Comment 13

10 years ago
Created attachment 294063 [details] [diff] [review]
diff against the right file this time
Attachment #294059 - Attachment is obsolete: true
Attachment #294063 - Flags: review?(nrthomas)
Attachment #294059 - Flags: review?(nrthomas)
(Reporter)

Comment 14

10 years ago
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+
(Reporter)

Updated

10 years ago
Summary: Teach verify-locales about tar.bz2 and alphas/betas → Teach verify-locales about tar.bz2 and betas
Blocks: 355309
(Reporter)

Comment 15

10 years ago
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 :-)
(Assignee)

Comment 16

10 years ago
(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.
(Assignee)

Comment 17

10 years ago
Created attachment 294209 [details] [diff] [review]
[checked in] fix regexes

Carrying review forward.

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

Comment 18

10 years ago
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
(Assignee)

Comment 19

10 years ago
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
Last Resolved: 10 years ago
Resolution: --- → FIXED
(Assignee)

Updated

10 years ago
Attachment #294209 - Attachment description: fix regexes → [checked in] fix regexes
(Assignee)

Updated

10 years ago
Attachment #293852 - Attachment description: use new verify-locales option → [checked in] use new verify-locales option
(Assignee)

Comment 20

10 years ago
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.
(Reporter)

Comment 21

10 years ago
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.