[recipes.py] refactor common argument parsing, directly validate --package

recipes.py

 #!/usr/bin/env python
 # Copyright 2015 The LUCI Authors. All rights reserved.
 # Use of this source code is governed under the Apache License, Version 2.0
 # that can be found in the LICENSE file.
 
 """Tool to interact with recipe repositories.
 
 This tool operates on the nearest ancestor directory containing an
 infra/config/recipes.cfg.
 """
@@ skipping 326 matching lines @@
 def _op_properties_to_dict(pmap):
   """Creates a properties dictionary from an arguments_pb2.PropertyMap entry.
 
   Args:
     pmap (arguments_pb2.PropertyMap): Map to convert to dictionary form.
   Returns (dict): A dictionary derived from the properties in "pmap".
   """
   return dict((k, _op_property_value(pmap[k])) for k in pmap)
 
 
-def main():
-  parser = argparse.ArgumentParser(
-      description='Interact with the recipe system.')
+def add_common_args(parser):

[dnj, 2017/04/26 21:08:15]

Not appreciating why this is pulled out into a separate function. Is something
else ever going to call it? If not, I'd prefer it just be part of the rest of
the parser setup.

+  from recipe_engine import package_io
+
+  def package_type(value):
+    if not os.path.exists(value):
+      raise argparse.ArgumentTypeError(
+        'Given recipes config file %s does not exist.' % (value,))
+    return package_io.PackageFile(value)
 
   parser.add_argument(
       '--package',
-      type=os.path.abspath,
+      type=package_type,
       help='Path to recipes.cfg of the recipe package to operate on'
         ', usually in infra/config/recipes.cfg')
   parser.add_argument(
       '--deps-path',
       type=os.path.abspath,
       help='Path where recipe engine dependencies will be extracted. Specify '
            '"-" to use a temporary directory for deps, which will be cleaned '
            'up on exit.')
   parser.add_argument(
       '--verbose', '-v', action='count',
@@ skipping 14 matching lines @@
   parser.add_argument(
       '--disable-bootstrap', action='store_false', dest='use_bootstrap',
       help='Disables bootstrap (see --use-bootstrap)')
   parser.add_argument(
       '--operational-args-path', action='store',
       type=os.path.abspath,
       help='The path to an operational Arguments file. If provided, this file '
            'must contain a JSONPB-encoded Arguments protobuf message, and will '
            'be integrated into the runtime parameters.')
 
+
+def post_process_common_args(parser, args):
+  if args.command == "remote":

[dnj, 2017/04/26 21:08:15]

Same here - why not just put this content at the call site. No need to split
this up.

+    # TODO(iannucci): this is a hack; remote doesn't behave like ANY other
+    # commands. A way to solve this will be to allow --package to take a remote
+    # repo and then simply remove the remote subcommand entirely.
+    return
+
+  if not args.package:
+    parser.error('%s requires --package' % args.command)
+
+
+def main():
+  parser = argparse.ArgumentParser(
+      description='Interact with the recipe system.')
+
+  add_common_args(parser)
+
   subp = parser.add_subparsers()
 
   fetch_p = subp.add_parser(
       'fetch',
       description='Fetch and update dependencies.')
   fetch_p.set_defaults(command='fetch')
 
   test_p = subp.add_parser(
     'test',
     description='Generate or check expectations by simulation')
@@ skipping 166 matching lines @@
       'doc',
       description='List all known modules reachable from the current package, '
           'with their documentation')
   doc_p.add_argument('recipe', nargs='?',
                      help='Restrict documentation to this recipe')
   doc_p.add_argument('--kind', default='jsonpb', choices=doc_kinds,
                      help='Output this kind of documentation')
   doc_p.set_defaults(command='doc')
 
   args = parser.parse_args()
+  post_process_common_args(parser, args)
 
   # Load/parse operational arguments.
   op_args = arguments_pb2.Arguments()
   if args.operational_args_path is not None:
     with open(args.operational_args_path) as fd:
       data = fd.read()
     jsonpb.Parse(data, op_args)
 
   # TODO(iannucci): We should always do logging.basicConfig() (probably with
   # logging.WARNING), even if no verbose is passed. However we need to be
@@ skipping 83 matching lines @@
 
 
 def _real_main(args, op_args):
   from recipe_engine import package, package_io
 
   # Commands which do not require config_file, package_deps, and other objects
   # initialized later.
   if args.command == 'remote':
     return remote(args)
 
-  assert args.package, 'No recipe config (--package) given.'
-  assert os.path.exists(args.package), (
-      'Given recipes config file %s does not exist.' % args.package)
-  repo_root = package.InfraRepoConfig().from_recipes_cfg(args.package)
-  config_file = package_io.PackageFile(args.package)
+  config_file = args.package
+  repo_root = package.InfraRepoConfig().from_recipes_cfg(args.package.path)
 
   try:
     # TODO(phajdan.jr): gracefully handle inconsistent deps when rolling.
     # This fails if the starting point does not have consistent dependency
     # graph. When performing an automated roll, it'd make sense to attempt
     # to automatically find a consistent state, rather than bailing out.
     # Especially that only some subcommands refer to package_deps.
     package_deps = package.PackageDeps.create(
         repo_root, config_file, allow_fetch=not args.no_fetch,
         deps_path=args.deps_path, overrides=args.project_override)
@@ skipping 54 matching lines @@
 
   if not isinstance(ret, int):
     if ret is None:
       ret = 0
     else:
       print >> sys.stderr, ret
       ret = 1
   sys.stdout.flush()
   sys.stderr.flush()
   os._exit(ret)