Closed Bug 940103 Opened 10 years ago Closed 9 years ago

Import compare-locales script into mozilla-central

Categories

(Firefox Build System :: General, defect)

Product:
Firefox Build System
Component:
General
Version:
28 Branch
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox39 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla39
Tracking Flags:
Tracking Status
firefox39 --- fixed

People

(Reporter: rnewman, Assigned: Pike)

References

Details

Attachments

(3 files, 1 obsolete file)

mach_commands.py
1.77 KB, text/plain
gps
: feedback+
Details
MozReview Request: bug 940103, import compare-locales in the tree
39 bytes, text/x-review-board-request
gps
: review+
Details
MozReview Request: bug 940103, add a mach command to call compare-locales
39 bytes, text/x-review-board-request
gps
: review+
Details 
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.
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)
+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.
Blocks: 973958
Blocks: 1013971
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?
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.
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)
Attached file 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+
Depends on: 1136733
Attached file MozReview Request: bz://940103/Pike (obsolete) — 
/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 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
(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.
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.
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.
Flagging this with a needinfo, is there a bug to follow about the change of the exception?
Flags: needinfo?(gps)
Depends on: 1144679
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
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)
Attachment #8569988 - Flags: review?(gps) → review+
Comment on attachment 8569988 [details]
MozReview Request: bz://940103/Pike

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

LGTM!
Thanks. Flagging that this is ready for landing. Yay.
Keywords: checkin-needed

        Attachment #8569988 -
        Attachment is obsolete: true

        Attachment #8618057 -
        Flags: review+

        Attachment #8618058 -
        Flags: review+

    
  
Product: Core → Firefox Build System

          You need to log in
          before you can comment on or make changes to this bug.