PR#3562: rpmdiff: replace deprecated rpm call - koji

		`@@ -117,8 +117,9 @@`

		`# compare the files`

		`- old_files_dict = self.__fileIteratorToDict(old.fiFromHeader())`
		`- new_files_dict = self.__fileIteratorToDict(new.fiFromHeader())`
		`+ old_files_dict = self.__getFilesDict(old)`
		`+ new_files_dict = self.__getFilesDict(new)`
		`+`
		`files = sorted(set(itertools.chain(six.iterkeys(old_files_dict),`
		`six.iterkeys(new_files_dict))))`
		`self.old_data['files'] = old_files_dict`
		`@@ -224,9 +225,37 @@`
		`(self.ADDED, tagname, newentry[0],`
		`self.sense2str(newentry[1]), newentry[2]))`

		`+ def __getFilesDict(self, hdr):`
		`+ if not hasattr(rpm, 'files'):`
		`+ # fall back to file iterator`
		`+ return self.__fileIteratorToDict(hdr.fiFromHeader())`
		`+ result = {}`
		`+ for file in rpm.files(hdr):`
		`+ # mimic old fi order because comparison and kojihash relies on it`
		`+ # (FN, FSize, FMode, FMtime, FFlags, FRdev, FInode, FNlink, FState, VFlags, FUser,`
		`+ # FGroup, Digest)`
		`+ filedata = [`
		`+ # name omitted`
		`+ file.size,`
		`+ file.mode,`
		`+ file.mtime,`
		`+ file.fflags,`
		`+ file.rdev,`
		`+ file.inode,`
		`+ file.nlink,`
		`+ file.state,`
		`+ file.vflags,`
		`+ file.user,`
		`+ file.group,`
		`+ file.digest,`
		`+ ]`
		`+ result[file.name] = filedata`
		`+ return result`
		`+`
		`def __fileIteratorToDict(self, fi):`
		`result = {}`
		`for filedata in fi:`
		`+ # index by name`
		`result[filedata[0]] = list(filedata[1:])`
		`return result`

tests/test_hub/test_rpmdiff.py

file modified

+17 -2

		`@@ -81,6 +81,21 @@`
		`d = koji.rpmdiff.Rpmdiff(rpm1, rpm2, ignore='S5TN')`
		`self.assertEqual(d.textdiff(), '')`

		`+ def test_rpmdiff_kojihash(self):`
		`+ data_path = os.path.abspath("tests/test_hub/data/rpms")`
		`+`
		`+ # the only differences between rpm1 and rpm2 are 1) create time 2) file name`
		`+ 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'`
		`+ for _ in range(2):`
		`+ # double check that kojihash is deterministic`
		`+ d = koji.rpmdiff.Rpmdiff(rpm1, rpm2)`
		`+ self.assertEqual(d.kojihash(), hash1)`
		`+ self.assertEqual(d.kojihash(new=True), hash2)`
		`+`
		`def test_rpmdiff_ignore_test(self):`
		`data_path = os.path.abspath("tests/test_hub/data/rpms")`

		`@@ -101,8 +116,8 @@`
		`attr[idx] = value`
		`rpm_dict_new = {'a_file': attr}`

		`- args[0]._Rpmdiff__fileIteratorToDict = mock.MagicMock()`
		`- args[0]._Rpmdiff__fileIteratorToDict.side_effect = [rpm_dict_old, rpm_dict_new]`
		`+ args[0]._Rpmdiff__getFilesDict = mock.MagicMock()`
		`+ args[0]._Rpmdiff__getFilesDict.side_effect = [rpm_dict_old, rpm_dict_new]`
		`orig_init(args, *kwargs)`

		`# compare with every option`

tkopecek commented 2 years ago

mikem commented 2 years ago

It seems strange to keep __fileIteratorToDict when we're not dealing with file iterators. This private function is only used in this one spot. I suggest we replace it with a new function that accepts the rpm header and returns the dict that we expect.

mikem commented 2 years ago

Well hmm.

I mocked up my idea here. This keeps the special case isolated in one spot. Sort of. The trouble is that we still have polymorphism in the files dict values: a list of file data vs an rpm.file object.

It looks like this code is going to fail when it tries to compare two file objects, as the FILEIDX values are no longer relevant.

I think the proper fix is going to be a little more involved. Most likely rewrite all the compare code around the new rpm.file interface and write some backwards compatible code to simulate that interface.

mikem commented 2 years ago

I was curious what rpmlint had done about this and I see they haven't updated these calls.

How pressing is this?

tkopecek commented a year ago

On F37 it throws additional warnings but I've not found if rpm is planning to drop the support soon. (Moved to next release at least)

rebased onto 410193f792b72762aefb1379bfd841cb518fa7a4

a year ago

mikem commented a year ago

As I alluded above, the rpm.file interface is not compatible with the compare code we have in rpmdiff.py. If I try to use this class to compare an rpm with itself, I get the following traceback.

$ PYTHONPATH=~/Devel/koji/koji python3 koji-rpmdiff.py /home/mikem/kubevirt-virtctl-4.13.0-918.el9.x86_64.rpm /home/mikem/kubevirt-virtctl-4.13.0-918.el9.x86_64.rpm
Traceback (most recent call last):
  File "/home/mikem/Devel/scripts/koji-rpmdiff.py", line 7, in <module>
    d = koji.rpmdiff.Rpmdiff(sys.argv[1], sys.argv[2], ignore='S5TN')
  File "/home/mikem/Devel/koji/koji/koji/rpmdiff.py", line 144, in __init__
    old_file[entry[1]] = None
TypeError: 'rpm.file' object does not support item assignment

Trivial test script:

$ cat koji-rpmdiff.py
#!/usr/bin/python3
import sys
import koji.rpmdiff
d = koji.rpmdiff.Rpmdiff(sys.argv[1], sys.argv[2], ignore='S5TN')
print(f'Differs: {d.differs()}')

mikem commented a year ago

If we are embracing the new way, I think it makes the most sense to rewrite the checks around the rpm.file interface and provide a backwards compatible layer for older rpm libs.

mikem commented a year ago

Also, looks like rpm has already dropped this upstream (last April)

commit 742be88cd97d95bae356a93f17ec0595b610cee7
Author: Panu Matilainen <pmatilai@redhat.com>
Date:   Fri Apr 8 12:18:33 2022 +0300

    Drop the klunky and ugly rpm.fi python binding finally

    Update our lone test-case to use rpm.files instead

mikem commented a year ago

If we are embracing the new way, I think it makes the most sense to rewrite the checks around the rpm.file interface and provide a backwards compatible layer for older rpm libs.

Hmm we may want to preserve the list interface so that kojihash will continue to work correctly.

Edited a year ago by mikem

mikem commented a year ago

Also, it would be good to add a unit test for this code

mikem commented a year ago

Possible update on same branch

rebased onto 20b5789

a year ago

4 new commits added