Steven Michaud [:smichaud] (Retired) Reporter
	Description • 16 years ago

The tryservers have continued to get more popular -- they're sometimes
queued three or four builds deep.  So it can sometimes take 2-3 hours
for a build to finish (and sometimes more than an hour for a build to
start).

Many of us who use the tryserver often only need a build on one
particular platform.  That's certainly true of me (I almost always
only want an OS X build).  So I suspect delays would be shorter if the
tryserver web interface (https://build.mozilla.org/sendchange.cgi)
allowed submitters to select among Windows, Linux and OS X builds.

The web interface should probably still default to building on all
three platforms -- i.e. there should be Windows, Linux and OS X
checkboxes whose default value is 'checked'.

	John O'Duinn [:joduinn] (please use "needinfo?" flag)
	Comment 1 • 16 years ago

Triaging over to Lukas who is doing other changes in the same area for adding unittest support. See bug#445611.

	Lukas Blakk [:lsblakk] use ?needinfo Assignee
	Comment 3 • 15 years ago

It's probably easier to look at this as the complete file:

http://pastebin.mozilla.org/669580

	Lukas Blakk [:lsblakk] use ?needinfo Assignee
	Comment 4 • 15 years ago

The web end of this has been tested, writing to the info file, validating buildType and platform selection.

Now I need to be able to test processchanges.pl which is a cron job on build. Will look into how to set up a staging test of this.

	Lukas Blakk [:lsblakk] use ?needinfo Assignee
	Comment 7 • 15 years ago

With this patch and the two preceding it in try-staging, I was able to submit patches to for specific build/platform combos eg: MAC UNITTEST only or LINUX WIN32 BUILD

The UI now has a section for selecting if you want BUILD or UNITTEST (which includes a build) and then another row for selecting platform.  These need to be filled out for the patch to be submitted through the web interface - an alert will show up if either are left empty to ensure that the .info files are generated in a way that will work with the new processchanges.pl

In processchanges.pl there is now a loop creating a single sendchange for each platform/buildtype combo and therefore kicking of builds according to the granularity requested by the submitter.

I had to make changes to tryserver.py to accomodate the addition of more "fake" branch names that we would want to have actually attempted to be pulled from hg.m.o

That's it.  I look forward to trying this in production now and seeing if developers use it.

	Nick Thomas [:nthomas] (UTC+12)
	Comment 8 • 15 years ago

A couple of quick comments on 
  http://4.bp.blogspot.com/_kUuZHNHDPdc/SqpnQAc7wNI/AAAAAAAAAR0/nwB46zNePFs/s1600-h/Screen+shot+2009-09-11+at+11.03.30+AM.png

I was wondering why BUILD and UNITTEST are radio buttons rather than checkboxes. Did you mean in comment #7 that UNITTEST implies BUILD ? It looks either/or in the UI. And there some other platforms to list too, WinMO, WinCE, Maemo for hg based builds.

Beware, someone will request a way to do this using push-to-try.

	Lukas Blakk [:lsblakk] use ?needinfo Assignee
	Comment 9 • 15 years ago

They are radio buttons because a unittest run does indeed include a build at this time.  Since you're the second person to ask about this I think that I should put a comment under the radio buttons saying that this is the case. 

It would be possible to make this checkboxes at a later date so there would be mobile build/unittest options and I suppose eventually we might have packaged builds on try too. 

It's not impossible to imagine doing this in push-to-try, but that's definitely for another quarter :)

	John O'Duinn [:joduinn] (please use "needinfo?" flag)
	Comment 10 • 15 years ago

From irc discussion, some points to consider:

1) Need to add linux-arm, WinCE, WinMO

2) tryserver still does build-and-unittest as one job, even though we converted over the main production systems to have separate jobs for build, and for unittests.

	bhearsum@mozilla.com (:bhearsum)
	Comment 11 • 15 years ago

(In reply to comment #10)
> From irc discussion, some points to consider:
> 2) tryserver still does build-and-unittest as one job, even though we converted
> over the main production systems to have separate jobs for build, and for
> unittests.

This is a separate bug.

	John O'Duinn [:joduinn] (please use "needinfo?" flag)
	Comment 12 • 15 years ago

(In reply to comment #11)
> (In reply to comment #10)
> > From irc discussion, some points to consider:
> > 2) tryserver still does build-and-unittest as one job, even though we converted
> > over the main production systems to have separate jobs for build, and for
> > unittests.
> 
> This is a separate bug.

Doing the factory cleanup, etc, yes, is separate. Filed bug#520222 to track.

However, changing the Tryserver UI is done here, hence my comment.

	John Daggett (:jtd)
	Comment 14 • 14 years ago

Any progress recently?  It would be extremely helpful to allow platform-specific tryserver runs, so that testing fixes for one specific platform doesn't burden the entire tryserver infrastructure.

	Mehdi Mulani [:mmm] (I don't check this)
	Comment 15 • 14 years ago

(In reply to comment #14)
> Any progress recently?  It would be extremely helpful to allow
> platform-specific tryserver runs, so that testing fixes for one specific
> platform doesn't burden the entire tryserver infrastructure.

A temporary work-around was to use extra mozconfig files for each platform with the line "exit 1". (Info on the mozconfig files at https://wiki.mozilla.org/Build:TryServerAsBranch#Using_a_custom_mozconfig ) It stops the build process and results in reds but does the job.

	John Daggett (:jtd)
	Comment 16 • 14 years ago

(In reply to comment #15)
> A temporary work-around was to use extra mozconfig files for each platform with
> the line "exit 1". (Info on the mozconfig files at
> https://wiki.mozilla.org/Build:TryServerAsBranch#Using_a_custom_mozconfig ) It
> stops the build process and results in reds but does the job.

So to run only on win32, one needs to add a file with 'exit 1' for all other platforms?  For example:

mozconfig-extra-macosx
mozconfig-extra-macosx64
mozconfig-extra-linux
mozconfig-extra-linux64
mozconfig-extra-mobile

Will that cover it?

	Mehdi Mulani [:mmm] (I don't check this)
	Comment 17 • 14 years ago

(In reply to comment #16)
>
> mozconfig-extra-macosx
> mozconfig-extra-macosx64
> mozconfig-extra-linux
> mozconfig-extra-linux64
> mozconfig-extra-mobile
> 
> Will that cover it?

With the exception of "mozconfig-extra-mobile", that sounds exactly like what I did.

	Lukas Blakk [:lsblakk] use ?needinfo Assignee
	Comment 18 • 14 years ago

Draft of a try server syntax that can be used in an hg commit message or a .info file that gets pushed with the try patch: https://wiki.mozilla.org/User:Lukasblakk/TryServerSyntax

	Nick Thomas [:nthomas] (UTC+12)
	Comment 19 • 14 years ago

Might be worth looking at OptionParser to parse the args rather than inventing your own parser. You can pass in a string split on ' ' and get it to do all the heavy lifting and clever stuff.

	Lukas Blakk [:lsblakk] use ?needinfo Assignee
	Comment 21 • 14 years ago

This is my test parser so far, it uses a valid_builders.py list (which I am now working on generating out of misc.py on a reconfig) so that it's always checking against an up to date list of what platforms/builders are available.

Notes:

* I used parse_known_args so that anything that is not known goes into its own area and the parser doesn't error out on bad input
* I went back to using '--' so that the arg parser could tell what was positional and what was optional there are probably other ways to make this work but this is the path I took that worked first

Looking forward to feedback

	Lukas Blakk [:lsblakk] use ?needinfo Assignee
	Comment 22 • 14 years ago

Took out a debug print.

	Lukas Blakk [:lsblakk] use ?needinfo Assignee
	Comment 24 • 14 years ago

This works right now in staging.  It requires installing the argparse module to python 2.6 (which has been done on staging-master's buildbot-0.8.0/bin/python). 

The current patch uses the getPage with http:// address.  Catlee pointed out that there is JSON available but I haven't had time to explore or test that yet. Maybe that can be in round 2 when we also work with commit info file?

	Lukas Blakk [:lsblakk] use ?needinfo Assignee
	Comment 25 • 14 years ago

Comment on attachment 468346 [details] [diff] [review]
[tested] allow for selecting of try builds/test/talos through hg commit comments

filed bug 589847 to address the TODO in valid_builders.py

	Chris AtLee [:catlee]
	Comment 26 • 14 years ago

Comment on attachment 468346 [details] [diff] [review]
[tested] allow for selecting of try builds/test/talos through hg commit comments

This couldn't have been tested:

  File "misc.py", line 590
    else:
        ^
IndentationError: unindent does not match any outer indentation level

	Lukas Blakk [:lsblakk] use ?needinfo Assignee
	Comment 27 • 14 years ago

sorry, bad merge when I updated my repo and reapplied the patch. the code has been tested, and now I have _also_ done a checkconfig with this exact patch.

	Chris AtLee [:catlee]
	Comment 28 • 14 years ago

Comment on attachment 468431 [details] [diff] [review]
[tested] allow for selecting of try builds/test/talos through hg commit comments

>@@ -1830,22 +1853,31 @@ def generateTalosBranchObjects(branch, b
>                     'slavenames': platform_config[slave_platform]['slaves'],
>                     'builddir': "%s_%s_test-%s" % (branch, slave_platform, suite),
>                     'factory': factory,
>                     'category': branch,
>                     'properties': {'branch': branch, 'platform': slave_platform},
>                 }
>                 if not merge:
>                     nomergeBuilders.append(builder['name'])
>-                s = MultiScheduler(
>+                extra_args = {}
>+                if branch == "tryserver":
>+                    scheduler_class = BuilderChooserScheduler
>+                    extra_args['chooserFunc'] = chooserFunc
>+                    numBuilds = 1
>+                else:
>+                    scheduler_class = MultiScheduler
>+                    numBuilds = tests

I think numBuilds should be 'tests' in both cases, right?

>@@ -1882,18 +1914,30 @@ def generateTalosBranchObjects(branch, b
>                                 suites_name, suites, branch_config.get('mochitest_leak_threshold', None),
>                                 branch_config.get('crashtest_leak_threshold', None),
>                                 platform_config[slave_platform]['slaves']))
> 
>                     for scheduler_name, test_builders, merge in triggeredUnittestBuilders:
>                         scheduler_branch = ('%s-%s-%s-unittest' % (branch, platform, test_type))
>                         if not merge:
>                             nomergeBuilders.extend(test_builders)
>-                        branchObjects['schedulers'].append(Scheduler(name=scheduler_name, branch=scheduler_branch, builderNames=test_builders, treeStableTimer=None))
>-
>+                        extra_args = {}
>+                        if branch == "tryserver":
>+                            scheduler_class = BuilderChooserScheduler
>+                            extra_args['chooserFunc'] = chooserFunc
>+                            extra_args['numberOfBuildsToTrigger'] = 1
>+                        else:
>+                            scheduler_class = Scheduler
>+                        branchObjects['schedulers'].append(scheduler_class(
>+                            name=scheduler_name, 
>+                            branch=scheduler_branch, 
>+                            builderNames=test_builders, 
>+                            treeStableTimer=0, 
>+                            **extra_args
>+                        ))

You've changed treeStableTimer from None to 0 here.  Why?

>+    def parseDataError(failure, c):
>+        log.msg("Couldn't parse data: %s/nSending default try set." % sys.stderr.write(str(failure)))

What is this doing?  Do you need sys.stderr.write?


>+def TryParser(message, builderNames):
>+
>+    parser = argparse.ArgumentParser(description='Pass in a commit message and a list \
>+                                     and tryParse populates the list with the builderNames\
>+                                     that need schedulers.')
>+
>+    parser.add_argument('--build',
>+                        default='d',
>+                        dest='build',
>+                        help='accepts the build types requested ')
>+    parser.add_argument('--p',
>+                        default='all',
>+                        dest='desktop',
>+                        help='provide a list of desktop platforms, or specify none (default is all)')
>+    parser.add_argument('--m',
>+                        default='all',
>+                        dest='mobile',
>+                        help='provide a list of mobile platform, or specify none (default is all)')
>+    parser.add_argument('--u',
>+                        default='none',
>+                        dest='test',
>+                        help='provide a list of unit tests, or specify all (default is None)')
>+    parser.add_argument('--t',
>+                        default='none',
>+                        dest='talos',
>+                        help='provide a list of talos tests, or specify all (default is None)')
>+
>+    (options, unknown_args) = parser.parse_known_args(message)

So it looks like you require 'message' to be split into a list already.  Can you make it so that this function accepts a string, and does the splitting itself?

	Lukas Blakk [:lsblakk] use ?needinfo Assignee
	Comment 29 • 14 years ago

ran against (adapted) tests, and also in sm01 with an empty commit comment to ensure default set triggered.  

This version addresses catlee's comments from first review.  treeStableTimer in generateTalosBranchObjects is back to None (I don't have a reason for it being at 0 other than most likely a c&p error).  The number of builds triggered in the talos branch section is set to |tests| for both which is correct, that allows us to set the number of tests run in the test-master's config.py.  The sys.stderr.write is out - and not needed str(failure) works just fine.  

Also I took out the need for misc_scheduler.py to make the list of commands for try_parser, it now just passes a string which is handled by processMessage in try_parser.

	Chris AtLee [:catlee]
	Comment 30 • 14 years ago

Comment on attachment 468801 [details] [diff] [review]
[tested] allow for selecting of try builds/test/talos through hg commit comments

Looks good!

	Lukas Blakk [:lsblakk] use ?needinfo Assignee
	Comment 31 • 14 years ago

tested on both staging-master and talos-staging-master02 - both of these masters have had the argparse module installed as well.  We will need argparse installed on the production masters for this to go in.

	Chris AtLee [:catlee]
	Comment 32 • 14 years ago

Comment on attachment 469129 [details] [diff] [review]
[tested] allow for selecting of try builds/test/talos through hg commit comments now with ability to manually override in sendchange comments

Ok, just nits left:

>     # schedulers
>     # this one gets triggered by the HG Poller
>+    # for Try we have a custom scheduler that can accept a function to read commit comments
>+    # in order to know what to schedule
>     extra_args = {}
>+    if config.get('enable_try'):
>+        scheduler_class = BuilderChooserScheduler
>+        extra_args['tryChooser'] = tryChooser

Leave the name of the scheduler's function as 'chooserFunc'.  So this would be extra_args['chooserFunc'] = tryChooser.  There a few places you need to do this.

>+def tryChooser(s, all_changes):

This one should stay as 'tryChooser'.

>+    log.msg("Looking at changes: %s" % all_changes)
>+
>+    buildersPerChange = {}
>+
>+    dl = []
>+    def parseData(data, c):
>+        # Grab comments, hand off to try_parser
>+        match = re.search("try:", c.comments)
>+        if match: 
>+            customBuilders = TryParser(c.comments,s.builderNames)
>+        else:
>+            comments = re.search("<div class=\"page_body\">(.*?)</div>", data, re.M + re.S)
>+            if not comments:
>+            # still need to parse a comment string to get the default set

weird indentation on this comment

>+                comments = ""
>+            else:
>+                comments = comments.group(1)
>+            customBuilders = TryParser(comments,s.builderNames)

Put a space after 'comments' please, i.e. TryParser(comments, s.builderNames).

>+        buildersPerChange[c] = customBuilders
>+
>+    def parseDataError(failure, c):
>+        log.msg("Couldn't parse data: Requesting default try set.")
>+        parseData("",c)

space needed after comma here too

>+class BuilderChooserScheduler(MultiScheduler):
>+    compare_attrs = Scheduler.compare_attrs + ('tryChooser',)
>+    def __init__(self, tryChooser, **kwargs):

As above, leave this called 'chooserFunc' here.

The rest looks good!

	Lukas Blakk [:lsblakk] use ?needinfo Assignee
	Comment 33 • 14 years ago

we'll need these enabled again now to handle custom selections

	Lukas Blakk [:lsblakk] use ?needinfo Assignee
	Comment 34 • 14 years ago

this one addresses the comments

	Chris AtLee [:catlee]
	Comment 35 • 14 years ago

Comment on attachment 469144 [details] [diff] [review]
[tested] allow for selecting of try builds/test/talos through hg commit comments now with ability to manually override in sendchange comments (with nits fixed)

Oh, one last thing, BuilderChooserScheduler's compare_attrs should be based off of MultiScheduler's compare_attrs, not Scheduler's

	Lukas Blakk [:lsblakk] use ?needinfo Assignee
	Comment 36 • 14 years ago

(In reply to comment #35)
> Comment on attachment 469144 [details] [diff] [review]
> [tested] allow for selecting of try builds/test/talos through hg commit
> comments now with ability to manually override in sendchange comments (with
> nits fixed)
> 
> Oh, one last thing, BuilderChooserScheduler's compare_attrs should be based off
> of MultiScheduler's compare_attrs, not Scheduler's

I have made this change in what will be pushed in tomorrow's downtime.

	Lukas Blakk [:lsblakk] use ?needinfo Assignee
	Comment 37 • 14 years ago

This would replace our current talos_sendchange.py script as it will no longer work with the new try selection scripts since the hg commit comments now override the sendchange from buildbot builds.  With this new script we can specify build type (opt, debug, both) as well as which platforms, tests, talos we want for a particular try changeset -- this is still given to us by the try user as a email-changeset so that we can call it as follows:
  python try_sendchange.py anygregor@gmail.com-2708a2e2b5c0 --build o --p all --u all --t all

and the script then scans through ftp, creates the appropriate sendchanges requested and put a comment in "try: " format to override the hg commit reading.

We can install this on the masters with an alias to help the buildduty person use the scripts when try requests are made.

	Lukas Blakk [:lsblakk] use ?needinfo Assignee
	Comment 38 • 14 years ago

cleaned up the first couple of lines of the new script to correct the name

	Nick Thomas [:nthomas] (UTC+12)
	Comment 39 • 14 years ago

Comment on attachment 469268 [details] [diff] [review]
remove talos_sendchange.py and replace with try_sendchange.py

>diff --git a/buildfarm/maintenance/try_sendchange.py b/buildfarm/maintenance/try_sendchange.py
>+    parser.add_argument('--build',
>+                        default='d',
>+                        dest='build',
>+                        help='accepts the build types requested ')

Nit: extra space

>+    parser.add_argument('--t',
>+                        default='all',
>+                        dest='talos',
>+                        help='provide a list of talos tests, or specify all (default is None)')

default is All.

>+
>+    (options, unknown_args) = parser.parse_known_args(args)
>+
>+    if options.build == 'do':

Do we also support 'od' ? Bet that'll be a common typo.

>+        options.build = ['opt', 'debug']
>+    elif options.build == 'd':
>+        options.build = ['debug']
>+    elif options.build == 'o':
>+        options.build = ['opt']
>+    else:
>+        options.build = ['opt']

Trigger opt if I go '--build bogus' ? An error would be better, or choices = ['do','d','o'] on the arg parser might work. Seems like you're implementing the parser again ? It seems a little different here, eg no 'all' for --build.

>+    if options.platforms == 'all':
>+        platforms = PLATFORMS
>+    else:
>+        options.platforms = options.platforms.split(',')
>+        platforms = options.platforms

Some input validation would be useful here to check that all the entries in platforms are in PLATFORMS.

>+    if len(args) > 1:

Should be >= 1 if the all the other arguments are truly optional.

>+                    if options.tests:

... and options.tests != 'none' ? Same for talos ? 

Stopping here, the changes to the try schdulers will take too long to wrap my head around.

	Rail Aliiev [:rail]
	Comment 40 • 14 years ago

Comment on attachment 469268 [details] [diff] [review]
remove talos_sendchange.py and replace with try_sendchange.py

Agree with Nick's comments. There are some additional comments from me.

>+import sys, os, argparse, re

Not sure why we need argparse here, which is not available in our current configuration.

>+TEST_MASTERS = ['production-master01.build.mozilla.org:9009']

It will be better to have this variable configurable, so other projects can use it.

>+PLATFORMS = ['linux', 'linux64', 'macosx', 'macosx64', 'win32']
>+    parser.add_argument('--build',
>+                        default='d',
>+                        dest='build',
>+                        help='accepts the build types requested ')

What about
default=['debug'], action='append'
so you can use
--build debug --build opt
and drop do od o d  parsing section?

>+    parser.add_argument('--p',
>+                        default='all',
>+                        dest='platforms',
>+                        help='provide a list of desktop platforms, or specify none (default is all)')

What about
default=PLATFORMS, action='append'
so you can use
--p win32 --p linux --p whatever
and drop if/else block for options.platforms == 'all'?

>+    parser.add_argument('--u',
>+                        default='none',

Did you default=None?

>+                    for f in filelist:
>+                        for suffix in ('.tar.bz2', '.win32.zip', '.dmg'):
>+                            if f.split(' ')[-1].endswith(suffix):
>+                                path = f

Why do you split here? As far as I can see there is no space in files at all.

>+                    if options.tests:
>+                        if packagedTests:

This to ifs may be combined in single one with 'and'.

	Lukas Blakk [:lsblakk] use ?needinfo Assignee
	Comment 41 • 14 years ago

carrying forward catlee's r+ on this, but changing the default set to match what currently happens on try (ie: debug + opt builds as well as all unittests).

Also corrected the compare_attrs for BuilderChooserScheduler.

	Lukas Blakk [:lsblakk] use ?needinfo Assignee
	Comment 43 • 14 years ago

I've addressed some of the feedback/review comments.  The current talos_sendchange.py no longer works now that the hg commit comments have been checked in so I haven't had time to do any kind of dynamic passing of PLATFORMS or TEST_MASTERS, that as well as user input validation (this is only a releng helper script) can come in another iteration.

We need something in place to do what talos_sendchange.py did for the buildduty person and this script does that by having --t default to all and therefore you can use this script as before with just an email-changeset and get all the talos build kicked off.

However, using argparse (which is now installed on all masters) this script now also allows us to use commands like the try_parser's that let us do a sendchange for a subset of unittest, talos, build, platforms (minus mobile at the moment).

	Lukas Blakk [:lsblakk] use ?needinfo Assignee
	Comment 44 • 14 years ago

Comment on attachment 469439 [details] [diff] [review]
[tested] allow for selecting of try builds/test/talos through hg commit comments now with ability to manually override in sendchange comments (with different defaults))

http://hg.mozilla.org/build/buildbotcustom/rev/f3e7d89cae13

	Lukas Blakk [:lsblakk] use ?needinfo Assignee
	Comment 45 • 14 years ago

Comment on attachment 469142 [details] [diff] [review]
turn on try talos sendchanges to test-masters

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

	Lukas Blakk [:lsblakk] use ?needinfo Assignee
	Comment 46 • 14 years ago

Comment on attachment 469440 [details] [diff] [review]
turn on try talos sendchanges to test-masters for staging too

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

	Rail Aliiev [:rail]
	Comment 47 • 14 years ago

Comment on attachment 469445 [details] [diff] [review]
remove talos_sendchange.py and replace with try_sendchange.py

(In reply to comment #43)
> We need something in place to do what talos_sendchange.py did for the buildduty
> person and this script does that by having --t default to all and therefore you
> can use this script as before with just an email-changeset and get all the
> talos build kicked off.

OK, r+ but if you promise to polish this tool in the future. :)

	Lukas Blakk [:lsblakk] use ?needinfo Assignee
	Comment 48 • 14 years ago

turns out i was sending a talos sendchange for all platforms in PLATFORMS and not comparing to what platforms exist in the ftp dir.

	Lukas Blakk [:lsblakk] use ?needinfo Assignee
	Comment 49 • 14 years ago

tested this (includes updates to the unittests)

this is to compensate for having made the default for unittests 'all' which left no way to do --u none since the default had been 'none' before.

adding 'all' and 'none' options for the args will give us more flexibility in changing default set as needed.

	Aki Sasaki (not active)
	Comment 50 • 14 years ago

Comment on attachment 470053 [details] [diff] [review]
add 'all' and 'none' input options to desktop/mobile platforms as well as unittests/talos

r=me if you take out the first four lines in the diff (test_try_parser).

	Lukas Blakk [:lsblakk] use ?needinfo Assignee
	Comment 51 • 14 years ago

Comment on attachment 470053 [details] [diff] [review]
add 'all' and 'none' input options to desktop/mobile platforms as well as unittests/talos

http://hg.mozilla.org/build/buildbotcustom/rev/c1848384d8df

	Lukas Blakk [:lsblakk] use ?needinfo Assignee
	Comment 52 • 14 years ago

removed two dep bugs that do not actually block this.  this functionality exists, works, and is starting to be widely adopted.  closing.