821954 - Add ability to specify branches and architectures for Autophone builds

Status: RESOLVED FIXED

(Testing Graveyard :: Autophone, enhancement)

Description

[Bob Clary [:bc] (inactive)]

Attachment: patch

Autophone has some hard coded branch and architecture dependencies. This patch adds the ability to specify the branches (mozilla-central, mozilla-inbound, mozilla-aurora, mozilla-beta) as well as architecture (arvm6) so that we can test different trees. The architecture argument only has an affect if it is set to armv6, otherwise it just assumes the default android arm architecture.

This patch is applied after v6 patch in bug 785129.

The real solution will be to add a job scheduler which uses mozpool but this works for now for testing.

Comment 1

[Mark Côté [:mcote]]

Comment on attachment 692544 [details] [diff] [review]
patch

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

So my only big problem here is that it seems that, unlike branches and build type, the architectures are mutually exclusive, that is, you can't run autophone with both armv6 and armv7 phones.  I think we'd want to have the flexibility of running one instance with a mix of architectures.  Is there some particular reason we couldn't do that?

::: autophone.py
@@ +127,5 @@
>  
>          if enable_pulse:
> +            suffix = ''
> +            if architecture == 'armv6':
> +                suffix = ['android-armv6']

I don't think this is supposed to be a list.

@@ +623,5 @@
> +                      'mozilla-aurora, mozilla-beta. Default is mozilla-central.')
> +    parser.add_option('--buildtypes', type='string',
> +                      dest='buildtypes', default='opt',
> +                      help='Comma separated list of build types: '
> +                      'One or more of opt,debug. Default is opt.')

I prefer the append action for this kind of thing--it allows you specify the option more than once, e.g. --branch mozilla-central --branch mozilla-inbound.  It just simplifies parsing and eliminates silly mistakes like adding a space after a comma.  But it's not a big deal.

::: builds.py
@@ +93,5 @@
> +        suffix = ''
> +        if self.architecture == 'armv6':
> +            suffix = '-armv6'
> +        for branch in self.branches:
> +            dirnames.append('%s%s-android%s/' % (self.main_ftp_url, branch, suffix))

List comprehension here instead, for brevity?

@@ +134,5 @@
>              self.lines.append(line)
>  
> +    def __init__(self, branches, buildtypes, architecture,
> +                 cache_dir='builds', override_build_dir = None,
> +                 enable_unittests = False):

Can you fix the excessive horizontal spacing here, while you're at it?

Comment 2

[Bob Clary [:bc] (inactive)]

(In reply to Mark Côté ( :mcote ) from comment #1)
> Comment on attachment 692544 [details] [diff] [review]
> patch
> 
> Review of attachment 692544 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> So my only big problem here is that it seems that, unlike branches and build
> type, the architectures are mutually exclusive, that is, you can't run
> autophone with both armv6 and armv7 phones.  I think we'd want to have the
> flexibility of running one instance with a mix of architectures.  Is there
> some particular reason we couldn't do that?
> 

Yes, the architecture choice is fixed per Autophone instance. Right now, jobs are farmed out to all of the available phones. The entire job and build selection and queuing would have to be rewritten to allow multiple architectures on a single instance of Autophone. It seemed like a pretty big job and I didn't think it was worth it at the moment since we will be using mozpool to manage the devices and will need a different job scheduler that will work with mozpool. This patch was primarily for use in getting Autophone to drive an APC.io board and was intended as a temporary solution. Now that APC.io boards are indefinitely deferred, we might not need this at all. It is still useful if you want to test out an armv6 device using Autophone though.

> ::: autophone.py
> @@ +127,5 @@
> >  
> >          if enable_pulse:
> > +            suffix = ''
> > +            if architecture == 'armv6':
> > +                suffix = ['android-armv6']
> 
> I don't think this is supposed to be a list.
> 

doh.

> @@ +623,5 @@
> > +                      'mozilla-aurora, mozilla-beta. Default is mozilla-central.')
> > +    parser.add_option('--buildtypes', type='string',
> > +                      dest='buildtypes', default='opt',
> > +                      help='Comma separated list of build types: '
> > +                      'One or more of opt,debug. Default is opt.')
> 
> I prefer the append action for this kind of thing--it allows you specify the
> option more than once, e.g. --branch mozilla-central --branch
> mozilla-inbound.  It just simplifies parsing and eliminates silly mistakes
> like adding a space after a comma.  But it's not a big deal.

Ok.

