PR#497: Passing additional arguments to underlaying commands - rpkg

rpkg

#497 Passing additional arguments to underlaying commands

Merged 3 years ago by onosek. Opened 3 years ago by onosek.

onosek/rpkg extra_args into master

Unittests for passing additional arguments

Ondrej Nosek • 3 years ago

bc70dc2

Passing additional arguments to underlaying commands

Ondrej Nosek • 3 years ago

fe638de

pyrpkg/__init__.py

file modified

+80 -33

		`@@ -1465,7 +1465,7 @@`
		`return`

		`def clone(self, repo, path=None, branch=None, bare_dir=None,`
		`- anon=False, target=None, depth=None):`
		`+ anon=False, target=None, depth=None, extra_args=None):`
		`"""Clone a repo, optionally check out a specific branch.`

		`:param str repo: the name of the repository to clone.`
		`@@ -1480,6 +1480,8 @@`
		`repo.`
		`:param int depth: create a shallow clone with a history truncated`
		`to the specified number of commits.`
		`+ :param list extra_args: additional arguments that are passed to`
		`+ the clone command.`
		`"""`

		`if not path:`
		`@@ -1517,7 +1519,10 @@`
		`if not bare_dir:`
		`# --bare and --origin are incompatible`
		`cmd.extend(['--origin', self.default_branch_remote])`
		`-`
		`+ if extra_args:`
		`+ cmd.extend(extra_args)`
		`+ self.log.debug("Extra args '{0}' are passed to git clone "`
		`+ "command".format(extra_args))`
		`if target:`
		`self.log.debug('Cloning into: %s', target)`
		`cmd.append(target)`
		`@@ -1546,7 +1551,8 @@`
		`return repo.split("/")[-1]`
		`return repo`

		`- def clone_with_dirs(self, repo, anon=False, target=None, depth=None):`
		`+ def clone_with_dirs(self, repo, anon=False, target=None, depth=None,`
		`+ extra_args=None):`
		`"""Clone a repo old style with subdirs for each branch.`

		`:param str repo: name of the repository to clone.`
		`@@ -1556,6 +1562,8 @@`
		`repo.`
		`:param int depth: create a shallow clone with a history truncated`
		`to the specified number of commits.`
		`+ :param list extra_args: additional arguments that are passed to`
		`+ the clone command.`
		`"""`

		`self._push_url = None`
		`@@ -1581,7 +1589,8 @@`

		`# Create a bare clone first. This gives us a good list of branches`
		`try:`
		`- self.clone(repo, top_path, bare_dir=repo_path, anon=anon, depth=depth)`
		`+ self.clone(repo, top_path, bare_dir=repo_path, anon=anon, depth=depth,`
		`+ extra_args=extra_args)`
		`except Exception:`
		`# Clean out our directory`
		`shutil.rmtree(top_path)`
		`@@ -2422,7 +2431,8 @@`
		`with open(os.path.join(self.path, 'clog'), 'w') as clog:`
		`clog.writelines(clog_lines)`

		`- def compile(self, arch=None, short=False, builddir=None, nocheck=False, define=None):`
		`+ def compile(self, arch=None, short=False, builddir=None, nocheck=False,`
		`+ define=None, extra_args=None):`
		`"""Run rpmbuild -bc`

		`optionally for a specific arch, or short-circuit it,`
		`@@ -2442,6 +2452,10 @@`
		`if define:`
		`for entry in define:`
		`cmd.extend(['--define', entry])`
		`+ if extra_args:`
		`+ cmd.extend(extra_args)`
		`+ self.log.debug("Extra args '{0}' are passed to rpmbuild "`
		`+ "command".format(extra_args))`
		`if short:`
		`cmd.append('--short-circuit')`
		`if nocheck:`
		`@@ -2481,8 +2495,8 @@`
		`# This should have a try and catch koji errors`
		`self.kojisession.uploadWrapper(file, path, name=name, callback=callback)`

		`- def install(self, arch=None, short=False, builddir=None,`
		`- nocheck=False, buildrootdir=None, define=None):`
		`+ def install(self, arch=None, short=False, builddir=None, nocheck=False,`
		`+ buildrootdir=None, define=None, extra_args=None):`
		"""Run ``rpmbuild -bi``

		`optionally for a specific arch, short-circuit it,`
		`@@ -2496,6 +2510,8 @@`
		`:param str builddir: alternate builddir.`
		`:param bool nocheck: do not check.`
		`:param str buildrootdir: alternate buildrootdir.`
		`+ :param list extra_args: additional arguments that are passed to`
		`+ the rpmbuild command.`

		`.. versionadded: 1.56`
		`Parameter buildrootdir.`
		`@@ -2512,6 +2528,10 @@`
		`if define:`
		`for entry in define:`
		`cmd.extend(['--define', entry])`
		`+ if extra_args:`
		`+ cmd.extend(extra_args)`
		`+ self.log.debug("Extra args '{0}' are passed to rpmbuild "`
		`+ "command".format(extra_args))`
		`if short:`
		`cmd.append('--short-circuit')`
		`if nocheck:`
		`@@ -2573,7 +2593,7 @@`
		`return self.kojisession.listSideTags(basetag=base_tag, user=user)`

		`def local(self, localargs, arch=None, hashtype=None, builddir=None,`
		`- buildrootdir=None, define=None):`
		`+ buildrootdir=None, define=None, extra_args=None):`
		`"""rpmbuild locally for given arch.`

		`Takes localargs (passed to rpmbuild), arch to build for, and hashtype`
		`@@ -2589,6 +2609,8 @@`
		`digests.`
		`:param str builddir: an alternative builddir.`
		`:param str buildrootdir: an alternative buildrootdir.`
		`+ :param list extra_args: additional arguments that are passed to`
		`+ the rpmbuild command.`
		:raises rpkgError: if underlying `rpmbuild` fails.

		`.. versionadded: 1.56`
		`@@ -2603,26 +2625,30 @@`
		`if builddir:`
		`# Tack on a new builddir to the end of the defines`
		`cmd.append("--define '_builddir %s'" % os.path.abspath(builddir))`
		`- # Figure out the hash type to use`
		`- if not hashtype:`
		`- # Try to determine the dist`
		`- hashtype = self._guess_hashtype()`
		`- # This may need to get updated if we ever change our checksum default`
		`- if not hashtype == 'sha256':`
		`- cmd.extend(["--define '_source_filedigest_algorithm %s'"`
		`- % hashtype,`
		`- "--define '_binary_filedigest_algorithm %s'"`
		`- % hashtype])`
		`if arch:`
		`cmd.extend(['--target', arch])`
		`if define:`
		`for entry in define:`
		`cmd.extend(['--define', entry])`
		`+ if extra_args:`
		`+ cmd.extend(extra_args)`
		`+ self.log.debug("Extra args '{0}' are passed to rpmbuild "`
		`+ "command".format(extra_args))`
		`if self.quiet:`
		`cmd.append('--quiet')`
		`if buildrootdir:`
		`cmd.append("--define '_buildrootdir {0}'".format(`
		`os.path.abspath(buildrootdir)))`
		`+ # Figure out the hash type to use`
		`+ if not hashtype:`
		`+ # Try to determine the dist`
		`+ hashtype = self._guess_hashtype()`
		`+ # This may need to get updated if we ever change our checksum default`
		`+ if not hashtype == 'sha256':`
		`+ cmd.extend(["--define '_source_filedigest_algorithm %s'"`
		`+ % hashtype,`
		`+ "--define '_binary_filedigest_algorithm %s'"`
		`+ % hashtype])`
		`cmd.extend(['-ba', os.path.join(self.path, self.spec)])`
		`logfile = '.build-%s-%s.log' % (self.ver, self.rel)`

		`@@ -2884,7 +2910,8 @@`

		`self.repo.index.add(['sources', '.gitignore'])`

		`- def prep(self, arch=None, builddir=None, buildrootdir=None, define=None):`
		`+ def prep(self, arch=None, builddir=None, buildrootdir=None, define=None,`
		`+ extra_args=None):`
		"""Run ``rpmbuild -bp``

		`:param str arch: optional to run prep section for a specific arch. By`
		`@@ -2892,6 +2919,8 @@`
		`:param list define: an optional list of rpmbuild macros.`
		`:param str builddir: an alternative builddir.`
		`:param str buildrootdir: an alternative buildrootdir.`
		`+ :param list extra_args: additional arguments that are passed to`
		`+ the rpmbuild command.`

		`.. versionadded: 1.56`
		`Parameter buildrootdir.`
		`@@ -2908,6 +2937,10 @@`
		`if define:`
		`for entry in define:`
		`cmd.extend(['--define', entry])`
		`+ if extra_args:`
		`+ cmd.extend(extra_args)`
		`+ self.log.debug("Extra args '{0}' are passed to rpmbuild "`
		`+ "command".format(extra_args))`
		`if self.quiet:`
		`cmd.append('--quiet')`
		`if buildrootdir:`
		`@@ -2917,7 +2950,8 @@`
		`# Run the command`
		`self._run_command(cmd, shell=True)`

		`- def srpm(self, hashtype=None, define=None, builddir=None, buildrootdir=None, arch=None):`
		`+ def srpm(self, hashtype=None, define=None, builddir=None, buildrootdir=None,`
		`+ arch=None, extra_args=None):`
		`"""Create an srpm using hashtype from content`

		`Requires sources already downloaded. The generated SRPM file will be`
		`@@ -2930,6 +2964,8 @@`
		`:param str buildrootdir: optionally define an alternate buildrootdir.`
		`:param str arch: optional to run prep section for a specific arch. By`
		`default, local system arch will be used.`
		`+ :param list extra_args: additional arguments that are passed to`
		`+ the rpmbuild command.`
		`"""`

		`self.srpmname = os.path.join(self.path,`
		`@@ -2942,19 +2978,23 @@`

		`cmd = ['rpmbuild']`
		`cmd.extend(self.rpmdefines)`
		`- if self.quiet:`
		`- cmd.append('--quiet')`
		`- if define:`
		`- for entry in define:`
		`- cmd.extend(['--define', entry])`
		`if builddir:`
		`# Tack on a new builddir to the end of the defines`
		`cmd.append("--define '_builddir %s'" % os.path.abspath(builddir))`
		`+ if arch:`
		`+ cmd.extend(['--target', arch])`
		`+ if define:`
		`+ for entry in define:`
		`+ cmd.extend(['--define', entry])`
		`+ if extra_args:`
		`+ cmd.extend(extra_args)`
		`+ self.log.debug("Extra args '{0}' are passed to rpmbuild "`
		`+ "command".format(extra_args))`
		`+ if self.quiet:`
		`+ cmd.append('--quiet')`
		`if buildrootdir:`
		`cmd.append("--define '_buildrootdir {0}'".format(`
		`os.path.abspath(buildrootdir)))`
		`- if arch:`
		`- cmd.extend(['--target', arch])`

		`# Figure out which hashtype to use, if not provided one`
		`if not hashtype:`
		`@@ -3025,12 +3065,15 @@`
		`line_num += 1`
		`return [line_num, offset - offset_inc + 1]`

		`- def verify_files(self, builddir=None, buildrootdir=None, define=None):`
		`+ def verify_files(self, builddir=None, buildrootdir=None, define=None,`
		`+ extra_args=None):`
		"""Run `rpmbuild -bl` to verify the `%files` section

		`:param list define: an optional list of rpmbuild macros.`
		`:param str builddir: optionally define an alternate builddir.`
		`:param str buildrootdir: optionally define an alternate buildrootdir.`
		`+ :param list extra_args: additional arguments that are passed to`
		`+ the rpmbuild command.`

		`.. versionadded:: 1.56`
		`Parameter buildrootdir.`
		`@@ -3039,17 +3082,21 @@`
		`# setup the rpm command`
		`cmd = ['rpmbuild']`
		`cmd.extend(self.rpmdefines)`
		`- if define:`
		`- for entry in define:`
		`- cmd.extend(['--define', entry])`
		`if builddir:`
		`# Tack on a new builddir to the end of the defines`
		`cmd.append("--define '_builddir %s'" % os.path.abspath(builddir))`
		`+ if define:`
		`+ for entry in define:`
		`+ cmd.extend(['--define', entry])`
		`+ if extra_args:`
		`+ cmd.extend(extra_args)`
		`+ self.log.debug("Extra args '{0}' are passed to rpmbuild "`
		`+ "command".format(extra_args))`
		`+ if self.quiet:`
		`+ cmd.append('--quiet')`
		`if buildrootdir:`
		`cmd.append("--define '_buildrootdir {0}'".format(`
		`os.path.abspath(buildrootdir)))`
		`- if self.quiet:`
		`- cmd.append('--quiet')`
		`cmd.extend(['-bl', os.path.join(self.path, self.spec)])`
		`# Run the command`
		`self._run_command(cmd, shell=True)`

