Closed Bug 789373 Opened 7 years ago Closed 7 years ago

use the new --disable-ion flag for specific b2g-related builds

Categories

(Release Engineering :: General, defect)

x86
macOS
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: joduinn, Assigned: aki)

References

Details

Attachments

(9 files, 2 obsolete files)

4.43 KB, patch
aki
: review+
bhearsum
: checked-in+
Details | Diff | Splinter Review
12.23 KB, patch
aki
: review+
dvander
: checked-in+
Details | Diff | Splinter Review
402 bytes, patch
dvander
: review+
Details | Diff | Splinter Review
4.65 KB, patch
dvander
: review+
Callek
: checked-in+
Details | Diff | Splinter Review
2.64 KB, patch
Callek
: review+
Details | Diff | Splinter Review
3.17 KB, patch
Callek
: review+
Details | Diff | Splinter Review
26.01 KB, patch
aki
: review+
Details | Diff | Splinter Review
4.07 KB, patch
aki
: review+
Details | Diff | Splinter Review
3.68 KB, patch
Callek
: review+
Details | Diff | Splinter Review
Per meeting w/dvander, dmandelin:

We need IonMonkey landed on mozilla-inbound, and then mozilla-central for the FF18 release. We also need to make sure that IonMonkey does not disrupt B2G work also now happening on mozilla-central. This means we need IonMonkey enabled and used by default on all builds *except* a specifically chosen list of builds. To do this:

1) dvander will create new "--disable-ion" flag and land it. Bug#789319 has details.

2) RelEng will ignore this new flag for Firefox builds, so FF18 users will see IonMonkey goodness once it lands. RelEng will update mozconfigs to use this new flag for these specific Fennec, B2G desktop builds and B2G builds in https://hg.mozilla.org/mozilla-central:
config/mozconfigs/ics_armv7a_gecko/*
(not sure if this is complete list of config files)

3) dvander will land IonMonkey on mozilla-inbound, and migrate to mozilla-central.

4) Until RelEng have B2G tests running on B2G builds, RelEng will generate some android builds with, and also without IonMonkey. This will require us to create a new type of build, for example: "android-no-ion". Each of these android builds then needs the usual Android tests run on it. This will also require changes to tbpl. 
Note: The "android-armv6" and "android-x86" builds should only be created with "--disable-ion", as IonMonkey is not relevant there. 

5) After RelEng has B2G tests running on B2G builds, we will disable the new "android-no-ion" builds.
Note: to be explicit, android-armv6 and android-x86 should continue to be run with "--disable-ion", as IonMonkey is not relevant there.
(In reply to John O'Duinn [:joduinn] from comment #0)
> 2) RelEng will ignore this new flag for Firefox builds, so FF18 users will
> see IonMonkey goodness once it lands. RelEng will update mozconfigs to use
> this new flag for these specific Fennec, B2G desktop builds and B2G builds
> in https://hg.mozilla.org/mozilla-central:
> config/mozconfigs/ics_armv7a_gecko/*
> (not sure if this is complete list of config files)

b2g/config/mozconfigs/
mobile/android/config/mozconfigs/

> 4) Until RelEng have B2G tests running on B2G builds, RelEng will generate
> some android builds with, and also without IonMonkey.

I think it would be simpler to enable android tests on the IonMonkey branch, and keep ionmonkey on for android there and off on m-c.  Then the android tests on the ionmonkey branch would be an indicator of android testing status with ionmonkey on.

However, if we're looking to get per-checkin test results on m-c for *non*-ionmonkey checkins (but with ionmonkey on), this might be the best solution.
(In reply to Aki Sasaki [:aki] from comment #1)
> I think it would be simpler to enable android tests on the IonMonkey branch,
> and keep ionmonkey on for android there and off on m-c.  Then the android
> tests on the ionmonkey branch would be an indicator of android testing
> status with ionmonkey on.
> 
> However, if we're looking to get per-checkin test results on m-c for
> *non*-ionmonkey checkins (but with ionmonkey on), this might be the best
> solution.

We really want to land IonMonkey on mozilla-central pref'd on so it can ship with desktop and mobile Firefox 18. Our concern is not conflicting with B2G's development. I don't think we could ship IonMonkey if it's pref'd off on m-c.
Tree rules in a nutshell: if it's visible on m-c, you cannot break it, so it must also run on try and on everything that merges to m-c. If there's visible android-no-ion on m-c, then it has to be on inbound, and fx-team, and services-central, and build-system, and try, and whatever else I'm forgetting. Do we actually have the tegra capacity to basically double the test load?

A fun alternative: run them hidden and on inbound only, for the retrospective pleasure of the b2g folks to look back at what broke them, the same system that jetpack uses.
If we do go ahead with this idea (I personally not liking the sound of doubling our tegra load and also sheriff android manual starring time, but oh well), then we'll need a bug to add TBPL support for the new android-no-ion builds, once the name has been confirmed:
https://bugzilla.mozilla.org/enter_bug.cgi?product=Webtools&component=Tinderboxpushlog
What is the ETA for completing step 2 of comment #1?

We've completed step #1 on our end, and are still blocked on landing IonMonkey.
Isn't step 4, the misnumbered one that needs to be 3, the killer? Step 2 doesn't even require someone who knows anything about b2g, just someone with a mozilla-central tree and the knowledge (which can be gained by opening logs off tbpl and looking for the "got mozconfig" buildstep) that they are in /b2g/config/mozconfigs/ics_armv7a_gecko/* rather than in /config/.
This disables IonMonkey for the ICS-based B2G builds as well as the Armv6 Android builds - I believe that covers all of the places we want them off.
Attachment #659850 - Flags: review?(aki)
Oh, and I removed the now-unused b2g GB configs while I was in there.
Attachment #659850 - Flags: review?(aki) → review+
(In reply to Phil Ringnalda (:philor) from comment #6)
> Isn't step 4, the misnumbered one that needs to be 3, the killer? Step 2
> doesn't even require someone who knows anything about b2g, just someone with
> a mozilla-central tree and the knowledge (which can be gained by opening
> logs off tbpl and looking for the "got mozconfig" buildstep) that they are
> in /b2g/config/mozconfigs/ics_armv7a_gecko/* rather than in /config/.

You are correct, anyone can do such modifications now. You can even push them to try!
Sweet, thank you! Does this mean TBPL will automatically disable IonMonkey now, and we're free to land?
(In reply to David Anderson [:dvander] from comment #11)
> Sweet, thank you! Does this mean TBPL will automatically disable IonMonkey
> now, and we're free to land?

Any pushes that come after my push to m-c will have ionmonkey disabled for ICS b2g builds & Android armv6 builds - if that's what you mean.
(In reply to David Anderson [:dvander] from comment #11)
> Sweet, thank you! Does this mean TBPL will automatically disable IonMonkey
> now, and we're free to land?

TBPL is just a dashboard, it doesn't compile anything or run any tests. Buildbot is what schedules the builds/tests; 'tinderbox (builds)' is/are the (albeit slightly out of date) name typically given to what is produced on each push.

All that has landed is the part that deals with:
(from comment 0) "Note: The "android-armv6" and "android-x86" builds should only be created with "--disable-ion", as IonMonkey is not relevant there." 

The rest of comment 0 step 4 (ie: Android on armv7) still needs buildbot-config patches to create both 'android' and 'android-no-ion' builds. The bug to add TBPL parser support for this new build name (mentioned in comment 4) hasn't been filed yet either.

However, I'm still not convinced the current plan is the best way forwards - given as yet, no one has responded to the capacity concerns in comment 3, comment 4 & comment 6.
Attached patch more disablingSplinter Review
Disable on the rest of B2G builds, as well as Fennec.
Attachment #659895 - Flags: review?(aki)
Comment on attachment 659895 [details] [diff] [review]
more disabling

To be clear, this will disable ionmonkey across the board for b2g (adding desktop) and android fennec (adding x86 + armv7).

Per dvander, we want to turn it back on for armv7 fennec but we need to either solve the no-ionmonkey builds/tests or the b2g testing first.
Attachment #659895 - Flags: review?(aki) → review+
This simply copies mobile/android/config/mozconfigs/android/nightly to mobile/android/config/mozconfigs/android-noion/nightly .  Since the former currently has --disable-ionmonkey, that should be sufficient.

The sooner this lands, the sooner this will merge to project branches; it'll also make staging the builds easier.
Attachment #660534 - Flags: review?(dvander)
Assignee: nobody → aki
This needs to land after:

a) attachment 660534 [details] [diff] [review] lands
b) android-noion builds are running
c) android-noion tests are running (potentially only jsreftests).
Attachment #660535 - Flags: review?(dvander)
Attachment #660534 - Flags: review?(dvander) → review+
Attachment #660535 - Flags: review?(dvander) → review+
Attachment #660662 - Flags: review?(bugspam.Callek)
Depends on: 790848
Attached patch [custom] android-noion (obsolete) — Splinter Review
Attachment #660663 - Flags: review?(bugspam.Callek)
Attachment #660662 - Flags: review?(bugspam.Callek) → review+
I ran a couple staging android-noion builds which:
* are named correctly
* uploading to the android-noion dir
* using the proper mozconfig
* are only on m-c and m-i per email thread
* sendchanged

Then I brought up the staging test master, and verified
* they're named correctly
* respond to the corresponding sendchange
* only on m-c and m-i per email thread
* not running reftests or crashtests per email thread (jsreftests are on though; is that right?)

I didn't actually run any tegra tests; I can run those tests if you think there might be reason to.
Attachment #660666 - Flags: review?(bugspam.Callek)
Comment on attachment 660663 [details] [diff] [review]
[custom] android-noion

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

::: process/factory.py
@@ +5644,5 @@
>              #32 bit (includes mac browsers)
>              if self.OS in ('xp', 'vista', 'win7', 'fedora', 'tegra_android', \
>                             'tegra_android-armv6', 'leopard', 'snowleopard', \
> +                           'leopard-o', 'lion', 'mountainlion',
> +                           'tegra_android-noion'):

nit: I'd prefer if the tegra* were grouped together, but I'm *ok* with how you listed it here
Attachment #660663 - Flags: review?(bugspam.Callek) → review+
Comment on attachment 660666 [details] [diff] [review]
[configs] android-noion builds and tests

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

r+ with my questions addressed.

::: mozilla-tests/config.py
@@ +139,5 @@
>  
> +PLATFORMS['android-noion']['slave_platforms'] = ['tegra_android-noion']
> +PLATFORMS['android-noion']['env_name'] = 'android-perf'
> +PLATFORMS['android-noion']['is_mobile'] = True
> +PLATFORMS['android-noion']['tegra_android-noion'] = {'name': "Android no-ionmonkey Tegra 250"}

Just as a note, we'll need a TBPL patch as well to account for this

@@ +638,5 @@
> +}
> +for suite in ANDROID_UNITTEST_DICT['opt_unittest_suites']:
> +    if suite[0].startswith('reftest') or suite[0].startswith('crashtest'):
> +        continue
> +    ANDROID_NOION_UNITTEST_DICT['opt_unittest_suites'].append(suite)

iirc bhearsum won't like this, but I'm ok with it. (note to ben, who i'm f?-ing due to this comment, these noion tests are to go away entirely when B2G tests are properly stood up)

@@ +1120,5 @@
>  
> +# XXX Bug 789373 hack - add android-noion until we have b2g testing
> +# Delete all references to android-noion once we have b2g testing
> +for branch in BRANCHES:
> +    if branch not in ('mozilla-central', 'mozilla-inbound'):

Are you sure we don't want try supported here?

::: mozilla/config.py
@@ +793,5 @@
>              'create_snippet': False,
>              'create_partial': False,
>              'slaves': SLAVES['mock'],
>              'platform_objdir': OBJDIR,
> +            'update_platform': 'Android_arm-eabi-gcc3-noion',

Why are we swapping to a different update_platform for Android-debug? If we do this, imo, we need to fix the debug builds on aus to continue getting updates.

@@ +1485,5 @@
> +
> +# XXX bug 789373 hack until we get b2g testing going.
> +# Delete all references to android-noion once we have b2g tests.
> +for b in BRANCHES.keys():
> +    if b not in ('mozilla-central', 'mozilla-inbound'):

again, do we need/want this on try?
Attachment #660666 - Flags: review?(bugspam.Callek)
Attachment #660666 - Flags: review+
Attachment #660666 - Flags: feedback?(bhearsum)
Attachment #660669 - Flags: review?(bugspam.Callek)
Attachment #660669 - Flags: review?(bugspam.Callek) → review+
Comment on attachment 660666 [details] [diff] [review]
[configs] android-noion builds and tests

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

I'm horrified that we need to do this at all. That's not your fault of course though, I have a few questions inline.

::: mozilla-tests/BuildSlaves.py.template
@@ +14,5 @@
>      "w764": "pass",
>      "vista": "pass",
>      "tegra_android": "pass",
> +    "tegra_android-armv6": "pass",
> +    "tegra_android-noion": "pass",

So, we're only doing noion for armv7?

::: mozilla-tests/config.py
@@ +1120,5 @@
>  
> +# XXX Bug 789373 hack - add android-noion until we have b2g testing
> +# Delete all references to android-noion once we have b2g testing
> +for branch in BRANCHES:
> +    if branch not in ('mozilla-central', 'mozilla-inbound'):

Indeed, if these are going to be builders that are intended to be unhidden we should have try support for them or else sheriffs will hide them whenever they break.

::: mozilla/config.py
@@ +736,5 @@
> +            'enable_codesighs': False,
> +            'create_partial': False,
> +            'slaves': SLAVES['mock'],
> +            'platform_objdir': OBJDIR,
> +            'update_platform': 'Android_arm-eabi-gcc3-noion',

Is there support in the build system for this? Unfortunately, this isn't a source of truth for update platform names, it merely reflects what the build system gives us. If we want nightly builds there's probably a few other things we need to do too.
(In reply to Ben Hearsum [:bhearsum] from comment #26)
> ::: mozilla-tests/BuildSlaves.py.template
> @@ +14,5 @@
> >      "w764": "pass",
> >      "vista": "pass",
> >      "tegra_android": "pass",
> > +    "tegra_android-armv6": "pass",
> > +    "tegra_android-noion": "pass",
> 
> So, we're only doing noion for armv7?

Correct.

> ::: mozilla-tests/config.py
> @@ +1120,5 @@
> >  
> > +# XXX Bug 789373 hack - add android-noion until we have b2g testing
> > +# Delete all references to android-noion once we have b2g testing
> > +for branch in BRANCHES:
> > +    if branch not in ('mozilla-central', 'mozilla-inbound'):
> 
> Indeed, if these are going to be builders that are intended to be unhidden
> we should have try support for them or else sheriffs will hide them whenever
> they break.

Easy enough to add.

> ::: mozilla/config.py
> @@ +736,5 @@
> > +            'enable_codesighs': False,
> > +            'create_partial': False,
> > +            'slaves': SLAVES['mock'],
> > +            'platform_objdir': OBJDIR,
> > +            'update_platform': 'Android_arm-eabi-gcc3-noion',
> 
> Is there support in the build system for this? Unfortunately, this isn't a
> source of truth for update platform names, it merely reflects what the build
> system gives us. If we want nightly builds there's probably a few other
> things we need to do too.

We already did this with android-xul, and I think we're still doing it with android-armv6.  I'm less concerned with having a good location for noion updates as I am not polluting android armv7 updates.
(In reply to Justin Wood (:Callek) from comment #24)
> ::: mozilla-tests/config.py
> @@ +139,5 @@
> >  
> > +PLATFORMS['android-noion']['slave_platforms'] = ['tegra_android-noion']
> > +PLATFORMS['android-noion']['env_name'] = 'android-perf'
> > +PLATFORMS['android-noion']['is_mobile'] = True
> > +PLATFORMS['android-noion']['tegra_android-noion'] = {'name': "Android no-ionmonkey Tegra 250"}
> 
> Just as a note, we'll need a TBPL patch as well to account for this

bug 790848

> @@ +638,5 @@
> > +}
> > +for suite in ANDROID_UNITTEST_DICT['opt_unittest_suites']:
> > +    if suite[0].startswith('reftest') or suite[0].startswith('crashtest'):
> > +        continue
> > +    ANDROID_NOION_UNITTEST_DICT['opt_unittest_suites'].append(suite)
> 
> iirc bhearsum won't like this, but I'm ok with it. (note to ben, who i'm
> f?-ing due to this comment, these noion tests are to go away entirely when
> B2G tests are properly stood up)

On IRC, Ben said he noted all of his concerns.
I don't like this either, but it's not any less programmatic than the rest of mozilla-tests/config.py and it's certainly a faster way to get where we want to [temporarily] go.

> > +# XXX Bug 789373 hack - add android-noion until we have b2g testing
> > +# Delete all references to android-noion once we have b2g testing
> > +for branch in BRANCHES:
> > +    if branch not in ('mozilla-central', 'mozilla-inbound'):
> 
> Are you sure we don't want try supported here?

Added.

> ::: mozilla/config.py
> @@ +793,5 @@
> >              'create_snippet': False,
> >              'create_partial': False,
> >              'slaves': SLAVES['mock'],
> >              'platform_objdir': OBJDIR,
> > +            'update_platform': 'Android_arm-eabi-gcc3-noion',
> 
> Why are we swapping to a different update_platform for Android-debug? If we
> do this, imo, we need to fix the debug builds on aus to continue getting
> updates.

android-debug was a mistake, good catch.

> @@ +1485,5 @@
> > +
> > +# XXX bug 789373 hack until we get b2g testing going.
> > +# Delete all references to android-noion once we have b2g tests.
> > +for b in BRANCHES.keys():
> > +    if b not in ('mozilla-central', 'mozilla-inbound'):
> 
> again, do we need/want this on try?

Done.
With fixes.  Besides the nits/fixes listed in this bug, I also turned off nightly updates for android-noion per Ben in IRC.

Carrying forward r+.
Attachment #660666 - Attachment is obsolete: true
Attachment #660666 - Flags: feedback?(bhearsum)
Attachment #660963 - Flags: review+
Reordering the platform list.  Passes test-masters with the fixed configs.
Carrying forward r+.
Attachment #660663 - Attachment is obsolete: true
Attachment #660964 - Flags: review+
Comment on attachment 660669 [details] [diff] [review]
[tools] update pm.json

http://hg.mozilla.org/build/puppet-manifests/rev/d6a05685baf1

I think this will propagate to the masters in the next 30min; I can push the tools patch, merge the configs/custom patches, and reconfig at that point.

Afterwards, a test build, land the android re-enable ionmonkey mozconfig patch, and send a dev.platform post.
Attachment #660669 - Flags: checked-in+
Comment on attachment 660662 [details] [diff] [review]
android-noion puppet

http://hg.mozilla.org/build/tools/rev/ba922e3fdac6

Oops, the previous comment should have marked this patch as checked-in, and this comment should have been on the tools patch.
Attachment #660662 - Flags: checked-in+
Bustage fix: http://hg.mozilla.org/build/tools/rev/81ad26d0844d
I wish json allowed trailing commas. And comments.
Attached patch fix thunderbirdSplinter Review
Attachment #661035 - Flags: review?(bugspam.Callek)
Attachment #661035 - Flags: review?(bugspam.Callek) → review+
android-noion builds+tests should be live now.  Testing...
Build was successful; I see tests running on tbpl.
I'm going to enable ionmonkey on armv7 and write a dev.planning post.
https://hg.mozilla.org/mozilla-central/rev/4f9d4f98f133
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Depends on: 791462
Depends on: 791519
Blocks: 791854
Blocks: 793994
Blocks: 849103
Product: mozilla.org → Release Engineering
Component: General Automation → General
You need to log in before you can comment on or make changes to this bug.