1293789 - Android single locale repacks as tier 2 on try

Status: RESOLVED FIXED

(Firefox Build System :: Task Configuration, task)

Description

[Chris Cooper [:coop] (he/him)]

These aren't official release artifacts, but we do produce them as part of the release process. Until that changes, we'll need to support then in TaskCluster.

Comment 1

[Justin Wood (:Callek)]

Attachment: Bug 1293789 - Single Local TC, mozharness bits.

Review commit: https://reviewboard.mozilla.org/r/72988/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/72988/

Comment 2

[Justin Wood (:Callek)]

..that WIP is working locally, without signing (and without verifying signing). I haven't done any actual verification of the resulting repacks though, so I suspect there's "more to do" :-)

Comment 4

[Justin Wood (:Callek)]

Attachment: Bug 1293789 - Single Locale in TC - Add task def for try.

Review commit: https://reviewboard.mozilla.org/r/76496/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/76496/

Comment 5

[Justin Wood (:Callek)]

Comment on attachment 8782994 [details]
Bug 1293789 - Single Local TC, mozharness bits.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/72988/diff/1-2/

Comment 6

[Justin Wood (:Callek)]

...and here we go, the official review!

https://treeherder.mozilla.org/#/jobs?repo=try&revision=519387160f26
https://tools.taskcluster.net/task-inspector/#Si6880-iTHaZWwOdNjdO7w/0

:flod, any chance you can do a 1 minute check on any two locales on the second link here (I note, there is *zero* signing on these jobs, so if thats a blocker for a check on your end, then you can just say so and not worry about it).

:jlund, I'm n-i'ing you since I'm not sure how to ask for feedback from someone with a mozreview push.

Comment 7

[Francesco Lodolo [:flod]]

I tried to install it and fr but not of them install on my phone (it simply say "App not installed").

Comment 8

[Rail Aliiev [:rail]]

Comment on attachment 8782994 [details]
Bug 1293789 - Single Local TC, mozharness bits.

https://reviewboard.mozilla.org/r/72988/#review75024

::: testing/mozharness/configs/single_locale/tc_android_api_15.py:24
(Diff revision 2)
> +    #    "DIST": "%(abs_objdir)s",
> +    #    "LOCALE_MERGEDIR": "%(abs_merge_dir)s/",
> +    #    "L10NBASEDIR": "../../l10n",
> +    #    "MOZ_MAKE_COMPLETE_MAR": "1",
> +    #    'TOOLTOOL_CACHE': os.environ.get('TOOLTOOL_CACHE'),
> +    #},

Can you remove this block if it's not required?

Comment 9

[Rail Aliiev [:rail]]

Comment on attachment 8787884 [details]
Bug 1293789 - Single Locale in TC - Add task def for try.

https://reviewboard.mozilla.org/r/76496/#review75026

::: taskcluster/ci/legacy/tasks/builds/android_l10n_base.yml:41
(Diff revision 1)
> +      MH_BUILD_POOL: taskcluster
> +      # image paths
> +      TOOLTOOL_CACHE: '/home/worker/tooltool-cache'
> +      NEED_XVFB: true
> +
> +    maxRunTime: 36000

Oooh, 10h? I'm not sure if this is what you want ;)

::: taskcluster/taskgraph/test/test_try_option_syntax.py:144
(Diff revision 1)
>          tos = TryOptionSyntax('try: -p linux,linux64', empty_graph)
> -        ridealongs = list(itertools.chain.from_iterable(RIDEALONG_BUILDS.itervalues()))
> +        ridealongs = list(task
> +                          for task in itertools.chain.from_iterable(
> +                                RIDEALONG_BUILDS.itervalues()
> +                          )
> +                          if 'android' not in task)  # Don't include android-l10n

Maybe instead of excluding "android" you should add only linux, linux64 to be explicit?

::: testing/mozharness/configs/single_locale/tc_android-api-15.py:24
(Diff revision 1)
>      #    "DIST": "%(abs_objdir)s",
>      #    "LOCALE_MERGEDIR": "%(abs_merge_dir)s/",
>      #    "L10NBASEDIR": "../../l10n",
>      #    "MOZ_MAKE_COMPLETE_MAR": "1",
>      #    'TOOLTOOL_CACHE': os.environ.get('TOOLTOOL_CACHE'),
>      #},

