Closed Bug 916927 Opened 7 years ago Closed 7 years ago

Create a new TBPL build flavor for desktop B2G builds

Categories

(Tree Management Graveyard :: TBPL, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: RyanVM, Assigned: RyanVM)

References

Details

Attachments

(2 files, 2 obsolete files)

Bug 916127 tracks turning on a more "full" suite of tests running on the desktop B2G builds. Currently these builds show up as "Bg" builds alongside all other builds of the same platform. However, this means that the tests will also show up on the same line next to the desktop Firefox tests, i.e. M(1 1 2 2 3 3 ...), which will be confusing.

Therefore, let's make B2G builds their own flavor so their builds and tests appear on their own line. This also means we can drop the special Bg/Ng symbols in favor of the regular B/N like other flavors.
Desktop builds follow the format of:
b2g_[TREE]_[PLATFORM]_gecko build
b2g_[PLATFORM] [TREE] opt test [NAME] (Gaia and Gaia UI tests)

And B2G emulator/device images use:
b2g_[TREE]_[TYPE]_dep (where TYPE is emulator, emulator_vm, hamachi, etc...)
b2g_[TYPE] [TREE] opt test [NAME]

We need to catch the former two cases while not accidentally catching the latter two, and I don't see a non-fragile way to do that for the tests. Would it be possible to make the desktop tests use b2g_[PLATFORM]_gecko? Then we could just use b2g.*gecko to find them.
Flags: needinfo?(catlee)
Also, bug 916111 tracks turning on debug B2G desktop builds. We need to know if this includes tests so we can decide whether we want to show both builds on one line a la ASAN or use separate lines for them.
Also, for presentation's sake, it would be nice to know what OSX and Windows versions the tests are intended to run on.
Attached patch basic sketch-up (obsolete) — Splinter Review
Just a basic sketch-up for what I've got in mind. Based on what I'm reading in some of the various "get tests running" bugs, it appears that the intent is indeed to run on both Opt and Debug builds. The attached appears to work as expected, though I obviously can't test it outside of the jobs we currently run. And due to comment 1, G and Gu still show on the Linux x64 Opt line.

Ed, any early comments on the approach? I'm assuming that the debug jobs will take the form of b2g_[TREE]_[TYPE]-debug_gecko like the emulator ICS/JB jobs.
Attachment #809592 - Flags: feedback?(emorley)
Comment on attachment 809592 [details] [diff] [review]
basic sketch-up

I think given that we are going to potentially have opt+debug, we should make the B2G desktop tests their own rows, eg:

B2G Desktop Windows Opt     Bg M(1 2 3 4 5) ...
B2G Desktop Windows Debug   Bg M(1 2 3 4 5) ...
B2G Desktop OS X Opt     Bg M(1 2 3 4 5) ...
B2G Desktop OS X Debug   Bg M(1 2 3 4 5) ...

etc

Also, ASan is more like a build flavour, whereas B2G desktop is almost another product type, in practice at least? :-)
Attachment #809592 - Flags: feedback?(emorley) → feedback-
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #1)
> Desktop builds follow the format of:
> b2g_[TREE]_[PLATFORM]_gecko build
> b2g_[PLATFORM] [TREE] opt test [NAME] (Gaia and Gaia UI tests)
> 
> And B2G emulator/device images use:
> b2g_[TREE]_[TYPE]_dep (where TYPE is emulator, emulator_vm, hamachi, etc...)
> b2g_[TYPE] [TREE] opt test [NAME]
> 
> We need to catch the former two cases while not accidentally catching the
> latter two, and I don't see a non-fragile way to do that for the tests.
> Would it be possible to make the desktop tests use b2g_[PLATFORM]_gecko?
> Then we could just use b2g.*gecko to find them.

If we take the comment 5 approach, we can match against the platform without needing to change the buildername - though it might be a nice tidy-up to make regardless :-)
(In reply to Ed Morley [:edmorley UTC+1] from comment #5)
> Comment on attachment 809592 [details] [diff] [review]
> basic sketch-up
> 
> I think given that we are going to potentially have opt+debug, we should
> make the B2G desktop tests their own rows, eg:

sorry s/rows/platforms/
Ah, that makes more sense :). I'll give that a shot instead. You're going to make me revisit that whitespace bug yet!
Yeah sorry wasn't too clear in comment 6. 

I think you've got what I was trying to (poorly) say, but just in case: I was just thinking that B2G desktop seems more like its own unique product (a la Firefox vs Firefox for Android vs FxOS for phones), rather than a compiler/build pref/optimisation level (a la opt vs debug vs pgo), so semantically they made sense as an addition entry under "os" in Data.js, rather than 'flavor'. They'll both be on their own rows, just via a different implementation (notably this will mean they report to OrangeFactor correctly, since it keys off of parts of the UI mappings [yucky I know]). 

But yeah we'll need to be creative with naming to make them not too wide! :-)
(In reply to Ed Morley [:edmorley UTC+1] from comment #6)
> If we take the comment 5 approach, we can match against the platform without
> needing to change the buildername - though it might be a nice tidy-up to
> make regardless :-)

We're still going to need to match up against b2g.*gecko, no? Otherwise we'll catch B2G emulator/device builds given |b2g.*_(?:dep|nightly)$/i.test(name) ? "b2g-device-image"|
Flags: needinfo?(emorley)
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #10)
> We're still going to need to match up against b2g.*gecko, no? Otherwise
> we'll catch B2G emulator/device builds given
> |b2g.*_(?:dep|nightly)$/i.test(name) ? "b2g-device-image"|

Job names/symbols will work as present - the only place we'll need to catch them is for the 'os' variable, which will use matches like:
/(?:b2g_ubuntu64_vm|b2g_.*_linux64_gecko)/
to match tests and builds for B2G Desktop Linux64 respectively.

