Closed Bug 1071875 Opened 10 years ago Closed 10 years ago

Add support for e10s Talos to TBPL

Categories

(Tree Management Graveyard :: TBPL, defect)

defect
Not set
normal

Tracking

(e10s+)

RESOLVED FIXED
Tracking Status
e10s + ---

People

(Reporter: jgriffin, Assigned: jgriffin)

References

Details

Attachments

(2 files, 1 obsolete file)

      No description provided.
Attached patch Add e10s Talos support to TBPL, (obsolete) — Splinter Review
I think this should add something like this to TBPL:

T-e10s(c d g1 o s tp)
Attachment #8494027 - Flags: review?(ryanvm)
Assignee: nobody → jgriffin
Blocks: 1071876
Comment on attachment 8494027 [details] [diff] [review]
Add e10s Talos support to TBPL,

Review of attachment 8494027 [details] [diff] [review]:
-----------------------------------------------------------------

Looks OK overall. Need to address the talos other question before I'm comfortable with this landing, though, given our previous history with this job.

::: js/Config.js
@@ +329,5 @@
> +                               "Talos g1 e10s",
> +                               "Talos other e10s",
> +                               "Talos svg e10s",
> +                               "Talos tp e10s",
> +                               "Talos xperf e10s"],

Let's move this block below the non-e10s ones to follow the same pattern as the other suites.

@@ +539,3 @@
>      "Talos dromaeojs" : "d",
>      "Talos dromaeojs Metro" : "d-m",
> +    "Talos dromaeojs e10s" : "d",

Above the Metro line.

@@ +556,5 @@
>      "Talos tp Metro" : "tp-m",
>      "Talos tp nochrome" : "tpn",
>      "Talos ts" : "ts",
>      "Talos tspaint" : "tsp",
> +    "Talos xperf e10s" : "x",

Below the non-e10s line.

::: js/Data.js
@@ +618,2 @@
>          /talos g1$/i.test(name) ? "Talos g1" :
> +        /talos other_(?:no)?l64-e10s/i.test(name) ? "Talos other e10s" :

Can we name these jobs other-e10s_<whatever> instead? Seems that would simply the regex greatly (and hopefully avoid running afoul of the "64" issues we've had previously with the non-e10s version of this job name.
Attachment #8494027 - Flags: review?(ryanvm) → feedback+
A simple suite name change per RyanVM's request...the builder diff's are:

Builders added:
+ Rev4 MacOSX Snow Leopard 10.6 holly talos other-e10s_nol64
+ Rev5 MacOSX Mountain Lion 10.8 holly talos other-e10s_nol64
+ Ubuntu HW 12.04 holly talos other-e10s_nol64
+ Ubuntu HW 12.04 x64 holly talos other-e10s_l64
+ WINNT 6.2 holly talos other-e10s_nol64
+ Windows 7 32-bit holly talos other-e10s_nol64
+ Windows XP 32-bit holly talos other-e10s_nol64
Builders removed
- Rev4 MacOSX Snow Leopard 10.6 holly talos other_nol64-e10s
- Rev5 MacOSX Mountain Lion 10.8 holly talos other_nol64-e10s
- Ubuntu HW 12.04 holly talos other_nol64-e10s
- Ubuntu HW 12.04 x64 holly talos other_l64-e10s
- WINNT 6.2 holly talos other_nol64-e10s
- Windows 7 32-bit holly talos other_nol64-e10s
- Windows XP 32-bit holly talos other_nol64-e10s
Attachment #8494650 - Flags: review?(jlund)
Feedback addressed.
Attachment #8494651 - Flags: review?(ryanvm)
Attachment #8494027 - Attachment is obsolete: true
Comment on attachment 8494651 [details] [diff] [review]
Add e10s Talos support to TBPL,

Review of attachment 8494651 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great, thanks!

::: js/Data.js
@@ +618,2 @@
>          /talos g1$/i.test(name) ? "Talos g1" :
> +        /talos other-e10s_/i.test(name) ? "Talos other e10s" :

This can be just talos other-e10s (no trailing underscore), right?

@@ +624,4 @@
>          /talos remote-trobopan$/i.test(name) ? "Talos robopan" :
>          /talos remote-troboprovider$/i.test(name) ? "Talos roboprovider" :
>          /talos (?:remote-t)?svg[rx]?-metro$/i.test(name) ? "Talos svg Metro" :
> +        /talos (?:remote-t)?svg[rx]?-e10s$/i.test(name) ? "Talos svg e10s" :

Move this above Metro.

@@ +630,1 @@
>          /talos (?:remote-)?tp5o-metro$/i.test(name) ? "Talos tp Metro" :

Can you remove the (?:remote-)? from this line while you're here? I somehow doubt we're ever going to run Metro Talos on Android :)
Attachment #8494651 - Flags: review?(ryanvm) → review+
Comment on attachment 8494650 [details] [diff] [review]
Update builder names for e10s other jobs,

Review of attachment 8494650 [details] [diff] [review]:
-----------------------------------------------------------------

lgtm.

WARNING: because of the way this works, I think we will need a mozharness patch too before landing this as that string is used to make the Builder name *and* decide what is passed to '--suite {suite}' :D
Attachment #8494650 - Flags: review?(jlund) → review+
Comment on attachment 8494651 [details] [diff] [review]
Add e10s Talos support to TBPL,

Additional feedback addressed:  https://hg.mozilla.org/webtools/tbpl/rev/952e7347779f
(In reply to Jordan Lund (:jlund) from comment #6)
> Comment on attachment 8494650 [details] [diff] [review]
> Update builder names for e10s other jobs,
> 
> Review of attachment 8494650 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> lgtm.
> 
> WARNING: because of the way this works, I think we will need a mozharness
> patch too before landing this as that string is used to make the Builder
> name *and* decide what is passed to '--suite {suite}' :D

We don't need a mozharness patch for this, we need a talos.json patch.  I'm landing without this patch, see https://bugzilla.mozilla.org/show_bug.cgi?id=1071310#c12

https://hg.mozilla.org/build/buildbot-configs/rev/e9d8d28c9e23
Blocks: 1072653
(In reply to Jonathan Griffin (:jgriffin) from comment #7)
> Comment on attachment 8494651 [details] [diff] [review]
> Add e10s Talos support to TBPL,
> 
> Additional feedback addressed: 
> https://hg.mozilla.org/webtools/tbpl/rev/952e7347779f

Thanks :)

Once the tests are up and running on Holly, I'll give things a quick check on tbpl-dev to make sure everything looks sane and then I'll push it to production.
No longer blocks: 1072653
Depends on: 1072653
(In reply to Jonathan Griffin (:jgriffin) from comment #7)
> Comment on attachment 8494651 [details] [diff] [review]
> Add e10s Talos support to TBPL,
> 
> Additional feedback addressed: 
> https://hg.mozilla.org/webtools/tbpl/rev/952e7347779f

Typo fixed in https://hg.mozilla.org/webtools/tbpl/rev/2857e0e59bbc
Depends on: 1073233
No longer depends on: 1073233
Make the e10s group show after the non-e10s group.
https://hg.mozilla.org/webtools/tbpl/rev/74a1a15f6593
In production :)
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Product: Webtools → Tree Management
Product: Tree Management → Tree Management Graveyard
You need to log in before you can comment on or make changes to this bug.