From 8667d5379161183b306bdd4a6733c666cd2ef310 Mon Sep 17 00:00:00 2001

From: Otto Liljalaakso <otto.liljalaakso@iki.fi>

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 <otto.liljalaakso@iki.fi>

---

 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<val>.*))\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