Closed Bug 1340564 Opened 4 years ago Closed 3 years ago

Do not use substring matching on labels in task-graph generation

Categories

(Firefox Build System :: Task Configuration, task)

task
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla55

People

(Reporter: dustin, Assigned: dustin)

Details

Attachments

(4 files, 1 obsolete file)

I see a lot of "if 'signing' in label", and that makes me nervous, as any new task that happens to contain that substring will get matched.
I'm pushing a patch shortly.  Testing shows the only difference to be the addition of some "signed" attributes to beetmover and signing tasks.
hah, looks like I failed to push to mozreview..
Comment on attachment 8843074 [details]
Bug 1340564: make follow-on task names append to the label;

Ah, that's right, flagging mtabara for review causes 500's from mozreview.
Attachment #8843074 - Flags: review?(mtabara)
Attachment #8843075 - Flags: review?(mtabara)
@dustin: just wanted to step by and apologize as I didn't have time to check those reviews yet, we're still caught with the migration this week. If these patches can wait until EOW or early next week, that'd be great. Thanks!
There's a minor risk of bitrot, but other than that no rush.
Attachment #8843075 - Flags: review?(mtabara) → review?(bugspam.Callek)
Attachment #8843074 - Flags: review?(mtabara) → review?(bugspam.Callek)
Comment on attachment 8843075 [details]
Bug 1340564: use an attribute to identify signed tasks;

https://reviewboard.mozilla.org/r/113456/#review128068

This has likely bitrotted already, *and* to make matters worse we have a situation where signing tasks are *not* what we'll pass on to beetmover directly for OSX, and a slightly more complicated way for Windows in the future, so `signed` as used may not be great.

Additionally kim is making changes to this very code on `date` to support the needed changes for OSX, I'd rather bitrot this cleanup patch than hers (since this is pretty simple overall)
Attachment #8843075 - Flags: review?(bugspam.Callek)
Comment on attachment 8843074 [details]
Bug 1340564: make follow-on task names append to the label;

https://reviewboard.mozilla.org/r/113454/#review128070

I'm not really a huge fan of the append variant, I find the prepent better as it groups the tasks of a specific kind together. (I think this is more a preference of how do we group, and in such a case, a better UX for a full graph may be needed, rather than relying on this magic name grouping, where `label` might as well be a uuid for how we actually expect it used in practice, except we *want* a readable string too)
Attachment #8843074 - Flags: review?(bugspam.Callek)
Fair enough.  I do find some of the labels unreadable, but if they make sense to releng, that's OK by me -- I don't think anyone else will be too concerned with them.
Is this ready to be worked on now?
Flags: needinfo?(bugspam.Callek)
Its probably ok to work on this now. I can envision some code cleanup that could make this even easier to work on, but I don't feel anything blocks here.

To say in-bug I've also thought of some very quick pseudocode that can help enforce this "don't compare labels" stuff...

>>> class Label(object):
...    def __str__(self):
...       return self.label
...    def __repr__(self):
...       return str(self)
...    def __cmp__(self, other):
...       raise Exception("Unable to compare")
...    def __init__(self, label):
...       self.label = label
... 
>>> l = Label("foo-signing/opt")
>>> l
foo-signing/opt
>>> l == 'foo'
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "<stdin>", line 7, in __cmp__
Exception: Unable to compare
>>> 'foo' in l
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: argument of type 'Label' is not iterable
Flags: needinfo?(bugspam.Callek)
Attachment #8843074 - Attachment is obsolete: true
I'm scared to subclass string as in comment 12, for performance and correctness reasons.  We use labels as dict keys a lot!

I've verified that the patch set included here makes no change to a nightly graph except adding a `signed` attribute to lots of tasks (with value "false" in some cases).
Comment on attachment 8861641 [details]
Bug 1340564: use attributes to identify talos jobs;

https://reviewboard.mozilla.org/r/133636/#review136504

Much nicer!
Attachment #8861641 - Flags: review?(bstack) → review+
Comment on attachment 8843075 [details]
Bug 1340564: use an attribute to identify signed tasks;

https://reviewboard.mozilla.org/r/113456/#review136910
Attachment #8843075 - Flags: review?(bugspam.Callek) → review+
Comment on attachment 8861639 [details]
Bug 1340564: filter for android tasks using the build_platform attribute;

https://reviewboard.mozilla.org/r/133632/#review137238

I tested the patch with the protocol detailed in bug 1352477 comment 4. I didn't notice any changes in the taskgraph. Thanks for making the dependencies less label-dependent!
Attachment #8861639 - Flags: review?(jlorenzo) → review+
Attachment #8861640 - Flags: review?(ahalberstadt) → review?(gps)
Comment on attachment 8861640 [details]
Bug 1340564: specify the target name explicitly for dependent tasks;

https://reviewboard.mozilla.org/r/133634/#review141308
Attachment #8861640 - Flags: review?(gps) → review+
Attachment #8861640 - Flags: review?(ahalberstadt)
Pushed by dmitchell@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1aeb99ce3e6c
use an attribute to identify signed tasks; r=Callek
https://hg.mozilla.org/integration/autoland/rev/45d01b30e9d1
filter for android tasks using the build_platform attribute; r=jlorenzo
https://hg.mozilla.org/integration/autoland/rev/516721f50c1e
specify the target name explicitly for dependent tasks; r=gps
https://hg.mozilla.org/integration/autoland/rev/fd1f5c916045
use attributes to identify talos jobs; r=bstack
Backout by cbook@mozilla.com:
https://hg.mozilla.org/mozilla-central/rev/f0c6a71517c2
Backed out changeset fd1f5c916045 
https://hg.mozilla.org/mozilla-central/rev/2d6baa44eff2
Backed out changeset 516721f50c1e 
https://hg.mozilla.org/mozilla-central/rev/4bb396799a31
Backed out changeset 45d01b30e9d1 
https://hg.mozilla.org/mozilla-central/rev/1ec1d8863720
Backed out changeset 1aeb99ce3e6c for hoping that fix the packageName issue
This wasn't at fault. Re-landing.
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/mozilla-central/rev/b1e27dba755b
use an attribute to identify signed tasks; r=Callek
https://hg.mozilla.org/mozilla-central/rev/44a0f9f0dff4
filter for android tasks using the build_platform attribute; r=jlorenzo
https://hg.mozilla.org/mozilla-central/rev/41b461000ca6
specify the target name explicitly for dependent tasks; r=gps
https://hg.mozilla.org/mozilla-central/rev/4d481af2abea
use attributes to identify talos jobs; r=bstack
Product: TaskCluster → Firefox Build System
You need to log in before you can comment on or make changes to this bug.