1188483 - desktop_l10n.py should support --locale $locale:$revision

Status: RESOLVED FIXED

(Release Engineering :: Applications: MozharnessCore, defect)

Description

[Rail Aliiev [:rail]]

.. so we can use it for releases

Comment 1

[Rail Aliiev [:rail]]

Attachment: l10n_revisions.diff

this should deal with 2 things:

* we need to pass a list of locale:rev using multiple methods: buildbot property for buildbot bridge, env variable for the future (TC), and existing methods (--locale or config and file) 

* parsing revision after colon for all methods except it's a changesets file

It also should fix the issue where "additional_locales" is set when changesets are defined. In this case self.l10n_revisions wouldn't have any revision set for that locale and self.l10n_revisions[locale] would fail with KeyError.

I'll probably test this on date before asking for final r+.

Comment 2

[Jordan Lund (:jlund)]

Comment on attachment 8650601 [details] [diff] [review]
l10n_revisions.diff

Review of attachment 8650601 [details] [diff] [review]:
-----------------------------------------------------------------

I am extremely unfamiliar with l10n but with aki/mgerva gone, I suppose everyone is :)

::: testing/mozharness/mozharness/mozilla/l10n/locales.py
@@ +50,3 @@
>  
> +        # Buildbot property
> +        if hasattr(self, 'read_buildbot_config'):

DesktopSingleLocale inherits from BuildbotMixin so won't this be condition be met every time for desktop_l10n.py jobs regardless if we are in TC or this is a developer job? I think it's safe to call read_buildbot_config() and you don't want to actually read buildprops.json *if* you don't have buildbot_json_path in self.config

@@ +50,5 @@
>  
> +        # Buildbot property
> +        if hasattr(self, 'read_buildbot_config'):
> +            self.read_buildbot_config()
> +            locales = self.query_buildbot_property("locales")

this will query self.buildbot_properties. I'm guessing because you just read build_buildbot_config() above, I'm guessing you want to look in self.buildbot_config. It's confusing, I know but iirc:

self.buildbot_config == buildprops.json (everything bbot prop set before mh started)
self.buildbot_properties == a dict of items that is updated as mh runs and is sent back to bbot

@@ +59,5 @@
> +        if not locales and "MOZ_LOCALES" in os.environ:
> +            locales = os.environ["MOZ_LOCALES"].split()
> +
> +        # Command line or config
> +        if not locales and c.get("locales", None):

I like the pattern 'if not locales ...' to avoid nested conditions, premature breaks/returns, and Falsey valued assignments. I think I'll start using this style.

Though you could argue that it is simpler to condense the last two conditions to:

if not locales:
     locales = os.environ["MOZ_LOCALES"].split() or c.get("locales")

but being more verbose always works for me if you prefer :)

@@ +65,5 @@
> +
> +        # parse locale:revision if set
> +        if locales:
> +            for l in locales:
> +                if ":" in l:

I guess we shouldn't silently check this

speaking of which, do we have to use $locale:$rev pattern? wouldn't it be more durable if this was something like json that TC/bbot created before running mh? Where does this data come from in the TC or bbot case?

@@ +70,5 @@
> +                    # revision specified in locale string
> +                    locale, revision = l.split(":", 1)
> +                    self.l10n_revisions[locale] = revision
> +            # clean up locale by removing revisions
> +            locales = [l.split(":")[0] for l in locales]

should we be using the maxsplit=1 here like above split() call? I guess you can argue we have more serious problems if we have more than one ':' in a locale str :)

@@ +85,5 @@
>          for locale in ignore_locales:
>              if locale in locales:
>                  self.debug("Ignoring locale %s." % locale)
>                  locales.remove(locale)
>          for locale in additional_locales:

so additional_locales and ignore_locales will mutate the `locales` var but self.l10n_revisions will stay as is.

iiuc, I guess that's okay because:

- the additional_locales case won't have a specific rev and we will default to 'default'
- the ignore_locales case will just mean that those locales in self.l10n_revisions will just be skipped as they are not in the authoritative `locales`