Does that make sense? :-)
Flags: needinfo?(emorley)
This goes Ed's suggested route of adding new B2G Desktop platforms instead of going the flavor route. It appears to work as expected. The M1 jobs already running on Cedar even correctly show up!

Catlee - What version of OS X and Windows are we going to be running the tests on? Are we ever going to want to run them on more than one version of each platform? I'm assuming not and I named the jobs accordingly.

I'm also assuming for now that we won't be running on Win64. If that changes down the road, it would be easy to add.
Attachment #809592 - Attachment is obsolete: true
Attachment #813906 - Flags: review?(emorley)
I'm reusing the Android l10n logic for the nightly localizer builds. Realistically, we're the only ones even looking at TBPL for these builds, so IMO this is more convenient than special-casing.
Attachment #813907 - Flags: review?(emorley)
> Catlee - What version of OS X and Windows are we going to be running the tests on? Are we ever going 
> to want to run them on more than one version of each platform? I'm assuming not and I named the jobs 
> accordingly.

We should start with Win7, and OSX 10.8.  It's not clear right now whether we'll need to support multiple versions of Windows/OSX going forward.
Attachment #813907 - Flags: review?(emorley) → review+
Comment on attachment 813906 [details] [diff] [review]
Add new B2G desktop product types

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

r- due to the regexp tweaks, but would also like to iterate on the Config.js friendly names a bit too first :-)

::: js/Config.js
@@ +294,5 @@
>                            "Talos xperf"]
>    },
>    OSNames: {
>      "linux32": "Linux",
> +    "linux32-b2g": "Linux B2G Desktop",

I think we should do this as:

"b2g-linux32": "B2G Desktop Linux",

...since "B2G Desktop" is effectively it's own product, so that is more consistent with say the Android entries, where we do "Android <varient>". 

I think we should also move all of the B2G desktop builds down to below the Android entries and above the B2G emulator entry. What do you think? :-)

::: js/Data.js
@@ +477,5 @@
>        // XXX clean all this up after all builders have been
>        // transitioned to the new bug 586664 form
>        var os =
>          // ** Linux **
> +        /b2g.*_(?:linux|ubuntu).*64/i.test(name) ? "linux64-b2g" :

The wildcard before the "64" isn't required.
We can also drop the 'i' on this and the others, since the case is consistent.
We should also use a start anchor at the beginning of the regexp (for this and the others), otherwise we'll false positive against repos like mozilla-b2g18 etc

@@ +485,1 @@
>          /linux|fedora|ubuntu/i.test(name) ? "linux32" :

Same for the "32"
Attachment #813906 - Flags: review?(emorley) → review-
(In reply to Ed Morley (Away until 9th Oct) [:edmorley UTC+1] from comment #15)
> @@ +485,1 @@
> >          /linux|fedora|ubuntu/i.test(name) ? "linux32" :
> 
> Same for the "32"

Wrong line was quoted - I mean the same "wildcard is unnecessary" as for the 64 entry.
(In reply to Ed Morley (Away until 9th Oct) [:edmorley UTC+1] from comment #15)
> I think we should do this as:
> 
> "b2g-linux32": "B2G Desktop Linux",
> 
> ...since "B2G Desktop" is effectively it's own product, so that is more
> consistent with say the Android entries, where we do "Android <varient>". 
> 
> I think we should also move all of the B2G desktop builds down to below the
> Android entries and above the B2G emulator entry. What do you think? :-)

I guess the issue here is that in the past, product and platform have basically been one combined entity. That is to say, our naming convention is basically <Product> <Platform> <Flavor>, where <Product> has been an implied "Firefox" for all non-B2G platforms to date and left off. So given that logic, I can go along with your proposed ordering, but I could also argue that things are getting more ambiguous as we keep adding products/platforms. Maybe in a Treeherder world we can better differentiate different products to reduce the amount of information we're carrying in the name.

> We should also use a start anchor at the beginning of the regexp (for this
> and the others), otherwise we'll false positive against repos like
> mozilla-b2g18 etc

I don't understand what you're saying here. AFAICT, the current regexes work fine on b2g18.
Flags: needinfo?(catlee)
(In reply to Ed Morley (Away until 9th Oct) [:edmorley UTC+1] from comment #15)
> We can also drop the 'i' on this and the others, since the case is
> consistent.

Honestly, I just used it because every other line in those regexes uses it too.
This implements all the suggested changes. As mentioned above, some of the regex changes didn't seem to make any difference one way or another, but they do at least make them more specific to exactly what they're testing for.

However, one could also argue that we should have a follow-up for making the same cleanups to the other lines as needed for consistency's sake.
Attachment #813906 - Attachment is obsolete: true
Attachment #814553 - Flags: review?(emorley)
Comment on attachment 814553 [details] [diff] [review]
Add new B2G desktop product types v2

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

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #17)
> > We should also use a start anchor at the beginning of the regexp (for this
> > and the others), otherwise we'll false positive against repos like
> > mozilla-b2g18 etc
> 
> I don't understand what you're saying here. AFAICT, the current regexes work
> fine on b2g18.

Yeah it's more that should the builder names for non-b2g jobs change slightly, we'd we at risk of the regex in this patch causing false positives, given the "b2g-inbound" tree name is in all of the job names (similar to the false positives we got with "build" and the tree "build-system"). However, this will probably be fine for now :-)

Thank you for this!
Attachment #814553 - Flags: review?(emorley) → review+
Depends on: 931759
Status: NEW → ASSIGNED
In production :-)
Status: ASSIGNED → RESOLVED
Closed: 7 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.