From a38a07f4a840130ceb25d5760ac85115f22f2dbe Mon Sep 17 00:00:00 2001 From: Otto Liljalaakso Date: Jan 21 2023 18:46:16 +0000 Subject: [PATCH 1/3] Reduce indentation in assert_build helper Nested if-else can be more clearly expressed as if-elif-else. Future modification of this function is easier after this is done. Signed-off-by: Otto Liljalaakso --- diff --git a/tests/test_cli.py b/tests/test_cli.py index dd1399a..b63e87e 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -3601,25 +3601,24 @@ class TestBuildPackage(FakeKojiCreds, CliTestCase): if sub_command == 'chain-build': self.assertEqual(expected_chain_urls, url) - else: - if '--srpm' in cli_cmd: - # Magic guess if a SRPM file name is given. - i = cli_cmd.index('--srpm') - if i + 1 >= len(cli_cmd) or cli_cmd[i + 1].startswith('--'): - filename = '{0}.src.rpm'.format(cli.cmd.nvr) - else: - filename = os.path.basename(cli_cmd[i + 1]) - match_regex = '{0}/{1}$'.format( - KOJI_UNIQUE_PATH_REGEX.rstrip('$'), filename) - six.assertRegex(self, url, match_regex) + elif '--srpm' in cli_cmd: + # Magic guess if a SRPM file name is given. + i = cli_cmd.index('--srpm') + if i + 1 >= len(cli_cmd) or cli_cmd[i + 1].startswith('--'): + filename = '{0}.src.rpm'.format(cli.cmd.nvr) else: - expected_url = '{0}#{1}'.format( - cli.config.get('rpkg', 'anongiturl', raw=True) % { - 'repo': 'docpkg' - }, - cli.cmd.commithash, - ) - self.assertEqual(expected_url, url) + filename = os.path.basename(cli_cmd[i + 1]) + match_regex = '{0}/{1}$'.format( + KOJI_UNIQUE_PATH_REGEX.rstrip('$'), filename) + six.assertRegex(self, url, match_regex) + else: + expected_url = '{0}#{1}'.format( + cli.config.get('rpkg', 'anongiturl', raw=True) % { + 'repo': 'docpkg' + }, + cli.cmd.commithash, + ) + self.assertEqual(expected_url, url) if '--background' in cli_cmd: magic_priority_number = 5 From b0b56d6807474f58f9b70dcdd08a963590792468 Mon Sep 17 00:00:00 2001 From: Otto Liljalaakso Date: Jan 21 2023 18:46:23 +0000 Subject: [PATCH 2/3] Code cleanup in tests/test_cli.py Just writing some small pieces a bit cleaner, preparing to extend these functions in the following commits. Signed-off-by: Otto Liljalaakso --- diff --git a/tests/test_cli.py b/tests/test_cli.py index b63e87e..a2cab01 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -3555,6 +3555,10 @@ class TestBuildPackage(FakeKojiCreds, CliTestCase): self.run_cmd(['git', 'reset', 'HEAD', 'hello.py'], cwd=self.cloned_repo_path) + def assert_srpm_url(self, url, filename): + re = '{0}/{1}$'.format(KOJI_UNIQUE_PATH_REGEX.rstrip('$'), filename) + six.assertRegex(self, url, re) + def assert_build(self, sub_command, cli_opts=[], expected_chain_urls=None, expected_opts={}, config_file=None): @@ -3608,9 +3612,7 @@ class TestBuildPackage(FakeKojiCreds, CliTestCase): filename = '{0}.src.rpm'.format(cli.cmd.nvr) else: filename = os.path.basename(cli_cmd[i + 1]) - match_regex = '{0}/{1}$'.format( - KOJI_UNIQUE_PATH_REGEX.rstrip('$'), filename) - six.assertRegex(self, url, match_regex) + self.assert_srpm_url(url, filename) else: expected_url = '{0}#{1}'.format( cli.config.get('rpkg', 'anongiturl', raw=True) % { @@ -3662,8 +3664,8 @@ class TestBuildPackage(FakeKojiCreds, CliTestCase): srpm_file, unique_path = args if expected_srpm_file is None: - self.assertEqual('{0}.src.rpm'.format(os.path.join(cli.cmd.path, cli.cmd.nvr)), - srpm_file) + nvr_path = os.path.join(cli.cmd.path, cli.cmd.nvr) + self.assertEqual('{0}.src.rpm'.format(nvr_path), srpm_file) else: self.assertEqual(expected_srpm_file, srpm_file) six.assertRegex(self, unique_path, r'^cli-build/\d+\.\d+\.[a-zA-Z]+$') From 753a7b9b140f5a61d27ac53a80dd9d1fe64deb5c Mon Sep 17 00:00:00 2001 From: Otto Liljalaakso Date: Jan 21 2023 18:53:35 +0000 Subject: [PATCH 3/3] Use srpm when scratch-building from dirty repo The most common use case for scratch builds is to test local changes before pushing them. Unfortunately, the defaults options in the scratch-build command did not support this case. Instead, an error was printed about uncommitted changes. To support the common case, scratch-build is changed to generate an srpm from local changes when they are detected. This way, the original behaviour is retained, and additionally the common case can be supported. This has been discussed in the Fedora devel mailing list: https://lists.fedoraproject.org/archives/list/devel@lists.fedoraproject.org/thread/VK4VOLPAHA3HWJ3323PZ5MRDH6FP4KAQ/#G4ACAGFW33YDWA5UPCB6CET7DQ24M7RI Fixes #652 Signed-off-by: Otto Liljalaakso --- diff --git a/pyrpkg/cli.py b/pyrpkg/cli.py index b0a16fb..8019fe9 100644 --- a/pyrpkg/cli.py +++ b/pyrpkg/cli.py @@ -1882,12 +1882,23 @@ class cliClient(object): if hasattr(self.args, 'srpm') and self.args.srpm: # See if we need to generate the srpm first if self.args.srpm == 'CONSTRUCT': - self.log.debug('Generating an srpm') - self.srpm() - self.args.srpm = '%s.src.rpm' % self.cmd.nvr - self.args.srpm = os.path.join(self.args.path, self.args.srpm) + self._generate_srpm() return self._upload_file_for_build(self.args.srpm) + if self.args.scratch and self.cmd.repo.is_dirty(): + self.log.info('Repository is dirty, generating an srpm.') + self._generate_srpm() + return self._upload_file_for_build(self.args.srpm) + + def _generate_srpm(self): + """Generate an SRPM from local module content, set args.srpm to the + resulting file path. + """ + self.log.debug('Generating an srpm') + self.srpm() + self.args.srpm = '%s.src.rpm' % self.cmd.nvr + self.args.srpm = os.path.join(self.args.path, self.args.srpm) + def _watch_build_tasks(self, task_ids): """Watch build tasks diff --git a/tests/test_cli.py b/tests/test_cli.py index a2cab01..d671d8e 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -3561,7 +3561,7 @@ class TestBuildPackage(FakeKojiCreds, CliTestCase): def assert_build(self, sub_command, cli_opts=[], expected_chain_urls=None, expected_opts={}, - config_file=None): + expected_srpm=False, config_file=None): session = self.mock_ClientSession.return_value cli_cmd = [ @@ -3613,6 +3613,8 @@ class TestBuildPackage(FakeKojiCreds, CliTestCase): else: filename = os.path.basename(cli_cmd[i + 1]) self.assert_srpm_url(url, filename) + elif expected_srpm: + self.assert_srpm_url(url, '{0}.src.rpm'.format(cli.cmd.nvr)) else: expected_url = '{0}#{1}'.format( cli.config.get('rpkg', 'anongiturl', raw=True) % { @@ -3646,16 +3648,7 @@ class TestBuildPackage(FakeKojiCreds, CliTestCase): cli_opts=['--scratch', '--nowait'], expected_opts={'scratch': True}) - def assert_option_srpm_use(self, expected_srpm_file=None): - # Ensure the fake srpm file exists. - with patch('os.path.exists', return_value=True): - opts = ['--srpm'] - if expected_srpm_file: - opts.append(expected_srpm_file) - cli = self.assert_build('scratch-build', - cli_opts=opts, - expected_opts={'scratch': True}) - + def assert_srpm_upload(self, cli, expected_srpm_file=None): session = self.mock_ClientSession.return_value session.uploadWrapper.assert_called_once() @@ -3672,6 +3665,34 @@ class TestBuildPackage(FakeKojiCreds, CliTestCase): self.assertEqual({'name': os.path.basename(srpm_file), 'callback': koji_cli.lib._progress_callback}, kwargs) + def assert_option_srpm_use(self, expected_srpm_file=None): + """Assert correct scratch-build invocation with explicit --srpm option. + """ + + # Ensure the fake srpm file exists. + with patch('os.path.exists', return_value=True): + opts = ['--srpm'] + if expected_srpm_file: + opts.append(expected_srpm_file) + cli = self.assert_build('scratch-build', + cli_opts=opts, + expected_opts={'scratch': True}) + + self.assert_srpm_upload(cli, expected_srpm_file) + + def assert_default_srpm_use(self): + """Assert correct scratch-build invocation in conditions where srpm is + generated and uploaded by default. + """ + + # Ensure the fake srpm file exists. + with patch('os.path.exists', return_value=True): + cli = self.assert_build('scratch-build', + expected_srpm=True, + expected_opts={'scratch': True}) + + self.assert_srpm_upload(cli) + def test_srpm_option_with_srpm_file(self): self.assert_option_srpm_use('/path/to/docpkg-0.1-1.fc28.src.rpm') @@ -3778,6 +3799,24 @@ class TestBuildPackage(FakeKojiCreds, CliTestCase): self.assert_build, 'build') @patch('pyrpkg.Commands.nvr', new_callable=PropertyMock) + @patch('pyrpkg.Commands._run_command') + def test_scratch_build_generates_srpm_if_repo_has_uncommitted_changed(self, _run_command, nvr): + self.make_changes(filename='hello.py', content='print()') + + with patch('pyrpkg.layout.build', return_value=MockLayout(root_dir=self.cloned_repo_path)): + nvr.return_value = 'docpkg-0.1-1.fc28' + self.assert_default_srpm_use() + + @patch('pyrpkg.Commands.nvr', new_callable=PropertyMock) + @patch('pyrpkg.Commands._run_command') + def test_scratch_build_from_build_command_generates_srpm__if_repo_has_uncommitted_changes(self, _run_command, nvr): + self.make_changes(filename='hello.py', content='print()') + + with patch('pyrpkg.layout.build', return_value=MockLayout(root_dir=self.cloned_repo_path)): + nvr.return_value = 'docpkg-0.1-1.fc28' + self.assert_default_srpm_use() + + @patch('pyrpkg.Commands.nvr', new_callable=PropertyMock) @patch('pyrpkg.Commands.commithash', new_callable=PropertyMock) @patch('subprocess.Popen') def test_chainbuild_do_not_check_nvr_existence(self, Popen, commithash, nvr):