1333424 - Implicitly add taskcluster config files affecting a task to its 'files-changed' list if applicable

Status: RESOLVED FIXED

(Firefox Build System :: Task Configuration, task)

Description

[Andrew Halberstadt [:ahal]]

Tasks that have a 'when': {'files-changed': [...]} config in them will only run if a file specified in the list was changed by one of the revisions in the push.

The list of files should include anything that could cause bustage for that task, including the task configuration files themselves. We should implicitly add these to the 'files-changed' list, so task creators don't forget to do so (and don't need to repeatedly add them each time).

I propose every task with this config in it, also have the following paths implicitly added:
taskcluster/ci/<kind>/**
taskcluster/taskgraph/**

Also, if the task has an 'in-tree' docker image defined, we should add:
taskcluster/docker/<image>/**

Ideally we would figure out exactly which files under these paths are relevant to any given task, but that is a hard problem to solve, and I figure it's better to be safe than sorry.

Comment 1

[Andrew Halberstadt [:ahal]]

Attachment: Bug 1333424 - Implicitly add relevant taskcluster configuration files to 'files-changed' attribute,

This will ensure that if the taskgraph module, taskcluster configs related to a task or docker image related
to a task are modified, the task will run.

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

Comment 2

[Andrew Halberstadt [:ahal]]

Here's what a try run that modifies taskcluster/taskgraph would look like (and also the try run for this patch):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=64d0de7f619c5b93c9ac557dd89bef52dd676fcf

Dustin, if you think including taskcluster/taskgraph/** here is overkill, I'd be fine with removing it. I think the docker image and files under the kind directory should at least be included though.

Another approach that might be better (though more work), would be to have an empty 'relevant_files' list on the base kind. Then kind implementations of that could fill that out however they wanted to.

Comment 3

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

Comment on attachment 8829890 [details]
Bug 1333424 - Implicitly add relevant taskcluster configuration files to 'files-changed' attribute,

https://reviewboard.mozilla.org/r/106850/#review108304

I think this is a good solution -- no need to do anything harder.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/259405ce7da2
Implicitly add relevant taskcluster configuration files to 'files-changed' attribute, r=dustin

Comment 5

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/259405ce7da2

Comment 6

[Mike Hommey [:glandium]]

Comment on attachment 8829890 [details]
Bug 1333424 - Implicitly add relevant taskcluster configuration files to 'files-changed' attribute,

https://reviewboard.mozilla.org/r/106850/#review109556

::: taskcluster/taskgraph/transforms/task.py:751
(Diff revision 1)
> +        task['when']['files-changed'].extend([
> +            '{}/**'.format(config.path),
> +            'taskcluster/taskgraph/**',
> +        ])

This is way too broad, especially the '{}/**'.format(config.path) part.

Take, for example, the toolchain tasks, this pattern essentially means that if you change ci/toolchain/macosx.yml, you also trigger all the builds for windows and linux toolchains.

For toolchain tasks, this will be worked around when I finish my patch queue to cache them (coming soon), but I can imagine the same problem happening for other tasks with file-changed configuration.

Comment 7

[Andrew Halberstadt [:ahal]]

Yes, ideally we would know which specific files affect which tasks, but that is not currently possible. Doing that would probably require something like I mentioned in comment 2.

My reasoning was that it is much preferable to run unnecessary tasks, than it is to miss running necessary ones, so better to be safe than sorry. I'll file a follow-up bug to improve this, but tbh, given how (relatively) infrequent config changes are, I don't think it's a very high priority.