Closed Bug 1275943 Opened 5 years ago Closed 4 years ago

Jobs with "file_patterns" should get considered on try even if no try syntax is specified


(Taskcluster Graveyard :: Scheduler, defect)

Not set


(Not tracked)



(Reporter: ahal, Assigned: ahal)



(1 file, 1 obsolete file)

I recently filed a bug to "stop scheduling all jobs on try if there is no try syntax", so I see the irony here.

But jobs that have a "file_patterns" clause are meant to be "smart". I.e, they can determine for themselves when they need to run and when they don't. This means there is no try syntax for them, as developers never need to worry about when to run them.

However, they don't get considered when a developer pushes to try without a try syntax, and I believe they should be (if they don't require any parent tasks). Otherwise, the only way to run them on try is to use a dummy try syntax that schedules jobs you don't need.
Here's an example to illustrate what I mean:

In that push, I was only interested in the ES job. But in order to get it to run, I had to schedule that linux64 build/xpcshell job. I should be able to just push with an empty try message and get the ES job automatically (if I touched the right files).
Here is the above patch in action:

Notice there's no try syntax, but the f8 and tg tasks were scheduled anyway because I modified python files under /taskcluster. One edge case might be a "when" task that has dependencies.. I don't know if such tasks exist, but with this patch, their dependencies will also be scheduled. I'm not sure if that is a good or bad thing.
Assignee: nobody → ahalberstadt
Comment on attachment 8809610 [details]
Bug 1275943 - Schedule tasks with a 'when' clause regardless of try syntax,

I trust your judgement more than mine for try usage patterns, so if you think this is a good idea, r+ for the implementation.

From what I can tell, this basically makes the `-j` option a no-op?

::: taskcluster/taskgraph/
(Diff revision 1)
>                  return False
>              if 'only_chunks' in test and attr('test_chunk') not in test['only_chunks']:
>                  return False
>              return True
> +        # tasks with a 'when' clause should be scheduled regardless of try syntax

"should be included in the target set .. They may later be optimized away"
Attachment #8809610 - Flags: review?(dustin) → review+
Comment on attachment 8809610 [details]
Bug 1275943 - Schedule tasks with a 'when' clause regardless of try syntax,

Ah, I didn't even know I could use -j to schedule lint jobs! I don't know if this makes -j obsolete or not, is it exclusively used for jobs with "when" clauses? It's probable that this at least removes one of the main use cases of -j though.

But either way, I still think this is a good idea. For example, imagine a developer makes a change under `taskcluster/taskgraph` but doesn't know how to run the 'tg' tests (or that they exist). This way, they can just push to try knowing the right thing will happen. On the flip side, if someone makes a change under `taskcluster/taskgraph`, I can't really imagine a scenario we *wouldn't* want to run the 'tg' tests on a try push.

From a more ideological sense, this is a baby step towards a world where try syntax isn't required and CI is smart enough to figure out what to run based on what changed.

p.s this comment is more for archaeology than trying to convince you :). I'll also do a newsgroup post before landing.
Here's gps' commit message when he added "-j":

So I think tasks that have a "when" clause are a subset of non-build/test tasks (i.e ones scheduled with -j). I also think, coincidentally, those two sets are currently identical. Also note, that -j tasks are "all" scheduled by default anyway unless "-j" is explicitly passed in.

This makes me feel confident that there is a bug here. They *should* be added when pushing to try without syntax. After reading that commit message, I now think my patch doesn't go quite far enough. It should schedule all "job" tasks, whether or not they have a "when" clause. Feel free to needinfo gps if you want confirmation about all this, but I'm confident this is the right thing to do and accept all blame if anyone gets angered :)

Uploading new patch which I think you'll like much better shortly.. Sorry in advance for the review churn!
Attachment #8809610 - Attachment is obsolete: true
Comment on attachment 8810516 [details]
Bug 1275943 - Ensure job tasks are added to target set even if no try syntax specified,

I think the better fix here would be to make all of the default values when `try:` does not appear in the commit message match those that are applied when individual options are omitted, rather than carefully address the differences between `[]` and `None`.  I admit I never really thought about the no-syntax situation (I thought one or both of `mach try` and the hg hook would reject that, in fact.. it certainly rejects things with bogus `-p` options), so those defaults are really just wrong and never given much thought.

That said, this patch works :)
Attachment #8810516 - Flags: review?(dustin) → review+
Pushed by
Ensure job tasks are added to target set even if no try syntax specified, r=dustin
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Product: Taskcluster → Taskcluster Graveyard
You need to log in before you can comment on or make changes to this bug.