pyrpkg/cli.py

file modified

+68 -34

		`@@ -320,7 +320,7 @@`
		`if re.match(r"^clone_config_\w+$", key):`
		`clone_config = items.get(key)`
		`if not clone_config:`
		`- self.log.debug("No clone config is set for '{}'".format(key))`
		`+ self.log.debug("No clone config is set for '{0}'".format(key))`
		`setattr(self._cmd, key, clone_config)`

		`# This function loads the extra stuff once we figure out what site`
		`@@ -540,6 +540,10 @@`
		`self.rpm_parser_common.add_argument(`
		`'--define', help='Pass custom macros to rpmbuild, may specify multiple times',`
		`action='append')`
		`+ self.rpm_parser_common.add_argument(`
		`+ "extra_args", default=None, nargs=argparse.REMAINDER,`
		`+ help="Custom arguments that are passed to the 'rpmbuild'. "`
		`+ "Use '--' to separate them from other arguments.")`

		`def register_build(self):`
		`"""Register the build target"""`
		`@@ -634,6 +638,7 @@`
		`'the configured repository base URL. By default it '`
		`'will also checkout the master branch for your '`
		`'working copy.')`
		`+`
		`# Allow an old style clone with subdirs for branches`
		`clone_parser.add_argument(`
		`'--branches', '-B', action='store_true',`
		`@@ -652,6 +657,10 @@`
		`clone_parser.add_argument(`
		`"clone_target", default=None, nargs="?",`
		`help='Directory in which to clone the repository')`
		`+ clone_parser.add_argument(`
		`+ "extra_args", default=None, nargs=argparse.REMAINDER,`
		`+ help="Custom arguments that are passed to the 'git clone'. "`
		`+ "Use '--' to separate them from other arguments.")`

		`def validator_integer_string(raw_value):`
		`"""checks if input is string that contains integer number"""`
		`@@ -1111,6 +1120,10 @@`
		`help='Pass disablerepo option to yum/dnf (may be used more than once)')`
		`mockbuild_parser.add_argument(`
		`'--enable-network', action='store_true', help='Enable networking')`
		`+ mockbuild_parser.add_argument(`
		`+ "extra_args", default=None, nargs=argparse.REMAINDER,`
		`+ help="Custom arguments that are passed to the 'mock'. "`
		`+ "Use '--' to separate them from other arguments.")`
		`mockbuild_parser.set_defaults(command=self.mockbuild)`

		`def register_mock_config(self):`
		`@@ -1937,13 +1950,15 @@`
		`self.cmd.clone_with_dirs(self.args.repo[0],`
		`anon=self.args.anonymous,`
		`target=self.args.clone_target,`
		`- depth=self.args.depth)`
		`+ depth=self.args.depth,`
		`+ extra_args=self.extra_args)`
		`else:`
		`self.cmd.clone(self.args.repo[0],`
		`branch=self.args.branch,`
		`anon=self.args.anonymous,`
		`target=self.args.clone_target,`
		`- depth=self.args.depth)`
		`+ depth=self.args.depth,`
		`+ extra_args=self.extra_args)`

		`def commit(self):`
		`if self.args.with_changelog and not self.args.message:`
		`@@ -1993,21 +2008,12 @@`

		`def compile(self):`
		`self.sources()`
		`-`
		`- arch = None`
		`- define = None`
		`- short = False`
		`- nocheck = False`
		`- if self.args.arch:`
		`- arch = self.args.arch`
		`- if self.args.define:`
		`- define = self.args.define`
		`- if self.args.short_circuit:`
		`- short = True`
		`- if self.args.nocheck:`
		`- nocheck = True`
		`- self.cmd.compile(arch=arch, define=define, short=short,`
		`- builddir=self.args.builddir, nocheck=nocheck)`
		`+ self.cmd.compile(builddir=self.args.builddir,`
		`+ arch=self.args.arch,`
		`+ define=self.args.define,`
		`+ extra_args=self.extra_args,`
		`+ short=self.args.short_circuit,`
		`+ nocheck=self.args.nocheck,)`

		`def container_build_koji(self):`
		`# Keep it around for backward compatibility`
		`@@ -2145,11 +2151,13 @@`

		`def install(self):`
		`self.sources()`
		`- self.cmd.install(arch=self.args.arch, define=self.args.define,`
		`+ self.cmd.install(builddir=self.args.builddir,`
		`+ arch=self.args.arch,`
		`+ define=self.args.define,`
		`+ extra_args=self.extra_args,`
		`short=self.args.short_circuit,`
		`- builddir=self.args.builddir,`
		`nocheck=self.args.nocheck,`
		`- buildrootdir=self.args.buildrootdir)`
		`+ buildrootdir=self.args.buildrootdir,)`

		`def lint(self):`
		`self.cmd.lint(self.args.info, self.args.rpmlintconf)`
		`@@ -2174,11 +2182,12 @@`
		`localargs.extend(['--without', arg])`

		`self.cmd.local(localargs,`
		`+ builddir=self.args.builddir,`
		`arch=self.args.arch,`
		`define=self.args.define,`
		`- hashtype=self.args.hash,`
		`- builddir=self.args.builddir,`
		`- buildrootdir=self.args.buildrootdir)`
		`+ extra_args=self.extra_args,`
		`+ buildrootdir=self.args.buildrootdir,`
		`+ hashtype=self.args.hash,)`

		`def mockbuild(self):`
		`try:`
		`@@ -2213,6 +2222,11 @@`
		`if self.args.enable_network:`
		`mockargs.append('--enable-network')`

		`+ if self.extra_args:`
		`+ mockargs.extend(self.extra_args)`
		`+ self.log.debug("Extra args '{0}' are passed to mock "`
		`+ "command".format(self.extra_args))`
		`+`
		`# Pick up any mockargs from the env`
		`try:`
		`mockargs += os.environ['MOCKARGS'].split()`
		`@@ -2535,9 +2549,11 @@`

		`def prep(self):`
		`self.sources()`
		`- self.cmd.prep(arch=self.args.arch, define=self.args.define,`
		`- builddir=self.args.builddir,`
		`- buildrootdir=self.args.buildrootdir)`
		`+ self.cmd.prep(builddir=self.args.builddir,`
		`+ arch=self.args.arch,`
		`+ define=self.args.define,`
		`+ extra_args=self.extra_args,`
		`+ buildrootdir=self.args.buildrootdir,)`

		`def pull(self):`
		`self.cmd.pull(rebase=self.args.rebase,`
		`@@ -2605,11 +2621,11 @@`
		`# are for all arches and not set via argparse.`
		`self.cmd.srpm(`
		`builddir=getattr(self.args, 'builddir', None),`
		`- define=getattr(self.args, 'define', None),`
		`- buildrootdir=getattr(self.args, 'buildroot', None),`
		`arch=getattr(self.args, 'arch', None),`
		`- hashtype=self.args.hash`
		`- )`
		`+ define=getattr(self.args, 'define', None),`
		`+ extra_args=self.extra_args,`
		`+ buildrootdir=getattr(self.args, 'buildrootdir', None),`
		`+ hashtype=self.args.hash,)`

		`def switch_branch(self):`
		`if self.args.branch:`
		`@@ -2647,8 +2663,9 @@`
		`print('\n'.join(unused))`

		`def verify_files(self):`
		`- self.cmd.verify_files(define=self.args.define,`
		`- builddir=self.args.builddir,`
		`+ self.cmd.verify_files(builddir=self.args.builddir,`
		`+ define=self.args.define,`
		`+ extra_args=self.extra_args,`
		`buildrootdir=self.args.buildrootdir)`

		`def verrel(self):`
		`@@ -2702,7 +2719,24 @@`
		`if argcomplete is not None:`
		`argcomplete.autocomplete(self.parser)`
		`# Parse the args`
		`- self.args = self.parser.parse_args()`
		`+ # There are two groups of arguments:`
		`+ # * standard arguments defined by individual parsers`
		`+ # * extra arguments that are relevant for some (individually allowed)`
		`+ # commands. They are passed to underlying command (usually appended`
		`+ # to the rest of arguments on commandline)`
		`+`
		`+ argv = sys.argv[1:]`
		`+ if "--" in argv:`
		`+ i = argv.index("--")`
		`+ self.args = self.parser.parse_args(argv[:i])`
		`+ self.extra_args = argv[i+1:]`
		`+ else:`
		`+ self.args = self.parser.parse_args()`
		`+ self.extra_args = getattr(self.args, 'extra_args', None)`
		`+ if self.extra_args:`
		`+ print("Warning: extra_args in the command are not separated "`
		`+ "with '--'. It might not work correctly.\n",`
		`+ file=sys.stderr)`

		`# This is due to a difference argparse behavior to Python 2 version.`
		`# In Python 3, argparse will proceed to here without reporting`

