Closed Bug 451398 Opened 14 years ago Closed 13 years ago

Need a BuildFactory containing all the necessary steps to perform l10n verification.

Categories

(Release Engineering :: General, defect, P2)

defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: coop, Assigned: coop)

References

Details

Attachments

(4 files, 2 obsolete files)

We need to write a BuildFactory to encapsulate all the necessary steps to perform l10n verification.
Priority: -- → P3
Status: NEW → ASSIGNED
Priority: P3 → P2
Hey Coop, is there any chance this will be done before Beta 2?
I'm testing this patch locally now. I'm using 2.0.0.x releases for my testing (no l10n in 3.1 alpha releases, and many more locales in 3.0), but it should make no difference logic-wise.
I ran into while testing locally. If the mounting of a DMG fails for whatever reason and the script can't extract the DEV_NAME or MOUNTPOINT from the output file, the rsync command will continue, but in the absence of a MOUNTPOINT will simply start rsync-ing the contents of the root dir (/). This is a great way to fill up your available space quickly.
Attachment #348791 - Flags: review?(bhearsum)
Attachment #348792 - Flags: review?(bhearsum)
Not sure if this factory is in the right place sequentially or not.
Attachment #348798 - Flags: review?(bhearsum)
I've run all these patches through my local buildbot setup successfully. I've used both 2.0.0.18 and 3.0.4 for testing purposes, and the output is comparable to the Bootstrap results in production for l10nverify for those two releases.
Attachment #348791 - Flags: review?(bhearsum) → review+
Comment on attachment 348792 [details] [diff] [review]
Adds an L10nVerify factory and associated build step

>Index: process/factory.py
>===================================================================

>+class L10nVerifyFactory(ReleaseFactory):
>+    def __init__(self, cvsroot, buildTools,
>+                stagingServer, productName,
>+                 currentAppVersion, currentBuildNumber,
>+                 previousAppVersion, previousBuildNumber,

Please make these consistent with the other factories. Eg, appVersion, buildNumber, oldAppVersion, oldBuildNumber

>+                 verifyDir='verify',
>+                 linuxExtension='bz2'):
>+
>+        ReleaseFactory.__init__(self)
>+
>+        productDir = 'build/%s/%s-%s' % (verifyDir, 
>+                                         productName,
>+                                         str(currentAppVersion))

version numbers are stored as strings - no need for the str() call.

>+        self.addStep(ShellCommand,
>+         description=['remove', 'verify', 'dir'],
>+         description=['removed', 'verify', 'dir'],

The one should be descriptionDone. It's totally fine to leave out descriptionDone too - your call.

>+                  '%s:/home/ftp/pub/%s/nightly/%s-candidates/build%s/*' %
>+                   (stagingServer, productName, str(currentAppVersion),
>+                    str(currentBuildNumber)),
>+                  '%s-%s-build%s/' % (productName, str(currentAppVersion),
>+                                      str(currentBuildNumber))

Same thing here about the str().


>Index: steps/l10n.py
>===================================================================

Can you move this to steps/release.py - I think it's better off living there.

>+class VerifyL10n(ShellCommand):
>+    """Run the l10n verification script.
>+    """
>+    def __init__(self, **kwargs):

Can you add product here explicitly? It's a little nicer when trying to figure how something is supposed to be called.

>+        if not 'command' in kwargs:
>+            assert 'product' in kwargs
>+            self.command = "./verify_l10n.sh " + kwargs['product']

["bash", "-c", "verify_l10n.sh"] is the more supported way to run commands - any reason we can't do this here?

>+        ShellCommand.__init__(self, **kwargs)
>+    
>+    def evaluateCommand(self, cmd):
>+        superResult = ShellCommand.evaluateCommand(self, cmd)
>+        fileWarnings = self.getProperty('fileWarnings')
>+        if fileWarnings and len.fileWarnings > 0:
>+            return WARNINGS
>+        return superResult
>+    
>+    def createSummary(self, log):

Nice :)
>+        if unmatchedFiles and len.unmatchedFiles > 0:

itym len(unmatchedFiles) here.

>+          self.addCompleteLog('Only in...', "\n".join([`unmatchedFile` for unmatchedFile in xrange(unmatchedFiles)]))

I'm not sure what's going on here - it looks like xrange() takes integer args.

