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/.
 
 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'}
-
-parser = argparse.ArgumentParser(
+KNOWN_PLATFORMS = {'chrome', 'gecko', 'edge', 'generic'}
+
+MAIN_PARSER = argparse.ArgumentParser(
     description=__doc__,
     formatter_class=argparse.RawDescriptionHelpFormatter)
 
-subs = parser.add_subparsers(title='Commands', dest='action',
-                             metavar='[command]')
+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(name=None, platforms=None, arguments=()):
+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 = subs.add_parser(
-            name or func.__name__,
-            description=long_desc,
-            formatter_class=argparse.RawDescriptionHelpFormatter,
-            help=short_desc,
-        )
-
+        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 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 subcommands, intersected with their respective valid
+    platforms.
+
+    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 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:
+        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 command_params in ALL_COMMANDS:
+        platforms = types.intersection(command_params.pop('valid_platforms'))
         if len(platforms) > 1:
-            new_parser.add_argument('-t', '--type', dest='platform',
-                                    choices=platforms, required=True)

[Wladimir Palant, 2017/11/17 09:26:10]

This is suboptimal for repositories where only one metadata file exists, an
explicit type shouldn't be required then. I suggest that you check existing
metadata files first, same as the old code. You can then build an intersection
between platforms and existing metadata files. If this results in only one entry
the type parameter won't be necessary. Also, if this results in no entries the
command should be omitted altogether.

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

Done. (See collect_platforms)

+            command_params['arguments'] += (
+                make_argument('-t', '--type', dest='platform', required=True,
+                              choices=platforms),
+            )
+            make_subcommand(**command_params)
         elif len(platforms) == 1:
-            new_parser.set_defaults(platform=platforms.pop())
-
-        for argument in arguments:
-            argument(parser=new_parser)
-
-        new_parser.set_defaults(function=func)
-        return func_wrapper
-
-    return wrapper
-
-
-@argparse_command(
-    name='build', platforms={'gecko', 'chrome', 'edge'},
+            sub_parser = make_subcommand(**command_params)
+            sub_parser.set_defaults(platform=platforms.pop())
+
+    build_available_subcommands._result = True
+    return True
+
+
+build_available_subcommands._result = None
+
+
+@argparse_command(
+    valid_platforms={'chrome', 'gecko', 'edge'},
     arguments=(
         make_argument(
-            '-b', '--build',
+            '-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(
             '-r', '--release', action='store_true',
             help='Create a release build'),
         make_argument('output_file', nargs='?')
     )
 )
-def run_build(base_dir, build, key_file, release, output_file, platform,

[Wladimir Palant, 2017/11/17 09:26:10]

Nit: Maybe rename this function into build and drop explicit name for the
command? Similarly with the other commands.

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

Done.

-              **kwargs):
+def build(base_dir, build_num, key_file, release, output_file, platform,
+          **kwargs):
     """
     Create a build.
 
     Creates an extension build with given file name. If output_file is missing
     a default name will be chosen.
     """
     kwargs = {}
-    if platform in {'chrome', 'gecko'}:
+    if platform == 'edge':
+        import buildtools.packagerEdge as packager
+    else:
         import buildtools.packagerChrome as packager
-    elif platform == 'edge':
-        import buildtools.packagerEdge as packager

[Wladimir Palant, 2017/11/17 09:26:10]

Nit: This is code you merely moved around but consider making the imports here
consistent to create_devenv below.

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

Done.

 
     kwargs['keyFile'] = key_file
     kwargs['outFile'] = output_file
     kwargs['releaseBuild'] = release
-    kwargs['buildNum'] = build
+    kwargs['buildNum'] = build_num
 
     packager.createBuild(base_dir, type=platform, **kwargs)
 
 
 @argparse_command(
-    name='devenv', platforms={'chrome', 'gecko', 'edge'}
-)
-def create_devenv(base_dir, platform, **kwargs):
+    valid_platforms={'chrome', 'gecko', 'edge'}
+)
+def devenv(base_dir, platform, **kwargs):
     """
     Set up a development environment.
 
     Will set up or update the devenv folder as an unpacked extension folder '
     for development.
     """
     if platform == 'edge':
         import buildtools.packagerEdge as packager
     else:
         import buildtools.packagerChrome as packager
@@ skipping 30 matching lines @@
     locale_config['base_path'] = locale_dir
 
     locales = [(locale.replace('_', '-'), os.path.join(locale_dir, locale))
                for locale in os.listdir(locale_dir)]
     locale_config['locales'] = dict(locales)
 
     return locale_config
 
 
 project_key_argument = make_argument(
-    'project_key', help='The crowdin project key (required).', nargs='?'

[Wladimir Palant, 2017/11/17 09:26:10]

This is a required parameter, why not nargs=1? This should also make saying
"required" in the description unnecessary.

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

nargs=1 would produce a list of one value, leaving us with having to unpack that
list.
This would be fine - however, leaving out any "nargs" parameter has the same
effect of making it required and also only consuming one argument.
Done.

-)
-
-
-@argparse_command(
-    name='setuptrans', platforms=KNOWN_PLATFORMS,
+    'project_key', help='The crowdin project key.'
+)
+
+
+@argparse_command(
     arguments=(project_key_argument, )
 )
-def setup_translations(base_dir, project_key, platform, **kwargs):
+def setuptrans(base_dir, project_key, platform, **kwargs):
     """
     Set up translation languages.
 
-    Set up translation languages for the project on crowdin.net.

[Wladimir Palant, 2017/11/17 09:26:10]

Here and for the commands below: it's crowdin.com.

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

Done.

+    Set up translation languages for the project on crowdin.com.
     """
     from buildtools.packager import readMetadata
     metadata = readMetadata(base_dir, platform)
 
     basename = metadata.get('general', 'basename')
     locale_config = read_locale_config(base_dir, platform, metadata)
 
     import buildtools.localeTools as localeTools
     localeTools.setupTranslations(locale_config, basename, project_key)
 
 
 @argparse_command(
-    name='translate', platforms=KNOWN_PLATFORMS,
     arguments=(project_key_argument, )
 )
-def update_translation_master(base_dir, project_key, platform, **kwargs):
+def translate(base_dir, project_key, platform, **kwargs):
     """
     Update translation master files.
 
-    Update the translation master files in the project on crowdin.net.
+    Update the translation master files in the project on crowdin.com.
     """
     from buildtools.packager import readMetadata
     metadata = readMetadata(base_dir, platform)
 
     basename = metadata.get('general', 'basename')
     locale_config = read_locale_config(base_dir, platform, metadata)
 
     default_locale_dir = os.path.join(locale_config['base_path'],
                                       locale_config['default_locale'])
 
     import buildtools.localeTools as localeTools
     localeTools.updateTranslationMaster(locale_config, metadata,
                                         default_locale_dir, basename,
                                         project_key)
 
 
 @argparse_command(
-    name='uploadtrans', platforms=KNOWN_PLATFORMS,
     arguments=(project_key_argument, )
 )
-def upload_translations(base_dir, project_key, platform, **kwargs):
+def uploadtrans(base_dir, project_key, platform, **kwargs):
     """
     Upload existing translations.
 
-    Upload already existing translations to the project on crowdin.net.
+    Upload already existing translations to the project on crowdin.com.
     """
     from buildtools.packager import readMetadata
     metadata = readMetadata(base_dir, platform)
 
     basename = metadata.get('general', 'basename')
     locale_config = read_locale_config(base_dir, platform, metadata)
 
     import buildtools.localeTools as localeTools
     for locale, locale_dir in locale_config['locales'].iteritems():
         if locale != locale_config['default_locale'].replace('_', '-'):
             localeTools.uploadTranslations(locale_config, metadata, locale_dir,
                                            locale, basename, project_key)
 
 
 @argparse_command(
-    name='gettranslations', platforms=KNOWN_PLATFORMS,
     arguments=(project_key_argument, )
 )
-def get_translations(base_dir, project_key, platform, **kwargs):
+def gettranslations(base_dir, project_key, platform, **kwargs):
     """
     Download translation updates.
 
-    Download updated translations from crowdin.net.
+    Download updated translations from crowdin.com.
     """
     from buildtools.packager import readMetadata
     metadata = readMetadata(base_dir, platform)
 
     basename = metadata.get('general', 'basename')
     locale_config = read_locale_config(base_dir, platform, metadata)
 
     import buildtools.localeTools as localeTools
     localeTools.getTranslations(locale_config, basename, project_key)
 
 
 @argparse_command(
-    name='docs', platforms={'chrome'},
+    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 generate_docs(base_dir, target_dir, quiet, platform, **kwargs):
+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')
-    sources = [source_dir]
 
     # JSDoc struggles wih huge objects:
     # https://github.com/jsdoc3/jsdoc/issues/976
-    if platform == 'chrome':

[Wladimir Palant, 2017/11/17 09:26:10]

Nit: This check can be removed, we have no other platforms.

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

Done.

-        sources = [os.path.join(source_dir, filename)
-                   for filename in os.listdir(source_dir)
-                   if filename != 'publicSuffixList.js']
+    sources = [os.path.join(source_dir, filename)
+               for filename in os.listdir(source_dir)
+               if filename != 'publicSuffixList.js']
 
     buildtools_path = os.path.dirname(__file__)
     config = os.path.join(buildtools_path, 'jsdoc.conf')
 
     command = ['npm', 'run-script', 'jsdoc', '--', '--destination', target_dir,
                '--configure', config] + sources
     if quiet:
         process = subprocess.Popen(command, stdout=subprocess.PIPE,
                                    stderr=subprocess.PIPE, cwd=buildtools_path)
         stderr = process.communicate()[1]
         retcode = process.poll()
         if retcode:
             sys.stderr.write(stderr)
             raise subprocess.CalledProcessError(command, retcode)
     else:
         subprocess.check_call(command, cwd=buildtools_path)
 
 
 def valid_version_format(value):
     if re.search(r'[^\d\.]', value):
         raise argparse.ArgumentTypeError('Wrong version number format')
 
     return value
 
 
 @argparse_command(
-    name='release', platforms={'chrome', 'edge'},
+    valid_platforms={'chrome', 'edge'},
     arguments=(
         make_argument(
             '-k', '--key', dest='key_file',
             help=('File containing private key and certificates required to '
                   'sign the release.')),
         make_argument(
-            '-d', '--downloads-repository', dest='download_repository',

[Wladimir Palant, 2017/11/17 09:26:10]

Is the naming download_repository rather than downloads_repository intentional?
It's inconsistent with the flag and actual repository name.

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

No, this was not intentional. Done.

-            default='../downloads',

[Wladimir Palant, 2017/11/17 09:26:10]

Please use os.path.join() here. Also, the default path is relative to the
project repository and not cwd. So it's probably best if you leave out the
default here and determine it in the command handler if downloads_repository is
None.

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

Done.

+            '-d', '--downloads-repository', dest='downloads_repository',
             help=('Directory containing downloads repository (if omitted '
                   '../downloads is assumed)')),
         make_argument(
-            'version', nargs='?', help='Version number of the release',

[Wladimir Palant, 2017/11/17 09:26:10]

This parameter isn't optional.

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

Done.

+            'version', help='Version number of the release',
             type=valid_version_format)
     )
 )
-def run_release_automation(base_dir, download_repository, key_file, platform,
-                           version, **kwargs):
+def release(base_dir, downloads_repository, key_file, platform, version,
+            **kwargs):
     """
     Run release automation.
 
     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,
-                          download_repository)
-
-
-@argparse_command(name='updatepsl', platforms={'chrome'})
-def update_psl(base_dir, **kwargs):
+                          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):
-    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 = parser.parse_args(args if len(args) > 0 else None)
-
-    function = arguments.function
-    del arguments.function
-    function(**vars(arguments))
+    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))