Improve l10n repack error message when locale doesn't contain necessary files

RESOLVED FIXED in Firefox 38

Status

RESOLVED FIXED
4 years ago
a year ago

People

(Reporter: glandium, Assigned: glandium)

Tracking

unspecified
mozilla39

Firefox Tracking Flags

(firefox38 fixed, firefox39 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Comment hidden (empty)
(Assignee)

Comment 3

4 years ago
Attachment #8582765 - Attachment is obsolete: true
Attachment #8582786 - Attachment is obsolete: true
Attachment #8582765 - Flags: review?(mshal)
Attachment #8582786 - Flags: review?(mshal)
Attachment #8582795 - Flags: review?(mshal)
Comment on attachment 8582795 [details] [diff] [review]
Improve l10n repack error message when locale doesn't contain necessary files

>+            if base not in l10n_paths:
>+                errors.fatal("Locale doesn't contain %s/" % base)
>+                continue
>+            if e.name not in l10n_paths[base]:
>+                errors.fatal("Locale doesn't have a manifest entry for '%s'" %
>+                    e.name)
>+                continue

Why did you need to add the continues here? It looks like errors.fatal() should raise an exception.
Attachment #8582795 - Flags: review?(mshal) → review+
(Assignee)

Comment 5

4 years ago
(In reply to Michael Shal [:mshal] from comment #4)
> Comment on attachment 8582795 [details] [diff] [review]
> Improve l10n repack error message when locale doesn't contain necessary files
> 
> >+            if base not in l10n_paths:
> >+                errors.fatal("Locale doesn't contain %s/" % base)
> >+                continue
> >+            if e.name not in l10n_paths[base]:
> >+                errors.fatal("Locale doesn't have a manifest entry for '%s'" %
> >+                    e.name)
> >+                continue
> 
> Why did you need to add the continues here? It looks like errors.fatal()
> should raise an exception.

errors.accumulate(), which is used in the caller, inhibits those exceptions, accumulates the errors, and then throws an aggregated exception with all the errors.
https://hg.mozilla.org/mozilla-central/rev/b23c836ff69a
Status: NEW → RESOLVED
Last Resolved: 4 years ago
status-firefox39: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Comment on attachment 8582795 [details] [diff] [review]
Improve l10n repack error message when locale doesn't contain necessary files

I thought we could get away without it, but it seems we need to uplift this one for bug 1147207 to cleanly apply. Please see that bug for approval comment.
Attachment #8582795 - Flags: approval-mozilla-beta?
Comment on attachment 8582795 [details] [diff] [review]
Improve l10n repack error message when locale doesn't contain necessary files

Scratch that, you've already uplifted it. Sorry I missed it!
Attachment #8582795 - Flags: approval-mozilla-beta?

Updated

a year ago
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.