@mikem I've added some 'proof-of-concept' for caching. It is not merge-ready. If we go this way, I need to move rpmdiff to koji frm hub, so it is accessible from hub and also builders. Any suggestions?

I'm fine with moving rpmdiff around. We should make it a lib to avoid the import workaround.

I'm concerned about the hash though. I don't think that the checksum of a sorted json dump is going to capture the checks that rpmdiff is currently doing. Should work fine for tags and PRCO, but I'm less sure about the files. Also, we need to be sure the sorting is done deeply.

Not a huge fan of having to compare the rpm with itself to get the hash, but I guess we'd have to refactor a lot more to avoid that.

Since this calculation is happening in the buildArch task, I'm somewhat concerned that different builders with different rpm versions could report different hashes for the same rpm. We might need to take extra care in rpmdiff to insulate against this

PR title is misleading. This is a lot more than just replacing an exec with a lib call

• I've moved rpmdiff to lib
• Fixed typo
• Any insight, what should be wrong for files? It is a dictionary, whose keys are sorted. All values are derived from original files, so they are not affected e.g. by environment variables. When I run it on some real-life rpms (texlive), it seems, that it is working pretty well.
• If there is a different hash produced, comparison will happen with original method. So, I think it is relatively safe to use it. What I would be afraid is if I am producing same hashes (e.g. empty json, or some error message) for different rpms. But I see it as unlikely for now.
• Is there any possibility of more deeply nested structures? AFAIK, all structures are simple lists of tuples, except files, which is dictionary with tuples values sorted by dumps.
• We can refactor Rpmdiff class more, so we don't need to open same file twice, but that's probably sustainable downside, if everything else will work.

The hash compare does not match the result in some cases. For testing, I added a simple assertion in main() to check that they match:

https://github.com/mikem23/koji-playground/commits/pagure/pr/774

It is possible to construct situations where they do not. E.g.

$ python koji/rpmdiff.py -i T /home/mike/Devel/fake/noarch/fake-1.1-25.noarch.rpm /tmp/fake-1.1-25.noarch.rpm 
Traceback (most recent call last):
  File "koji/rpmdiff.py", line 271, in <module>
    main()
  File "koji/rpmdiff.py", line 267, in main
    raise Exception('hash compare disagrees with rpmdiff')
Exception: hash compare disagrees with rpmdiff

In this case, it is because the old and new data dictionaries do not omit the ignored fields. There are other places where the rpmdiff compare is not a straightforward compare. There is also the bit where it filters out the self provide.

I haven't quite yet convinced myself that there are not others.

Also, it looks like we won't need main() anymore. Maybe it would be useful for testing (as it just was) or as a utility, but it doesn't seem like it belongs in a lib.

I've added your commit and fixed tests. To main - what about removing it from lib and put rpmdiff script to koji-tools? I was thinking about making it part of koji-utils rpm, but it doesn't fit there much as those are only services now. As it is not used by base koji, koji-tools would make sense, not?

I've added your commit and fixed tests.

Ok, but we still need to fix the underlying issue with the hash compare

To main - what about removing it from lib and put rpmdiff script to koji-tools? I was thinking about making it part of koji-utils rpm, but it doesn't fit there much as those are only services now. As it is not used by base koji, koji-tools would make sense, not?

Yeah, it's sort of an odd thing. Really only useful for debugging. I guess koji-tools will work.

koji-tools move: https://pagure.io/koji-tools/pull-request/6

Is there a part missing? You have:

rpmdiff_hash[self.id][rpmf] = d.kojihash(ignore='S5TN')

but, the kojihash method does not take such an argument. The __init__ method does, but it still doesn't look like kojihash is honoring it.

I'm also concerned about the bit where it filters out the self provide, though I don't have a test case the checks that yet.

It is a typo there (kojihash() is correct). Ingore is put to data dictionary, so it is reflected in hashed json later. kojihash itself doesn't need to use it explicitly.

The -i option is still not honored correctly for the hash approach.

[mike@localhost koji-tools]$ PYTHONPATH=~/Devel/koji/koji ./src/bin/koji-rpmdiff  /home/mike/Devel/fake/noarch/fake-1.1-24.noarch.rpm /tmp/fake-1.1-24.noarch.rpm 
..........T /usr/bin/nosuchfile
[mike@localhost koji-tools]$ PYTHONPATH=~/Devel/koji/koji ./src/bin/koji-rpmdiff -i T  /home/mike/Devel/fake/noarch/fake-1.1-24.noarch.rpm /tmp/fake-1.1-24.noarch.rpm 
Traceback (most recent call last):
  File "./src/bin/koji-rpmdiff", line 64, in <module>
    main()
  File "./src/bin/koji-rpmdiff", line 60, in main
    raise Exception('hash compare disagrees with rpmdiff')
Exception: hash compare disagrees with rpmdiff

All I did to construct these two rpms was to build the same test spec twice.

I've forgot to erase those ignored fields before hashing. Updated.

There's still the self provide bit that we do not match. I can hit this by comparing two rpms (with null epoch) that differ only by release. E.g.

[mike@localhost koji-tools]$ PYTHONPATH=~/Devel/koji/koji ./src/bin/koji-rpmdiff -i T /home/mike/Devel/fake/noarch/fake-1.1-25.noarch.rpm /tmp/fake-1.1-24.noarch.rpm 
Traceback (most recent call last):
  File "./src/bin/koji-rpmdiff", line 64, in <module>
    main()
  File "./src/bin/koji-rpmdiff", line 60, in main
    raise Exception('hash compare disagrees with rpmdiff')
Exception: hash compare disagrees with rpmdiff

This is easy enough to fix (just move the prco data assignment until after the block that filters out the self requires). However, that filter seems wrong and I'm really not sure why it is there.

I guess this bit is probably there because the original rpmlint tool was probably used to compare one version against the next. It doesn't care of the e:vr changes either. This is a case where the e:vr is embedded in a field that we do care about.

For our case, we don't expect the e:vr to change.

So tl;dr - either we account for the filtering in the hash, or we remove the filtering. I'm inclined to err on the side of caution and take the first path now, but look into the second for next release.

https://github.com/mikem23/koji-playground/commits/pagure/pr/774

:thumbsup:

Commit 7b559a0f fixes this pull-request