tests/test_cli.py

file modified

+98 -9

		`@@ -577,6 +577,52 @@`
		`self.assertEqual('', cli.cmd.repo.git.log('--merges'))`


		`+ class TestClone(CliTestCase):`
		`+ """Test clone command"""`
		`+ def shortDescription(self):`
		`+ return None`
		`+`
		`+ @patch('sys.stderr', new=StringIO())`
		`+ @patch('pyrpkg.Commands._clone_config', new_callable=Mock())`
		`+ @patch('pyrpkg.Commands._run_command')`
		`+ def test_extra_args(self, _run_command, _clone_config):`
		`+ cli_cmd = ['rpkg', '--user', 'dude', '--path', self.cloned_repo_path,`
		`+ 'clone', 'repository_name', '--', '--progress']`
		`+`
		`+ with patch('sys.argv', new=cli_cmd):`
		`+ cli = self.new_cli()`
		`+ cli.clone()`
		`+`
		`+ expected_cmd = ['git', 'clone', 'ssh://dude@localhost/repository_name',`
		`+ '--origin', 'origin', '--progress']`
		`+ _run_command.assert_called_once_with(expected_cmd,`
		`+ cwd=self.cloned_repo_path)`
		`+ output = sys.stderr.getvalue().strip()`
		`+ self.assertEqual('', output)`
		`+`
		`+ @patch('sys.stderr', new=StringIO())`
		`+ @patch('pyrpkg.Commands._clone_config', new_callable=Mock())`
		`+ @patch('pyrpkg.Commands._run_command')`
		`+ def test_extra_args_incorrect(self, _run_command, _clone_config):`
		`+ """Missing '--' separator between standard arguments and extra`
		`+ arguments. This can be valid command but might have different`
		`+ and unexpected behaviour. Warning is shown."""`
		`+`
		`+ cli_cmd = ['rpkg', '--user', 'dude', '--path', self.cloned_repo_path,`
		`+ 'clone', 'repository_name', '--progress']`
		`+`
		`+ with patch('sys.argv', new=cli_cmd):`
		`+ cli = self.new_cli()`
		`+ cli.clone()`
		`+`
		`+ expected_cmd = ['git', 'clone', 'ssh://dude@localhost/repository_name',`
		`+ '--origin', 'origin', '--progress']`
		`+ _run_command.assert_called_once_with(expected_cmd,`
		`+ cwd=self.cloned_repo_path)`
		`+ output = sys.stderr.getvalue().strip()`
		`+ self.assertIn('extra_args in the command are not separated', output)`
		`+`
		`+`
		`class TestSrpm(CliTestCase):`
		`"""Test srpm command"""`

		`@@ -595,14 +641,28 @@`
		`@patch('pyrpkg.Commands._run_command')`
		`def test_srpm_with_options(self, _run_command):`
		`cli_cmd = ['rpkg', '--path', self.cloned_repo_path, '--release', 'rhel-6', 'srpm',`
		`- '--define', 'macro1 meansthis']`
		`+ '--define', '"macro1 meansthis"']`

		`with patch('sys.argv', new=cli_cmd):`
		`cli = self.new_cli()`
		`cli.srpm()`

		`expected_cmd = ['rpmbuild'] + cli.cmd.rpmdefines + \`
		`- ['--define', 'macro1 meansthis', '--nodeps', '-bs',`
		`+ ['--define', '"macro1 meansthis"', '--nodeps', '-bs',`
		`+ os.path.join(cli.cmd.path, cli.cmd.spec)]`
		`+ _run_command.assert_called_once_with(expected_cmd, shell=True)`
		`+`
		`+ @patch('pyrpkg.Commands._run_command')`
		`+ def test_srpm_with_extra_args(self, _run_command):`
		`+ cli_cmd = ['rpkg', '--path', self.cloned_repo_path, '--release', 'rhel-6', 'srpm',`
		`+ '--', '--define', '"name body"', '--undefine', 'python']`
		`+`
		`+ with patch('sys.argv', new=cli_cmd):`
		`+ cli = self.new_cli()`
		`+ cli.srpm()`
		`+`
		`+ expected_cmd = ['rpmbuild'] + cli.cmd.rpmdefines + \`
		`+ ['--define', '"name body"', '--undefine', 'python', '--nodeps', '-bs',`
		`os.path.join(cli.cmd.path, cli.cmd.spec)]`
		`_run_command.assert_called_once_with(expected_cmd, shell=True)`

		`@@ -630,7 +690,7 @@`

		`cli_cmd = ['rpkg', '--path', self.cloned_repo_path, '--release', 'rhel-6', '-q', 'compile',`
		`'--builddir', builddir, '--short-circuit', '--arch', 'i686', '--nocheck',`
		`- '--define', 'macro1 meansthis']`
		`+ '--define', 'macro1 meansthis', '--', '--rmspec']`

		`with patch('sys.argv', new=cli_cmd):`
		`cli = self.new_cli()`
		`@@ -639,7 +699,7 @@`
		`spec = os.path.join(cli.cmd.path, cli.cmd.spec)`
		`rpmbuild = ['rpmbuild'] + cli.cmd.rpmdefines + \`
		`["--define '_builddir %s'" % builddir, '--target', 'i686',`
		`- '--define', 'macro1 meansthis', '--short-circuit',`
		`+ '--define', 'macro1 meansthis', '--rmspec', '--short-circuit',`
		`'--nocheck', '--quiet', '-bc', spec]`

		`_run_command.assert_called_once_with(rpmbuild, shell=True)`
		`@@ -669,7 +729,7 @@`
		`cli_cmd = [`
		`'rpkg', '--path', self.cloned_repo_path, '--release', 'rhel-6',`
		`'-q', 'compile', '--arch', 'i686', '--builddir', builddir,`
		`- '--buildrootdir', buildrootdir`
		`+ '--buildrootdir', buildrootdir, '--', '-v'`
		`]`

		`with patch('sys.argv', new=cli_cmd):`
		`@@ -678,7 +738,7 @@`

		`spec = os.path.join(cli.cmd.path, cli.cmd.spec)`
		`rpmbuild = ['rpmbuild'] + cli.cmd.rpmdefines + [`
		`- "--define '_builddir %s'" % builddir, '--target', 'i686',`
		`+ "--define '_builddir %s'" % builddir, '--target', 'i686', '-v',`
		`'--quiet', "--define '_buildrootdir %s'" % buildrootdir,`
		`'--nodeps', '-bp', spec`
		`]`
		`@@ -756,7 +816,8 @@`
		`cli_cmd = [`
		`'rpkg', '--path', self.cloned_repo_path, '--release', 'rhel-6',`
		`'-q', 'local', '--builddir', builddir, '--arch', 'i686',`
		`- '--with', 'a', '--without', 'b', '--buildrootdir', buildrootdir]`
		`+ '--with', 'a', '--without', 'b', '--buildrootdir', buildrootdir,`
		`+ '--', '--clean']`

		`with patch('sys.argv', new=cli_cmd):`
		`cli = self.new_cli()`
		`@@ -766,7 +827,7 @@`
		`rpmbuild = ['rpmbuild'] + cli.cmd.rpmdefines + [`
		`'--with', 'a', '--without', 'b',`
		`"--define '_builddir %s'" % builddir,`
		`- '--target', 'i686', '--quiet',`
		`+ '--target', 'i686', '--clean', '--quiet',`
		`"--define '_buildrootdir %s'" % buildrootdir,`
		`'-ba', spec`
		`]`
		`@@ -813,8 +874,9 @@`
		`spec = os.path.join(cli.cmd.path, cli.cmd.spec)`
		`rpmbuild = ['rpmbuild'] + cli.cmd.rpmdefines + [`
		`"--define '_builddir %s'" % builddir,`
		`+ '--quiet',`
		`"--define '_buildrootdir %s'" % buildrootdir,`
		`- '--quiet', '-bl', spec`
		`+ '-bl', spec`
		`]`
		`_run_command.assert_called_once_with(rpmbuild, shell=True)`

		`@@ -1621,6 +1683,33 @@`
		`]`
		`self.mock_run_command.assert_called_with(expected_cmd)`

		`+ def test_extra_args(self):`
		`+ cli_cmd = ['rpkg', '--path', self.cloned_repo_path,`
		`+ '--release', 'rhel-6', 'mockbuild',`
		`+ '--root', '/etc/mock/some-root',`
		`+ '--with', 'a',`
		`+ '--', '--with', 'd', '--without', '"bb"']`
		`+ cli = self.mockbuild(cli_cmd)`
		`+`
		`+ expected_cmd = ['mock', '--with', 'a', '--with', 'd',`
		`+ '--without', '"bb"', '-r', '/etc/mock/some-root',`
		`+ '--resultdir', cli.cmd.mock_results_dir, '--rebuild',`
		`+ cli.cmd.srpmname]`
		`+ self.mock_run_command.assert_called_with(expected_cmd)`
		`+`
		`+ def test_extra_args_new_arguments(self):`
		`+ cli_cmd = ['rpkg', '--path', self.cloned_repo_path,`
		`+ '--release', 'rhel-6', 'mockbuild',`
		`+ '--root', '/etc/mock/some-root',`
		`+ '--', '--nocheck', '--arch=amd64', '-q']`
		`+ cli = self.mockbuild(cli_cmd)`
		`+`
		`+ expected_cmd = ['mock', '--nocheck', '--arch=amd64', '-q',`
		`+ '-r', '/etc/mock/some-root',`
		`+ '--resultdir', cli.cmd.mock_results_dir, '--rebuild',`
		`+ cli.cmd.srpmname]`
		`+ self.mock_run_command.assert_called_with(expected_cmd)`
		`+`

		`class TestCoprBuild(CliTestCase):`
		`"""Test copr command"""`

onosek commented 3 years ago

There are two groups of arguments:
* standard arguments defined by individual parsers
* extra arguments that are allowed for some (individually allowed)
commands. They are passed to underlying command (usually appended
to the rest of arguments on commandline)
This commit adds a common way that allow user to use extra arguments
for some commands. Commands include mockbuild, srpm, clone.

JIRA: RHELCMP-101
JIRA: RHELCMP-141
Fixes: #432
Fixes: #413

Signed-off-by: Ondrej Nosek onosek@redhat.com

rebased onto 188e3390264730e369929dc81f5c9f1ebc53a5a1

3 years ago

onosek commented 3 years ago

With this feature is possible to remove previously manually added arguments like
mockbuild --disablerepo/--enablerepo, local --with/--without, ...
But I think when we already have them and some users possibly use them, removing could cause little confusion. It means removing them from help (functionality would be the same). Argparse help generator would not show them.

onosek commented 3 years ago

'update' command mentioned in the request will be treated in the fedpkg, because it is not part of rpkg common library.

onosek commented 3 years ago

I found "--" string separates regular arguments and extra arguments unnecessary. Is it OK? Or does it have a special function or is it a convention?

lsedlar commented 3 years ago

Personally I would expect the usage would be to always separate put the extra arguments at the end and separate them by --. That should prevent conflicts and I think -- is a relatively common way to indicate parsing arguments should stop.

For example fedpkg mockbuild -N --enable-network -- --with-ssl --without-python2. Anything before -- will go to fedpkg, anything after that will be given to mock directly.

onosek commented 3 years ago

I was considering it. But I didn't find the way how to do this via argparse library.
It is probably possible. But this approach would require lot of manual parsing because argparse will not even give me even a ordering of arguments.

lsedlar commented 3 years ago

I think it should be doable with argparse.REMAINDER. I'm not sure it always requires the -- in there, but it should support it.

lsedlar commented 3 years ago

I had a similar problem in compose-partial-copy, and in the end the solution is very similar to what is in this PR. I must have misremembered it.

https://pagure.io/compose-utils/blob/master/f/bin/compose-partial-copy

Right now it's possible to use the -- to avoid conflicts, but it's not required, right? If so, I guess it should be fine.

rebased onto ac416257d4b6a9fa8f9e033c429b30cd7f0254b3

3 years ago

rebased onto 61255c88b812c79387eacff9a55fa1971e293a72

3 years ago

onosek commented 3 years ago

Despite I didn't find inspiration in compose-partial-copy, I found argparse.REMAINDER good tip for implementation. Thanks. It is significantly simpler than the original solution.
Current code lacks tests, I am gonna add some.
A lot of code changes in the PR modifies ordering of argument processing in methods regarding rpmbuild operations, hopefully with no functional consequences. The intension was better readability because methods do almost the same work and therefore they should look similar.