884074 - Base config settings that ride the trains on the gecko version

Status: RESOLVED FIXED

(Release Engineering :: General, defect)

Description

[Steve Fink [:sfink] [:s:]]

A number of config settings are guarded by "# MERGE DAY" comments and list various branches that the settings should apply to. It might be less manual work and hopefully less error-prone to base them off of the version.

Comment 1

[Steve Fink [:sfink] [:s:]]

Attachment: Use numerical gecko_version testing for config settings that ride the trains

No time pressure on this review at all. It's mostly a dump of already-written code that I wanted to submit for your consideration.

Also note that I don't know which project branches feed into mozilla-central, so I guessed. But given that these settings are done by changing the default setting and then reverting on the older branches, it doesn't really matter; only gecko_version less than mozilla-central's will do anything.

With this in place, you *could* distinguish trees that feed into mozilla-central from other random trees if you wanted. It's kind of unnecessary gunk, though; it might be better to remove the 'gecko_version_from' stuff and just explicitly set 'gecko_version' on aurora, beta, release, esr, and b2g branches.

Comment 2

[[e-mail] (:bhearsum)]

Comment on attachment 763824 [details] [diff] [review]
Use numerical gecko_version testing for config settings that ride the trains

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

I did a quick config.py of this vs. the version you're based on. There was one difference that I couldn't account for:
+mozilla-esr17:                                            'pulseaudio-libs-devel',

On a more conceptual level, I think I like this...it's much clearer, and should make merge days a bit better. In order for this be landable we need it implemented in all mozilla{,-tests}/{firefox,thunderbird,b2g}_config.py. I also have one suggestion for improvement below.

::: mozilla/config.py
@@ +1447,5 @@
> +# Everything feeding into mozilla-central is the same version as mozilla-central
> +for branch in BRANCHES.values():
> +    version_source = branch.get('gecko_version_from', None)
> +    if version_source:
> +        branch['gecko_version'] = BRANCHES[version_source]['gecko_version']

I'm not a huge fan of using a pointer here, but it's better than needing to bump the version in multiple places. I'd like to see this be more generic though, as I suspect there will be other use cases for it. How about "branch_of"?

Comment 3

[[e-mail] (:bhearsum)]

Oh, and this needs to be rebased against the latest changes...all the merge day stuff that landed in the last week makes this patch massively conflict.

Comment 4

[Steve Fink [:sfink] [:s:]]

Attachment: Use numerical gecko_version testing for config settings that ride the trains

