Add more implementation for the version comparisons

Author: aignas

python/private/py_wheel_normalize_pep440.bzl

@@ -553,11 +553,10 @@ def _version_eq(left, right):
         ##and left.local == right.local
     )
 
-# TODO @aignas 2025-05-04: add tests for the comparison
 def _version_lt(left, right):
     if left.epoch > right.epoch:
         return False
-    if left.epoch < right.epoch:
+    elif left.epoch < right.epoch:
         return True
 
     release_len = max(len(left.release), len(right.release))
@@ -569,14 +568,22 @@ def _version_lt(left, right):
     elif left_release < right_release:
         return True
 
-    return (
-        left.pre < right.pre and
-        left.post < right.post and
-        left.dev < right.dev and
-        left.local < right.local
-    )
+    # the release is equal, check for pre version
+    if right.pre:
+        if left.pre != None:
+            # PEP440: The exclusive ordered comparison <V MUST NOT allow a pre-release of
+            # the specified version unless the specified version is itself a pre-release.
+            return False
+
+        if left.pre > right.pre:
+            return False
+        elif left.pre < right.pre:
+            return True
+    elif left.pre:
+        return True
+
+    return False
 
-# TODO @aignas 2025-05-04: add tests for the comparison
 def _version_gt(left, right):
     if left.epoch > right.epoch:
         return True
@@ -603,19 +610,39 @@ def _version_gt(left, right):
         elif left.post < right.post:
             return False
 
+    if right.pre:
+        return True
+
     return False
 
 def _new_version(*, epoch = 0, release, pre = "", post = "", dev = "", local = "", is_prefix = False, norm):
     epoch = epoch or 0
     _release = tuple([int(d) for d in release.split(".")])
-    pre = pre or ""
+
+    if pre:
+        if pre.startswith("rc"):
+            prefix = "rc"
+        else:
+            prefix = pre[0]
+
+        pre = (prefix, int(pre[len(prefix):]))
+    else:
+        pre = None
+
     if post:
         if not post.startswith(".post"):
             fail("post release identifier must start with '.post', got: {}".format(post))
         post = int(post[len(".post"):])
     else:
         post = None
