rpkg

#262 lint: Avoid checking rpm's multiple times
Merged by cqi. Opened by tmz.
tmz/rpkg lint-avoid-dupes  into  master

Download 262.patch

When using the lint command, the rpm list includes duplicate packages
which are then checked via rpmlint multiple times.

Simplify the listing of rpm files using glob rather than looping over
os.listdir(). Use set rather than list to ensure there are no
duplicates in the rpms or arches lists. Sort the rpms when calling
rpmlint for consistent ordering across lint runs.

Signed-off-by: Todd Zullinger tmz@pobox.com

When using the lint command, the rpm list includes duplicate packages
which are then checked via rpmlint multiple times.

Can you write a test for this fix?

Hmm, I'd have to look a bit. I'm not at all familiar with the rpkg tests.

I don't see any lint tests that use anything other than the spec file. Do you know of any tests which might serve as an example of doing something similar with a file list? It seems like the lint function would need to learn a parameter (noop, list_only or something) to be able to verify that it listed the files only once in the rpms list. While I was debugging this, I just added some quick and dirty code to print the srpm and rpms values like this:

--- pyrpkg/__init__.py~ 2017-11-10 11:15:23.830122025 -0500
+++ pyrpkg/__init__.py  2017-11-06 23:33:19.535468146 -0500
@@ -2140,8 +2163,6 @@
         srpm = "%s-%s-%s.src.rpm" % (self.module_name, self.ver, self.rel)
         if not os.path.exists(os.path.join(self.path, srpm)):
             log.warning('No srpm found')
+        else:
+            log.info('srpm: {0}'.format(srpm))
         # Get the possible built arches
         arches = set(self._get_build_arches_from_spec())
@@ -2154,8 +2175,6 @@
                     '*-%s-%s.*.rpm' % (self.ver, self.rel))))
         if not rpms:
             log.warning('No rpm found')
+        else:
+            log.info('rpms:\n{0}'.format('\n'.join(sorted(rpms))))
         cmd = ['rpmlint']
         if info:
             cmd.extend(['-i'])

I'm not sure how many people use fedpkg lint to check rpm/srpm files, since the location that it looks for files is a bit odd ($arch subdirs rather than results/$ver/$rel where a fedpkg mockbuild would put rpms).

The more I think about this, the less clear I become about how we could add tests for this issue. The two methods I can think of area (both of which require adding some rpm files to tests/fixtures):

  • add a flag to lint to list the rpm's found and then confirm that has no duplicates
  • ensure the rpm's used in the test produce warnings and then parse the output to see that the same package wasn't listed twice

