940103 - Import compare-locales script into mozilla-central

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Richard Newman [:rnewman]]

See Bug 934196 for details.

Comment 1

[Nick Alexander :nalexander [he/him]]

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.

Comment 2

[Nick Alexander :nalexander [he/him]]

(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 <[e-mail]>
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?

Comment 3

[Nick Alexander :nalexander [he/him]]

(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
...

Comment 4

[Nick Alexander :nalexander [he/him]]

(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.

Comment 5

[Axel Hecht [:Pike]]

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.

Comment 6

[Gregory Szorc [:gps]]

+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.

Comment 7

[Axel Hecht [:Pike]]

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?

Comment 8

[Nick Alexander :nalexander [he/him]]

(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.

Comment 9

[Axel Hecht [:Pike]]

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.

Comment 10

[Gregory Szorc [:gps]]

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).

Comment 11

[Axel Hecht [:Pike]]

Attachment: 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.

Comment 12

[Gregory Szorc [: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.

Comment 13

[Axel Hecht [:Pike]]

Attachment: 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

Comment 14

[Axel Hecht [:Pike]]

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

Comment 15

[Gregory Szorc [: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.

Comment 16

[Gregory Szorc [:gps]]

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.)

Comment 17

[Gregory Szorc [:gps]]

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

Comment 18

[Axel Hecht [:Pike]]

(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.

Comment 19

[Axel Hecht [:Pike]]

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.

Comment 20

[Gregory Szorc [:gps]]

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.

Comment 21

[Axel Hecht [:Pike]]

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.

Comment 22

[Axel Hecht [:Pike]]

Flagging this with a needinfo, is there a bug to follow about the change of the exception?

Comment 23

[Axel Hecht [:Pike]]

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

Comment 24

[Axel Hecht [:Pike]]

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.

Comment 25

[Gregory Szorc [:gps]]

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

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

LGTM!

Comment 26

[Axel Hecht [:Pike]]

Thanks. Flagging that this is ready for landing. Yay.

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/153252a2e6bc
https://hg.mozilla.org/integration/mozilla-inbound/rev/84b451463f06

Comment 28

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/153252a2e6bc
https://hg.mozilla.org/mozilla-central/rev/84b451463f06

Comment 30

[Axel Hecht [:Pike]]

Attachment: MozReview Request: bug 940103, import compare-locales in the tree

Comment 31

[Axel Hecht [:Pike]]

Attachment: MozReview Request: bug 940103, add a mach command to call compare-locales