Closed Bug 650217 Opened 9 years ago Closed 9 years ago

L10n verification always failing on release builds

Categories

(Release Engineering :: General, defect, P2)

defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: standard8, Assigned: rail)

References

Details

(Whiteboard: [l10n][automation])

Attachments

(1 file, 1 obsolete file)

I've noticed this for the last few releases that we've done. It appears that the L10n metadiff step of L10n verification is failing on the diff.

However in the evaluateCommand function of L10nVerifyMetaDiff, the code states this:

'''We ignore failures here on purpose, since diff will 
   return 1(FAILURE) if it actually finds anything to output.
'''

This is at: http://hg.mozilla.org/build/buildbotcustom/file/c99163a10cb8/steps/release.py#l49

See also: http://hg.mozilla.org/build/buildbotcustom/file/c99163a10cb8/steps/release.py#l33

I'm pretty sure this was originally ignoring failure, and somehow it is now broken.

Here's a log from the 3.6.17 builds that shows this happening:

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox3.6-Release/1302807793.1302808578.7132.gz

created metadiff failed
=== Output ===
diff -r firefox-3.6.17-build2/diffs firefox-3.6.16-build1/diffs
 in dir /builds/moz2_slave/rel-192-w32-l10n-verification/tools/release/l10n (timeout 1200 secs)
 watching logfiles {}
 argv: ['diff', '-r', 'firefox-3.6.17-build2/diffs', 'firefox-3.6.16-build1/diffs']
...
<lots of diffs>
...
< < # NOTE: The restartLaterButton string is also used in
< < # mozapps/extensions/content/blocklist.js
program finished with exit code 1
elapsedTime=3.346318
=== Output ended ===
======== BuildStep ended ========
Rail tells me on irc that the change was intentional:

rail: Standard8, it fails if you update l10n changesets
rail: initial purpose of it was to check if something changed in translation since prev stable release

I don't see the changeset where that was intentional though.

If it is really what we want, then I propose the following changes:

- Remove the comment in L10nVerifyMetaDiff as it is clearly not correct
- Change the build to warn if the l10n meta diff is different. Preferably printing that changesets have updated or somehow note that the result can generally be expected.

My reasoning for the change to a warning: Nothing in the l10n verification or the builds is actually broken. This step is really intended as a test and showing a test failure is more logical.
Blocks: 627271
Whiteboard: [l10n][automation]
(In reply to comment #1)
> My reasoning for the change to a warning: Nothing in the l10n verification or
> the builds is actually broken. This step is really intended as a test and
> showing a test failure is more logical.

I wholeheartedly agree.
Priority: -- → P3
Assignee: nobody → rail
Priority: P3 → P2
Attached patch don't use self.super_class (obsolete) — Splinter Review
Not sure what happens here, but self.super_class.evaluateCommand in http://hg.mozilla.org/build/buildbotcustom/file/28ca612b4a2c/steps/release.py#l52
is resolved as <unbound method C.evaluateCommand>, so parent's .evaluateCommand is not called.

Calling it as TinderboxShellCommand.evaluateCommand fixes the problem, but I'm not sure if the solution is reconfig safe.
Attachment #550312 - Flags: feedback?(dustin)
(In reply to comment #3)
Last time I got an error like that, I checked my reload() statements to make sure there were no duplicates.  Found and removed a duplicate reload(), and the problem went away.
Your solution doesn't sound reconfig-safe, no.  Recall the problem occurs on a *failed* reconfig, not a successful one.

I don't immediately see why the parent class would evaluate incorrectly, though.  Is TinderboxShellCommand ever replaced with C?
This line http://hg.mozilla.org/build/buildbotcustom/file/d76595b99f4e/steps/misc.py#l250 assigns buildbotcustom.steps.base.C instead of ShellCommand which is replaced here: http://hg.mozilla.org/build/buildbotcustom/file/d76595b99f4e/steps/base.py#l24. addErrorCatching adds its own checks if log_eval_func is not set and uses the worst value. Faking log_eval_func for L10nVerifyMetaDiff resolves the problem so addErrorCatching doesn't run its own checks.
Attachment #550312 - Attachment is obsolete: true
Attachment #550312 - Flags: feedback?(dustin)
Attachment #550503 - Flags: review?(bhearsum)
Attachment #550503 - Flags: review?(bhearsum) → review+
Will be available after merge/reconfig.
Status: NEW → RESOLVED
Closed: 9 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.