Closed Bug 403993 Opened 17 years ago Closed 16 years ago

Make python compare-locales script work with SeaMonkey

Categories

(SeaMonkey :: Build Config, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: kairo, Assigned: asrail)

References

Details

Attachments

(4 files, 10 obsolete files)

257 bytes, patch
Details | Diff | Splinter Review
245 bytes, patch
Details | Diff | Splinter Review
213 bytes, patch
Pike
: review+
Details | Diff | Splinter Review
1.64 KB, patch
asrail
: review+
Details | Diff | Splinter Review
Bug 395396 made python compare-locales exceptions app-dependent, we should add a filter.py to suite for those.

Axel says we could also figure out there how to make extensions/irc optional for l10n.mk, if I understood correctly what he said on IRC.
You'll need an l10n.ini now, too. See bug 445217.
Summary: add suite/locales/filter.py to work with python compare-locales script → Make python compare-locales script work with SeaMonkey
Attached file Proposed l10n.ini for suite (obsolete) —
Requesting Kairo review for the list of files.

Remember to ask for Axel review latter.
Attachment #334303 - Flags: review?
Attachment #334303 - Flags: review? → review?(kairo)
Depends on: 445146
Comment on attachment 334303 [details]
Proposed l10n.ini for suite

Bug 445146 will require changes here.
Attachment #334303 - Attachment is obsolete: true
Attachment #334303 - Flags: review?(kairo)
probably, yes. The reason why I haven't looked into this yet is that we probably want a filter.py as well and I've had an attempt at that sitting around on my disk for some time now, but i still don't have compare-locales set up to be able to use it. Is there a good doc about how to set it up?
(In reply to comment #4)
> Is there a good doc about how to set it up?

http://developer.mozilla.org/en/Compare-locales
It will require a change on the file: remove the reference to mozilla/editor/ui.
Probably, nothing else. But I do prefer to wait and test,

SeaMonkey will require the changes to the dtd's, to remove the incorrect comments before working properly.
Some solution to the "#expand" will have to be found.

SeaMonkey will also require a filter.py, but the work here doesn't depend on that.
(In reply to comment #6)
> It will require a change on the file: remove the reference to
> mozilla/editor/ui.
> Probably, nothing else. But I do prefer to wait and test,
> 
> SeaMonkey will require the changes to the dtd's, to remove the incorrect
> comments before working properly.

Those not-actually-incorrect comments have a bug filed, right? And AFAIK, there was a positive review for it.

> Some solution to the "#expand" will have to be found.

Part of bug 451601.

> SeaMonkey will also require a filter.py, but the work here doesn't depend on
> that.

Oh, that is what this bug was originally filed for :)
Depends on: 451601
(In reply to comment #7)
> Those not-actually-incorrect comments have a bug filed, right? And AFAIK, there
> was a positive review for it.

I haven't found it. But that should block this bug.

> > Some solution to the "#expand" will have to be found.
> 
> Part of bug 451601.

Blocking this one now.

> > SeaMonkey will also require a filter.py, but the work here doesn't depend on
> > that.
> 
> Oh, that is what this bug was originally filed for :)

I meant the patches are orthogonal.
(In reply to comment #8)
> (In reply to comment #7)
> > Those not-actually-incorrect comments have a bug filed, right? And AFAIK, there
> > was a positive review for it.
> 
> I haven't found it. But that should block this bug.

Bug 451065 - unless it's other comments you're talking of.
Depends on: 451065
This is the filter.py I had lying around for a while and the l10n.ini from asrail. Not sure if all this is correct for the current compare-locales version, so requesting review from Axel, who probably knows compare-locales better than anyone else. ;-)

Even if bug 445146 probably needs us to change the editor/ui stuff, I want us to get something in.
Assignee: nobody → kairo
Status: NEW → ASSIGNED
Attachment #336195 - Flags: review?(l10n)
+  if mod not in ("netwerk", "dom", "toolkit", "security/manager",
+                 "extensions/reporter", "extensions/spellcheck",
+                 "extensions/irc", "extensions/venkman",
+                 "editor/ui", "suite"):
+    return False
+  if mod != "suite" and mod != "extensions/spellcheck" and mod != "extensions/irc" and mod != "extensions/venkman":

nit: you can use "mot not in" tuple on the second if, just like the first one.

+  if mod == "extensions/irc" and loc in irc_locales:
+    # compare chatzilla if current locale is said to be present for it
+    return True
+  if mod == "extensions/venkman" and loc in venkman_locales:
+    # compare venkman if current locale is said to be present for it
+    return True
...
+  return True

Are you willing to not compare venkman and CZ if the language is not said to be present for it?

Both will fail to the last "return True" statement and will be compared anyway.




What to do about the import wizard variants?
The translator may choose to add or remove some of the entries - I do that.

I use this:
if re.match(r".*_(ie|opera|dogbert|seamonkey|phoenix|safari|macie)", entity):
  return False

But this solution is far from perfect.
Maybe Axel could tell us if there is any elegant solution supported on compare-locales.
Assignee: kairo → asrail
Attached patch Update to the patch (obsolete) — Splinter Review
See comment #11.
Attachment #336195 - Attachment is obsolete: true
Attachment #336226 - Flags: review?(l10n)
Attachment #336195 - Flags: review?(l10n)
Comment on attachment 336226 [details] [diff] [review]
Update to the patch

>diff --git a/suite/locales/l10n.ini b/suite/locales/l10n.ini
>+[compare]
>+dirs = suite
>+    mozilla/editor/ui
>+    mozilla/extensions/venkman
>+    mozilla/extensions/inspector
>+    mozilla/extensions/typeaheadfind
>+    mozilla/extensions/webdav

webdav is dead and not even checked out any more by client.py, so it's wrong for sure. typeaheadfind contains no L10n as far as I know, so I wonder why it's listed here. And inspector L10n is not tracked by L10n repositories yet, is it really supposed to be listed here?

>+[extras]
>+dirs = mozilla/extensions/spellcheck
>+    mozilla/extensions/irc

Why is irc listed in [extras] and venkman in [compare]? They are both set up the same way, so I'd guess they should be treated the same. Both those dirs contain optional L10n, depending on their all-locales files.
Attached patch WIP (obsolete) — Splinter Review
Sorry, the issue with irc and inspector was a mistake.

I've removed the other dirs.


I've found a lot of issues with filter.py today.
I've made some changes, but some points still not working properly.


Kairo: you first wanted not to compare spellchek and latter wanted to compare it.
I've taken into account the latter and the comment which comes with it.



"loc" is not defined and the check for loc in irc or venkman locales fails.
I'll take a look into that and in why netwerk is not compared.


I've changed the entity check and noticed that the only files outside mozilla/extensions which are not compared are the .src and .png files, so I've changed that 'if'.


Not requesting review due the pending points.
Attachment #336226 - Attachment is obsolete: true
Attachment #336226 - Flags: review?(l10n)
(In reply to comment #14)
> I've found a lot of issues with filter.py today.

I'm sure of that, I created it without a lot of knowledge and based on an old copy of the Firefox one.

> Kairo: you first wanted not to compare spellchek and latter wanted to compare
> it.
> I've taken into account the latter and the comment which comes with it.

We want to go the same way Firefox goes there, and I guess that's not comparing spellcheck.

> "loc" is not defined and the check for loc in irc or venkman locales fails.
> I'll take a look into that and in why netwerk is not compared.

I suppose it was defined in an older compare-locales version then - it's supposed to hold the code of the compared locale.

> Not requesting review due the pending points.

I guess we probably need some input from Axel there.
With your newest patch, I get "// add and localize this file" warnings about all files in
mozilla/editor/ui/chrome
mozilla/extensions/irc
mozilla/extensions/spellcheck/hunspell
mozilla/extensions/venkman

So this doesn't really work that well, actually.
(In reply to comment #16)
> With your newest patch, I get "// add and localize this file" warnings about
> all files in
> mozilla/editor/ui/chrome
> mozilla/extensions/irc
> mozilla/extensions/spellcheck/hunspell
> mozilla/extensions/venkman
> 
> So this doesn't really work that well, actually.

compare-locales looks for files on "mozilla/extensions" on the translation side, so it doesn't find the folders.

hunspell won't be checked anymore.
About irc and venkman, maybe the solution will be to create l10n.ini for each one.

I think this solution is better than to hardcode the "mozilla" directory on compare-locales.
Attached patch l10n.ini for chatzilla (obsolete) — Splinter Review
This is required for using the newest compare-locales with Chatzilla.
Attached patch l10n.ini for venkman (obsolete) — Splinter Review
Attached patch l10n.ini for chatzilla (obsolete) — Splinter Review
Attachment #336303 - Attachment is obsolete: true
Attached patch Proposed patch (obsolete) — Splinter Review
I won't request review now because it requires a change on compare-locales.

You can test with compare-locales from here:
http://hg.mozilla.org/users/asrail_gmail.com/tooling

The web interface for this repository is broken as of now, but it works.
Attachment #336260 - Attachment is obsolete: true
Depends on: 453248
Attachment #336304 - Attachment is obsolete: true
Attachment #336305 - Attachment is obsolete: true
How would a localizer go about when they want to start translating, say, cz? I find it tough to require them to hack all-locales right from the start.

I probably need to redo the [includes] anyway, going for something that works both remote and local, I picture having something like

includes=irc,venkman,toolkit in [general], and make all those separate sections like
[irc]
localpath=mozilla/extensions/irc/locales/l10n.ini
url=http://mxr.mozilla.org/mozilla/source/extensions/irc/locales/l10n.ini
optional=True

and then have a param that would say

compare-locales --with=irc,venkman ...l10n.ini l10n-central ab-CD

We could make the build process pass in the params based on all-locales, so that by default it does the right thing.

What do you think?

PS: I'm not yet sure how to get changes detected in buildbot.
Axel, I like your idea, and if it's going to be implemented this year still, then I feel good about just excluding chatzilla and venkman from compare-locales in SeaMonkey for now, they both are running compare-locales.pl at build time right now, so we have some kind of check there, and once we got your solution, we'll have something that really works.
l10.ini required, since it is inside "mozilla" directory.
Attachment #336778 - Flags: review?(l10n)
- Updating for the new location of "editor/ui"
 - Removing the references to venkman and chatzilla
 - Not requiring anymore changes to the parameters passed to the test function
 - Requires l10n.ini for "reporter" (attached)
Attachment #336309 - Attachment is obsolete: true
Attachment #336780 - Flags: review?(l10n)
(In reply to comment #24)
> How would a localizer go about when they want to start translating, say, cz? I
> find it tough to require them to hack all-locales right from the start.

That's a good point.
The localizer always could use the l10n.ini from chatzilla or venkman and run separately from SM, but your proposal is better. 

> I probably need to redo the [includes] anyway, going for something that works
> both remote and local, I picture having something like
> 
> includes=irc,venkman,toolkit in [general], and make all those separate sections
> like
> [irc]
> localpath=mozilla/extensions/irc/locales/l10n.ini
> url=http://mxr.mozilla.org/mozilla/source/extensions/irc/locales/l10n.ini
> optional=True

I would prefer to list the obligatory sections on a proper place instead of using a boolean.

> and then have a param that would say
> 
> compare-locales --with=irc,venkman ...l10n.ini l10n-central ab-CD

Maybe the syntax used by the the autotools is more powerful and better to this case:
--with-irc (alternatively --with-irc=yes), --without-irc (alternatively --with-irc=no), --with-irc=[path] (assume --with-irc=yes)

So we can change the default from no to yes, from yes to no and use an alternative file, for whatever reason - for instance: a lot of trees, in case the person is also a developer.

> We could make the build process pass in the params based on all-locales, so
> that by default it does the right thing.

That's a nice idea.

> PS: I'm not yet sure how to get changes detected in buildbot.

I didn't understand this.
Attached patch Updating (obsolete) — Splinter Review
I had made a wrong assumption about new files and its entities.

This patch fix this bug and I've added a check to ignore hidden files (files starting with a dot), temporary files (files starting with a dot or a "#") and files from rejects of patches (files ending on ".orig" and ".rej").
Attachment #336780 - Attachment is obsolete: true
Attachment #336973 - Flags: review?(l10n)
Attachment #336780 - Flags: review?(l10n)
Axel: ping on the reviews.
Comment on attachment 336973 [details] [diff] [review]
Updating

Looks good to me, just that the comment on entity == None is wrong.

Make that

if entity is None:
  # missing and obsolete files

?
Attachment #336973 - Flags: review?(l10n) → review+
Attachment #336778 - Flags: review?(l10n) → review+
Attachment #336973 - Flags: approval-seamonkey2.0a1+
Keywords: checkin-needed
Keywords: push-needed
The last attachment has the approved patch as should be pushed on comm-central - just a comment and a minor changes.


Attachment 338778 [details] [diff] should be checked in on CVS.
The correct keyword is checkin-needed.

And attachment 338778 [details] [diff] [review] actually should go into mozilla-central, that's where reporter lives now.
Attachment #336430 - Attachment description: Addning all-locales → venkman patch, adding all-locales
Attachment #336431 - Attachment description: Adding all-locales → chatzilla patch, adding all-locales
Attachment 336778 [details] [diff] it is, actually.

Are the venkman and chatzilla patches still needed or obsolete?
And it's usual to mark the old patch obsolete and forward the review flags, i.e. mark them + yourself if they have been granted on the old one with comments.
I've seen some people using push-needed to triage mercurial patches.

Venkman and Chatzilla patches aren't obsoletes, but they won't be required for this part of the patch.

They can be used freely manually for each one if venkman or chatzilla people wants so, but they won't be used on suite until compare-locales got incremented.
Comment on attachment 337995 [details] [diff] [review]
Addressing Axel comments (checked in)

Forwarding Axel review.
Attachment #337995 - Flags: review+
Attachment #336973 - Attachment is obsolete: true
(In reply to comment #36)
> I've seen some people using push-needed to triage mercurial patches.

Wrongly... thanks.
Keywords: push-needed
Comment on attachment 337995 [details] [diff] [review]
Addressing Axel comments (checked in)

forwarding the approval.

And yes, push-needed is only for patches that need to be pushed *by the sheriff* for times when the tree only allows checkins from that sheriff - which is never true for comm-central ;-)
Attachment #337995 - Flags: approval-seamonkey2.0a1+
Pushed suite/ part as http://hg.mozilla.org/comm-central/rev/d1004044a681 and reporter part as http://hg.mozilla.org/mozilla-central/rev/ab5bd6c98c1e

asrail, I'm leaving it to you to mark fixed, or leave open until it fully works, however you like it :)
Keywords: checkin-needed
Attachment #336778 - Attachment description: l10n.ini for reporter → l10n.ini for reporter (checked in)
Attachment #337995 - Attachment description: Addressing Axel comments [should be pushed] → Addressing Axel comments (checked in)
From what I see, compare-locales works with SeaMonkey now, so closing this bug.
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: