From 44e84d7dd9fa7f67d88cd6366612ac1a22fac3d4 Mon Sep 17 00:00:00 2001 From: Tomas Kopecek Date: Feb 17 2026 09:03:52 +0000 Subject: 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)