Add tests for and fix RPM version sorting by dralley · Pull Request #4146 · pulp/pulp_rpm

dralley · 2025-09-22T14:55:25Z

No description provided.

dralley · 2025-09-23T04:10:01Z

As it turns out, the Python version was not in fact as correct or complete as I was thinking it was. So, this is an overhaul on the algorithm to pass the test suite as ported from my Rust implementation.

This exercise and refresher on the complexities of the algorithm make me heavily suspect that a correct SQL-based implementation is going to be near impossible.

dralley · 2025-09-23T13:21:43Z

+        self.assertEqual(sorted(epochpkg_ages), [1, 2])
+
+    @skip("The implementation of package age is broken w/r/t '^' and '~' characters")
+    def test_age_with_complex_versions(self):


Test failed successfully https://github.com/pulp/pulp_rpm/actions/runs/17947265654/job/51036889972?pr=4146#step:14:3451

dralley · 2025-09-23T13:36:28Z

I intend this to backport this to all the supported branches. But I have to do it manually because there's multiple commits and I'd rather not squash them.

mdellweg · 2025-09-23T14:03:32Z

+
+
+# internal use: each individual component of the EVR is compared using this function
+def compare_version_strings(first, second):


You could make it more obvious that it's only for internal use:

Suggested change

def compare_version_strings(first, second):

def _compare_version_strings(first, second):

pedro-psb · 2025-09-23T17:58:10Z

+Unit tests for RPM version comparison functionality.
+
+These tests are designed to match the behavior tested in the Rust implementation
+to ensure compatibility between different RPM version comparison implementations.


Can you add a link to the Rust implementation you are refering to?
I know its yours, but someone / sometime later that may not be clear.

I'll just remove the comment and maybe add a direct link

pedro-psb · 2025-09-23T20:00:13Z

+        assert from_evr(":0-") == ("", "0", "")
+        assert from_evr("0:") == ("0", "", "")
+        assert from_evr("asdf:") == ("asdf", "", "")
+        assert from_evr("~:") == ("~", "", "")


A bit of a tangent for the purpose of this PR (and without thinking much on the consequences), I would argue that most of these are obviously invalid and should raise errors.

For what it's worth this is only exercised during tests so precisely how it behaves doesn't matter much. Strictly speaking we can entirely delete these tests and the from_evr function, it's just helpful to use in the tests

pedro-psb · 2025-09-23T20:05:46Z

+        # compare versions with tilde parsing as older
+        evr1 = RpmVersion.from_string("~3:12.2.3-45")
+        evr2 = RpmVersion.from_string("0:1.2.3-45")
+        assert evr1 < evr2


From the fedora docs I thought the tilde could only be used in the version part.
Wild.

Nothing strictly speaking prevents it. It's a very bad idea though

pedro-psb · 2025-09-23T20:14:22Z

+        """Test some version comparison behavior that is a bit non-intuitive."""
+        # (but needs to be maintained for compatibility)
+        assert compare_version_strings("1e.fc33", "1.fc33") < 0
+        assert compare_version_strings("1g.fc33", "1.fc33") > 0


pedro-psb · 2025-09-23T20:38:55Z

+            # both have caret, strip and continue
+            version1_part = version1_stripped
+            version2_part = version2_stripped
+            continue


Since you are commenting # first < second here, I'd suggest to extract the numbers -1,0,1 to constants such as "FIRST_GREATER", etc, or anything like that.

And can you remove comments 206, 212 and 218? The condition is pretty self explanatory.

pedro-psb · 2025-09-23T20:46:36Z


-def vercmp(first, second):
-    return Vercmp.compare(first, second)
+    # Should not reach here due to the checks above, but just in case


If it really should not reach here why return 0?
Wdyt of raising an error?

pedro-psb · 2025-09-23T20:56:59Z

The skipped ones will be fixed in this other PR, right?

pedro-psb · 2025-09-23T21:02:55Z

Not saying we should do that here, but it ocurred to me:

If we have reference implementations of this (e.g, rpmdev-vercmp and/or your rust one), we could just feed both with a set of inputs and assert this implementation output matches the reference one. The advantage is the we don't have to duplicate the source of truth for the specification correctness here.

dralley · 2025-09-24T02:00:45Z

The test suite is essentially copied over from my Rust implementation (literally the same test strings), which in turn is more or less adapted (not exactly copied but it's hitting the same cases) from the upstream tests in rpm. So that's more or less already the case.

I agree with you that it would be nice to get rid of this implementation entirely though.

pedro-psb

Thanks for the clarifications.
Guess you said it better, it would be better to not even have the implementation duplicated here. But if we have to, we are in good shape here.

github-actions Bot added no-changelog no-issue labels Sep 22, 2025

dralley force-pushed the rpmver-tests branch from 045185b to 05adc6a Compare September 22, 2025 15:00

github-actions Bot added the multi-commit Added when a PR consists of more than one commit label Sep 22, 2025

dralley force-pushed the rpmver-tests branch 2 times, most recently from ff88096 to 0d8881a Compare September 23, 2025 02:09

dralley marked this pull request as ready for review September 23, 2025 04:08

dralley force-pushed the rpmver-tests branch 8 times, most recently from 85dcb86 to 4e1ff56 Compare September 23, 2025 05:30

Add tests for RPM version sorting

63ff18f

dralley force-pushed the rpmver-tests branch 2 times, most recently from 3e8c3f9 to dc4d08e Compare September 23, 2025 13:20

dralley commented Sep 23, 2025

View reviewed changes

dralley changed the title ~~Add tests for RPM version sorting~~ Fix and add tests for RPM version sorting Sep 23, 2025

dralley changed the title ~~Fix and add tests for RPM version sorting~~ Add tests for and fix RPM version sorting Sep 23, 2025

mdellweg reviewed Sep 23, 2025

View reviewed changes

dralley force-pushed the rpmver-tests branch 2 times, most recently from e713f81 to ff1f960 Compare September 23, 2025 17:05

Change implementation of Python version sort

fe5a30e

dralley force-pushed the rpmver-tests branch from ff1f960 to 0522756 Compare September 23, 2025 17:32

pedro-psb requested changes Sep 23, 2025

View reviewed changes

Add unit tests for Package.objects.with_age()

e336ee5

dralley force-pushed the rpmver-tests branch from 0522756 to e336ee5 Compare September 24, 2025 05:27

pedro-psb approved these changes Sep 24, 2025

View reviewed changes

dralley merged commit 5e6f3e8 into pulp:main Sep 24, 2025
25 of 27 checks passed



		# internal use: each individual component of the EVR is compared using this function
		def compare_version_strings(first, second):

	def compare_version_strings(first, second):
	def _compare_version_strings(first, second):

Conversation

dralley commented Sep 22, 2025

Uh oh!

dralley commented Sep 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dralley commented Sep 23, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pedro-psb commented Sep 23, 2025

Uh oh!

dralley commented Sep 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pedro-psb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

dralley commented Sep 23, 2025 •

edited

Loading

dralley commented Sep 24, 2025 •

edited

Loading