From 44e84d7dd9fa7f67d88cd6366612ac1a22fac3d4 Mon Sep 17 00:00:00 2001 From: Tomas Kopecek Date: Feb 17 2026 09:03:52 +0000 Subject: [PATCH 1/6] Rpmdiff doesn't correctly check hashes Keys to hash dict are incorrectly derived. So, even if the hashes are same, rpmdiff is run every time. Related: https://pagure.io/koji/issue/4541 --- diff --git a/kojihub/kojihub.py b/kojihub/kojihub.py index 97ade40..62df40f 100644 --- a/kojihub/kojihub.py +++ b/kojihub/kojihub.py @@ -10670,11 +10670,11 @@ def rpmdiff(basepath, rpmlist, hashes): if len(rpmlist) < 2: return first_rpm = rpmlist[0] - task_id = first_rpm.split('/')[1] - first_hash = hashes.get(task_id, {}).get(os.path.basename(first_rpm), False) + task_id = first_rpm.split('/')[2] + first_hash = hashes.get(task_id, {}).get(os.path.basename(first_rpm)) for other_rpm in rpmlist[1:]: if first_hash: - task_id = other_rpm.split('/')[1] + task_id = other_rpm.split('/')[2] other_hash = hashes[task_id][os.path.basename(other_rpm)] if first_hash == other_hash: logger.debug("Skipping noarch rpmdiff for %s vs %s" % (first_rpm, other_rpm)) diff --git a/tests/test_hub/test_rpmdiff.py b/tests/test_hub/test_rpmdiff.py index bc9c93f..9e58fe7 100644 --- a/tests/test_hub/test_rpmdiff.py +++ b/tests/test_hub/test_rpmdiff.py @@ -13,7 +13,7 @@ class TestRPMDiff(unittest.TestCase): def test_rpmdiff_empty_invocation(self, Rpmdiff): kojihub.rpmdiff('basepath', [], hashes={}) Rpmdiff.assert_not_called() - kojihub.rpmdiff('basepath', ['foo'], hashes={}) + kojihub.rpmdiff('basepath', ['tasks/12/1234/foo'], hashes={}) Rpmdiff.assert_not_called() @mock.patch('koji.rpmdiff.Rpmdiff') @@ -21,9 +21,10 @@ class TestRPMDiff(unittest.TestCase): d = mock.MagicMock() d.differs.return_value = False Rpmdiff.return_value = d - self.assertFalse(kojihub.rpmdiff('basepath', ['12/1234/foo', '23/2345/bar'], hashes={})) + self.assertFalse(kojihub.rpmdiff('basepath', + ['tasks/12/1234/foo', 'tasks/23/2345/bar'], hashes={})) Rpmdiff.assert_called_once_with( - 'basepath/12/1234/foo', 'basepath/23/2345/bar', ignore='S5TN') + 'basepath/tasks/12/1234/foo', 'basepath/tasks/23/2345/bar', ignore='S5TN') @mock.patch('koji.rpmdiff.Rpmdiff') def test_rpmdiff_simple_failure(self, Rpmdiff): @@ -31,9 +32,9 @@ class TestRPMDiff(unittest.TestCase): d.differs.return_value = True Rpmdiff.return_value = d with self.assertRaises(koji.BuildError): - kojihub.rpmdiff('basepath', ['12/1234/foo', '13/1345/bar'], hashes={}) + kojihub.rpmdiff('basepath', ['tasks/12/1234/foo', 'tasks/13/1345/bar'], hashes={}) Rpmdiff.assert_called_once_with( - 'basepath/12/1234/foo', 'basepath/13/1345/bar', ignore='S5TN') + 'basepath/tasks/12/1234/foo', 'basepath/tasks/13/1345/bar', ignore='S5TN') d.textdiff.assert_called_once_with() def test_rpmdiff_real_target(self): @@ -169,7 +170,7 @@ class TestCheckNoarchRpms(unittest.TestCase): @mock.patch('kojihub.kojihub.rpmdiff') def test_check_noarch_rpms_simple_invocation(self, rpmdiff): - originals = ['12/1234/foo.noarch.rpm', '23/2345/foo.noarch.rpm'] + originals = ['tasks/12/1234/foo.noarch.rpm', 'tasks/23/2345/foo.noarch.rpm'] result = kojihub.check_noarch_rpms('basepath', copy.copy(originals)) self.assertEqual(result, originals[0:1]) self.assertEqual(len(rpmdiff.mock_calls), 1) @@ -177,28 +178,300 @@ class TestCheckNoarchRpms(unittest.TestCase): @mock.patch('kojihub.kojihub.rpmdiff') def test_check_noarch_rpms_with_duplicates(self, rpmdiff): originals = [ - 'bar.noarch.rpm', - 'bar.noarch.rpm', - 'bar.noarch.rpm', + 'tasks/34/1234/bar.noarch.rpm', + 'tasks/45/2345/bar.noarch.rpm', + 'tasks/55/5555/bar.noarch.rpm', ] result = kojihub.check_noarch_rpms('basepath', copy.copy(originals)) - self.assertEqual(result, ['bar.noarch.rpm']) + # pick the first one + self.assertEqual(result, ['tasks/34/1234/bar.noarch.rpm']) rpmdiff.assert_called_once_with('basepath', originals, hashes={}) @mock.patch('kojihub.kojihub.rpmdiff') def test_check_noarch_rpms_with_mixed(self, rpmdiff): originals = [ - 'foo.x86_64.rpm', - 'bar.x86_64.rpm', - 'bar.noarch.rpm', - 'bar.noarch.rpm', + 'tasks/34/1234/foo.x86_64.rpm', + 'tasks/34/2234/bar.x86_64.rpm', + 'tasks/12/1212/bar.noarch.rpm', + 'tasks/23/2323/bar.noarch.rpm', ] result = kojihub.check_noarch_rpms('basepath', copy.copy(originals)) self.assertEqual(result, [ - 'foo.x86_64.rpm', 'bar.x86_64.rpm', 'bar.noarch.rpm' + 'tasks/34/1234/foo.x86_64.rpm', + 'tasks/34/2234/bar.x86_64.rpm', + 'tasks/12/1212/bar.noarch.rpm' ]) rpmdiff.assert_called_once_with( 'basepath', - ['bar.noarch.rpm', 'bar.noarch.rpm'], + ['tasks/12/1212/bar.noarch.rpm', 'tasks/23/2323/bar.noarch.rpm'], hashes={} ) + + +class TestRPMDiffHub(unittest.TestCase): + @mock.patch('koji.rpmdiff.Rpmdiff') + def test_rpmdiff_differing_hashes_fails(self, Rpmdiff): + """When different archs have different pre-computed hashes, task must fail.""" + d = mock.MagicMock() + d.textdiff.return_value = 'mock diff' + Rpmdiff.return_value = d + # Paths: tasks/2345/12345/pkg.noarch.rpm and tasks/6789/56789/pkg.noarch.rpm + rpmlist = ['tasks/2345/12345/pkg.noarch.rpm', 'tasks/6789/56789/pkg.noarch.rpm'] + hashes = { + 12345: {'pkg.noarch.rpm': 'hash_from_arch1'}, + 56789: {'pkg.noarch.rpm': 'hash_from_arch2'}, + } + with self.assertRaises(koji.BuildError) as cm: + kojihub.rpmdiff('basepath', rpmlist, hashes=hashes) + self.assertIn('built differently on different architectures', str(cm.exception)) + Rpmdiff.assert_called_once_with( + 'basepath/tasks/2345/12345/pkg.noarch.rpm', + 'basepath/tasks/6789/56789/pkg.noarch.rpm', + ignore='S5TN') + + @mock.patch('koji.rpmdiff.Rpmdiff') + def test_rpmdiff_same_hash_skips(self, Rpmdiff): + """When pre-computed hashes are equal, skip Rpmdiff (optimization).""" + rpmlist = ['tasks/2345/12345/pkg.noarch.rpm', 'tasks/6789/56789/pkg.noarch.rpm'] + hashes = { + "12345": {'pkg.noarch.rpm': "same_hash"}, + "56789": {'pkg.noarch.rpm': "same_hash"}, + } + kojihub.rpmdiff('basepath', rpmlist, hashes=hashes) + Rpmdiff.assert_not_called() + + +class TestRealBuild(unittest.TestCase): + # https://kojihub.stream.rdu2.redhat.com/koji/taskinfo?taskID=6138158 + + @mock.patch('koji.rpmdiff.Rpmdiff') + @mock.patch('koji.load_json') + def test_real_build(self, load_json, rpmdiff): + # workdir /volume/work + # taskrelpath = tasks/34/1234 + uploadpath = '/mnt/koji/work' + results = { + 6138160: { + "rpms": [ + "tasks/8160/6138160/golang-src-1.25.3-7.el10.noarch.rpm", + "tasks/8160/6138160/golang-tests-1.25.3-7.el10.noarch.rpm", + "tasks/8160/6138160/go-toolset-1.25.3-7.el10.aarch64.rpm", + "tasks/8160/6138160/golang-bin-1.25.3-7.el10.aarch64.rpm", + "tasks/8160/6138160/golang-1.25.3-7.el10.aarch64.rpm", + "tasks/8160/6138160/golang-race-1.25.3-7.el10.aarch64.rpm", + "tasks/8160/6138160/golang-docs-1.25.3-7.el10.noarch.rpm", + "tasks/8160/6138160/golang-misc-1.25.3-7.el10.noarch.rpm" + ], + "srpms": [ + "tasks/8160/6138160/golang-1.25.3-7.el10.src.rpm" + ], + "logs": [ + "tasks/8160/6138160/state.log", + "tasks/8160/6138160/build.log", + "tasks/8160/6138160/root.log", + "tasks/8160/6138160/dnf.librepo.log", + "tasks/8160/6138160/hw_info.log", + "tasks/8160/6138160/dnf.log", + "tasks/8160/6138160/dnf.rpm.log", + "tasks/8160/6138160/installed_pkgs.log", + "tasks/8160/6138160/mock_output.log", + "tasks/8160/6138160/mock_config.log", + "tasks/8160/6138160/noarch_rpmdiff.json" + ], + "brootid": 771895 + }, + 6138161: { + "rpms": [ + "tasks/8161/6138161/golang-docs-1.25.3-7.el10.noarch.rpm", + "tasks/8161/6138161/go-toolset-1.25.3-7.el10.ppc64le.rpm", + "tasks/8161/6138161/golang-race-1.25.3-7.el10.ppc64le.rpm", + "tasks/8161/6138161/golang-bin-1.25.3-7.el10.ppc64le.rpm", + "tasks/8161/6138161/golang-tests-1.25.3-7.el10.noarch.rpm", + "tasks/8161/6138161/golang-src-1.25.3-7.el10.noarch.rpm", + "tasks/8161/6138161/golang-1.25.3-7.el10.ppc64le.rpm", + "tasks/8161/6138161/golang-misc-1.25.3-7.el10.noarch.rpm" + ], + "srpms": [], + "logs": [ + "tasks/8161/6138161/mock_config.log", + "tasks/8161/6138161/mock_output.log", + "tasks/8161/6138161/build.log", + "tasks/8161/6138161/installed_pkgs.log", + "tasks/8161/6138161/hw_info.log", + "tasks/8161/6138161/root.log", + "tasks/8161/6138161/dnf.log", + "tasks/8161/6138161/dnf.rpm.log", + "tasks/8161/6138161/state.log", + "tasks/8161/6138161/dnf.librepo.log", + "tasks/8161/6138161/noarch_rpmdiff.json" + ], + "brootid": 771896 + }, + 6138162: { + "rpms": [ + "tasks/8162/6138162/golang-bin-1.25.3-7.el10.s390x.rpm", + "tasks/8162/6138162/go-toolset-1.25.3-7.el10.s390x.rpm", + "tasks/8162/6138162/golang-tests-1.25.3-7.el10.noarch.rpm", + "tasks/8162/6138162/golang-1.25.3-7.el10.s390x.rpm", + "tasks/8162/6138162/golang-race-1.25.3-7.el10.s390x.rpm", + "tasks/8162/6138162/golang-src-1.25.3-7.el10.noarch.rpm", + "tasks/8162/6138162/golang-docs-1.25.3-7.el10.noarch.rpm", + "tasks/8162/6138162/golang-misc-1.25.3-7.el10.noarch.rpm" + ], + "srpms": [], + "logs": [ + "tasks/8162/6138162/mock_output.log", + "tasks/8162/6138162/dnf.librepo.log", + "tasks/8162/6138162/installed_pkgs.log", + "tasks/8162/6138162/mock_config.log", + "tasks/8162/6138162/hw_info.log", + "tasks/8162/6138162/dnf.log", + "tasks/8162/6138162/dnf.rpm.log", + "tasks/8162/6138162/state.log", + "tasks/8162/6138162/build.log", + "tasks/8162/6138162/root.log", + "tasks/8162/6138162/noarch_rpmdiff.json" + ], + "brootid": 771893 + }, + 6138163: { + "rpms": [ + "tasks/8163/6138163/golang-src-1.25.3-7.el10.noarch.rpm", + "tasks/8163/6138163/golang-race-1.25.3-7.el10.x86_64.rpm", + "tasks/8163/6138163/golang-bin-1.25.3-7.el10.x86_64.rpm", + "tasks/8163/6138163/golang-docs-1.25.3-7.el10.noarch.rpm", + "tasks/8163/6138163/go-toolset-1.25.3-7.el10.x86_64.rpm", + "tasks/8163/6138163/golang-misc-1.25.3-7.el10.noarch.rpm", + "tasks/8163/6138163/golang-tests-1.25.3-7.el10.noarch.rpm", + "tasks/8163/6138163/golang-1.25.3-7.el10.x86_64.rpm" + ], + "srpms": [], + "logs": [ + "tasks/8163/6138163/dnf.log", + "tasks/8163/6138163/dnf.rpm.log", + "tasks/8163/6138163/state.log", + "tasks/8163/6138163/hw_info.log", + "tasks/8163/6138163/installed_pkgs.log", + "tasks/8163/6138163/build.log", + "tasks/8163/6138163/mock_output.log", + "tasks/8163/6138163/dnf.librepo.log", + "tasks/8163/6138163/mock_config.log", + "tasks/8163/6138163/root.log", + "tasks/8163/6138163/noarch_rpmdiff.json" + ], + "brootid": 771894 + } + } + logs = { + 'aarch64': [ + "vol/koji02/packages/golang/1.25.3/7.el10,draft_93372/data/logs/aarch64/state.log", + "vol/koji02/packages/golang/1.25.3/7.el10,draft_93372/data/logs/aarch64/build.log", + "vol/koji02/packages/golang/1.25.3/7.el10,draft_93372/data/logs/aarch64/root.log", + "vol/koji02/packages/golang/1.25.3/7.el10,draft_93372/data/logs/aarch64/dnf.librepo.log", + "vol/koji02/packages/golang/1.25.3/7.el10,draft_93372/data/logs/aarch64/hw_info.log", + "vol/koji02/packages/golang/1.25.3/7.el10,draft_93372/data/logs/aarch64/dnf.log", + "vol/koji02/packages/golang/1.25.3/7.el10,draft_93372/data/logs/aarch64/dnf.rpm.log", + "vol/koji02/packages/golang/1.25.3/7.el10,draft_93372/data/logs/aarch64/installed_pkgs.log", + "vol/koji02/packages/golang/1.25.3/7.el10,draft_93372/data/logs/aarch64/mock_output.log", + "vol/koji02/packages/golang/1.25.3/7.el10,draft_93372/data/logs/aarch64/mock_config.log", + "vol/koji02/packages/golang/1.25.3/7.el10,draft_93372/data/logs/aarch64/noarch_rpmdiff.json", + ], + 'ppc64le': [ + "vol/koji02/packages/golang/1.25.3/7.el10,draft_93372/data/logs/ppc64le/mock_config.log", + "vol/koji02/packages/golang/1.25.3/7.el10,draft_93372/data/logs/ppc64le/mock_output.log", + "vol/koji02/packages/golang/1.25.3/7.el10,draft_93372/data/logs/ppc64le/build.log", + "vol/koji02/packages/golang/1.25.3/7.el10,draft_93372/data/logs/ppc64le/installed_pkgs.log", + "vol/koji02/packages/golang/1.25.3/7.el10,draft_93372/data/logs/ppc64le/hw_info.log", + "vol/koji02/packages/golang/1.25.3/7.el10,draft_93372/data/logs/ppc64le/root.log", + "vol/koji02/packages/golang/1.25.3/7.el10,draft_93372/data/logs/ppc64le/dnf.log", + "vol/koji02/packages/golang/1.25.3/7.el10,draft_93372/data/logs/ppc64le/dnf.rpm.log", + "vol/koji02/packages/golang/1.25.3/7.el10,draft_93372/data/logs/ppc64le/state.log", + "vol/koji02/packages/golang/1.25.3/7.el10,draft_93372/data/logs/ppc64le/dnf.librepo.log", + "vol/koji02/packages/golang/1.25.3/7.el10,draft_93372/data/logs/ppc64le/noarch_rpmdiff.json", + ], + 's390x': [ + "vol/koji02/packages/golang/1.25.3/7.el10,draft_93372/data/logs/s390x/mock_output.log", + "vol/koji02/packages/golang/1.25.3/7.el10,draft_93372/data/logs/s390x/dnf.librepo.log", + "vol/koji02/packages/golang/1.25.3/7.el10,draft_93372/data/logs/s390x/installed_pkgs.log", + "vol/koji02/packages/golang/1.25.3/7.el10,draft_93372/data/logs/s390x/mock_config.log", + "vol/koji02/packages/golang/1.25.3/7.el10,draft_93372/data/logs/s390x/hw_info.log", + "vol/koji02/packages/golang/1.25.3/7.el10,draft_93372/data/logs/s390x/dnf.log", + "vol/koji02/packages/golang/1.25.3/7.el10,draft_93372/data/logs/s390x/dnf.rpm.log", + "vol/koji02/packages/golang/1.25.3/7.el10,draft_93372/data/logs/s390x/state.log", + "vol/koji02/packages/golang/1.25.3/7.el10,draft_93372/data/logs/s390x/build.log", + "vol/koji02/packages/golang/1.25.3/7.el10,draft_93372/data/logs/s390x/root.log", + "vol/koji02/packages/golang/1.25.3/7.el10,draft_93372/data/logs/s390x/noarch_rpmdiff.json", + ], + 'x86_64': [ + "vol/koji02/packages/golang/1.25.3/7.el10,draft_93372/data/logs/x86_64/dnf.log", + "vol/koji02/packages/golang/1.25.3/7.el10,draft_93372/data/logs/x86_64/dnf.rpm.log", + "vol/koji02/packages/golang/1.25.3/7.el10,draft_93372/data/logs/x86_64/state.log", + "vol/koji02/packages/golang/1.25.3/7.el10,draft_93372/data/logs/x86_64/hw_info.log", + "vol/koji02/packages/golang/1.25.3/7.el10,draft_93372/data/logs/x86_64/installed_pkgs.log", + "vol/koji02/packages/golang/1.25.3/7.el10,draft_93372/data/logs/x86_64/build.log", + "vol/koji02/packages/golang/1.25.3/7.el10,draft_93372/data/logs/x86_64/mock_output.log", + "vol/koji02/packages/golang/1.25.3/7.el10,draft_93372/data/logs/x86_64/dnf.librepo.log", + "vol/koji02/packages/golang/1.25.3/7.el10,draft_93372/data/logs/x86_64/mock_config.log", + "vol/koji02/packages/golang/1.25.3/7.el10,draft_93372/data/logs/x86_64/root.log", + "vol/koji02/packages/golang/1.25.3/7.el10,draft_93372/data/logs/x86_64/noarch_rpmdiff.json", + ] + } + + hashes = { + "6138160": { + "golang-docs-1.25.3-7.el10.noarch.rpm": "9f7fe1181c2fe2aaf5341df4aae68008b53947fdf26fed09244849a5a3c64dd5", + "golang-misc-1.25.3-7.el10.noarch.rpm": "b1d80aef9a7705af366c2b2a3178d8fac6dabc8b3569f1b9b65ccf719f903dbb", + "golang-src-1.25.3-7.el10.noarch.rpm": "1ed89a428dfbbd4f32e2782bb2b2d51f7aa2ff1950c82fc97d53b591b4ae8d52", + "golang-tests-1.25.3-7.el10.noarch.rpm": "11ca9a335851cf0ff197bb2f586de03617c30569497b196ec39c0a866f6629b3" + }, + "6138161": { + "golang-docs-1.25.3-7.el10.noarch.rpm": "9f7fe1181c2fe2aaf5341df4aae68008b53947fdf26fed09244849a5a3c64dd5", + "golang-misc-1.25.3-7.el10.noarch.rpm": "b1d80aef9a7705af366c2b2a3178d8fac6dabc8b3569f1b9b65ccf719f903dbb", + "golang-src-1.25.3-7.el10.noarch.rpm": "1ed89a428dfbbd4f32e2782bb2b2d51f7aa2ff1950c82fc97d53b591b4ae8d52", + "golang-tests-1.25.3-7.el10.noarch.rpm": "11ca9a335851cf0ff197bb2f586de03617c30569497b196ec39c0a866f6629b3" + }, + "6138162": { + "golang-docs-1.25.3-7.el10.noarch.rpm": "9f7fe1181c2fe2aaf5341df4aae68008b53947fdf26fed09244849a5a3c64dd5", + "golang-misc-1.25.3-7.el10.noarch.rpm": "b1d80aef9a7705af366c2b2a3178d8fac6dabc8b3569f1b9b65ccf719f903dbb", + "golang-src-1.25.3-7.el10.noarch.rpm": "1ed89a428dfbbd4f32e2782bb2b2d51f7aa2ff1950c82fc97d53b591b4ae8d52", + "golang-tests-1.25.3-7.el10.noarch.rpm": "11ca9a335851cf0ff197bb2f586de03617c30569497b196ec39c0a866f6629b3" + }, + "6138163": { # x86_64 differs + "golang-docs-1.25.3-7.el10.noarch.rpm": "9f7fe1181c2fe2aaf5341df4aae68008b53947fdf26fed09244849a5a3c64dd5", + "golang-misc-1.25.3-7.el10.noarch.rpm": "b1d80aef9a7705af366c2b2a3178d8fac6dabc8b3569f1b9b65ccf719f903dbb", + "golang-src-1.25.3-7.el10.noarch.rpm": "0f0e890f6128e9159e6409390915bd2f1d4c1917eda722c8ea2fc066a85718de", + "golang-tests-1.25.3-7.el10.noarch.rpm": "11ca9a335851cf0ff197bb2f586de03617c30569497b196ec39c0a866f6629b3" + }, + } + + rpms = [] + for x in results.values(): + for y in x['rpms']: + rpms.append(y) + + load_json.side_effect = [ + {'6138160': hashes['6138160']}, + {'6138161': hashes['6138161']}, + {'6138162': hashes['6138162']}, + {'6138163': hashes['6138163']}, + ] + + + # first two diffs (60/61, 60/62) hits the hash architectures + # third differs (because of inode changes), so rpmdiff is called 6138163 + # Anyway, in this case rpms are valid + diff_same = mock.MagicMock(name="diff_same") + diff_same.differs.return_value = False + + rpmdiff.return_value = diff_same + + # should pass without exceptions + kojihub.check_noarch_rpms(uploadpath, rpms, logs=logs) + + # called just one for non-matching hashes + rpmdiff.assert_called_once_with( + '/mnt/koji/work/tasks/8160/6138160/golang-src-1.25.3-7.el10.noarch.rpm', + '/mnt/koji/work/tasks/8163/6138163/golang-src-1.25.3-7.el10.noarch.rpm', + ignore='S5TN' + ) + self.assertEqual(diff_same.differs.call_count, 1) From 24407909c0db6c9673ecbf277738b5feea53c551 Mon Sep 17 00:00:00 2001 From: Mike McLean Date: Feb 19 2026 01:52:21 +0000 Subject: [PATCH 2/6] fix kojihash --- diff --git a/koji/rpmdiff.py b/koji/rpmdiff.py index d20cd37..d79c420 100644 --- a/koji/rpmdiff.py +++ b/koji/rpmdiff.py @@ -115,15 +115,23 @@ class Rpmdiff: for tag in self.PRCO: self.__comparePRCOs(old, new, tag) - # compare the files + # list of file indexes to compare + indexes = [ofs for (code, ofs) in self.__FILEIDX if code not in ignore] + # filter the data to only these indexes old_files_dict = self.__getFilesDict(old) new_files_dict = self.__getFilesDict(new) + old_files_filtered = {} + new_files_filtered = {} + for f in old_files_dict: + old_files_filtered[f] = dict([(k, old_files_dict[f][k]) for k in indexes]) + for f in new_files_dict: + new_files_filtered[f] = dict([(k, new_files_dict[f][k]) for k in indexes]) files = sorted(set(itertools.chain(six.iterkeys(old_files_dict), six.iterkeys(new_files_dict)))) - self.old_data['files'] = old_files_dict - self.new_data['files'] = new_files_dict + self.old_data['files'] = old_files_filtered + self.new_data['files'] = new_files_filtered for f in files: diff = 0 @@ -140,9 +148,6 @@ class Rpmdiff: for entry in self.__FILEIDX: # entry = [character, value] if entry[0] in ignore: - # erase fields which are ignored - old_file[entry[1]] = None - new_file[entry[1]] = None format = format + '.' elif old_file[entry[1]] != new_file[entry[1]]: format = format + entry[0] From 942deb4578accba2098a72574e94957796d9f3fc Mon Sep 17 00:00:00 2001 From: Mike McLean Date: Feb 19 2026 01:52:21 +0000 Subject: [PATCH 3/6] fix FILEIDX values --- diff --git a/koji/rpmdiff.py b/koji/rpmdiff.py index d79c420..ad9ef76 100644 --- a/koji/rpmdiff.py +++ b/koji/rpmdiff.py @@ -51,12 +51,13 @@ class Rpmdiff: # {fname : (size, mode, mtime, flags, dev, inode, # nlink, state, vflags, user, group, digest)} + # note: field 7 (state) is not considered __FILEIDX = [['S', 0], ['M', 1], ['5', 11], ['D', 4], - ['N', 6], - ['L', 7], + ['N', 5], + ['L', 6], ['V', 8], ['U', 9], ['G', 10], From 5b1d2b1c3fe821641ba3302ac35535b5072f36cf Mon Sep 17 00:00:00 2001 From: Mike McLean Date: Feb 19 2026 21:15:12 +0000 Subject: [PATCH 4/6] new unit tests for rpmdiff --- diff --git a/koji/rpmdiff.py b/koji/rpmdiff.py index ad9ef76..b7d92e8 100644 --- a/koji/rpmdiff.py +++ b/koji/rpmdiff.py @@ -94,8 +94,8 @@ class Rpmdiff: else: ignore = set(ignore) - old = self.__load_pkg(old) - new = self.__load_pkg(new) + old = self._load_pkg(old) + new = self._load_pkg(new) # Compare single tags for tag in self.TAGS: @@ -120,8 +120,8 @@ class Rpmdiff: indexes = [ofs for (code, ofs) in self.__FILEIDX if code not in ignore] # filter the data to only these indexes - old_files_dict = self.__getFilesDict(old) - new_files_dict = self.__getFilesDict(new) + old_files_dict = self._getFilesDict(old) + new_files_dict = self._getFilesDict(new) old_files_filtered = {} new_files_filtered = {} for f in old_files_dict: @@ -171,7 +171,7 @@ class Rpmdiff: self.result.append((format, data)) # load a package from a file or from the installed ones - def __load_pkg(self, filename): + def _load_pkg(self, filename): ts = rpm.ts() f = os.open(filename, os.O_RDONLY) hdr = ts.hdrFromFdno(f) @@ -231,7 +231,7 @@ class Rpmdiff: (self.ADDED, tagname, newentry[0], self.sense2str(newentry[1]), newentry[2])) - def __getFilesDict(self, hdr): + def _getFilesDict(self, hdr): if not hasattr(rpm, 'files'): # fall back to file iterator return self.__fileIteratorToDict(hdr.fiFromHeader()) diff --git a/tests/test_lib/test_rpmdiff.py b/tests/test_lib/test_rpmdiff.py new file mode 100644 index 0000000..74c7b99 --- /dev/null +++ b/tests/test_lib/test_rpmdiff.py @@ -0,0 +1,212 @@ +from unittest import mock +import shutil +import tempfile +import unittest +from collections import namedtuple + +import koji.rpmdiff +import rpm +import six + + +""" +These tests exercise the rpmdiff class in the library, They are different from +similarly named hub tests that are focused mainly on the hub code. +""" + + +class TestRPMDiff(unittest.TestCase): + + def setUp(self): + # mock the places in the class that read the rpm data + self.load_pkg = mock.patch.object(koji.rpmdiff.Rpmdiff, '_load_pkg').start() + self.getFilesDict = mock.patch.object(koji.rpmdiff.Rpmdiff, '_getFilesDict').start() + + # with the mocks, we shouldn't have file access, but we use temp paths just in case + self.tempdir = tempfile.mkdtemp() + self.oldfile = self.tempdir + '/old.rpm' + self.newfile = self.tempdir + '/new.rpm' + + # simple default data + self.oldhdr = make_fake_header() + self.newhdr = make_fake_header() # should be identical + self.load_pkg.side_effect = [self.oldhdr, self.newhdr] + self.oldfiles = make_fake_files(3) + self.newfiles = make_fake_files(3) # should be identical + self.getFilesDict.side_effect = [self.oldfiles, self.newfiles] + + def tearDown(self): + mock.patch.stopall() + shutil.rmtree(self.tempdir) + + def test_rpmdiff_simple(self): + # our baseline mock setup has identical files + d = koji.rpmdiff.Rpmdiff(self.oldfile, self.newfile) + self.assertFalse(d.differs()) + self.assertEqual(d.textdiff(), "") + self.assertEqual(d.kojihash(), d.kojihash(new=True)) + + def test_rpmdiff_changed_tag(self): + self.newhdr[rpm.RPMTAG_NAME] = 'different' + d = koji.rpmdiff.Rpmdiff(self.oldfile, self.newfile) + self.assertTrue(d.differs()) + self.assertEqual(d.textdiff(), 'S.5........ NAME') + self.assertNotEqual(d.kojihash(), d.kojihash(new=True)) + + def test_rpmdiff_dropped_tag(self): + self.newhdr[rpm.RPMTAG_URL] = None + d = koji.rpmdiff.Rpmdiff(self.oldfile, self.newfile) + self.assertTrue(d.differs()) + self.assertEqual(d.textdiff(), 'removed URL') + self.assertNotEqual(d.kojihash(), d.kojihash(new=True)) + + def test_rpmdiff_added_tag(self): + self.oldhdr[rpm.RPMTAG_URL] = None + d = koji.rpmdiff.Rpmdiff(self.oldfile, self.newfile) + self.assertTrue(d.differs()) + self.assertEqual(d.textdiff(), 'added URL') + self.assertNotEqual(d.kojihash(), d.kojihash(new=True)) + + def test_rpmdiff_dropped_files(self): + self.newfiles.clear() + d = koji.rpmdiff.Rpmdiff(self.oldfile, self.newfile) + self.assertTrue(d.differs()) + expect = '\n'.join(['removed /usr/test_file_%i' % i for i in range(3)]) + self.assertEqual(d.textdiff(), expect) + self.assertNotEqual(d.kojihash(), d.kojihash(new=True)) + + def test_rpmdiff_added_files(self): + self.oldfiles.clear() + d = koji.rpmdiff.Rpmdiff(self.oldfile, self.newfile) + self.assertTrue(d.differs()) + expect = '\n'.join(['added /usr/test_file_%i' % i for i in range(3)]) + self.assertEqual(d.textdiff(), expect) + self.assertNotEqual(d.kojihash(), d.kojihash(new=True)) + + def test_rpmdiff_changed_file(self): + fn = '/usr/test_file_1' + self.newfiles[fn] = self.newfiles[fn]._replace(size='different') + d = koji.rpmdiff.Rpmdiff(self.oldfile, self.newfile) + self.assertTrue(d.differs()) + expect = 'S.......... /usr/test_file_1' + self.assertEqual(d.textdiff(), expect) + self.assertNotEqual(d.kojihash(), d.kojihash(new=True)) + + def test_rpmdiff_ignore_changed_file(self): + fn = '/usr/test_file_1' + self.newfiles[fn] = self.newfiles[fn]._replace(size='different') + d = koji.rpmdiff.Rpmdiff(self.oldfile, self.newfile, ignore='S') + self.assertFalse(d.differs()) + self.assertEqual(d.textdiff(), '') + self.assertEqual(d.kojihash(), d.kojihash(new=True)) + + def test_rpmdiff_bytes(self): + # kojihash should be able to handle bytes values + fn = '/usr/test_file_1' + self.newfiles[fn] = self.newfiles[fn]._replace(digest=six.b('asdfjhgqkjhf')) + d = koji.rpmdiff.Rpmdiff(self.oldfile, self.newfile) + self.assertTrue(d.differs()) + expect = '..5........ /usr/test_file_1' + self.assertEqual(d.textdiff(), expect) + self.assertNotEqual(d.kojihash(), d.kojihash(new=True)) + + def test_rpmdiff_added_dep(self): + self.newhdr.update(make_prcos([{'name': 'foo'}])) + d = koji.rpmdiff.Rpmdiff(self.oldfile, self.newfile) + self.assertTrue(d.differs()) + expect = 'added PROVIDES foo >= 0.99.1' + self.assertEqual(d.textdiff(), expect) + self.assertNotEqual(d.kojihash(), d.kojihash(new=True)) + + def test_rpmdiff_dropped_dep(self): + self.oldhdr.update(make_prcos([{'name': 'foo'}])) + d = koji.rpmdiff.Rpmdiff(self.oldfile, self.newfile) + self.assertTrue(d.differs()) + expect = 'removed PROVIDES foo >= 0.99.1' + self.assertEqual(d.textdiff(), expect) + self.assertNotEqual(d.kojihash(), d.kojihash(new=True)) + + +def make_fake_header(prcos=None): + tags = [ + # should align with koji.rpmdiff.Rpmdiff.TAGS + 'name', + 'summary', + 'description', + 'group', + 'license', + 'url', + 'prein', + 'postin', + 'preun', + 'postun', + ] + extra_tags = [ + # not in TAGS, but accessed by the pcro code + 'version', + 'release', + ] + hdr = {} + for tag in tags + extra_tags: + code = getattr(rpm, "RPMTAG_" + tag.upper()) + hdr[code] = "fake_%s" % tag + # prco code expects lowercase string keys + hdr[tag] = "fake_%s" % tag + prcos = prcos or [] + hdr.update(make_prcos(prcos)) + return hdr + + +PRCO = namedtuple('PRCO', ['dtype', 'name', 'flag', 'version']) +depmap = { + '<': rpm.RPMSENSE_LESS, + '>': rpm.RPMSENSE_GREATER, + '=': rpm.RPMSENSE_EQUAL, + '<=': rpm.RPMSENSE_LESS | rpm.RPMSENSE_EQUAL, + '>=': rpm.RPMSENSE_GREATER | rpm.RPMSENSE_EQUAL, +} + + +def make_prcos(prcos): + # prcos should be a list of dicts + # all pcro fields have defaults + for dep in prcos: + dep.setdefault('dtype', 'provides') + dep.setdefault('name', 'somepackage') + dep.setdefault('version', '0.99.1') + dep.setdefault('flag', '>=') + if dep['flag'] in depmap: + dep['flag'] = depmap[dep['flag']] + tags = [ + 'provides', + 'requires', + 'conflicts', + 'obsoletes', + ] + data = {} + for tag in tags: + # the class uses string keys here + deps = [d for d in prcos if d['dtype'] == tag] + tag = tag.upper() + ftag = tag[:-1] + 'FLAGS' + vtag = tag[:-1] + 'VERSION' + data[tag] = [d['name'] for d in deps] + data[ftag] = [d['flag'] for d in deps] + data[vtag] = [d['version'] for d in deps] + return data + + +filekeys = ['size', 'mode', 'mtime', 'fflags', 'rdev', 'inode', 'nlink', + 'state', 'vflags', 'user', 'group', 'digest'] +fileinfo = namedtuple('fileinfo', filekeys) + + +def make_fake_files(count=0): + files = {} + for i in range(count): + name = '/usr/test_file_%i' % i + files[name] = fileinfo(*filekeys) + return files + + +# the end From bb07a1cc9e780214c113aa40b65870954862c8b2 Mon Sep 17 00:00:00 2001 From: Mike McLean Date: Feb 19 2026 21:15:31 +0000 Subject: [PATCH 5/6] fix old tests --- diff --git a/tests/test_hub/test_rpmdiff.py b/tests/test_hub/test_rpmdiff.py index 9e58fe7..d2ba2bf 100644 --- a/tests/test_hub/test_rpmdiff.py +++ b/tests/test_hub/test_rpmdiff.py @@ -89,8 +89,8 @@ class TestRPMDiff(unittest.TestCase): rpm1 = os.path.join(data_path, 'different_size_a.noarch.rpm') rpm2 = os.path.join(data_path, 'different_size_b.noarch.rpm') - hash1 = 'ed0bae957653692a7d2ff9d90dbc3eaf08486994af315e11a9e80929092f6d0e' - hash2 = '8cdd40b738ac156af09f31445831f324cedacd028bf1dc3e12cc48b2137ed190' + hash1 = '38e6838cfc14ae00265b0134ebb2e9c31f014b3e96b044173787e773e91c86bf' + hash2 = '8d55616a6ca4c270f9adf76eba0a352c26c6c91d016735d2f5a3b880ad21a0ed' for _ in range(2): # double check that kojihash is deterministic d = koji.rpmdiff.Rpmdiff(rpm1, rpm2) @@ -117,8 +117,8 @@ class TestRPMDiff(unittest.TestCase): attr[idx] = value rpm_dict_new = {'a_file': attr} - args[0]._Rpmdiff__getFilesDict = mock.MagicMock() - args[0]._Rpmdiff__getFilesDict.side_effect = [rpm_dict_old, rpm_dict_new] + args[0]._getFilesDict = mock.MagicMock() + args[0]._getFilesDict.side_effect = [rpm_dict_old, rpm_dict_new] orig_init(*args, **kwargs) # compare with every option @@ -143,10 +143,10 @@ class TestRPMDiff(unittest.TestCase): check_diff_result('D', 4, 4, "...D....... a_file") # case 6 inode different - check_diff_result('N', 6, 6, "....N...... a_file") + check_diff_result('N', 5, 5, "....N...... a_file") # case 7 number of links different - check_diff_result('L', 7, 7, ".....L..... a_file") + check_diff_result('L', 6, 6, ".....L..... a_file") # case 8 vflag different check_diff_result('V', 8, 8, "......V.... a_file") From 6c7a3ba80dcfcb375464707efc15e06d1d2759ab Mon Sep 17 00:00:00 2001 From: Mike McLean Date: Feb 27 2026 17:23:48 +0000 Subject: [PATCH 6/6] update index mapping to align with rpmlint --- diff --git a/koji/rpmdiff.py b/koji/rpmdiff.py index b7d92e8..5efdab4 100644 --- a/koji/rpmdiff.py +++ b/koji/rpmdiff.py @@ -51,13 +51,18 @@ class Rpmdiff: # {fname : (size, mode, mtime, flags, dev, inode, # nlink, state, vflags, user, group, digest)} - # note: field 7 (state) is not considered + # Notes: + # - field 5 (inode) is not considered for backwards compatibility + # - the N code maps to nlink, not inode + # - the L code maps to state, not nlink + # - field 7 (state) is not part of the rpm file, but is included for compatibility + # - see https://github.com/rpm-software-management/rpmlint/issues/1465 __FILEIDX = [['S', 0], ['M', 1], ['5', 11], ['D', 4], - ['N', 5], - ['L', 6], + ['N', 6], + ['L', 7], ['V', 8], ['U', 9], ['G', 10], diff --git a/tests/test_hub/test_rpmdiff.py b/tests/test_hub/test_rpmdiff.py index d2ba2bf..0ab662a 100644 --- a/tests/test_hub/test_rpmdiff.py +++ b/tests/test_hub/test_rpmdiff.py @@ -89,8 +89,8 @@ class TestRPMDiff(unittest.TestCase): rpm1 = os.path.join(data_path, 'different_size_a.noarch.rpm') rpm2 = os.path.join(data_path, 'different_size_b.noarch.rpm') - hash1 = '38e6838cfc14ae00265b0134ebb2e9c31f014b3e96b044173787e773e91c86bf' - hash2 = '8d55616a6ca4c270f9adf76eba0a352c26c6c91d016735d2f5a3b880ad21a0ed' + hash1 = '9ce5aa1c35d0b18bd666207349ca089bd96a974906ac32219c2536b42380c256' + hash2 = '08a9e7f1dd86fed2062cac7c69f271c5f0b1ea8fa41518d551e33fea709fdc18' for _ in range(2): # double check that kojihash is deterministic d = koji.rpmdiff.Rpmdiff(rpm1, rpm2) @@ -142,11 +142,11 @@ class TestRPMDiff(unittest.TestCase): # case 5 device different check_diff_result('D', 4, 4, "...D....... a_file") - # case 6 inode different - check_diff_result('N', 5, 5, "....N...... a_file") + # case 6 nlinks different + check_diff_result('N', 6, 6, "....N...... a_file") - # case 7 number of links different - check_diff_result('L', 6, 6, ".....L..... a_file") + # case 7 state different + check_diff_result('L', 7, 7, ".....L..... a_file") # case 8 vflag different check_diff_result('V', 8, 8, "......V.... a_file")