1368733 - source-test tasks not being automatically scheduled on try

Status: RESOLVED FIXED

(Firefox Build System :: Task Configuration, task)

Description

[Andrew Halberstadt [:ahal]]

As far as I can tell this was regressed by bug 1337903.. though I'm kind of surprised that no one has complained more vocally about eslint not showing up on their try pushes for the past ~3 months, so maybe there is more to the story.

Comment 1

[Andrew Halberstadt [:ahal]]

There is a bit more to the story.. Having "job" tasks run automatically on a push with *no try syntax at all* has been broken since bug 1337903. But having "job" tasks run automatically on a push *with try syntax* has merely been broken since bug 1365646 (~2 weeks). I believe the fix from bug 1365646 was likely correct, but it inadvertently closed the loophole that was making job tasks still work.

Before I can come up with a patch, I need to understand the motivation behind this line:
https://dxr.mozilla.org/mozilla-central/source/taskcluster/taskgraph/try_option_syntax.py#590

This says "only run job tasks if they happen to run on a platform that was specified by -p", but for source-test tasks at least, that behaviour is not desirable. Is it meant to filter out other "job" kinds like toolchain? If so, maybe we can either whitelist source-test or blacklist toolchain here. I also see that bug 1345863 removed the JOB_KINDS_MATCHING_BUILD_PLATFORM variable.. is that what we need to re-introduce?

I think instead of a needinfo, I'll come up with my best guess at a patch so this is easier to talk about.

Comment 2

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

Ah, I think this is the same problem we just encountered in bug 1368463.

Basically, some jobs have platforms, and should be governed by -p.  Some don't, and they shouldn't.

Comment 3

[Andrew Halberstadt [:ahal]]

Attachment: Bug 1368733 - [taskcluster] Ensure job tasks run automatically on try when they're supposed to,

The logic here is a bit tricky to grok, but essentially there are two kinds of "job" tasks, those that
should always be considered (and possibly be optimized away later due to "skip-unless-changed"), and
those that should only be considered if their associated build-platform is also going to be scheduled.
If -j is specified, that should supercede both cases.

This patch uses the prescence of the 'build_platform' attribute to draw the distinction.

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

Comment 4

[Andrew Halberstadt [:ahal]]

Comment on attachment 8872696 [details]
Bug 1368733 - [taskcluster] Ensure job tasks run automatically on try when they're supposed to,

https://reviewboard.mozilla.org/r/144232/#review147954

::: taskcluster/taskgraph/try_option_syntax.py:594
(Diff revision 1)
> +            elif not self.jobs and attr('build_platform'):
>                  if self.platforms is None or attr('build_platform') in self.platforms:
>                      return True
> +                return False

I'm still confused as to what this block is supposed to accomplish. As of this patch, there are 0 tasks that have both a 'build_platform' attribute and a 'try_syntax_name'.

I think either 'toolchain' and 'android-stuff' should be setting the 'build_platform' attribute, or else we should remove this code block.

Comment 6

[Mark Banner (:standard8)]

Just to note this is fairly important from a sheriffing perspective, as it is causing more backouts due to ESLint not running on the try server.

Comment 7

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

Comment on attachment 8872696 [details]
Bug 1368733 - [taskcluster] Ensure job tasks run automatically on try when they're supposed to,

https://reviewboard.mozilla.org/r/144232/#review147996

::: taskcluster/taskgraph/try_option_syntax.py:594
(Diff revision 1)
> +            elif not self.jobs and attr('build_platform'):
>                  if self.platforms is None or attr('build_platform') in self.platforms:
>                      return True
> +                return False

From irc, it seems like we should leave this in.  I think this makes the most sense for source-test tasks that require a build, but maybe also for toolchain, where it doesn't make much sense to build a linux toolchain for `-p win32`.  I don't know much about android-stuff :)

Comment 8

[Andrew Halberstadt [:ahal]]

Comment on attachment 8872696 [details]
Bug 1368733 - [taskcluster] Ensure job tasks run automatically on try when they're supposed to,

https://reviewboard.mozilla.org/r/144232/#review147996

> From irc, it seems like we should leave this in.  I think this makes the most sense for source-test tasks that require a build, but maybe also for toolchain, where it doesn't make much sense to build a linux toolchain for `-p win32`.  I don't know much about android-stuff :)

Ok I'm happy to leave that in. I do like the concept, there just wasn't anything using it. We can revisit whether toolchain or build-dependent source-test tasks should be using it in follow-up bugs.

Btw, here's a try push with no syntax that schedules all the source-test tasks (since patch is changing /taskcluster/taskgraph):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0314cf93c0003c72e95cdc04c612239387d2ecf7

And here's one with a try syntax:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=17fad4e001c557b31ebaea65a9442463bd182e89

Both cases seem to be behaving as expected.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b487d2321fe4
[taskcluster] Ensure job tasks run automatically on try when they're supposed to, r=dustin

Comment 11

[Ryan VanderMeulen [:RyanVM]]

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