From bbb61b63fa1a87154381b6addb73fd0ed02232d6 Mon Sep 17 00:00:00 2001 From: Nick Coghlan Date: Nov 23 2017 09:18:39 +0000 Subject: Issue #49: Migrate CLI processing to click The main maintainability benefit this gives is access to click's native subcommand support, which allows the options and arguments for subcommands to be defined directly alongside the implementations of those subcommands. It also provides access to click's native testing infrastructure --- diff --git a/src/_fedmod/cli.py b/src/_fedmod/cli.py index cb099b9..63a4631 100644 --- a/src/_fedmod/cli.py +++ b/src/_fedmod/cli.py @@ -1,184 +1,118 @@ import sys -import argparse +import click import logging from .module_generator import ModuleGenerator from .module_repoquery import ModuleRepoquery from . import _depchase, _repodata -# TODO: Switch this over to click (already a dependency for progress bars) - -class ModtoolsCLI(object): - """ Class for processing data from commandline """ - - @staticmethod - def build_parser(): - parser = argparse.ArgumentParser(description="Generates module related files") - base_parser = argparse.ArgumentParser(add_help=False) - base_parser.add_argument( - "--verbose", - "-v", - action="store_true", - help="verbose operation" - ) - - subparsers = parser.add_subparsers(dest="cmd_name") - - parser_rpm2module = subparsers.add_parser( - 'rpm2module', parents=[base_parser], - help="Generates modulemd file", - description="Gets package info and dependencies and creates modulemd file." - ) - parser_rpm2module.add_argument( - "--output", - "-o", - metavar="FILE", - dest="output_fname", - help="Write to FILE instead of stdout." - ) - parser_rpm2module.add_argument( - "--build-deps", - metavar="N", - default=0, - dest="build_deps_iterations", - help="Attempt to ensure N levels of build dependencies (Default: %(default)s)." - ) - parser_rpm2module.add_argument( - "pkgs", - metavar='PKGS', - nargs='+', - help="Specify list of packages for module.", - ) - - parser_metadata = subparsers.add_parser( - 'fetch-metadata', parents=[base_parser], - help="Fetches repository metadata", - description="Caches needed repository metadata locally" - ) - - parser_list_modules = subparsers.add_parser( - 'list-modules', parents=[base_parser], - help='Lists all modules.', - ) - - parser_list_rpms = subparsers.add_parser( - 'list-rpms', parents=[base_parser], - help='Lists all modularized rpm packages.', - ) - parser_list_rpms.add_argument( - "--duplicate-only", - action='store_true', - help="Only packages that are in more than one modules.", - ) - parser_list_rpms.add_argument( - "--list-modules", - action='store_true', - help="List modules for every package.", - ) - - parser_resolve_deps = subparsers.add_parser( - 'resolve-deps', parents=[base_parser], - help='List dependencies of given rpm packages.', - ) - parser_resolve_deps.add_argument( - "--module-dependency", - "-m", - action="append", - metavar="MODULE", - help="Module to be used as a dependency. Can be used multiple times.", - ) - parser_resolve_deps.add_argument( - "pkgs", - metavar='PKGS', - nargs='+', - help="Specify list of packages.", - ) - - parser_module_packages = subparsers.add_parser( - 'module-packages', parents=[base_parser], - help='Lists packages in a given module', - ) - parser_module_packages.add_argument( - "module", - metavar='MODULE', - help="Name of the module.", - ) - parser_module_packages.add_argument( - "--full-nevra", - action='store_true', - help="Print the full NEVRA instead of only name.", - ) - - parser_where_is_package = subparsers.add_parser( - 'where-is-package', parents=[base_parser], - help='Check if a package has been modularized and where', - ) - parser_where_is_package.add_argument( - "pkg", - metavar='PKG', - help="Name of the package.", - ) - - parser_srpm_of_rpm = subparsers.add_parser( - 'srpm-of-rpm', parents=[base_parser], - help='Check if a package has been modularized and where', - ) - parser_srpm_of_rpm.add_argument( - "pkg", - metavar='PKG', - help="Name of the package.", - ) - - return parser - - def __init__(self, args=None): - self.parser = ModtoolsCLI.build_parser() - self.args = self.parser.parse_args(args) - - def __getattr__(self, name): - try: - return getattr(self.args, name) - except AttributeError: - return object.__getattribute__(self, name) - +# fedmod uses click for argument parsing, but currently does its own +# standardised exception handling. This also requires handling click's standard +# exception types as described in http://click.pocoo.org/5/exceptions/ def run(): try: - cli = ModtoolsCLI(sys.argv[1:]) - if cli.args.verbose: - logging.basicConfig(level=logging.INFO) - if cli.args.cmd_name == 'rpm2module': - mg = ModuleGenerator(cli.args.pkgs) - mg.run(cli.args.output_fname, cli.args.build_deps_iterations) - elif cli.args.cmd_name == 'fetch-metadata': - _repodata.download_repo_metadata() - elif cli.args.cmd_name == 'list-modules': - rq = ModuleRepoquery() - rq.list_modules() - elif cli.args.cmd_name == 'list-rpms': - rq = ModuleRepoquery() - rq.list_modularized_pkgs( - duplicate_only=cli.args.duplicate_only, - list_modules=cli.args.list_modules, - ) - elif cli.args.cmd_name == 'resolve-deps': - rq = ModuleRepoquery() - rq.list_pkg_deps(cli.args.pkgs, cli.args.module_dependency) - elif cli.args.cmd_name == 'module-packages': - rq = ModuleRepoquery() - rq.list_rpms_in_module(cli.args.module, full_nevra=cli.args.full_nevra) - elif cli.args.cmd_name == 'where-is-package': - rq = ModuleRepoquery() - rq.list_modules_for_rpm(cli.args.pkg) - elif cli.args.cmd_name == 'srpm-of-rpm': - rq = ModuleRepoquery() - rq.get_srpm_for_rpm(cli.args.pkg) - - except KeyboardInterrupt: + rc = _cli_commands.main(sys.argv[1:], standalone_mode=False) + except (KeyboardInterrupt, click.Abort): print('\nInterrupted by user') + rc = 1 + except click.ClickException as e: + e.show() + rc = e.exit_code except _repodata.MissingMetadata as e: print(e, file=sys.stderr) - sys.exit(2) + rc = 2 except Exception as e: logging.exception("Unexpected exception") - sys.exit(1) + rc = 3 + sys.exit(rc) + + +@click.group() +@click.option("--verbose", "-v", is_flag=True, default=False, + help="Show additional info messages") +def _cli_commands(verbose): + """Utilities for working with modular Fedora RPM repositories""" + if verbose: + logging.basicConfig(level=logging.INFO) + + +# Fetching metadata +@_cli_commands.command('fetch-metadata') +def fetch_metadata(): + """Fetch and cache required module and RPM metadata""" + _repodata.download_repo_metadata() + + +# modulemd generation +@_cli_commands.command() +@click.option("--output", "-o", metavar="FILE", help="Write to FILE instead of stdout.") +@click.option("--build-deps", metavar="N", default=0, + help="Attempt to ensure N levels of build dependencies (Default: 0).") +@click.argument("pkgs", metavar='PKGS', nargs=-1, required=True) +def rpm2module(pkgs, output, build_deps): + """Generate a draft modulemd from an RPM (or list of RPMs)""" + mg = ModuleGenerator(pkgs) + mg.run(output, build_deps) + +# Checking availability of dependencies through module streams +@_cli_commands.command('resolve-deps') +@click.option("--module-dependency", "-m", multiple=True, metavar="MODULE", + help="Module to be used as a dependency. Can be given multiple times.") +@click.argument("pkgs", metavar='PKGS', nargs=-1, required=True) +def resolve_deps(pkgs, module_dependency): + """Report dependencies of the given RPM (or list of RPMs)""" + rq = ModuleRepoquery() + rq.list_pkg_deps(pkgs, module_dependency) + + +# Listing modules +@_cli_commands.command('list-modules') +def list_modules(): + """List the available modules""" + rq = ModuleRepoquery() + rq.list_modules() + + +# Looking for RPM duplication across modules +@_cli_commands.command('list-rpms') +@click.option("--duplicate-only", is_flag=True, default=False, + help="Only list packages that are in more than one module.") +@click.option("--list-modules", is_flag=True, help="List modules for every package.") +def list_rpms(duplicate_only, list_modules): + """List RPMs available through modules""" + rq = ModuleRepoquery() + rq.list_modularized_pkgs( + duplicate_only=duplicate_only, + list_modules=list_modules, + ) + +# Checking module contents +# (ncoghlan): Perhaps `module-rpms` would be more consistent? +@_cli_commands.command('module-packages') +@click.option("--full-nevra", is_flag=True, default=False, + help="Print the full NEVRA instead of only name.") +@click.argument("module") +def module_rpms(module, full_nevra): + """Lists RPMS in a given module""" + rq = ModuleRepoquery() + rq.list_rpms_in_module(module, full_nevra=full_nevra) + + +# Checking availability of specific packages +# (ncoghlan): Perhaps `where-is-rpm` would be more consistent? +@_cli_commands.command('where-is-package') +@click.argument("pkg") +def which_module(pkg): + """Reports which module (if any) provides the given RPM""" + rq = ModuleRepoquery() + rq.list_modules_for_rpm(pkg) + + +# Checking what to build to satisfy particular binary dependencies +@_cli_commands.command('srpm-of-rpm') +@click.argument("pkg") +def which_srpm(pkg): + """Reports which SRPM produces the given RPM""" + rq = ModuleRepoquery() + rq.get_srpm_for_rpm(pkg) diff --git a/tests/test_cli_ux.py b/tests/test_cli_ux.py index 707a6f4..edca483 100644 --- a/tests/test_cli_ux.py +++ b/tests/test_cli_ux.py @@ -1,4 +1,4 @@ -"""End-to-end subprocess based testing for the CLI behaviour""" +"""Tests for the general behaviour of the CLI""" import pytest import os @@ -31,3 +31,27 @@ class TestMissingMetadata(object): err_msg = re.sub(b"'.*'", b"''", result.stderr) assert err_msg == expected_err_msg +EXPECTED_SUBCOMMANDS = ( + 'rpm2module', + 'fetch-metadata', + 'list-modules', + 'list-rpms', + 'resolve-deps', + 'module-packages', + 'where-is-package', + 'srpm-of-rpm', +) + +class TestHelpMessages(object): + # We don't test the details of the help messages, as we trust click + # to handle that correctly. + # + # Instead, these tests just ensure that all expected subcommands exist + # and that they all accept an explicit `--help` option + + def test_default_help(self): + assert _run_fedmod(["--help"]).returncode == 0 + + def test_subcommand_help(self): + for subcommand in EXPECTED_SUBCOMMANDS: + assert _run_fedmod([subcommand, "--help"]).returncode == 0 diff --git a/tests/test_module_generator.py b/tests/test_module_generator.py index 4cd65ae..39ad8c7 100644 --- a/tests/test_module_generator.py +++ b/tests/test_module_generator.py @@ -1,39 +1,30 @@ """In-process tests for the modulemd generator""" import pytest -import os.path -from _fedmod.cli import ModtoolsCLI +from click.testing import CliRunner + +import modulemd +from _fedmod.cli import _cli_commands from _fedmod.module_generator import ModuleGenerator def _generate_modulemd(rpms, build_deps_iterations=0): - # TODO: Actually test the CLI arg parsing via a subprocess - cmd_input = list(['rpm2module']) - cmd_input.extend(rpms) - cli = ModtoolsCLI(cmd_input) - mg = ModuleGenerator(cli.pkgs) - if len(rpms) == 1: - output_fname = rpms[0] + '.yaml' - else: - output_fname = 'modulemd-output.yaml' - mg.run(output_fname, build_deps_iterations) - return mg, output_fname + cmd = ['rpm2module'] + if build_deps_iterations: + cmd.extend(['--build-deps', str(build_deps_iterations)]) + cmd.extend(rpms) + runner = CliRunner() + result = runner.invoke(_cli_commands, cmd) + assert result.exit_code == 0 + modmd = modulemd.ModuleMetadata() + modmd.loads(result.output) + return modmd class TestSinglePackageInput(object): - def setup(self): - self.input_rpms = input_rpms = ('grep',) - self.md, self.output_fname = _generate_modulemd(input_rpms) - - def teardown(self): - os.remove(self.output_fname) - def test_generated_modulemd_file(self): - # File exists with expected name - assert os.path.isfile(self.input_rpms[0] + '.yaml') - - # Check details of generated module metadata - modmd = self.md.mmd + input_rpms = ('grep',) + modmd = _generate_modulemd(input_rpms) # Expected description for 'grep' assert modmd.summary == "Pattern matching utilities" @@ -49,32 +40,21 @@ class TestSinglePackageInput(object): assert modmd.content_licenses == {'GPLv3+'} # Only given modules are listed in the public API - assert sorted(modmd.api.rpms) == sorted(self.input_rpms) + assert sorted(modmd.api.rpms) == sorted(input_rpms) # Expected components for grep - assert set(modmd.components.rpms) == set(self.input_rpms) + assert set(modmd.components.rpms) == set(input_rpms) # Expected module dependencies for grep assert set(modmd.buildrequires) == set() assert set(modmd.requires) == {'platform',} - class TestMultiplePackageInput(object): - def setup(self): - self.input_rpms = input_rpms = ('grep', 'haproxy') - self.md, self.output_fname = _generate_modulemd(input_rpms) - - def teardown(self): - os.remove(self.output_fname) - def test_generated_modulemd_file(self): - # File exists with expected name - assert os.path.isfile('modulemd-output.yaml') - - # Check details of generated module metadata - modmd = self.md.mmd + input_rpms = ('grep', 'haproxy') + modmd = _generate_modulemd(input_rpms) # Can't generate descriptive metadata when given multiple RPMs assert modmd.summary == "" @@ -85,10 +65,10 @@ class TestMultiplePackageInput(object): assert len(modmd.content_licenses) == 0 # This doesn't seem right... # Only given modules are listed in the public API - assert sorted(modmd.api.rpms) == sorted(self.input_rpms) + assert sorted(modmd.api.rpms) == sorted(input_rpms) # Expected components for grep + haproxy - expected_components = set(self.input_rpms) + expected_components = set(input_rpms) assert set(modmd.components.rpms) == expected_components # Expected module dependencies for grep + haproxy @@ -98,19 +78,9 @@ class TestMultiplePackageInput(object): class TestRecursiveBuildDeps(object): - def setup(self): - self.input_rpms = input_rpms = ('mariadb',) - self.md, self.output_fname = _generate_modulemd(input_rpms, 100) - - def teardown(self): - os.remove(self.output_fname) - def test_generated_modulemd_file(self): - # File exists with expected name - assert os.path.isfile('mariadb.yaml') - - # Check details of generated module metadata - modmd = self.md.mmd + input_rpms = ('mariadb',) + modmd = _generate_modulemd(input_rpms, 100) # Descriptive metadata assert modmd.summary == "A community developed branch of MySQL" @@ -127,14 +97,14 @@ class TestRecursiveBuildDeps(object): assert modmd.content_licenses == {'GPLv2 with exceptions and LGPLv2 and BSD'} # Only given modules are listed in the public API - assert sorted(modmd.api.rpms) == sorted(self.input_rpms) + assert sorted(modmd.api.rpms) == sorted(input_rpms) # MariaDB's complicated test suite poses some real challenges for # build dependency resolution, even when the generator is kind of # cheating and relying on the actual Fedora MariaDB module at runtime # Expected components - expected_components = set(self.input_rpms) + expected_components = set(input_rpms) expected_components |= { 'systemtap', 'multilib-rpm-config', @@ -172,19 +142,9 @@ class TestRecursiveBuildDeps(object): class TestNonRecursiveBuildDeps(object): - def setup(self): - self.input_rpms = input_rpms = ('graphite-web',) - self.md, self.output_fname = _generate_modulemd(input_rpms) - - def teardown(self): - os.remove(self.output_fname) - def test_generated_modulemd_file(self): - # File exists with expected name - assert os.path.isfile('graphite-web.yaml') - - # Check details of generated module metadata - modmd = self.md.mmd + input_rpms = ('graphite-web',) + modmd = _generate_modulemd(input_rpms) # Descriptive metadata assert modmd.summary == "A Django web application for enterprise scalable realtime graphing" @@ -205,10 +165,10 @@ class TestNonRecursiveBuildDeps(object): assert modmd.content_licenses == {'ASL 2.0'} # Only given modules are listed in the public API - assert sorted(modmd.api.rpms) == sorted(self.input_rpms) + assert sorted(modmd.api.rpms) == sorted(input_rpms) # Expected components - expected_components = set(self.input_rpms) + expected_components = set(input_rpms) expected_components |= { 'python-twisted', 'python-simplejson', diff --git a/tests/test_module_repoquery.py b/tests/test_module_repoquery.py index 6ac7dc6..9ee25d5 100644 --- a/tests/test_module_repoquery.py +++ b/tests/test_module_repoquery.py @@ -2,7 +2,6 @@ import pytest import os.path -from _fedmod.cli import ModtoolsCLI from _fedmod.module_repoquery import ModuleRepoquery class TestListingModules(object):