>+          self.addCompleteLog('Warnings', "\n".join([`fileWarning` for fileWarning in xrange(fileWarnings)]))

Same here



Overall this looks good - fix the nits above and this is good to go.
Attachment #348792 - Flags: review?(bhearsum) → review-
Attachment #348798 - Flags: review?(bhearsum) → review+
Comment on attachment 348798 [details] [diff] [review]
Add L10nVerify factory to staging release config

>@@ -191,6 +192,24 @@ builders.append({
>     'factory': updates_factory
> })
> 
>+l10n_verification_factory = L10nVerifyFactory(

To be consistent with 1.8/1.9, let's put this directly before updates. There's no need to hook up any schedulers to it, though, since we don't have a "wait until signing log is published" Builder yet.

>+    cvsroot=cvsroot,
>+    buildTools=buildTools,
>+    stagingServer=stagingServer,
>+    productName=productName,
>+    currentAppVersion=appVersion,
>+    currentBuildNumber=buildNumber,
>+    previousAppVersion=oldVersion,
>+    previousBuildNumber=oldBuildNumber
>+)

Some of these will need updating.


r=bhearsum with the aforementioned changes.
Addressed your comments and removed the abortive xrange code I was using to test various join options for speediness.

More importantly though, I revisited the Bootstrap logs from 1.8 and 1.9 and noticed I had things a little wrong. I've now changed the l10nverify step to be a simple ShellCommand and made the metadiff step gather the abbreviated logs. The actual l10nverify step never would have found anything for those logs anyway.
Attachment #348792 - Attachment is obsolete: true
Attachment #349192 - Flags: review?(bhearsum)
Attachment #349192 - Flags: review?(bhearsum) → review+
Comment on attachment 349192 [details] [diff] [review]
Adds an L10nVerify factory and associated build step, v2

Looks good
Comment on attachment 348791 [details] [diff] [review]
Check MOUNTPOINT and DEV_NAME for empty strings

changeset:   23:b9e9751d68df
Attachment #348791 - Flags: checked‑in+
Comment on attachment 349192 [details] [diff] [review]
Adds an L10nVerify factory and associated build step, v2

Checking in process/factory.py;
/cvsroot/mozilla/tools/buildbotcustom/process/factory.py,v  <--  factory.py
new revision: 1.44; previous revision: 1.43
done
Checking in steps/release.py;
/cvsroot/mozilla/tools/buildbotcustom/steps/release.py,v  <--  release.py
new revision: 1.2; previous revision: 1.1
done
Attachment #349192 - Flags: checked‑in+
Attachment #350150 - Flags: review?(ccooper) → review+
Attachment #348798 - Attachment is obsolete: true
Attachment #350150 - Flags: checked‑in+
Comment on attachment 350150 [details] [diff] [review]
update master patch for staging and production

changeset:   542:6d27e3a5eb0f
Hm, looks like a problem with this related to the new directory structure.

The factory failed to download the new builds because the --exclude=* rule catches directory names, too, and ends up tossing everything out:
rsync -Lav -e ssh --include=*.dmg --include=*.exe --include=*.tar.bz2 --exclude=* stage-old.mozilla.org:/home/ftp/pub/firefox/nightly/3.1b2-candidates/build1/* firefox-3.1b2-build1/
receiving file list ... done
client: nothing to do: perhaps you need to specify some filenames or the --recursive option?

I fixed it with:
rsync -Lav -e ssh --exclude=*.asc --exclude=source --exclude=xpi --exclude=unsigned --exclude=update .....
Attachment #350338 - Flags: review?(ccooper)
Comment on attachment 350338 [details] [diff] [review]
fix the download rsync for l10nverify

Did you try --recursive?

I'm fine with this since there shouldn't be any random crap in those dirs to pollute the rsync, but that would normally be my worry with using explicit --excludes
Attachment #350338 - Flags: review?(ccooper) → review+
Hm, I didn't try that - no.
Attachment #350338 - Flags: checked‑in+
Comment on attachment 350338 [details] [diff] [review]
fix the download rsync for l10nverify

Checking in factory.py;
/cvsroot/mozilla/tools/buildbotcustom/process/factory.py,v  <--  factory.py
new revision: 1.45; previous revision: 1.44
done
How's it working for you, bhearsum? Can we resolve this yet?
Yep, it worked great for 3.1b2! Thanks for all your work here coop!
Status: ASSIGNED → 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.