Closed Bug 589847 Opened 11 years ago Closed 11 years ago

Either auto-generate valid_builders.py or expand try_parser to pull from config.py

Categories

(Release Engineering :: General, defect, P3)

x86
All
defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: lsblakk, Assigned: lsblakk)

References

Details

(Whiteboard: [tryserver])

Attachments

(1 file, 3 obsolete files)

In order to do the try specific build/test/talos selection, there is currently a flat file called valid_builders.py that give the lists of DESKTOP_PLATFORMS, MOBILE_PLATFORMS, TALOS_SUITES, TEST_SUITES, and PRETTY_NAMES in order to help try_parser.py validate and generate the custom builder set to be scheduled.

To keep this from getting out of sync with try configs (and ultimately m-c configs) we need it to be auto-generated from config.py or to have try_parser.py import config.py and use that to validate/generate.
I think everything you need is in config.py.  PLATFORMS, SUITES, and UNITTEST_SUITES have all the values.
Yes, and then also we need the PRETTY_NAME mapping from base_name for each platform
Assignee: nobody → lsblakk
Blocks: 473184
No longer depends on: 473184
No longer blocks: 473184
Blocks: 600296
Attached patch [WIP] removing valid_builders.py (obsolete) — Splinter Review
So this is working in my local staging setup for both the builders as well as the talos/test sendchanges.

It will need more extensive testing in staging and a re-working of the try_parser unit test to makes sure that various use cases are tested before putting this in production but I wanted to show how I was doing it for feedback before going too far into that side of things.
Attachment #485939 - Flags: feedback?(catlee)
Comment on attachment 485939 [details] [diff] [review]
[WIP] removing valid_builders.py

there are a bunch of places where we're generating the same pretty name. e.g. "%s build" % base_name appears a bunch of places.  can you factor that out into a new variable?

There's also a TODO left in there, are you still working on that piece?
Attachment #485939 - Flags: feedback?(catlee) → feedback+
So I've run a lot of tests on this and right now there is just one little bug that I couldn't figure out - when I run it on the test master, sending in unittest values, a platform with more than one slave_platform (in my test case macosx64 which has two) I ended up with adding request for all the 10.5 unittests requested and then double the 10.6 builders.  So that's what is happening on lines 47/48 because it was the only way I could find to get around this and get just one set of unittest builders for each slave_platform.  I can't figure out where the second set is being generated.
Attachment #485939 - Attachment is obsolete: true
Attachment #490814 - Flags: review?(catlee)
Comment on attachment 490814 [details] [diff] [review]
[very close] removes valid_builders.py from both try_parser and test_try_parser

>diff --git a/misc.py b/misc.py
>--- a/misc.py
>+++ b/misc.py
>@@ -484,40 +484,48 @@ def generateBranchObjects(config, name):
>     }
>     builders = []
>     unittestBuilders = []
>     triggeredUnittestBuilders = []
>     nightlyBuilders = []
>     xulrunnerNightlyBuilders = []
>     debugBuilders = []
>     weeklyBuilders = []
>+    # prettyNames is a mapping to pass to the try_parser for validation
>+    PRETTY_NAME = '%s build'
>+    prettyNames = {}
>+    unittestPrettyNames = {}
>+    unittestSuites = []
>     # These dicts provides mapping between en-US dep and nightly scheduler names
>     # to l10n dep and l10n nightly scheduler names. It's filled out just below here.
>     l10nBuilders = {}
>     l10nNightlyBuilders = {}
>     pollInterval = config.get('pollInterval', 60)
>     l10nPollInterval = config.get('l10nPollInterval', 5*60)
>     # generate a list of builders, nightly builders (names must be different)
>     # for easy access
>     for platform in config['platforms'].keys():
>         pf = config['platforms'][platform]
>         base_name = pf['base_name']

Can you put "pretty_name = PRETTY_NAME % base_name" here?  It'll get rid of the same expansion 4 times below.

>         if platform.endswith("-debug"):
>-            debugBuilders.append('%s build' % base_name)
>+            debugBuilders.append(PRETTY_NAME % base_name)
>+            prettyNames[platform] = PRETTY_NAME % base_name
>             # Debug unittests
>             if pf.get('enable_unittests'):
>                 test_builders = []
>                 base_name = config['platforms'][platform.replace("-debug", "")]['base_name']
>                 for suites_name, suites in config['unittest_suites']:
>+                    unittestPrettyNames[platform] = '%s debug test' % base_name
>                     test_builders.extend(generateTestBuilderNames('%s debug test' % base_name, suites_name, suites))
>                 triggeredUnittestBuilders.append(('%s-%s-unittest' % (name, platform), test_builders, config.get('enable_merging', True)))
>             # Skip l10n, unit tests and nightlies for debug builds
>             continue
>         else:
>-            builders.append('%s build' % base_name)
>+            builders.append(PRETTY_NAME % base_name)
>+            prettyNames[platform] = PRETTY_NAME % base_name
> 

> def generateTalosBranchObjects(branch, branch_config, PLATFORMS, SUITES,
>         ACTIVE_UNITTEST_PLATFORMS, factory_class=TalosFactory):
>     branchObjects = {'schedulers': [], 'builders': [], 'status': [], 'change_source': []}
>     branch_builders = {}
>     all_test_builders = {}
>     all_builders = []
>+    # prettyNames is a mapping to pass to the try_parser for validation
>+    prettyNames = {}
> 
>     buildBranch = branch_config['build_branch']
>     talosCmd = branch_config['talos_command']
> 
>     for platform, platform_config in PLATFORMS.items():
>         if platform_config.get('is_mobile', False):
>             branchName = branch_config['mobile_branch_name']
>             tinderboxTree = branch_config['mobile_tinderbox_tree']
>@@ -1967,16 +1984,22 @@ def generateTalosBranchObjects(branch, b
> 
>         if tinderboxTree not in branch_builders:
>             branch_builders[tinderboxTree] = []
>         if tinderboxTree not in all_test_builders:
>             all_test_builders[tinderboxTree] = []
> 
>         for slave_platform in platform_config['slave_platforms']:
>             platform_name = platform_config[slave_platform]['name']
>+            # this is to handle how a platform has more than one slave platform
>+            if prettyNames.has_key(platform):
>+                prettyNames[platform] = [prettyNames[platform]]
>+                prettyNames[platform].append(platform_name)
>+            else:
>+                prettyNames[platform] = platform_name

So prettyNames[platform] can contain either a list of platform names or a single platform name?  Can you change it so that it always contains a list, even if there's only one platform?

>diff --git a/test/test_try_parser.py b/test/test_try_parser.py
>--- a/test/test_try_parser.py
>+++ b/test/test_try_parser.py
>@@ -1,167 +1,150 @@
>-from buildbotcustom.try_parser import TryParser
>+from ..try_parser import TryParser

Why change to relative imports here?

>-
>-def getTestBuilders(platforms, testType, tests, builderNames, buildTypes):
>+def getTestBuilders(platforms, testType, tests, builderNames, buildTypes, prettyNames, unittestPrettyNames):
>     testBuilders = []
>     # for all possible suites, add in the builderNames for that platform
>     if tests != 'none':
>         if testType == "test":
>             for buildType in buildTypes:
>                 for platform in platforms:
>-                    for test in tests:
>-                        # we only do opt unittests on Rev3 WINNT 6.1, the debug tests still run on pm02
>-                        if platform == 'win32':
>-                          if buildType == 'debug':
>-                            custom_builder = "%s tryserver %s %s %s" % (PRETTY_NAMES['desktop_win32'], buildType, testType, test)
>-                          else:
>-                            custom_builder = "%s tryserver %s %s %s" % (PRETTY_NAMES[platform][1], buildType, testType, test)
>+                    if platform in prettyNames.keys():
>+                        if type(prettyNames[platform]) is list:

As above, let's make sure that this always is a list, and we can simplify this block a bit.

>+                            for test in tests:
>+                                for slave_platform in prettyNames[platform]:
>+                                    custom_builder = "%s tryserver %s %s %s" % (slave_platform, buildType, testType, test)
>+                                    # note: can anyone else see why without the 'not in' the second slave platform is doubled?
>+                                    if custom_builder in (builderNames) and custom_builder not in testBuilders:
>+                                        testBuilders.extend([custom_builder])
>                         else:
>-                            custom_builder = "%s tryserver %s %s %s" % (PRETTY_NAMES[platform], buildType, testType, test)
Attachment #490814 - Flags: review?(catlee) → review-
* I originally tried "pretty_name = PRETTY_NAME % base_name" and got an error on base_name not existing yet. 

* prettyNames[platform] can contain either a list of platform names or a
single platform name because in the parser I want to be able to find out if there is more than one slave_platform for a single platform by checking if it is a list type. so I was doing that on purpose. if I make every platform a list then I suppose I can check for length in the parser instead - is that a better way to do this?

* i can remove the relative import, that was for local testing
(In reply to comment #7)
> * I originally tried "pretty_name = PRETTY_NAME % base_name" and got an error
> on base_name not existing yet. 

and I see now that is not what you were suggesting. carrying on.
addressed all comments from last review and tested on local builder master and tests master to make sure I'm still getting the builders I want.
Attachment #490814 - Attachment is obsolete: true
Attachment #491259 - Flags: review?(catlee)
Comment on attachment 491259 [details] [diff] [review]
[tested] removing valid_builders.py from try_parser

As per our 1x1, let's not lose the various test aliases we already have (e.g. 'mochitest-1' or 'mochitests')
Attachment #491259 - Flags: review?(catlee)
Completely re-wrote the unittests for this and actually have useful tests that cover 100% of try_parser

Also ran against local builder_master and tests_master to double-check.
Attachment #491259 - Attachment is obsolete: true
Attachment #491967 - Flags: review?(catlee)
Comment on attachment 491967 [details] [diff] [review]
[tested] removing valid_builders.py from try_parser new and improved

Looks good!
Attachment #491967 - Flags: review?(catlee) → review+
Flags: needs-reconfig+
Comment on attachment 491967 [details] [diff] [review]
[tested] removing valid_builders.py from try_parser new and improved

http://hg.mozilla.org/build/buildbotcustom/rev/ba4d2c17b0b6
Attachment #491967 - Flags: checked-in+
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Product: mozilla.org → Release Engineering
You need to log in before you can comment on or make changes to this bug.