Can this block go?

::: testing/mozharness/configs/single_locale/try_android-api-15.py:8
(Diff revision 1)
> +EN_US_BINARY_URL = "http://archive.mozilla.org/pub/" \
> +                   "mobile/nightly/latest-mozilla-central-android-api-15/en-US"
> +
> +config = {
> +    "branch": "try",
> +    "log_name": "single_locale", # XXX ???

XXX ??? ???!!! :)
Is it required?

Comment 10

[Justin Wood (:Callek)]

Comment on attachment 8787884 [details]
Bug 1293789 - Single Locale in TC - Add task def for try.

https://reviewboard.mozilla.org/r/76496/#review75026

> Oooh, 10h? I'm not sure if this is what you want ;)

curbed from the other (desktop) l10n jobs, which I probably copied from elsewhere too. I'm not sure I need 10h either, but probably doesn't hurt short-term, in the end I'll halve that and go from there. (:dustin can choose to change it, if it makes sense, on his legacy code refactor)

> Maybe instead of excluding "android" you should add only linux, linux64 to be explicit?

Not going to work actually, this is to deal with the following:
```
RIDEALONG_BUILDS = {
    'android-api-15': [
        'android-api-15-l10n',
    ],
    'linux': [
        'linux-l10n',
    ],
    'linux64': [
        'linux64-l10n',
        'sm-plain',
        'sm-nonunified',
        'sm-arm-sim',
        'sm-arm64-sim',
        'sm-compacting',
        'sm-rootanalysis',
        'sm-package',
        'sm-tsan',
        'sm-asan',
        'sm-msan',
    ],
}
```
Which will be iterating across all the values not keys. Since this part of legacy isn't long for this world I didn't think it'd be *that* much of a hack here.

> Can this block go?

yep, will-do.

> XXX ??? ???!!! :)
> Is it required?

the XXX isn't, and technically neither is the name, but might as well keep it (was curbed from other single locale stuff).

Was mostly a pointer for me to dxr-search where/what uses that arg so I can determine if needed. -- https://dxr.mozilla.org/mozilla-central/source/testing/mozharness/mozharness/base/log.py#386 is the long and short of it.

I'm removing the comment.

Comment 11

[Justin Wood (:Callek)]

Comment on attachment 8782994 [details]
Bug 1293789 - Single Local TC, mozharness bits.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/72988/diff/2-3/

Comment 12

[Justin Wood (:Callek)]

Comment on attachment 8787884 [details]
Bug 1293789 - Single Locale in TC - Add task def for try.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/76496/diff/1-2/

Comment 13

[Rail Aliiev [:rail]]

Comment on attachment 8787884 [details]
Bug 1293789 - Single Locale in TC - Add task def for try.

https://reviewboard.mozilla.org/r/76496/#review75044

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7c27f2405376
Single Local TC, mozharness bits. r=rail
https://hg.mozilla.org/integration/autoland/rev/80a5b362f9a7
Single Locale in TC - Add task def for try. r=rail

Comment 15

[Dustin J. Mitchell [:dustin] (he/him)]

I am not changing maxRunTime's -- at least with tests, they seem to be very carefully balanced and I've gotten a lot of pushback to altering them (e.g., to make all runs of a particular suite have the same maxRunTime).  So that's entirely up to you.

In general, copy/pasting task definitions around without thinking too hard about them is going to make a lot of mess, even after bug 1286075 lands.  As a rule of thumb, it'd be great to have a justification in mind for every line in the task definition, or leave it out.  This rule will work better after 1286075 lands, since it is more semantic.  For example, if you have an "index" property then the task will be indexed, so you should have a reason in mind for indexing.

The try option syntax stuff is *not* part of the legacy task-generation work, and I do not have plans to remove it.  It is, indeed, a big old mess.

In the test rail commented on, it would be better to add android to the -p option on the line above, rather than construct the odd list of results.

Comment 16

[Justin Wood (:Callek)]

