Closed
Bug 1368733
Opened 7 years ago
Closed 7 years ago
source-test tasks not being automatically scheduled on try
Categories
(Firefox Build System :: Task Configuration, task)
Firefox Build System
Task Configuration
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.
Assignee | ||
Comment 1•7 years ago
|
||
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
Comment 2•7 years ago
|
||
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 hidden (mozreview-request) |
Assignee | ||
Comment 4•7 years ago
|
||
mozreview-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/#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•7 years ago
|
||
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 7•7 years ago
|
||
mozreview-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
::: 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+
Assignee | ||
Comment 8•7 years ago
|
||
mozreview-review-reply |
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
Comment 11•7 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
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
•