-    dev = dev or ""
+
+    if dev:
+        if not dev.startswith(".dev"):
+            fail("dev release identifier must start with '.dev', got: {}".format(dev))
+        dev = int(dev[len(".dev"):])
+    else:
+        dev = None
+
     local = local or ""
 
     self = struct(
@@ -634,11 +661,20 @@ def _new_version(*, epoch = 0, release, pre = "", post = "", dev = "", local = "
         gt = lambda x: _version_gt(self, x),  # buildifier: disable=uninitialized
         le = lambda x: not _version_gt(self, x),  # buildifier: disable=uninitialized
         ge = lambda x: not _version_lt(self, x),  # buildifier: disable=uninitialized
+        str = lambda: norm,
+        key = lambda: (
+            epoch,
+            release,
+            pre or ("release",),
+            post if post != None else -1,
+            dev if dev != None else -1,
+            local,
+        ),
     )
 
     return self
 
-def parse_version(version):
+def parse_version(version, strict = False):
     """Parse a PEP4408 compliant version
 
     TODO: finish
@@ -648,13 +684,14 @@ def parse_version(version):
 
     Args:
       version: version string to be normalized according to PEP 440.
+      strict: fail if the version is invalid.
 
     Returns:
       string containing the normalized version.
     """
 
-    parser = _new(version.strip(" .*"))  # PEP 440: Leading and Trailing Whitespace and .*
-    parser_2 = _new(version.strip(" .*"))  # PEP 440: Leading and Trailing Whitespace and .*
+    parser = _new(version.strip(" " if strict else " .*"))  # PEP 440: Leading and Trailing Whitespace and .*
+    parser_2 = _new(version.strip(" " if strict else " .*"))  # PEP 440: Leading and Trailing Whitespace and .*
     accept(parser, _is("v"), "")  # PEP 440: Preceding v character
     accept(parser_2, _is("v"), "")  # PEP 440: Preceding v character
 
@@ -677,12 +714,19 @@ def parse_version(version):
     is_prefix = version.endswith(".*")
     parts["is_prefix"] = is_prefix
     if is_prefix and (parts["local"] or parts["post"] or parts["dev"] or parts["pre"]):
-        # local version part has been obtained, but only public segments can have prefix
-        # matches. Just return None.
+        if strict:
+            fail("local version part has been obtained, but only public segments can have prefix matches")
+
         # https://peps.python.org/pep-0440/#public-version-identifiers
         return None
 
-    if parser.input[parser.context()["start"]:]:
+    if parser_2.input[parser.context()["start"]:]:
+        if strict:
+            fail(
+                "Failed to parse PEP 440 version identifier '%s'." % parser.input,
+                "Parse error at '%s'" % parser.input[parser.context()["start"]:],
+            )
+
         # If we fail to parse the version return None
         return None
 

python/private/pypi/pep508_evaluate.bzl

@@ -18,6 +18,9 @@
 load("//python/private:enum.bzl", "enum")
 load("//python/private:py_wheel_normalize_pep440.bzl", "parse_version")
 
+# TODO @aignas 2025-05-06: this is exposed for tests only
+version = parse_version
+
 # The expression parsing and resolution for the PEP508 is below
 #
 

tests/pypi/pep508/evaluate_tests.bzl

@@ -15,7 +15,7 @@
 
 load("@rules_testing//lib:test_suite.bzl", "test_suite")
 load("//python/private/pypi:pep508_env.bzl", pep508_env = "env")  # buildifier: disable=bzl-visibility
-load("//python/private/pypi:pep508_evaluate.bzl", "evaluate", "tokenize")  # buildifier: disable=bzl-visibility
+load("//python/private/pypi:pep508_evaluate.bzl", "evaluate", "tokenize", "version")  # buildifier: disable=bzl-visibility
 
 _tests = []
 
@@ -283,7 +283,15 @@ _MISC_EXPRESSIONS = [
     _expr_case('python_version > "1.7.post2"', True, {"python_version": "1.7.post3"}),
     _expr_case('python_version > "1.7.post2"', False, {"python_version": "1.7.0"}),
     _expr_case('python_version > "1.7.1+local"', False, {"python_version": "1.7.1"}),
-    # TODO @aignas 2025-05-05: add tests for pre-releases
+    _expr_case('python_version > "1.7.1+local"', True, {"python_version": "1.7.2"}),
+    _expr_case('python_version < "1.7.1"', False, {"python_version": "1.7.2"}),
+    _expr_case('python_version < "1.7.3"', True, {"python_version": "1.7.2"}),
+    _expr_case('python_version < "1.7.1"', True, {"python_version": "1.7"}),
+    _expr_case('python_version < "1.7.1-rc2"', True, {"python_version": "1.7"}),
+    _expr_case('python_version < "1.7-rc2"', False, {"python_version": "1.7"}),
+    _expr_case('python_version < "1.7-rc2"', True, {"python_version": "1.7-rc1"}),
+    _expr_case('python_version < "1.7-rc2"', False, {"python_version": "1.7-rc3"}),
+    _expr_case('python_version < "1.7-rc12"', True, {"python_version": "1.7-rc3"}),
 ]
 
 def _misc_expressions(env):
@@ -292,6 +300,60 @@ def _misc_expressions(env):
 
 _tests.append(_misc_expressions)
 
+def _test_ordering(env):
+    want = [
+        # The items are sorted from lowest to highest version
+        "0.0.1",
+        "0.1.0-rc",
+        "0.1.0",
+        "0.9.11",
+        "0.9.12.dev",
+        "0.9.12",
+        "1.0.0-alpha",
+        "1.0.0-alpha1",
+        "1.0.0-beta",
+        "1.0.0-beta.dev",
+        "1.0.0-beta2",
+        "1.0.0-beta11",
+        "1.0.0-rc1",
+        "1.0.0-rc2.dev",
+        "1.0.0-rc2",
+        "1.0.0",
+        # Also handle missing minor and patch version strings
+        "2.0",
+        "3",
+    ]
+
+    for lower, higher in zip(want[:-1], want[1:]):
+        lower = version(lower, strict = True)
+        higher = version(higher, strict = True)
+
+        if not lower.key() < higher.key():
+            env.fail("Expected '{}'.key() to be smaller than '{}'.key(), but got otherwise".format(
+                lower.str(),
+                higher.str(),
+            ))
+
+        if not lower.lt(higher):
+            env.fail("Expected '{}' to be smaller than '{}', but got otherwise".format(
+                lower.str(),
+                higher.str(),
+            ))
+
+        if not (higher.key() > lower.key()):
+            env.fail("Expected '{}'.key() to be greater than '{}'.key(), but got otherwise".format(
+                higher.str(),
+                lower.str(),
+            ))
+
+        if not (higher.gt(lower)):
+            env.fail("Expected '{}' to be greater than '{}', but got otherwise".format(
+                higher.str(),
+                lower.str(),
+            ))
+
+_tests.append(_test_ordering)
+
 def evaluate_test_suite(name):  # buildifier: disable=function-docstring
     test_suite(
         name = name,