From 8667d5379161183b306bdd4a6733c666cd2ef310 Mon Sep 17 00:00:00 2001 From: Otto Liljalaakso Date: Sun, 2 Apr 2023 17:21:00 +0300 Subject: [PATCH 1/2] Use release's rpmdefines in unused sources check Conditional Source: tags are problematic and, in fact, forbidden in at least Fedora. However, there are packages that conditionalize packages based on macros such as %{rhel} or %{fedora}. 'x-pkg sources' did not handle such packages correctly, because when the specfile was parsed to check for unused sources, values for those macros were not set. This was different from other commands which set such macros based on the value of --release parameter or Git branch name. Improve support for conditional Source: tags by using the standard set of rpmdefines when the specfile is parsed in 'fedpkg sources'. Fixes: #671 JIRA: RHELCMP-11465 Merges: https://pagure.io/rpkg/pull-request/678 Signed-off-by: Otto Liljalaakso --- pyrpkg/__init__.py | 21 +++++++++++++++------ pyrpkg/spec.py | 12 +++++++----- tests/test_cli.py | 21 ++++++++++++++++++++- tests/test_spec.py | 8 ++++++-- 4 files changed, 48 insertions(+), 14 deletions(-) diff --git a/pyrpkg/__init__.py b/pyrpkg/__init__.py index 3f934d3..817ef33 100644 --- a/pyrpkg/__init__.py +++ b/pyrpkg/__init__.py @@ -2261,13 +2261,22 @@ class Commands(object): sourcesf = SourcesFile(self.sources_filename, self.source_entry_type) try: - specf = SpecFile(os.path.join(self.layout.specdir, self.spec), - self.layout.sourcedir) - spec_parsed = True - except Exception: - self.log.warning("Parsing specfile for used sources failed. " - "Falling back to downloading all sources.") + # Try resolving rpmdefines separately. This produces a clear error + # message in the common failure case of custom branch name. + self.rpmdefines + except Exception as err: + self.log.warning("Parsing specfile for used sources failed: %s" % err) + self.log.warning("Falling back to downloading all sources.") spec_parsed = False + else: + try: + specf = SpecFile(os.path.join(self.layout.specdir, self.spec), + self.rpmdefines) + spec_parsed = True + except Exception: + self.log.warning("Parsing specfile for used sources failed. " + "Falling back to downloading all sources.") + spec_parsed = False args = dict() if self.lookaside_request_params: diff --git a/pyrpkg/spec.py b/pyrpkg/spec.py index d72f1fb..5400de3 100644 --- a/pyrpkg/spec.py +++ b/pyrpkg/spec.py @@ -18,16 +18,16 @@ class SpecFile(object): r'^((source[0-9]*|patch[0-9]*)\s*:\s*(?P.*))\s*$', re.IGNORECASE) - def __init__(self, spec, sourcedir): + def __init__(self, spec, rpmdefines): self.spec = spec - self.sourcedir = sourcedir + self.rpmdefines = rpmdefines self.sources = [] self.parse() def parse(self): """Call rpmspec and find source tags from the result.""" - stdout = run(self.spec, self.sourcedir) + stdout = run(self.spec, self.rpmdefines) for line in stdout.splitlines(): m = self.sourcefile_expression.match(line) if not m: @@ -38,8 +38,10 @@ class SpecFile(object): self.sources.append(val) -def run(spec, sourcedir): - cmdline = ['rpmspec', '--define', "_sourcedir %s" % sourcedir, '-P', spec] +def run(spec, rpmdefines): + cmdline = ['rpmspec'] + cmdline.extend(rpmdefines) + cmdline.extend(['-P', spec]) try: process = subprocess.Popen(cmdline, stdout=subprocess.PIPE, diff --git a/tests/test_cli.py b/tests/test_cli.py index 02620ef..58df047 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -1607,6 +1607,25 @@ class TestSources(LookasideCacheMock, CliTestCase): def test_unused_sources_are_not_downloaded(self): self._upload_unused() + cli_cmd = ['rpkg', '--path', self.cloned_repo_path, 'sources'] + with patch('sys.argv', new=cli_cmd): + with patch('pyrpkg.Commands.rpmdefines', + new=['--define', '_sourcedir %s' % self.cloned_repo_path]): + cli = self.new_cli() + with patch('pyrpkg.lookaside.CGILookasideCache.download', + new=self.lookasidecache_download): + cli.sources() + + path = os.path.join(self.cloned_repo_path, 'unused.patch') + self.assertFalse(os.path.exists(path)) + + @patch('pyrpkg.Commands.load_rpmdefines') + def test_download_sources_including_unused(self, rpmdefines): + self._upload_unused() + # SpecFile parsing executes 'rpmspec', that needs '--define' arguments from rpmdefines + # when rpmdefines raises eception, SpecFile parsing fails --> all sources are downloaded. + rpmdefines.side_effect = rpkgError + cli_cmd = ['rpkg', '--path', self.cloned_repo_path, 'sources'] with patch('sys.argv', new=cli_cmd): cli = self.new_cli() @@ -1615,7 +1634,7 @@ class TestSources(LookasideCacheMock, CliTestCase): cli.sources() path = os.path.join(self.cloned_repo_path, 'unused.patch') - self.assertFalse(os.path.exists(path)) + self.assertTrue(os.path.exists(path)) def test_force_option_downloads_unused_sources(self): self._upload_unused() diff --git a/tests/test_spec.py b/tests/test_spec.py index eefc475..0c7907a 100644 --- a/tests/test_spec.py +++ b/tests/test_spec.py @@ -10,6 +10,10 @@ from pyrpkg.errors import rpkgError class SpecFileTestCase(unittest.TestCase): def setUp(self): self.workdir = tempfile.mkdtemp(prefix='rpkg-tests.') + self.rpmdefines = ["--define", "_sourcedir %s" % self.workdir, + "--define", "_specdir %s" % self.workdir, + "--define", "_builddir %s" % self.workdir, + "--eval", "%%undefine rhel"] self.specfile = os.path.join(self.workdir, self._testMethodName) # Write common header @@ -43,7 +47,7 @@ class SpecFileTestCase(unittest.TestCase): "PAtch999: https://remote.patch-sourcce.org/another-patch.bz2\n") spec_fd.close() - s = spec.SpecFile(self.specfile, self.workdir) + s = spec.SpecFile(self.specfile, self.rpmdefines) actual = s.sources expected = [ "tarball.tar.gz", @@ -65,4 +69,4 @@ class SpecFileTestCase(unittest.TestCase): self.assertRaises(rpkgError, spec.SpecFile, self.specfile, - self.workdir) + self.rpmdefines) -- 2.40.0