Run rpmdev-vercmp during tests and compare the return value to pkg_re…

[gordonmessmer]

…sources result.

[gordonmessmer]

I've added some tests to determine whether rpm results are faithful to the pkg_resources results in practice. This resulted in a minor change to handling ordered comparisons with prefix matching, whose behavior is undefined in the python standards.

For other mismatching behavior, the tests provide documentation of the cases where the conversions will differ, initially offering a reference for such differences and possibly a starting point for discussion and further changes if they are warranted.

[hroncok]

Awesome findings!

I'll read the new test file later, but the remaining changes make sense.

[hroncok]

The comparison tests are awesome. Thank you so much for this.

[hroncok]

A nit pick. I consider this way of writing it more common:

Suggested change

	(rpm_op, ver_b) = requirement.split(' ')[1:3]
	rpm_op, ver_b = requirement.split(' ')[1:3]

Feel free to disregard this.

[torsava]

I understand the changes reflect the actual behaviour of pkg_resources, but to me they're obviously wrong. Is this reported upstream?

I'm not sure we want to do it wrong just because pkg_resources do it wrong too :/

[hroncok]

I'm not here to judge what's wrong or not, but just a note: it's packaging, not pkg_resources, that does the thing. pkg_resources bundles packaging.

[gordonmessmer]

Python's behavior with regard to ordered comparison and prefix matching is definitely irrational in my opinion, but it's also undefined, so I don't know how much interest the maintainers will have in changing it.

My inclination was initially to translate toward something reasonable, but I changed my mind for a few reasons. First, because I haven't seen any of those undefined specifications in projects. Second, because if anyone is using those requirements, they're almost certainly looking at the results and getting the packages they expect. And third, because I think that minimizing differences between Requirements and rpm dependencies gives us fewer skipped test cases, which I hope also means we're more likely to be notified if the behavior of Python tools changes.

Rational code is better than irrational code, but tested code is usually best. :)

We can definitely submit an issue upstream with examples of some of the odd results and see where we get... If they'll adopt rational behavior, we can always update this again.

[torsava]

If the comparisons are undefined and also not used and also result in irrational behaviour, I personally would much rather not test that it indeed behaves irrationally :/

So I would definitely like this to be reported upstream. Do you have some additional examples? I'll try to find time to open an issue for it.

And in the meantime, if you really want to keep the irrationality-tests in there, I think it would be at least good to make it very explicit in the code that it is irrational and should not be used. Would that work?

[gordonmessmer]

I think the clear examples are:

('2.4.8', '<=', '2.4.8.*', False),
('2.4.8.0', '<=', '2.4.8.*', False),
('2.4.8.1', '<=', '2.4.8.*', False),
('2.4.8.post1', '<=', '2.4.8.*', False),

I think that all of those should match the '=', logically.

The other set of odd results is:

('2.4.8', '>', '2.4.8.*', True),
('2.4.8.0', '>', '2.4.8.*', True),
('2.4.8.1', '>', '2.4.8.*', True),
('2.4.8b5', '>', '2.4.8.*', True),
('2.4.8.post1', '>', '2.4.8.*', True),

Of those, 'b5' is probably the best example of something that shouldn't be > 2.4.8.*

if you really want to keep the irrationality-tests in there

To be clear, my previous response was intended only to clarify my thinking. I also started from the position that this code should do something reasonable, even if Python doesn't. I came around to the other point of view because I think this code is too niche and too far downstream to be a suitable place to make that argument.

If you and Miro want those results rolled back, I'm fine with that. This repo exists because encukou mentioned testing in rpm pull 951, and I hoped this would be helpful. :)

I think it would be at least good to make it very explicit in the code that it is irrational

Do you mean in code comments?

[hroncok]

If you and Miro want those results rolled back

I don't. I agree that:

1. The generators should match upstream behavior as much as possible. If we can test it, we can fix it.
2. If the behavior is irrational, we should report it.

I think we shouldn't follow the "If the upstream behavior is irrational, we shall do the correct thing" idea. It is a slippery slope.

[torsava]

Do you mean in code comments?

Yeah. When somebody reads the tests they'll be very confused as to what the hell's happening.

[torsava]

I think we shouldn't follow the "If the upstream behavior is irrational, we shall do the correct thing" idea. It is a slippery slope.

Good point, that's not this project's scope. And we need the tests to know when upstream fixes it, if ever.

[torsava]

Thanks for the examples, @gordonmessmer . I filed the bug here: pypa/packaging#320