still, I wonder if code comprehension wise, it is worth keeping self.l10n_revisions and query_locales() in sync
in `ignore_locales` (even though it may be confusing in code)

Comment 3

[Jordan Lund (:jlund)]

oops, I forgot to mention:

this patch rocks. I really like how you slipped this extra logic in without having to touch many places

Comment 4

[Rail Aliiev [:rail]]

Attachment: l10n_revs-mozharness.diff

interdiff: https://gist.github.com/rail/4b27a3a80e4d9b152a26

(In reply to Jordan Lund (:jlund) from comment #2)
> DesktopSingleLocale inherits from BuildbotMixin so won't this be condition
> be met every time for desktop_l10n.py jobs regardless if we are in TC or
> this is a developer job? I think it's safe to call read_buildbot_config()

I'm a bit paranoid here because this mixin is used in multiple places, including b2g builds. It's unfortunate that we have to use such approach, but I don't really see any better way to have guards in mixins. :/ It's not the end of the world though, I can survive with this. :)

> and you don't want to actually read buildprops.json *if* you don't have
> buildbot_json_path in self.config

read_buildbot_config() does that for me ;)

> this will query self.buildbot_properties. I'm guessing because you just read
> build_buildbot_config() above, I'm guessing you want to look in
> self.buildbot_config.

Bah, fixed.

> Though you could argue that it is simpler to condense the last two
> conditions to:
> 
> if not locales:
>      locales = os.environ["MOZ_LOCALES"].split() or c.get("locales")
> 
> but being more verbose always works for me if you prefer :)

I'll leave it as is to be consistent with other parts.

> > +                if ":" in l:
> 
> I guess we shouldn't silently check this

Adding self.debug()

> speaking of which, do we have to use $locale:$rev pattern? wouldn't it be
> more durable if this was something like json that TC/bbot created before
> running mh? Where does this data come from in the TC or bbot case?

We want to schedule these from TC and use BBB to run. In this case the only thing that you can pass runtime is buildbot properties. In the future, when we can run there in TC, we'll be explicitly passing them as --locale. But even in this case we don't have any good structured way to pass these runtime (probably except using TC's "extra").


> > +            locales = [l.split(":")[0] for l in locales]
> 
> should we be using the maxsplit=1 here like above split() call? I guess you
> can argue we have more serious problems if we have more than one ':' in a
> locale str :)

Meh, the split()[0] pattern works in any case. I didn't want to be too defensive/verbose.
 
> - the additional_locales case won't have a specific rev and we will default
> to 'default'

Yup. This is used in android partner repacks, which has not been touched in ages. If we want to support revisions there, we can do that whenever we are ready to use that code.

> still, I wonder if code comprehension wise, it is worth keeping
> self.l10n_revisions and query_locales() in sync
> in `ignore_locales` (even though it may be confusing in code)

Good point. Fixed.

Comment 5

[Rail Aliiev [:rail]]

Attachment: l10n_revs-mozharness.diff

Use en_us_binary_url from buildbot properties

Interdiff: https://gist.github.com/rail/cfdff31297a73aa471f5

Comment 6

[Rail Aliiev [:rail]]

I pushed the patch to https://hg.mozilla.org/projects/date/rev/0313a6bbfdbd
we can play with that version

Comment 7

[Rail Aliiev [:rail]]

Attachment: l10n.diff

I'd like to land this and make sure nothing is broken on nightlies as well.

Comment 8

[Jordan Lund (:jlund)]

Comment on attachment 8664952 [details] [diff] [review]
l10n.diff

Review of attachment 8664952 [details] [diff] [review]:
-----------------------------------------------------------------

sheep eet

::: testing/mozharness/mozharness/mozilla/l10n/locales.py
@@ +86,5 @@
> +                                        c['locales_file'])
> +            locales = self.parse_locales_file(locales_file)
> +
> +        if not locales:
> +            self.fatal("No locales set!")

this should log "you must have really done something wrong, I'm exhausted from trying to determine the locales you want!" :)

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a9a4292b2df3

Comment 10

[Carsten Book [:Tomcat]]

sorry had to back this out for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=14640908&repo=mozilla-inbound

