Issue 6021 - Refactoring build.py

build.py

 # This Source Code Form is subject to the terms of the Mozilla Public
 # License, v. 2.0. If a copy of the MPL was not distributed with this
 # file, You can obtain one at http://mozilla.org/MPL/2.0/.
 
-from __future__ import print_function
-
 import argparse
+import logging
 import os
+import re
+import shutil
+import subprocess
 import sys
-import re
-import subprocess
-import shutil
 from functools import partial
 from StringIO import StringIO
 from zipfile import ZipFile
 
-KNOWN_PLATFORMS = {'gecko', 'chrome', 'generic', 'edge'}
+KNOWN_PLATFORMS = {'chrome', 'gecko', 'edge', 'generic'}
 
 MAIN_PARSER = argparse.ArgumentParser(
     description=__doc__,
     formatter_class=argparse.RawDescriptionHelpFormatter)
 
 SUB_PARSERS = MAIN_PARSER.add_subparsers(title='Commands', dest='action',
                                          metavar='[command]')
+
+ALL_COMMANDS = []
 
 
 def make_argument(*args, **kwargs):
     def _make_argument(*args, **kwargs):
         parser = kwargs.pop('parser')
         parser.add_argument(*args, **kwargs)
 
     return partial(_make_argument, *args, **kwargs)
 
 
 def argparse_command(valid_platforms=None, arguments=()):
     def wrapper(func):
         def func_wrapper(*args, **kwargs):
             return func(*args, **kwargs)
 
         short_desc, long_desc = func.__doc__.split(os.linesep + os.linesep, 1)
 
-        new_parser = SUB_PARSERS.add_parser(
-            func.__name__,
-            description=long_desc,
-            formatter_class=argparse.RawDescriptionHelpFormatter,
-            help=short_desc,
-        )
-
-        new_parser.__valid_platforms = valid_platforms or KNOWN_PLATFORMS
-
-        for argument in arguments:
-            argument(parser=new_parser)
-
-        new_parser.set_defaults(function=func)
+        ALL_COMMANDS.append({
+            'name': func.__name__,
+            'description': long_desc,
+            'help_text': short_desc,
+            'valid_platforms': valid_platforms or KNOWN_PLATFORMS,
+            'function': func,
+            'arguments': arguments,
+        })
         return func_wrapper
 
     return wrapper
 
 
-def collect_platforms(base_dir):
-    """Construct valid platforms for each subcommand.
+def make_subcommand(name, description, help_text, function, arguments):
+    new_parser = SUB_PARSERS.add_parser(
+        name, description=description, help=help_text,
+        formatter_class=argparse.RawDescriptionHelpFormatter,
+    )
+
+    for argument in arguments:
+        argument(parser=new_parser)
+
+    new_parser.set_defaults(function=function)
+    return new_parser
+
+
+def build_available_subcommands(base_dir):
+    """Build subcommands, which are available for the repository in base_dir.
 
     Search 'base_dir' for existing metadata.<type> files and make <type> an
-    avaible choice for the subcommand, intersected with their respective valid
+    avaible choice for the subcommands, intersected with their respective valid
     platforms.
-    """
-    if collect_platforms._result is not None:
+
+    If no valid platform is found for a subcommand, it get's omitted.
+    """
+    if build_available_subcommands._result is not None:
         # Tests might run this code multiple times, make sure the collection
         # of platforms is only run once.
-        return collect_platforms._result
+        return build_available_subcommands._result
 
     types = set()
     for p in KNOWN_PLATFORMS:
         if os.path.exists(os.path.join(base_dir, 'metadata.' + p)):
             types.add(p)
 
     if len(types) == 0:
-        print('No metadata file found in this repository, a metadata file '
-              'metadata.* is required.', file=sys.stderr)

[Wladimir Palant, 2017/11/17 11:51:58]

This message is confusing now, with only known platforms being considered. Maybe
actually spell out valid files? "Expecting one of metadata.chrome,
metadata.gecko, ..."

Also, a while ago we realized that print (at least in Python 2.x) has trouble
printing Unicode information which is why we decided to use logging module for
stderr output.

[tlucas, 2017/11/17 13:32:04]

Done.

-        collect_platforms._result = False
+        logging.error('No metadata file found in this repository. Expecting '
+                      'one or more of {} to be present.'.format(
+                          ', '.join('metadata.' + p for p in KNOWN_PLATFORMS)))
+        build_available_subcommands._result = False
         return False
 