Both seem like the effort is far greater than the reward. (And that's not just me trying to be lazy. I genuinely appreciate the goal of proper testing.) Maybe someone else has other suggestions on how to test this? It could easily be my lack of familiarity with python unittest that makes it harder than it really is.

Beyond this fix, having lint look in results_$name/$ver/$rel/ rather than in $arch/ for the packages would make checking local builds easier. That's likely something to make configurable and then override in fedpkg if that's not the default. An easier change which is a small improvement on top of this patch is:

commit 8338073e4897459862b36bc1ae66b491f73316df
Author: Todd Zullinger <tmz@pobox.com>
Date:   Mon Nov 6 23:33:30 2017 -0500
    lint: Restrict glob matches to version/release
    When running rpmlint and searching for rpm packages, match only files
    which are the same version/release as the target release.  The srpm
    already does so.  Make the rpms the same.
diff --git a/pyrpkg/__init__.py b/pyrpkg/__init__.py
index 318863b..ea16e87 100644
--- a/pyrpkg/__init__.py
+++ b/pyrpkg/__init__.py
@@ -2171,7 +2171,8 @@ class Commands(object):
             if os.path.exists(os.path.join(self.path, arch)):
                 # For each available arch folder, lists file and keep
                 # those ending with .rpm
-                rpms.update(glob.glob(os.path.join(self.path, arch, '*.rpm')))
+                rpms.update(glob.glob(os.path.join(self.path, arch,
+                    '*-%s-%s.*.rpm' % (self.ver, self.rel))))
         if not rpms:
             log.warning('No rpm found')
         cmd = ['rpmlint']

I believe @cqi had something like what follows in mind. It's not necessary to modify the code, just to mock the calls that drive it and check that the command would be invoked correctly, without actually doing any work.

diff --git a/tests/test_commands.py b/tests/test_commands.py
index ca63377..6175d06 100644
--- a/tests/test_commands.py
+++ b/tests/test_commands.py
@@ -907,3 +907,38 @@ class TestConfigMockConfigDirWithNecessaryFiles(CommandTestCase):
             m.side_effect = IOError
             self.assertRaises(rpkgError,
                               cmd._config_dir_other, '/path/to/config-dir')
+
+
+class TestLint(CommandTestCase):
+    @patch('glob.glob')
+    @patch('os.path.exists')
+    @patch('pyrpkg.Commands._run_command')
+    @patch('pyrpkg.Commands.load_rpmdefines', new=mock_load_rpmdefines)
+    def test_lint_each_file_once(self, run, exists, glob):
+        cmd = self.make_commands()
+        srpm_path = os.path.join(cmd.path, 'docpkg-1.2-2.fc26.src.rpm')
+        bin_path = os.path.join(cmd.path, 'x86_64', 'docpkg-1.2-2.fc26.x86_64.rpm')
+
+        def _mock_exists(path):
+            return path in [
+                srpm_path,
+                os.path.join(cmd.path, 'x86_64'),
+            ]
+
+        def _mock_glob(g):
+            return {
+                os.path.join(cmd.path, 'x86_64', '*.rpm'): [bin_path],
+            }[g]
+        exists.side_effect = _mock_exists
+        glob.side_effect = _mock_glob
+        cmd._get_build_arches_from_spec = Mock(return_value=['x86_64', 'x86_64'])
+
+        cmd.lint()
+
+        self.assertEqual(
+            run.call_args_list,
+            [call(['rpmlint',
+                   os.path.join(cmd.path, 'docpkg.spec'),
+                   srpm_path,
+                   bin_path,
+                   ], shell=True)])

rebased onto ded77b0369736b080f9fcb09f6501c6dc0040f78

Ahh, thanks for providing the test @lsedlar! I squashed that into the commit and added a 'Helped-by:' trailer. It's entirely your work and I thought about making it a second commit, but that would mean adding my commit without tests rather than doing it all together. I can easily do it either way you prefer though.

It might be best to add the new test first, marking it as expected to fail, then my commit with the code changes and marking the test as expected to pass? (Trying this using @unittest.expectedFailure it seems that nose does not support that decorator -- though it does mark the test as passing, so the test suite succeeds.)

BTW, while setting up a container to run the tests, I noticed some modules which are not mentioned in either install_requires or tests_require: 

python2-coverage
python2-koji
rpm-build # mostly for rpmspec, it seems

Should these be documented (the python modules in tests_require and rpm-build perhaps in README.rst or somewhere?

The command I used to install all the packages required on top of the stock fedora-26 docker image was:

dnf install GitPython python2-{cccolutils,coverage,koji,mock,nose,pycurl,six} rpm-build

@tmz Sorry for not response in time. Thanks for fixing this. Patch looks good to me.

There are several files in requirements listing packages, either Fedora or Python packages. But, yeah, to be honest, it should make it easier to install required packages as much as possible as your suggestion.

Merging. :tada:

Commit 9aec701d fixes this pull-request

Pull-Request has been merged by cqi

Pull-Request has been merged by cqi
Metadata
None
No Tags
Static archive. pagure.io was sunset in 2026; this is a read-only snapshot.
Red Hat Logo

Fedora is sponsored by Red Hat. Learn more about the relationship between Red Hat and Fedora »

© Red Hat, Inc. and others.