Closed Bug 1371497 Opened 7 years ago Closed 7 years ago

Stylo Talos jobs stopped running on m-c

Categories

(Firefox Build System :: Task Configuration, task)

task
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla55

People

(Reporter: bholley, Assigned: gps)

Details

Attachments

(1 file)

This is a big problem. I suspect bug 1370663 may be related?
Flags: needinfo?(kmoir)
I'm on it.
Assignee: nobody → gps
Status: NEW → ASSIGNED
Flags: needinfo?(kmoir)
Comment on attachment 8875963 [details]
Bug 1371497 - More robust handling of spaces in variant processing for Talos BBB;

https://reviewboard.mozilla.org/r/147366/#review151844

So, basically, we were adding spaces after matching, and attempting to compensate for the spaces by adjusting the formatting string correctly.  But then when we started switching on the value of the variant, we forgot to add spaces to the switch cases themselves, and therefore started formatting the job names wrong, and things went downhill from there?

::: taskcluster/taskgraph/transforms/job/mozharness_test.py:450
(Diff revision 2)
>          if variant in ('stylo', 'stylo-sequential'):
> -            buildername = '{} {}{} talos {}'.format(
> +            buildername = '{} {} {} talos {}'.format(
> +                BUILDER_NAME_PREFIX[platform],
> +                variant,
> +                branch,
> +                test_name
> +            )
> +        elif variant:
> +            buildername = '{} {} {} talos {}'.format(
>                  BUILDER_NAME_PREFIX[platform],
>                  variant,
>                  branch,
>                  test_name
>              )

Why are we duplicating these cases?  It looks like we don't need separate cases for stylo now...
ni? to gps for comment 4.
Flags: needinfo?(gps)
Comment on attachment 8875963 [details]
Bug 1371497 - More robust handling of spaces in variant processing for Talos BBB;

https://reviewboard.mozilla.org/r/147366/#review151844

> Why are we duplicating these cases?  It looks like we don't need separate cases for stylo now...

Because I refactored this patch while developing and I was too stupid to notice the redundancy in the new version.
Comment on attachment 8875963 [details]
Bug 1371497 - More robust handling of spaces in variant processing for Talos BBB;

https://reviewboard.mozilla.org/r/147366/#review151844

> Because I refactored this patch while developing and I was too stupid to notice the redundancy in the new version.

Or not. The intent was correct. I messed up the order things are formatted in.
Comment on attachment 8875963 [details]
Bug 1371497 - More robust handling of spaces in variant processing for Talos BBB;

https://reviewboard.mozilla.org/r/147366/#review151930

Something between an r=me and an rs=me.  Interested in your thoughts on the below.

::: taskcluster/taskgraph/transforms/job/mozharness_test.py:453
(Diff revision 3)
>                  variant,
>                  branch,
>                  test_name
>              )
> -        else:
> -            buildername = '{} {} {}talos {}'.format(
> +        elif variant:
> +            buildername = '{} {} {} talos {}'.format(
>                  BUILDER_NAME_PREFIX[platform],
>                  branch,
>                  variant,

That difference is...kind of subtle. :(

I wonder, does:

```python
if variant in ('stylo', 'stylo_sequential'):
    name = "{prefix} {variant} {branch} talos {test_name}"
elif variant:
    name = "{prefix} {branch} {variant} talos {test_name}"
else:
    name = "{prefix} {branch} talos {test_name}"
buildername = name.format(prefix=BUILDER_NAME_PREFIX[platform],
                          branch=branch,
                          variant=variant,
                          test_name=test_name)
```

read any better to you?  Would be nice if we had some explanation of why the stylo ones are switched around in the first place.
Attachment #8875963 - Flags: review?(nfroyd) → review+
Comment on attachment 8875963 [details]
Bug 1371497 - More robust handling of spaces in variant processing for Talos BBB;

https://reviewboard.mozilla.org/r/147366/#review151930

> That difference is...kind of subtle. :(
> 
> I wonder, does:
> 
> ```python
> if variant in ('stylo', 'stylo_sequential'):
>     name = "{prefix} {variant} {branch} talos {test_name}"
> elif variant:
>     name = "{prefix} {branch} {variant} talos {test_name}"
> else:
>     name = "{prefix} {branch} talos {test_name}"
> buildername = name.format(prefix=BUILDER_NAME_PREFIX[platform],
>                           branch=branch,
>                           variant=variant,
>                           test_name=test_name)
> ```
> 
> read any better to you?  Would be nice if we had some explanation of why the stylo ones are switched around in the first place.

Yes, your version reads better.

There is a prior inline comment linking to bug 1338871 regarding the ordering. But that bug is resolved. So I dunno what's going on.
Pushed by nfroyd@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2019434e6aac
More robust handling of spaces in variant processing for Talos BBB; r=froydnj
Flags: needinfo?(gps)
https://hg.mozilla.org/mozilla-central/rev/2019434e6aac
Status: ASSIGNED → RESOLVED
Closed: 7 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.

Attachment

General

Created:
Updated:
Size: