[bisect] - Add --path_to_config parameter to run script.

tools/run-bisect-perf-regression.py

 #!/usr/bin/env python
 # Copyright (c) 2013 The Chromium Authors. All rights reserved.
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
 """Run Performance Test Bisect Tool
 
 This script is used by a trybot to run the src/tools/bisect-perf-regression.py
 script with the parameters specified in run-bisect-perf-regression.cfg. It will
 check out a copy of the depot in a subdirectory 'bisect' of the working
@@ skipping 97 matching lines @@
     execfile(path_to_file, local_vars)
 
     return local_vars['config']
   except:
     print
     traceback.print_exc()
     print
     return {}
 
 
+def _ValidateConfigFile(config_contents, valid_parameters):
+  """Validates that the config file contents is a valid perf config, and all

[qyearsley, 2014/04/10 17:58:22]

Nit: This wording is a little bit redundant, since "to validate" means "to check
whether sth is valid".

Maybe something like: "Validates the config file contents, checking whether all
values are non-empty."

[shatch, 2014/04/10 18:21:11]

Done.

+  the values are non-empty.
+
+  Args:
+    config_contents: Contents of the config file passed from _LoadConfigFile.
+    valid_parameters: A list of parameters to check for.
+
+  Returns:
+    True if valid.
+  """
+  try:
+    [config_contents[current_parameter]
+        for current_parameter in valid_parameters]
+    config_has_values = [v for v in config_contents.values() if v]

[qyearsley, 2014/04/10 17:58:22]

What if the value used for truncate_percent was 0?

It looks like we're expecting that all of the values in the config dict must be
non-empty strings.

If you think that later someone might want to directly put integers in the
config file, maybe here you could check  
[v != '' and v is not None for v in config_values.values() if v]

OR, if we want to assert that the values in the config must be strings, perhaps
you could use:

[v and type(v) is str for v in config_values.values() if v]

[shatch, 2014/04/10 18:21:11]

Yeah, they've traditionally just been strings. Added the extra string check.

+    return config_has_values
+  except KeyError:
+    return False
+
+
+def _ValidatePerfConfigFile(config_contents):
+  """Validates that the config file contents is a valid perf config.

[qyearsley, 2014/04/10 17:58:22]

I wasn't aware before that there was such a thing as a "perf config" (usually at
run-perf-test.cfg, right?)

Maybe add a note saying that a "perf config file" is a config used when we only
want to run one perf test once, and we're not actually bisecting; it's usually
called run-perf-test.cfg by default.

[shatch, 2014/04/10 18:21:11]

Done.

+
+  Args:
+    config_contents: Contents of the config file passed from _LoadConfigFile.
+
+  Returns:
+    True if valid.
+  """
+  valid_parameters = ['command', 'metric', 'repeat_count', 'truncate_percent',
+      'max_time_minutes']
+  return _ValidateConfigFile(config_contents, valid_parameters)
+
+
+def _ValidateBisectConfigFile(config_contents):
+  """Validates that the config file contents is a valid bisect config.
+
+  Args:
+    config_contents: Contents of the config file passed from _LoadConfigFile.
+
+  Returns:
+    True if valid.
+  """
+  valid_params = ['command', 'good_revision', 'bad_revision', 'metric',
+      'repeat_count', 'truncate_percent', 'max_time_minutes']
+  return _ValidateConfigFile(config_contents, valid_params)
+
+
 def _OutputFailedResults(text_to_print):
   bisect_utils.OutputAnnotationStepStart('Results - Failed')
   print
   print text_to_print
   print
   bisect_utils.OutputAnnotationStepClosed()
 
 
 def _CreateBisectOptionsFromConfig(config):
   opts_dict = {}
@@ skipping 234 matching lines @@
   parser = optparse.OptionParser(usage=usage)
   parser.add_option('-w', '--working_directory',
                     type='str',
                     help='A working directory to supply to the bisection '
                     'script, which will use it as the location to checkout '
                     'a copy of the chromium depot.')
   parser.add_option('-p', '--path_to_goma',
                     type='str',
                     help='Path to goma directory. If this is supplied, goma '
                     'builds will be enabled.')
+  parser.add_option('--path_to_config',
+                    type='str',
+                    help='Path to the config file to use. If this is supplied, '
+                    'the bisect script will use this to override the default '
+                    'config file path.')
   parser.add_option('--extra_src',
                     type='str',
                     help='Path to extra source file. If this is supplied, '
                     'bisect script will use this to override default behavior.')
   parser.add_option('--dry_run',
                     action="store_true",
                     help='The script will perform the full bisect, but '
                     'without syncing, building, or running the performance '
                     'tests.')
   (opts, args) = parser.parse_args()
 
   path_to_current_directory = os.path.abspath(os.path.dirname(sys.argv[0]))
-  path_to_bisect_cfg = os.path.join(path_to_current_directory,
-      'run-bisect-perf-regression.cfg')
+
+  # If they've specified their own config file, use that instead.
+  if opts.path_to_config:
+    path_to_bisect_cfg = opts.path_to_config

[qyearsley, 2014/04/10 17:58:22]

At this point, we're not 100% sure whether they've supplied a "bisect config" or
"perf config".

[shatch, 2014/04/10 18:21:11]

True, added some changes so that if loading it as a bisect config fails, it
falls back to treating it as a perf config.

+  else:
+    path_to_bisect_cfg = os.path.join(path_to_current_directory,
+        'run-bisect-perf-regression.cfg')
 
   config = _LoadConfigFile(path_to_bisect_cfg)
 
-  # Check if the config is empty
-  config_has_values = [v for v in config.values() if v]
+  # Check if the config is valid.
+  config_is_valid = _ValidateBisectConfigFile(config)
 
-  if config and config_has_values:
+  if config and config_is_valid:
     if not opts.working_directory:
       print 'Error: missing required parameter: --working_directory'
       print
       parser.print_help()
       return 1
 
     return _RunBisectionScript(config, opts.working_directory,
         path_to_current_directory, opts.path_to_goma, opts.extra_src,
         opts.dry_run)
   else:
     perf_cfg_files = ['run-perf-test.cfg', os.path.join('..', 'third_party',
         'WebKit', 'Tools', 'run-perf-test.cfg')]
 
     for current_perf_cfg_file in perf_cfg_files:
       path_to_perf_cfg = os.path.join(
           os.path.abspath(os.path.dirname(sys.argv[0])), current_perf_cfg_file)
 
       config = _LoadConfigFile(path_to_perf_cfg)
-      config_has_values = [v for v in config.values() if v]
+      config_is_valid = _ValidatePerfConfigFile(config)
 
-      if config and config_has_values:
+      if config and config_is_valid:
         return _SetupAndRunPerformanceTest(config, path_to_current_directory,
             opts.path_to_goma)
 
     print 'Error: Could not load config file. Double check your changes to '\
           'run-bisect-perf-regression.cfg/run-perf-test.cfg for syntax errors.'
     print
     return 1
 
 
 if __name__ == '__main__':
   sys.exit(main())