Closed
Bug 1293789
Opened 8 years ago
Closed 8 years ago
Android single locale repacks as tier 2 on try
Categories
(Firefox Build System :: Task Configuration, task)
Firefox Build System
Task Configuration
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla51
People
(Reporter: coop, Assigned: Callek)
References
Details
Attachments
(2 files)
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.
Reporter | ||
Updated•8 years ago
|
Assignee: nobody → bugspam.Callek
Comment hidden (mozreview-request) |
Assignee | ||
Comment 2•8 years ago
|
||
..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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 6•8 years ago
|
||
...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.
Flags: needinfo?(jlund)
Flags: needinfo?(francesco.lodolo)
Comment 7•8 years ago
|
||
I tried to install it and fr but not of them install on my phone (it simply say "App not installed").
Flags: needinfo?(francesco.lodolo)
Comment 8•8 years ago
|
||
mozreview-review |
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?
Attachment #8782994 -
Flags: review?(rail) → review+
Comment 9•8 years ago
|
||
mozreview-review |
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?
Assignee | ||
Comment 10•8 years ago
|
||
mozreview-review-reply |
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 13•8 years ago
|
||
mozreview-review |
Comment on attachment 8787884 [details]
Bug 1293789 - Single Locale in TC - Add task def for try.
https://reviewboard.mozilla.org/r/76496/#review75044
Attachment #8787884 -
Flags: review?(rail) → review+
Comment 14•8 years ago
|
||
Pushed by Callek@gmail.com:
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•8 years ago
|
||
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.
Assignee | ||
Comment 16•8 years ago
|
||
(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•8 years ago
|
||
mozreview-review |
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?
Updated•8 years ago
|
Flags: needinfo?(jlund)
Assignee | ||
Updated•8 years ago
|
Summary: Android single locale repacks as tier 2 → Android single locale repacks as tier 2 on try
Assignee | ||
Comment 18•8 years ago
|
||
(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?
Flags: needinfo?(jlund)
Comment 19•8 years ago
|
||
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
Flags: needinfo?(jlund)
Comment 20•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/7c27f2405376
https://hg.mozilla.org/mozilla-central/rev/80a5b362f9a7
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Updated•7 years ago
|
Product: TaskCluster → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•