Import compare-locales script into mozilla-central

RESOLVED FIXED in Firefox 39

Status

()

Core
Build Config
RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: rnewman, Assigned: Pike)

Tracking

(Blocks: 2 bugs)

28 Branch
mozilla39
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox39 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

4 years ago
See Bug 934196 for details.
There are (at least) three parts to this bug.

1. Land some version of compare-locales into the tree.  (This ticket.)

2. Stop developing alternate versions and instead develop the in tree version.  It's not clear to me who updates which repositories; more in a future comment.

3. Update automation to use the in tree version.
(In reply to Nick Alexander :nalexander from comment #1)
> There are (at least) three parts to this bug.
> 
> 1. Land some version of compare-locales into the tree.  (This ticket.)

I see several versions in the ether:

* http://hg.mozilla.org/build/compare-locales is tagged version 0.9.5 and is clearly getting tagged by ffxbld and tbirdbld automation.  So this appears to be the official repo used by Firefox automation.  It's not clear that this is the repo used by B2G automation.

* There's a reference to an l10n/compare-locales repository in the build/compare-locales repository:

changeset:   391:d72caa859d1b
parent:      368:a1d7b5cc1319
parent:      390:f4effbecaaec
user:        Aki Sasaki <asasaki@mozilla.com>
date:        Tue Feb 28 15:49:49 2012 -0800
summary:     merge l10n/compare-locales into build/compare-locales (RELEASE_0_9_5).

* https://github.com/Pike/compare-locales is tagged version 0.9.6 and yet claims to be a mirror build/compare-locales

pike, are there others that you know about?  Which do you consider authoritative?  Do you know if all automation (including B2G automation) is using build/compare-locales?
Flags: needinfo?(l10n)
(In reply to Nick Alexander :nalexander from comment #2)
> (In reply to Nick Alexander :nalexander from comment #1)
> > There are (at least) three parts to this bug.
> > 
> > 1. Land some version of compare-locales into the tree.  (This ticket.)
> 
> I see several versions in the ether:
> 
> * http://hg.mozilla.org/build/compare-locales is tagged version 0.9.5 and is
> clearly getting tagged by ffxbld and tbirdbld automation.  So this appears
> to be the official repo used by Firefox automation.  It's not clear that
> this is the repo used by B2G automation.

Curiously, if you |sudo easy_install -U compare-locales| (as recommended at https://developer.mozilla.org/en/docs/Compare-locales) you actually install a different version:

Searching for compare-locales
Reading https://pypi.python.org/simple/compare-locales/
Best match: compare-locales 0.9.6
Downloading https://pypi.python.org/packages/source/c/compare-locales/compare-locales-0.9.6.tar.gz#md5=bf3d54195f87e92b38ba787f1f6ad390
...
(In reply to Nick Alexander :nalexander from comment #1)
> There are (at least) three parts to this bug.
> 
> 1. Land some version of compare-locales into the tree.  (This ticket.)

I suppose part 1 b) is to get the Python sandbox to install compare-locales, and perhaps to expose it as a mach command.
(Assignee)

Comment 5

4 years ago
http://hg.mozilla.org/l10n/compare-locales/ and https://github.com/Pike/compare-locales are in sync, and the hg version is the official upstream repo.

The latest version isn't released on pypi at this point.
Flags: needinfo?(l10n)

Comment 6

4 years ago
+1 to moving this in tree. Core build system work keeps flirting with l10n repack code. We're moving in the direction of having pretty much everything except the translated files in tree. Might as well start here.
(Assignee)

Updated

3 years ago
Blocks: 973958

Updated

3 years ago
Blocks: 1013971
(Assignee)

Comment 7

3 years ago
I'm getting closer, I've started to think about good mach commands for this now.

The params I need are the path to the l10n.ini, the locale(s) to compare, and possibly a merge dir and whether to clobber it.

I'd prefer for these to have good defaults:

$(MOZ_BUILD_APP)/locales/l10n.ini for l10n.ini
$(OBJDIR)/$(MOZ_BUILD_APP)/locales/merge-$(AB_CD), clobbered, if given and object dir exists

Then you could do 
./mach compare-locales de fr it
and it would do the right thing.

Now, I'm trying to find out how to get the values above, and only found self.mozconfig['configure_args'] so far. Is parsing those really the best way to go about it?
(Reporter)

Updated

3 years ago
Flags: needinfo?(gps)
(In reply to Axel Hecht [:Pike] from comment #7)
> I'm getting closer, I've started to think about good mach commands for this
> now.
> 
> The params I need are the path to the l10n.ini, the locale(s) to compare,
> and possibly a merge dir and whether to clobber it.
> 
> I'd prefer for these to have good defaults:
> 
> $(MOZ_BUILD_APP)/locales/l10n.ini for l10n.ini
> $(OBJDIR)/$(MOZ_BUILD_APP)/locales/merge-$(AB_CD), clobbered, if given and
> object dir exists
> 
> Then you could do 
> ./mach compare-locales de fr it
> and it would do the right thing.
> 
> Now, I'm trying to find out how to get the values above, and only found
> self.mozconfig['configure_args'] so far. Is parsing those really the best
> way to go about it?

Try self.mozconfig.substs and self.mozconfig.defines to get the build configuration details.  I expect AB_CD to be funky -- is that actually known at configure/build-backend time?  I thought it was determined at build time and passed around as a shell variable.  Admittedly I am not expert in this area.
(Assignee)

Comment 9

3 years ago
Yeah, AB_CD should come from the commandline.

I found MOZ_BUILD_APP and L10NBASEDIR in self.substs, and there's self._topobjdir, so that seems to have all the things I need :-) Thanks.
self.config_environment has all of the build system state. Many of its properties are available directly on self. e.g. self.substs.

If you run |mach --debug-command <command>|, a Python debugger will activate on the first line of the mach command being executed. You can then poke around and see what variables are set. From the debugger, run "pp dir(self)" to print out the list of attributes available on "self". Pretty much all the build system state is hung off there (assuming your mach command class derives from mozbuild.base.MachCommandBase).
Flags: needinfo?(gps)
(Assignee)

Comment 11

2 years ago
Created attachment 8568213 [details]
mach_commands.py

Gregory, this kinda works, I'm interested in your feedback.

Concerns I have myself:

- path arguments and how to normalize values coming via self.substs.
- error handling. In particular, I have values that can come from the build but also via command line. I like that, but I don't know how to give proper user feedback. raise Exception() looked weird.
Assignee: nobody → l10n
Attachment #8568213 - Flags: feedback?(gps)
Comment on attachment 8568213 [details]
mach_commands.py

It would be easier to leave feedback if this were uploaded as a patch. But it's small, so it's OK.

There are a few nits in this code, but nothing major.

self.substs comes from values in <objdir>/config.status. Paths will either be relative or absolute. Normalize appropriately.

As for error handling, mach commands are like int main(). Just print() and return an integer status code. Returning 0 or None means success. If you raise an exception, it will get caught by mach's error handler and will be reported as an uncaught exception, which mach says to report as a bug. mach command handlers should catch exceptions if you know they may occur. However, if you expect the command to work, leaving them uncaught will print a stack and give you more to go on when an error report is filed.
Attachment #8568213 - Flags: feedback?(gps) → feedback+
(Assignee)

Updated

2 years ago
Depends on: 1136733
(Assignee)

Comment 13

2 years ago
Created attachment 8569988 [details]
MozReview Request: bz://940103/Pike

/r/4365 - bug 940103, import compare-locales in the tree
/r/4367 - bug 940103, add a mach command to call compare-locales

Pull down these commits:

hg pull review -r d30bb2ad1b4d9d26354129c1c2afb3b55d87e321
(Assignee)

Comment 14

2 years ago
Comment on attachment 8569988 [details]
MozReview Request: bz://940103/Pike

/r/4365 - bug 940103, import compare-locales in the tree
/r/4367 - bug 940103, add a mach command to call compare-locales

Pull down these commits:

hg pull review -r d30bb2ad1b4d9d26354129c1c2afb3b55d87e321
Attachment #8569988 - Flags: review?(gps)
https://reviewboard.mozilla.org/r/4365/#review3713

I assume this is just a dump. That's fine.

We'll probably want to hook up tests, etc to `make check` so compare-locales tests run as part of automation. See python/Makefile.in.
https://reviewboard.mozilla.org/r/4365/#review3717

I just noticed the comment referencing the upstream repo.

So, mach commands have the ability to download and install remote-hosted Python packages. If your intent is for compare-locales to be a Python package that lives outside of the tree, we may just want to restructure the mach command to install compare-locales from an external source.

Is there still a good reason to have compare-locales in tree? (Making mozilla-central the canonical location for compare locales would be a good reason.)
https://reviewboard.mozilla.org/r/4367/#review3719

::: python/compare-locales/mach_commands.py
(Diff revision 1)
> +        except Exception:

Should limit this to a mozbuild.base.BadEnvironmentException.

::: python/compare-locales/mach_commands.py
(Diff revision 1)
> +            if l10n_ini is None or l10n_base is None:

`is None` is only needed in comparisons when you need to be absolutely sure a value is exactly None, as opposed to other types that evaluate to false, such as `False` and the empty string.

In almost all circumstances (including those in this file), doing a simple `if x` or `if not x` check is sufficient.

::: python/compare-locales/mach_commands.py
(Diff revision 1)
> +        if l10n_ini is None:
> +            l10n_ini = mozpack.path.join(
> +                self.topsrcdir,
> +                self.substs['MOZ_BUILD_APP'],
> +                'locales', 'l10n.ini'
> +            )
> +
> +        if l10n_base is None:
> +            l10n_base = mozpack.path.join(
> +                self.topsrcdir,
> +                self.substs['L10NBASEDIR']
> +            )

I would move these assignment to inside the block where you access `self.substs`.

::: python/compare-locales/mach_commands.py
(Diff revision 1)
> +                # self.substs is raising an Exception if we're not configured
> +                # don't merge if we're not
> +                merge_dir = mozpack.path.join(
> +                    self.topobjdir,
> +                    self.substs['MOZ_BUILD_APP'],
> +                    'locales', 'merge-{ab_CD}'
> +                )

Move this into the same block accessing self.substs as well.

::: python/compare-locales/mach_commands.py
(Diff revision 1)
> +        print(codecs.utf_8_encode(observer.serialize())[0])

To go from a str/bytes to a unicode, do `s.encode('utf-8')`. By default, this barfs on invalid UTF-8 byte sequences. You can do `s.encode('utf-8', 'replace')` to replace invalid sequences with the special codepoint that represents an unknown character (I'm sure I butchered terminology here). See https://docs.python.org/2/library/stdtypes.html#str.encode
(Assignee)

Comment 18

2 years ago
(In reply to Gregory Szorc [:gps] from comment #16)
> https://reviewboard.mozilla.org/r/4365/#review3717
> 
> I just noticed the comment referencing the upstream repo.
> 
> So, mach commands have the ability to download and install remote-hosted
> Python packages. If your intent is for compare-locales to be a Python
> package that lives outside of the tree, we may just want to restructure the
> mach command to install compare-locales from an external source.
> 
> Is there still a good reason to have compare-locales in tree? (Making
> mozilla-central the canonical location for compare locales would be a good
> reason.)

The intent is to be able to update compare-locales and have it ride the trains. That way we can make changes to compare-locales which effectively change the strings we ship without having to watch their impact on ESR releases or beta etc.

Thanks for the other comments, I'll have a look at those later.
(Assignee)

Comment 19

2 years ago
https://reviewboard.mozilla.org/r/4367/#review3727

> Should limit this to a mozbuild.base.BadEnvironmentException.

This is paired to http://mxr.mozilla.org/mozilla-central/source/python/mozbuild/mozbuild/base.py#256

255         if not os.path.exists(config_status):
256             raise Exception('config.status not available. Run configure.')

Can't limit this unless I tweak mozbuild.

> Move this into the same block accessing self.substs as well.

I'd agree if I could limit the check if self.substs to be a specific exception.
https://reviewboard.mozilla.org/r/4367/#review3729

> This is paired to http://mxr.mozilla.org/mozilla-central/source/python/mozbuild/mozbuild/base.py#256
> 
> 255         if not os.path.exists(config_status):
> 256             raise Exception('config.status not available. Run configure.')
> 
> Can't limit this unless I tweak mozbuild.

I wrote a patch to change this exception earlier today. BadEnvironmentException will work in hopefully <24 hours.
(Assignee)

Comment 21

2 years ago
https://reviewboard.mozilla.org/r/4367/#review3931

> I wrote a patch to change this exception earlier today. BadEnvironmentException will work in hopefully <24 hours.

Do you have a bug number? I don't see that change in the repo yet.

> To go from a str/bytes to a unicode, do `s.encode('utf-8')`. By default, this barfs on invalid UTF-8 byte sequences. You can do `s.encode('utf-8', 'replace')` to replace invalid sequences with the special codepoint that represents an unknown character (I'm sure I butchered terminology here). See https://docs.python.org/2/library/stdtypes.html#str.encode

I'll make this change in compare-locales, too, where I copied that part from.
(Assignee)

Comment 22

2 years ago
Flagging this with a needinfo, is there a bug to follow about the change of the exception?
Flags: needinfo?(gps)
(Assignee)

Updated

2 years ago
Depends on: 1144679
(Assignee)

Comment 23

2 years ago
Comment on attachment 8569988 [details]
MozReview Request: bz://940103/Pike

/r/4365 - bug 940103, import compare-locales in the tree
/r/4367 - bug 940103, add a mach command to call compare-locales

Pull down these commits:

hg pull review -r 06acceb5dfad43d9e1d7cf4b12d1634d67df8165
(Assignee)

Comment 24

2 years ago
I rebased and fixed most of the comments.

I ended up leaving the mergedir config outside of the self.subst block, 'cause it's exception handling is different, aka, for the first, it's "you gotta have both of these". For merge, it's "either way is fine".

I didn't bother waiting for a more precise exception from base.
Flags: needinfo?(gps)

Updated

2 years ago
Attachment #8569988 - Flags: review?(gps) → review+
Comment on attachment 8569988 [details]
MozReview Request: bz://940103/Pike

https://reviewboard.mozilla.org/r/4363/#review4817

LGTM!
(Assignee)

Comment 26

2 years ago
Thanks. Flagging that this is ready for landing. Yay.
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/153252a2e6bc
https://hg.mozilla.org/integration/mozilla-inbound/rev/84b451463f06
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/153252a2e6bc
https://hg.mozilla.org/mozilla-central/rev/84b451463f06
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox39: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
(Assignee)

Comment 29

2 years ago
Comment on attachment 8569988 [details]
MozReview Request: bz://940103/Pike
Attachment #8569988 - Attachment is obsolete: true
Attachment #8618057 - Flags: review+
Attachment #8618058 - Flags: review+
(Assignee)

Comment 30

2 years ago
Created attachment 8618057 [details]
MozReview Request: bug 940103, import compare-locales in the tree
(Assignee)

Comment 31

2 years ago
Created attachment 8618058 [details]
MozReview Request: bug 940103, add a mach command to call compare-locales
You need to log in before you can comment on or make changes to this bug.