(In reply to Dustin J. Mitchell [:dustin] from comment #15)
> I am not changing maxRunTime's -- at least with tests, they seem to be very
> ...

Agreed on the "bad idea to copy/paste" :-)

> The try option syntax stuff is *not* part of the legacy task-generation
> work, and I do not have plans to remove it.  It is, indeed, a big old mess.

I thought a lot of the try syntax stuff was closely next on hit list (though not landing with your main work)

> In the test rail commented on, it would be better to add android to the -p
> option on the line above, rather than construct the odd list of results.

I was about to say I'm not a fan of arbitrarily expanding that `-p` list with platforms to care about for this, then I realized thats basically what I'm doing here anyway. Though this already landed, if you care enough to have me adjust it, file a followup bug with me assigned (n-i me to be safe that I don't forget it for a while) and I'll fix, otherwise I'm inclined to leave it be.

Comment 17

[Jordan Lund (:jlund)]

Comment on attachment 8782994 [details]
Bug 1293789 - Single Local TC, mozharness bits.

https://reviewboard.mozilla.org/r/72988/#review75124

f+ looks good :)

one comment inline.

::: testing/mozharness/scripts/mobile_l10n.py:619
(Diff revision 3)
> -                    success = True
> -            if not success:
> -                self.add_failure(locale, message="Failed to detect %s url in make upload!" % (locale))
> -                print output
> -                continue
>              success_count += 1

I still think that success_count is a lie here.

I'll explain my reasoning so I can be corrected :)

if `if m:` evaluates to falsey, we wouldn't have found and extended `self.upload_urls` with a url yet `summarize_success_count()` will always say 100% of locales uploaded successfully.

I see that in `submit_to_balrog()` you check for which locales have upload urls. So if we are using that for our sanity check, maybe remove the conflicting `summarize_success_count()` in this method?

Comment 18

[Justin Wood (:Callek)]

(In reply to Jordan Lund (:jlund) from comment #17)
> Comment on attachment 8782994 [details]
> Bug 1293789 - Single Local TC, mozharness bits.
> 
> https://reviewboard.mozilla.org/r/72988/#review75124
> 
> f+ looks good :)
> 
> one comment inline.
> 
> ::: testing/mozharness/scripts/mobile_l10n.py:619
> (Diff revision 3)
> > -                    success = True
> > -            if not success:
> > -                self.add_failure(locale, message="Failed to detect %s url in make upload!" % (locale))
> > -                print output
> > -                continue
> >              success_count += 1
> 
> I still think that success_count is a lie here.
> 
> I'll explain my reasoning so I can be corrected :)
> 
> if `if m:` evaluates to falsey, we wouldn't have found and extended
> `self.upload_urls` with a url yet `summarize_success_count()` will always
> say 100% of locales uploaded successfully.
> 
> I see that in `submit_to_balrog()` you check for which locales have upload
> urls. So if we are using that for our sanity check, maybe remove the
> conflicting `summarize_success_count()` in this method?

It is *sometimes* possibly lying.

Heres the workflow, and why:

* We call `make upload`
  -- This either simply moves files to a local folder (Taskcluster)
     ~~ There is no URL output here, because the files are only `local`
  -- Or actually uploads these files to product-deliveries S3.
     ~~ In the S3 case, we also call the command designated by POST_UPLOAD_CMD to perform some remote actions.
     ~~ This command echo's URL(s) on where to find the various binaries.
     ~~ The script captures that URL output and stores it for use, *only* with balrog submission.

So, since the output where it contains a URL is only used for balrog, *and* make itself would output an error if there is an error returned from the upload command(s) anyway, I opted to have the "check-for-url" thing be error-inducing only when we try to submit to balrog via the script. (AKA: in the buildbot way, we would still fail, in the TC way, no harm no foul)

Does that make sense?

Comment 19

[Jordan Lund (:jlund)]

we discussed this over vidyo. I recommended putting a comment in or even a `if not tc` block around the code in question so it doesn't cause confusion and remember to remove it when we drop bb. not blocking though

Comment 20

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

https://hg.mozilla.org/mozilla-central/rev/7c27f2405376
https://hg.mozilla.org/mozilla-central/rev/80a5b362f9a7