-    for _, sub_parser in SUB_PARSERS.choices.items():

[Wladimir Palant, 2017/11/17 11:51:58]

If you don't need the keys, why iterate over items? .values() should do.

[tlucas, 2017/11/17 13:32:04]

This will be obsolete with addressing the below comment.

-        platforms = types.intersection(sub_parser.__valid_platforms)
+    for command_params in ALL_COMMANDS:
+        platforms = types.intersection(command_params.pop('valid_platforms'))
         if len(platforms) > 1:
-            sub_parser.add_argument('-t', '--type', dest='platform',
-                                    choices=platforms)
-        else:
+            command_params['arguments'] += (
+                make_argument('-t', '--type', dest='platform', required=True,
+                              choices=platforms),
+            )
+            make_subcommand(**command_params)
+        elif len(platforms) == 1:
+            sub_parser = make_subcommand(**command_params)
             sub_parser.set_defaults(platform=platforms.pop())

[Wladimir Palant, 2017/11/17 11:51:58]

What about len(platforms) == 0?

[tlucas, 2017/11/17 13:32:04]

This was not trivial - but i came up with a solution, which only actually
constructs subcommands if the would be available (see
build_available_subcommands / make_subcommand). Done.

 
-    collect_platforms._result = True
+    build_available_subcommands._result = True
     return True
 
 
-collect_platforms._result = None

[tlucas, 2017/11/17 11:36:45]

I did not see the necessity to introduce a fully fletched singleton-pattern
here. However, if desired i can change this.

-
-
-@argparse_command(
-    valid_platforms={'gecko', 'chrome', 'edge'},
+build_available_subcommands._result = None
+
+
+@argparse_command(
+    valid_platforms={'chrome', 'gecko', 'edge'},
     arguments=(
         make_argument(
             '-b', '--build-num', dest='build_num',
             help=('Use given build number (if omitted the build number will '
                   'be retrieved from Mercurial)')),
         make_argument(
             '-k', '--key', dest='key_file',
             help=('File containing private key and certificates required to '
                   'sign the package')),
         make_argument(
@@ skipping 163 matching lines @@
     locale_config = read_locale_config(base_dir, platform, metadata)
 
     import buildtools.localeTools as localeTools
     localeTools.getTranslations(locale_config, basename, project_key)
 
 
 @argparse_command(
     valid_platforms={'chrome'},
     arguments=(
         make_argument('target_dir'),
-        make_argument('-q', '--quiet', help='Suppress JsDoc output'),
+        make_argument('-q', '--quiet', help='Suppress JsDoc output',
+                      action='store_true', default=False),
     )
 )
 def docs(base_dir, target_dir, quiet, platform, **kwargs):
     """
     Generate documentation (requires node.js).
 
     Generate documentation files and write them into the specified directory.
     """
     source_dir = os.path.join(base_dir, 'lib')
 
@@ skipping 51 matching lines @@
     Note: If you are not the project owner then you probably don't want to run
     this!
 
     Run release automation: create downloads for the new version, tag source
     code repository as well as downloads and buildtools repository.
     """
     if downloads_repository is None:
         downloads_repository = os.path.join(base_dir, os.pardir, 'downloads')
 
     if platform == 'chrome' and key_file is None:
-        raise ValueError('You must specify a key file for this release')
+        logging.error('You must specify a key file for this release')
+        return
 
     import buildtools.releaseAutomation as releaseAutomation
     releaseAutomation.run(base_dir, platform, version, key_file,
                           downloads_repository)
 
 
 @argparse_command(valid_platforms={'chrome'})
 def updatepsl(base_dir, **kwargs):
     """Update Public Suffix List.
 
     Downloads Public Suffix List (see http://publicsuffix.org/) and generates
     lib/publicSuffixList.js from it.
     """
     import buildtools.publicSuffixListUpdater as publicSuffixListUpdater
     publicSuffixListUpdater.updatePSL(base_dir)
 
 
 def process_args(base_dir, *args):
-    if collect_platforms(base_dir):
+    if build_available_subcommands(base_dir):
         MAIN_PARSER.set_defaults(base_dir=base_dir)
 
         # If no args are provided, this module is run directly from the command
         # line. argparse will take care of consuming sys.argv.
         arguments = MAIN_PARSER.parse_args(args if len(args) > 0 else None)
 
         function = arguments.function
         del arguments.function
         function(**vars(arguments))