(In reply to Ben Hearsum [:bhearsum] from comment #2)
> Comment on attachment 763824 [details] [diff] [review]
> Use numerical gecko_version testing for config settings that ride the trains
> 
> Review of attachment 763824 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I did a quick config.py of this vs. the version you're based on. There was
> one difference that I couldn't account for:
> +mozilla-esr17:                                           
> 'pulseaudio-libs-devel',

Weird. The rebased version doesn't have this difference.

> On a more conceptual level, I think I like this...it's much clearer, and
> should make merge days a bit better. In order for this be landable we need
> it implemented in all mozilla{,-tests}/{firefox,thunderbird,b2g}_config.py.

Ok. I haven't done that yet. Posting an updated patch anyway, mostly because I changed the version-based branch selection to read a little better but even better, preserve the existing level of indentation so the diff is way, way easier to read. :-) No action requested right now, though.

> I also have one suggestion for improvement below.
> 
> ::: mozilla/config.py
> @@ +1447,5 @@
> > +# Everything feeding into mozilla-central is the same version as mozilla-central
> > +for branch in BRANCHES.values():
> > +    version_source = branch.get('gecko_version_from', None)
> > +    if version_source:
> > +        branch['gecko_version'] = BRANCHES[version_source]['gecko_version']
> 
> I'm not a huge fan of using a pointer here, but it's better than needing to
> bump the version in multiple places. I'd like to see this be more generic
> though, as I suspect there will be other use cases for it. How about
> "branch_of"?

I'm not sure what you mean; that terminology doesn't make sense. Oh! Unless you're thinking of 'branch' in 'branch_of' as having no relation to the keys in BRANCHES? If so, that seems confusing. I'd rather have 'based_on' or something.

But my vote is to leave this out completely. Now that I see that all of the version-specific stuff is done by making the default configuration be for the latest version, and undoing changes everywhere that isn't ready for them yet, I don't see the point. At least not yet, anyway -- if we come up with a use case, can we just add it then?

The attached patch omits gecko_version_from completely, and the diff of config.py output is clean.

Comment 5

[[e-mail] (:bhearsum)]

(In reply to Steve Fink [:sfink] from comment #4)
> > ::: mozilla/config.py
> > @@ +1447,5 @@
> > > +# Everything feeding into mozilla-central is the same version as mozilla-central
> > > +for branch in BRANCHES.values():
> > > +    version_source = branch.get('gecko_version_from', None)
> > > +    if version_source:
> > > +        branch['gecko_version'] = BRANCHES[version_source]['gecko_version']
> > 
> > I'm not a huge fan of using a pointer here, but it's better than needing to
> > bump the version in multiple places. I'd like to see this be more generic
> > though, as I suspect there will be other use cases for it. How about
> > "branch_of"?
> 
> I'm not sure what you mean; that terminology doesn't make sense. Oh! Unless
> you're thinking of 'branch' in 'branch_of' as having no relation to the keys
> in BRANCHES? If so, that seems confusing. I'd rather have 'based_on' or
> something.

I was thinking more in the sense of "mozilla-inbound is a branch that regularly merges from mozilla-central" or "hypothetical branch foo is a branch that regularly merges mozilla-beta".

> But my vote is to leave this out completely. Now that I see that all of the
> version-specific stuff is done by making the default configuration be for
> the latest version, and undoing changes everywhere that isn't ready for them
> yet, I don't see the point. At least not yet, anyway -- if we come up with a
> use case, can we just add it then?

I thought I came up with a use case yesterday...but I can't remember it now. It may have been b2g18 branches, but they don't count because they're not in project_branches.py, I think. I'd be fine with "based_on" if we need this later, though.

Comment 6

[Steve Fink [:sfink] [:s:]]

Attachment: A few missing version bumps

When converting various configs to partly automate the version bumps, I found a handful of what looks to be missing branches. rail okayed some of these on IRC. Doing these as a separate patch makes the before/after diff of the following patch much cleaner.

Comment 7

[Rail Aliiev [:rail]]

Comment on attachment 767399 [details] [diff] [review]
A few missing version bumps

comm-release still tracks mozilla-esr17. Otherwise lgtm

Comment 8

[Steve Fink [:sfink] [:s:]]

Attachment: Use numerical gecko_version testing for config settings that ride the trains

I did firefox and thunderbird in both mozilla and mozilla-tests, but the b2g configs seem to have a different structure, and each one only has a single setting that is gecko version-dependent. There are a whole bunch of branch/platform dependencies, but I don't understand what those are based on, and they'd use a different key at the very least.

Note that this patch should be applied *after* the other patch on this bug, because I updated a few of the branch lists to better match the version dependencies in their comments. So when comparing with

  diff --ignore-matching-lines='instance at 0x' -u config.{pre,post}

where config.{pre,post} is the output of config.py, only the gecko_version settings should show up in the output.

Comment 9

[Steve Fink [:sfink] [:s:]]

Attachment: A few missing version bumps

comm-release tracks esr17

Comment 10

[Steve Fink [:sfink] [:s:]]

Attachment: Use numerical gecko_version testing for config settings that ride the trains

comm-release tracks esr17

Comment 11

[Steve Fink [:sfink] [:s:]]

(In reply to Ben Hearsum [:bhearsum] from comment #5)
> (In reply to Steve Fink [:sfink] from comment #4)
> > But my vote is to leave this out completely. Now that I see that all of the
> > version-specific stuff is done by making the default configuration be for
> > the latest version, and undoing changes everywhere that isn't ready for them
> > yet, I don't see the point. At least not yet, anyway -- if we come up with a
> > use case, can we just add it then?

Whoa. This sounds ambiguous when I re-read it. By "I don't see the point", I was *not* referring to the way the version-specific configuration is handled. In fact, I think that's great! I would've done it the wrong way. (And I'm saddened because I've now found that mozilla-tests/config.py does it the other way. I didn't look very close to see if there's a good reason for that.)

> I thought I came up with a use case yesterday...but I can't remember it now.
> It may have been b2g18 branches, but they don't count because they're not in
> project_branches.py, I think. I'd be fine with "based_on" if we need this
> later, though.

Well, now I've looked at the b2g configs, and it does seem like there's something similar that could be done there. I don't see why it matters if they're in project_branches.py or not. But I don't know what sort of linear versioning scheme would make sense and be useful. Maybe it's because I don't understand the connection between b2g versions and the various hardware and emulators that are floating around.

Comment 12

[[e-mail] (:bhearsum)]

Comment on attachment 767416 [details] [diff] [review]
Use numerical gecko_version testing for config settings that ride the trains

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

(In reply to Steve Fink [:sfink] from comment #11)
> > I thought I came up with a use case yesterday...but I can't remember it now.
> > It may have been b2g18 branches, but they don't count because they're not in
> > project_branches.py, I think. I'd be fine with "based_on" if we need this
> > later, though.
> 
> Well, now I've looked at the b2g configs, and it does seem like there's
> something similar that could be done there. I don't see why it matters if
> they're in project_branches.py or not. But I don't know what sort of linear
> versioning scheme would make sense and be useful. Maybe it's because I don't
> understand the connection between b2g versions and the various hardware and
> emulators that are floating around.

Can you at least add gecko_version to the b2g configs, for consistency? I know there's no train riding in those configs right now, but I expect that we'll have some of that in the future.

Also, I got reminded of mozilla-tests/mobile_config.py the other day, so that file needs it too.

Thanks again for doing this, we're very excited for this change!

Comment 13

[Steve Fink [:sfink] [:s:]]

Attachment: Use numerical gecko_version testing for config settings that ride the trains

A bunch of changes. dump_masters.py on all of the masters comes out identical now.

The {keys,values,branches}_{before,at_least} stuff is ugly, I know. Suggestions welcome. Also, note that the way I'm comparing works for b2g versions as well as simple numeric versions, since cmp((1,0,1), (1,1,0)) == -1. Not sure if that'll ever be useful, though.

Comment 14

[[e-mail] (:bhearsum)]

Comment on attachment 776962 [details] [diff] [review]
Use numerical gecko_version testing for config settings that ride the trains

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

Without major restructuring (eg, class-y configs rather than simple dicts), I don't really see a better way to handle the keys/values stuff.

I was going to double check that this doesn't change anything, but it's already bitrotten :(.

r- for the config_common stuff, which I really think should be in master_common. The other stuff is negotiable.

::: mozilla-tests/mobile_config.py
@@ +1,5 @@
>  from copy import deepcopy
>  
>  import config_common
>  reload(config_common)
> +from config_common import TALOS_CMD, loadDefaultValues, loadMainVersions, loadCustomTalosSuites, loadTalosSuites

Wow, I just noticed that we reload config_common in multiple files. I'm surprised that this works. I guess it's okay since we're just creating data structures....

::: mozilla/b2g_config.py
@@ -911,5 @@
> -        'otoro': {},
> -        'inari': {},
> -        'leo': {},
> -        'leo_eng': {},
> -        'hamachi': {},

I assume these get removed because the list is the same as what's in PLATFORMS?

@@ +1182,5 @@
>  BRANCHES['try']['platforms']['emulator']['mozharness_config']['extra_args'] = ['--target', 'generic', '--config', 'b2g/releng-try.py', '--b2g-config-dir', 'emulator', '--gaia-languages-file', 'locales/languages_dev.json', '--gecko-languages-file', 'gecko/b2g/locales/all-locales']
>  BRANCHES['try']['platforms']['emulator_debug']['slaves'] = TRY_SLAVES['mock']
>  BRANCHES['try']['platforms']['emulator_debug']['mozharness_config']['extra_args'] = ['--target', 'generic', '--config', 'b2g/releng-try.py', '--b2g-config-dir', 'emulator', '--debug', '--gaia-languages-file', 'locales/languages_dev.json', '--gecko-languages-file', 'gecko/b2g/locales/all-locales']
>  
> +branches_before = lambda key, maxval: values_before(BRANCHES, key, maxval)

I'd prefer not to have this indirection...I think it's more confusing than helpful.

::: mozilla/config_common.py
@@ +1,1 @@
> +def loadMainVersions(BRANCHES):

master_common.py already exists and is used by both build and test masters - any reason not to put this stuff in there? Then you wouldn't have to duplicate it, either.

@@ +15,5 @@
> +def keys_before(map, key, maxval):
> +    for k, v in map.items():
> +        value = v.get(key)
> +        if value and cmp(value, maxval) < 0:
> +            yield k

This common code could be factored, or you could yield both and let the consumer deal with it. Both of those assume that it's not very expensive to yield both initially. Certainly not r-'ing on this.

Comment 15

[Steve Fink [:sfink] [:s:]]

(In reply to Ben Hearsum [:bhearsum] from comment #14)
> Comment on attachment 776962 [details] [diff] [review]
> Use numerical gecko_version testing for config settings that ride the trains
> 
> Review of attachment 776962 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Without major restructuring (eg, class-y configs rather than simple dicts),
> I don't really see a better way to handle the keys/values stuff.
> 
> I was going to double check that this doesn't change anything, but it's
> already bitrotten :(.
> 
> r- for the config_common stuff, which I really think should be in
> master_common. The other stuff is negotiable.

Oh! I didn't see master_common.py. Hm, I guess maybe because it's only included in *.cfg, and I keep grepping through *.py. Then again, I was just reading this file to figure out how priorizeBuilders works...

Awesome, that's what I wanted.

Oh, wait. Except config.py needs to use it, and config.py doesn't import master_common. Er... would it be ok if it did? Seems a little weird.

> ::: mozilla-tests/mobile_config.py
> @@ +1,5 @@
> >  from copy import deepcopy
> >  
> >  import config_common
> >  reload(config_common)
> > +from config_common import TALOS_CMD, loadDefaultValues, loadMainVersions, loadCustomTalosSuites, loadTalosSuites
> 
> Wow, I just noticed that we reload config_common in multiple files. I'm
> surprised that this works. I guess it's okay since we're just creating data
> structures....

I should probably make an effort to understand all this reloading business someday.

> ::: mozilla/b2g_config.py
> @@ -911,5 @@
> > -        'otoro': {},
> > -        'inari': {},
> > -        'leo': {},
> > -        'leo_eng': {},
> > -        'hamachi': {},
> 
> I assume these get removed because the list is the same as what's in
> PLATFORMS?

These get removed because they're confusing (according to me) and ignored (according to catlee). BRANCHES[*]['platforms']['otoro'] is used. BRANCHES[*]['otoro'] is not. mozilla/b2g_config.py is the only file that uses this structure, and the code right after it is set up puts things under ['platforms'] like everything else. So I'm pretty sure it's just a bug.

I do kind of wonder why the b2g config doesn't use 'lock_platforms', and instead has a huge set of MERGE DAY chunks that remove all the unwanted stuff. Seems like it would be easier to just enumerate the allowed platforms for each branch. But maybe I'm not understanding exactly what's going on here.

> @@ +1182,5 @@
> >  BRANCHES['try']['platforms']['emulator']['mozharness_config']['extra_args'] = ['--target', 'generic', '--config', 'b2g/releng-try.py', '--b2g-config-dir', 'emulator', '--gaia-languages-file', 'locales/languages_dev.json', '--gecko-languages-file', 'gecko/b2g/locales/all-locales']
> >  BRANCHES['try']['platforms']['emulator_debug']['slaves'] = TRY_SLAVES['mock']
> >  BRANCHES['try']['platforms']['emulator_debug']['mozharness_config']['extra_args'] = ['--target', 'generic', '--config', 'b2g/releng-try.py', '--b2g-config-dir', 'emulator', '--debug', '--gaia-languages-file', 'locales/languages_dev.json', '--gecko-languages-file', 'gecko/b2g/locales/all-locales']
> >  
> > +branches_before = lambda key, maxval: values_before(BRANCHES, key, maxval)
> 
> I'd prefer not to have this indirection...I think it's more confusing than
> helpful.

Would you prefer

  branches_before = funcutils.partial(values_before, BRANCHES)

? :-)

Yeah, I don't like it much either, but I really wanted the eventual uses to read well. "values_before" is confusing. I guess I'll ignore the fact that values_before is generic, and rename it to branches_before and then pass in BRANCHES directly. (That's the only reason I didn't do it that way in the first place -- it feels wrong to write a totally generic function and then name it after one particular use. But it *is* the only use for now, and the lambda thing is truly ugly.)

> ::: mozilla/config_common.py
> @@ +1,1 @@
> > +def loadMainVersions(BRANCHES):
> 
> master_common.py already exists and is used by both build and test masters -
> any reason not to put this stuff in there? Then you wouldn't have to
> duplicate it, either.

As above, the reason is that I need to use this function from files that do not import master_common.py, and I'm not sure they should.

> @@ +15,5 @@
> > +def keys_before(map, key, maxval):
> > +    for k, v in map.items():
> > +        value = v.get(key)
> > +        if value and cmp(value, maxval) < 0:
> > +            yield k
> 
> This common code could be factored, or you could yield both and let the
> consumer deal with it. Both of those assume that it's not very expensive to
> yield both initially. Certainly not r-'ing on this.

You're right, may as well yield both.

Comment 16

[Steve Fink [:sfink] [:s:]]

I'm landing the patch to fix up the current state. The patch is smaller now, since one of the places it touched is now gone.

http://hg.mozilla.org/build/buildbot-configs/rev/ac570e465dc9

Comment 17

[Steve Fink [:sfink] [:s:]]

Attachment: Use numerical gecko_version testing for config settings that ride the trains

I've rebased to the current (in flux) state, and I think I addressed all review comments. The diff with the previous config is empty.

Comment 18

[Nick Thomas [:nthomas] (UTC+12)]

In production

Comment 19

[[e-mail] (:bhearsum)]

FYI: I'm really anxious to get this in, but I won't be able to look at it until next week. I'm fine for someone else to review it if they're up for it sooner.

Comment 20

[Steve Fink [:sfink] [:s:]]

(In reply to Ben Hearsum [:bhearsum] from comment #19)
> FYI: I'm really anxious to get this in, but I won't be able to look at it
> until next week. I'm fine for someone else to review it if they're up for it
> sooner.

I wouldn't rush -- I'll need to rebase past this uplift, and having this functionality is only relevant during uplifts. I started this latest rebase too late for it to be of any help during *this* uplift.

Comment 21

[Steve Fink [:sfink] [:s:]]

Attachment: Use numerical gecko_version testing for config settings that ride the trains

Rebased. Once again, the only hard problem was setting the versions properly. Though I continue to do the stupid "rebase, see a bunch of problems, painstakingly track down one of them to not having set the versions correctly, update the versions to something a little better but still not quite right, repeat." I'm too disconnected from the release process to ever have any clue what versions we're on.

In this rebase, I also split setMainVersions into setMainFirefoxVersions and setMainCommVersions, since it turns out I was setting the gecko versions of comm-beta and comm-aurora in two different places. (And I was setting them to different values until I tracked it down, of course.)

Comment 22

[[e-mail] (:bhearsum)]

Comment on attachment 802471 [details] [diff] [review]
Use numerical gecko_version testing for config settings that ride the trains

Steve told me that a new patch is coming.

Comment 23

[Steve Fink [:sfink] [:s:]]

Attachment: Use numerical gecko_version testing for config settings that ride the trains

Nice trivial rebase, though it looks like someone updated a bunch of the comments in a way that's kind of redundant with the new code. Oh well.

I'm wondering if we should have separate "MERGE DAY" and "EOL" comments to scan for updates?

Or items_before() etc could spew a warning if its return value is empty. That would catch the train-based EOLs.

Comment 24

[[e-mail] (:bhearsum)]

Comment on attachment 806135 [details] [diff] [review]
Use numerical gecko_version testing for config settings that ride the trains

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

Please land this!!!!

::: mozilla-tests/config.py
@@ +6,5 @@
>      loadTalosSuites, nested_haskey, get_talos_slave_platforms
>  
> +import master_common
> +reload(master_common)
> +from master_common import setMainFirefoxVersions, items_before, items_at_least

The multiple reloads of this are a little scary, but they _should_ be OK. We need to watch out for bad reference errors after landing. If you want some background on our issues with it, see http://atlee.ca/blog/posts/blog20081121python-reload-danger-here-be-dragons.html

Comment 25

[[e-mail] (:bhearsum)]

(In reply to Steve Fink [:sfink] from comment #23)
> Created attachment 806135 [details] [diff] [review]
> Use numerical gecko_version testing for config settings that ride the trains
> 
> Nice trivial rebase, though it looks like someone updated a bunch of the
> comments in a way that's kind of redundant with the new code. Oh well.
> 
> I'm wondering if we should have separate "MERGE DAY" and "EOL" comments to
> scan for updates?
> 
> Or items_before() etc could spew a warning if its return value is empty.
> That would catch the train-based EOLs.

I think this is a fantastic idea. We could probably get releng-ci to make some noise when we see such messages.

Comment 26

[Steve Fink [:sfink] [:s:]]

http://hg.mozilla.org/build/buildbot-configs/rev/97b9e0b83e84