Closed
Bug 1397847
Opened 7 years ago
Closed 6 years ago
Find toolchains in `./mach artifact toolchain --from-build` using indexes
Categories
(Firefox Build System :: General, enhancement)
Tracking
(firefox-esr60 fixed, firefox57 wontfix, firefox65 fixed)
RESOLVED
FIXED
mozilla65
People
(Reporter: dustin, Assigned: tomprince)
References
Details
Attachments
(8 files)
(deleted),
text/x-review-board-request
|
glandium
:
review-
|
Details |
(deleted),
text/x-review-board-request
|
glandium
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
Details | |
(deleted),
text/x-review-board-request
|
Details | |
(deleted),
text/x-review-board-request
|
Details | |
(deleted),
text/x-review-board-request
|
glandium
:
review+
|
Details |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details |
In bug 1383880 I came across some code that seems to be trying to fake out parts of the taskgraph generation to determine the taskId of an artifact build. This is fragile as it relies on implementation details of the task-graph generation code.
A slightly easier option would be to download the most recent decision task's artifacts and use those to find the desired task. That's what I'll try to implement in bug 1383880.
A *much* easier option would be to just index those tasks at a route that can be reproduced from the given inputs, and look it up at that index path.
Reporter | ||
Updated•7 years ago
|
Assignee: nobody → dustin
Comment 1•7 years ago
|
||
> A *much* easier option would be to just index those tasks at a route that can be reproduced from the given inputs, and look it up at that index path.
That's exactly what the code *currently* does, by "reusing" the taskcluster code. Except the taskcluster API doesn't have an API for that, so there's some abuse going on.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Reporter | ||
Comment 7•7 years ago
|
||
tl;dr is this:
1. pull toolchain definitions out of the taskgraph config
2. standardize hashing toolchains
3. use that in taskgraph and in ./mach artifact toolchain
Comment 8•7 years ago
|
||
While your approach works for this usecase, it's not sustainable for other similar things. Such as bug 1356529, which would then require to pull definitions out of toolchains/ci/build/ which doesn't seem reasonable. Or a helper command to download docker images.
Reporter | ||
Comment 9•7 years ago
|
||
Bug 1356529 hasn't seen any action for five months. I'm going to push back on scope-creeping this bug to cover that work. My work here is to maintain current functionality while fixing bustage caused by the "abuse" you referenced in comment 1.
By the way, there is a helper command to download docker images, already.
Comment hidden (mozreview-request) |
Comment 11•7 years ago
|
||
mozreview-review |
Comment on attachment 8908409 [details]
Bug 1397847: allow jobs-from to be topsrcdir-relative, move toolchains;
https://reviewboard.mozilla.org/r/180030/#review186354
I don't think this is desirable. TC job definitions shouldn't spread outside the taskcluster/ directory.
Attachment #8908409 -
Flags: review?(mh+mozilla) → review-
Comment 12•7 years ago
|
||
mozreview-review |
Comment on attachment 8908410 [details]
Bug 1397847: rename 'toolchain' transform to 'use_toolchains';
https://reviewboard.mozilla.org/r/180032/#review186356
This is nice but unrelated. It should go in its own bug.
Attachment #8908410 -
Flags: review?(mh+mozilla) → review+
Comment 13•7 years ago
|
||
mozreview-review |
Comment on attachment 8908411 [details]
Bug 1397847: make toolchain definitions a bit more generic;
https://reviewboard.mozilla.org/r/180034/#review186358
Nice simplifications, but they should go in their own bug (well, maybe they could share a separate bug with the previous patch).
::: taskcluster/taskgraph/transforms/job/toolchain.py:46
(Diff revision 1)
> - Optional('toolchain-alias'): basestring,
> })
>
>
> def add_optimizations(config, run, taskdesc):
> - files = list(run.get('resources', []))
> + files = [] # fixed in later commits..
huh
::: taskcluster/taskgraph/transforms/job/toolchain.py:165
(Diff revision 1)
> attributes = taskdesc.setdefault('attributes', {})
> - attributes['toolchain-artifact'] = run['toolchain-artifact']
> if 'toolchain-alias' in run:
> attributes['toolchain-alias'] = run['toolchain-alias']
Seems like this should all go, like fot docker-worker.
::: taskcluster/taskgraph/transforms/toolchain.py:101
(Diff revision 1)
> + run_on = tcd['run-on']
> + if run_on == 'linux':
> + job['worker-type'] = 'aws-provisioner-v1/gecko-{level}-b-linux'
> + job['worker'] = {
> + 'docker-image': {'in-tree': 'desktop-build'},
> + 'max-run-time': 36000,
It would be good to be able to override this. Particularly, these values are cargo culted and wrong, and we should change them, possibly to different values depending on the job. Bug 1391427 defines some values that are not 36000 for instance.
Attachment #8908411 -
Flags: review?(mh+mozilla)
Comment 14•7 years ago
|
||
mozreview-review |
Comment on attachment 8908412 [details]
Bug 1397847: build common code for finding and hashing toolchains;
https://reviewboard.mozilla.org/r/180036/#review186360
I'm not particularly convinced by this. I still think we should be able to get useful partial task graphs through mach taskgraph or some direct API.
::: taskcluster/taskgraph/transforms/job/toolchain.py:63
(Diff revision 1)
> + if 'toolchain-alias' in toolchain:
> + subs['name'] = toolchain['toolchain-alias']
> + routes.append('index.{}'.format(TOOLCHAIN_INDEX.format(**subs)))
I can see why you're adding this, but this is a separate matter, it should be in a separate patch or even bug.
Attachment #8908412 -
Flags: review?(mh+mozilla)
Comment 15•7 years ago
|
||
mozreview-review |
Comment on attachment 8908413 [details]
Bug 1397847: use hashing and the index to download toolchains;
https://reviewboard.mozilla.org/r/180038/#review186362
::: python/mozbuild/mozbuild/mach_commands.py:1899
(Diff revision 1)
> 'Could not find a toolchain build named `{build}`')
> return 1
> + toolchain = toolchains[b]
> + digest = hash_toolchain(toolchains, b)
>
> - task_id = optimize_task(task, {})
> + index_path = 'gecko.cache.level-{level}.toolchains.v1.{name}.{digest}'
This is the kind of thing that I didn't want to have duplicated between mach_commands and the taskcluster code. Plus, this doesn't try all levels above MOZ_SCM_LEVEL like the toolchain optimization code does.
::: taskcluster/taskgraph/util/taskcluster.py:85
(Diff revision 1)
> + except requests.exceptions.HTTPError as e:
> + if e.response.status_code == 404:
> + raise KeyError
> + raise
Seems like a nice api change, that could be done separately.
Attachment #8908413 -
Flags: review?(mh+mozilla)
Comment 16•7 years ago
|
||
mozreview-review |
Comment on attachment 8908609 [details]
Bug 1397847: --from-builds is a dev tool, not for automation;
https://reviewboard.mozilla.org/r/180272/#review186364
Fair enough
::: commit-message-19b51:1
(Diff revision 1)
> +Bug 1397847: --from-builds is a dev tool, not for automation; r?glandium
--from-build (no s)
Attachment #8908609 -
Flags: review?(mh+mozilla) → review+
Reporter | ||
Comment 17•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8908412 [details]
Bug 1397847: build common code for finding and hashing toolchains;
https://reviewboard.mozilla.org/r/180036/#review186360
I think ahal's work on tryselect for `mach try` is the right way to do this -- generate and cache a full taskgraph. That might need a slight bit of generalization to make it usable here, but it's *very* close to what you need. Callek was interested in such a thing, too. You're right that moving task configuration out of taskcluster/ci starts to set a bad precedent, especially if we move build definitions out.
Updated•7 years ago
|
status-firefox57:
--- → wontfix
Reporter | ||
Updated•7 years ago
|
Assignee: dustin → nobody
Updated•7 years ago
|
Product: Core → Firefox Build System
Assignee | ||
Comment 18•6 years ago
|
||
`mach artifact toolchain` gets task definitions from taskgraph, to get the
index path to find the artifacts at. Now that these index paths depend on the
digests of fetch tasks, those kinds need to be loaded as well. This adds a
supported API to get task definitions for a given kind, which loads all the
kind dependencies.
Comment 19•6 years ago
|
||
Pushed by mozilla@hocat.ca:
https://hg.mozilla.org/integration/autoland/rev/393f02fb1ad7
Add supported API to get tasks of a given kind from a taskgraph; r=dustin
Comment 20•6 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox65:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 65
Updated•6 years ago
|
Assignee: nobody → mozilla
Assignee | ||
Comment 21•6 years ago
|
||
Updated•6 years ago
|
Comment 22•6 years ago
|
||
Pushed by mozilla@hocat.ca:
https://hg.mozilla.org/mozilla-central/rev/19480c535117
Pass the taskcluster root to `load_tasks_for_kind` in `mach artifact toolchain`; r=dustin a=tomprince
Comment 23•6 years ago
|
||
Updated•6 years ago
|
Target Milestone: Firefox 65 → mozilla65
Assignee | ||
Comment 24•6 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-esr60/rev/4cf0b2332bce
https://hg.mozilla.org/releases/mozilla-esr60/rev/ba9634bec13e
status-firefox-esr60:
--- → fixed
You need to log in
before you can comment on or make changes to this bug.
Description
•