Closed Bug 1368733 Opened 2 years ago Closed 2 years ago

source-test tasks not being automatically scheduled on try

Categories

(Firefox Build System :: Task Configuration, task, major)

task
Not set
major

Tracking

(Not tracked)

RESOLVED FIXED
mozilla55

People

(Reporter: ahal, Assigned: ahal)

References

Details

Attachments

(1 file)

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.
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.
Blocks: 1365646
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 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.
Duplicate of this bug: 1368774
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.
Severity: normal → major
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 :)
Attachment #8872696 - Flags: review?(dustin) → review+
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.
Pushed by ahalberstadt@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b487d2321fe4
[taskcluster] Ensure job tasks run automatically on try when they're supposed to, r=dustin
Duplicate of this bug: 1368463
https://hg.mozilla.org/mozilla-central/rev/b487d2321fe4
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Product: TaskCluster → Firefox Build System
You need to log in before you can comment on or make changes to this bug.