Comment 11

[Rail Aliiev [:rail]]

Comment on attachment 8664952 [details] [diff] [review]
l10n.diff

It was backed out for some weird bustage:

 ========= Started 'scripts/scripts/b2g_build.py --target ...' failed (results: 2, elapsed: 0 secs) (at 2015-09-24 04:40:54.419148) =========
scripts/scripts/b2g_build.py --target emulator --config b2g/releng-emulator.py --debug --gaia-languages-file locales/languages_dev.json --gecko-languages-file gecko/b2g/locales/all-locales --repotool-revision v1.12.16
 in dir /builds/slave/b2g_m-in_emu-d_dep-00000000000/. (timeout 3600 secs) (maxTime 21600 secs)
 watching logfiles {}
 argv: ['scripts/scripts/b2g_build.py', '--target', 'emulator', '--config', 'b2g/releng-emulator.py', '--debug', '--gaia-languages-file', 'locales/languages_dev.json', '--gecko-languages-file', 'gecko/b2g/locales/all-locales', '--repotool-revision', 'v1.12.16']
 environment:
  CCACHE_HASHDIR=
  G_BROKEN_FILENAMES=1
  HG_SHARE_BASE_DIR=/builds/hg-shared
  HISTCONTROL=ignoredups
  HISTSIZE=1000
  HOME=/home/cltbld
  HOSTNAME=bld-linux64-spot-423.build.releng.usw2.mozilla.com
  LANG=en_US.UTF-8
  LESSOPEN=|/usr/bin/lesspipe.sh %s
  LOGNAME=cltbld
  MAIL=/var/spool/mail/cltbld
  MOZ_AUTOMATION=1
  MOZ_CRASHREPORTER_NO_REPORT=1
  PATH=/usr/local/bin:/usr/lib64/ccache:/usr/local/bin:/bin:/usr/bin:/usr/local/sbin:/usr/sbin:/sbin:/home/cltbld/bin
  PROPERTIES_FILE=/builds/slave/b2g_m-in_emu-d_dep-00000000000/buildprops.json
  PWD=/builds/slave/b2g_m-in_emu-d_dep-00000000000
  SHELL=/bin/bash
  SHLVL=1
  TERM=linux
  TINDERBOX_OUTPUT=1
  TMOUT=86400
  USER=cltbld
  _=/tools/buildbot/bin/python
 using PTY: False
04:40:54     INFO - MultiFileLogger online at 20150924 04:40:54 in /builds/slave/b2g_m-in_emu-d_dep-00000000000
04:40:54    FATAL - Must specify --repo
04:40:54    FATAL - Running post_fatal callback...
04:40:54    FATAL - Exiting -1
program finished with exit code 255
elapsedTime=0.198414


https://treeherder.mozilla.org/logviewer.html#?job_id=14640908&repo=mozilla-inbound

Comment 12

[Rail Aliiev [:rail]]

Attachment: l10n_v2.diff

Interdiff: https://gist.github.com/rail/014572ecd21cc04e30d9

It turned out I was hitting this condition https://dxr.mozilla.org/mozilla-central/source/testing/mozharness/mozharness/mozilla/building/buildb2gbase.py#149 with self.buildbot_config set to {}, while in some places we rely on it set to None :/ like in https://dxr.mozilla.org/mozilla-central/source/testing/mozharness/mozharness/mozilla/building/buildb2gbase.py#125

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7c7330c33365 (still waiting, but it looks much better now)

Comment 13

[Jordan Lund (:jlund)]

Comment on attachment 8665467 [details] [diff] [review]
l10n_v2.diff

Review of attachment 8665467 [details] [diff] [review]:
-----------------------------------------------------------------

doh, and I even thought to myself 'I wonder if there is code that expects buildbot_config is None. mh needs moar tests :)

Comment 14

[Rail Aliiev [:rail]]

Comment on attachment 8665467 [details] [diff] [review]
l10n_v2.diff

https://hg.mozilla.org/integration/mozilla-inbound/rev/e58caa8dad52

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e58caa8dad52

Comment 16

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/06d223e905d5