> 
> ::: builds.py
> @@ +93,5 @@
> > +        suffix = ''
> > +        if self.architecture == 'armv6':
> > +            suffix = '-armv6'
> > +        for branch in self.branches:
> > +            dirnames.append('%s%s-android%s/' % (self.main_ftp_url, branch, suffix))
> 
> List comprehension here instead, for brevity?
> 

As you wish.

> @@ +134,5 @@
> >              self.lines.append(line)
> >  
> > +    def __init__(self, branches, buildtypes, architecture,
> > +                 cache_dir='builds', override_build_dir = None,
> > +                 enable_unittests = False):
> 
> Can you fix the excessive horizontal spacing here, while you're at it?

Not sure what you mean here. I normally let emacs format it for me and checking http://www.python.org/dev/peps/pep-0008/#indentation it appears I'm ok with the # Aligned with opening delimiter example and the others that look similar. Do you want me to format it like the # Further indentation required as indentation is not distinguishable example?

Comment 3

[Mark Côté [:mcote]]

(In reply to Bob Clary [:bc:] from comment #2)
> (In reply to Mark Côté ( :mcote ) from comment #1)
> > Comment on attachment 692544 [details] [diff] [review]
> > patch
> > 
> > Review of attachment 692544 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > So my only big problem here is that it seems that, unlike branches and build
> > type, the architectures are mutually exclusive, that is, you can't run
> > autophone with both armv6 and armv7 phones.  I think we'd want to have the
> > flexibility of running one instance with a mix of architectures.  Is there
> > some particular reason we couldn't do that?
> > 
> 
> Yes, the architecture choice is fixed per Autophone instance. Right now,
> jobs are farmed out to all of the available phones. The entire job and build
> selection and queuing would have to be rewritten to allow multiple
> architectures on a single instance of Autophone. It seemed like a pretty big
> job and I didn't think it was worth it at the moment since we will be using
> mozpool to manage the devices and will need a different job scheduler that
> will work with mozpool. This patch was primarily for use in getting
> Autophone to drive an APC.io board and was intended as a temporary solution.
> Now that APC.io boards are indefinitely deferred, we might not need this at
> all. It is still useful if you want to test out an armv6 device using
> Autophone though.

Bah, right, I wasn't thinking about the details of autophone job dispersal.  You're right; in the current form, this restriction would apply.  But you are also right about APC.io being deferred.  Given that autophone should be using mozpool by the time we get to armv6, how about you apply this patch without the architecture option.  I'll switch to an r+ for that.

> > @@ +134,5 @@
> > >              self.lines.append(line)
> > >  
> > > +    def __init__(self, branches, buildtypes, architecture,
> > > +                 cache_dir='builds', override_build_dir = None,
> > > +                 enable_unittests = False):
> > 
> > Can you fix the excessive horizontal spacing here, while you're at it?
> 
> Not sure what you mean here. I normally let emacs format it for me and
> checking http://www.python.org/dev/peps/pep-0008/#indentation it appears I'm
> ok with the # Aligned with opening delimiter example and the others that
> look similar. Do you want me to format it like the # Further indentation
> required as indentation is not distinguishable example?

No no, I just meant the spaces around the = with respect to optional function arguments. :) THe indentation is fine.

Comment 4

[Mark Côté [:mcote]]

Comment on attachment 692544 [details] [diff] [review]
patch

r+ with architecture option removed.  If we, for some reason, need armv6 before mozpool, we can revisit this.

Comment 5

[Bob Clary [:bc] (inactive)]

Attachment: patch v2

Mark, I think this does it. I renamed the --branch option to --repo to better reflect its real meaning.  If you want to take a quick look at the interdiff and let me know if anything stands out, that would be nice. I'll push this after the patch for bug 785129 lands.

Comment 6

[Bob Clary [:bc] (inactive)]

Attachment: patch v3

crap, forgot to amend the commit.

Comment 7

[Mark Côté [:mcote]]

Comment on attachment 694020 [details] [diff] [review]
patch v3

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

::: autophone.py
@@ +606,3 @@
>  
>      (options, args) = parser.parse_args()
> +    if options.repos is None:

I think this can just be "if not options.repos:", just in case we get an empty list, which should follow the same behaviour. Same with options.buildtypes below.

::: trigger_runs.py
@@ +125,5 @@
> +        options.repos = ['mozilla-central']
> +
> +    if options.buildtypes is None:
> +        options.buildtypes = ['opt']
> +

As above.

Comment 8

[Bob Clary [:bc] (inactive)]

https://github.com/mozilla/autophone/commit/d687f599e7214609f51c61983b0fd9dd02983bad

did i mention i hate git?