From 10faf65d675b2e180495829ede261f32c5e4bd7a Mon Sep 17 00:00:00 2001 From: Harshit Date: Sat, 29 Aug 2026 02:10:54 +0530 Subject: [PATCH] fix(packages): stop a non-numeric version taking down registry lookup PackageRecipe.validate() accepts any non-empty version string, but the registry ordered versions with sorted(versions, key=lambda v: [int(x) for x in v.split('.')]) which raises ValueError for anything that is not dotted integers. Real recipes are full of those: a leading v (littlefs and FreeRTOS both publish their tags that way -- the recipe example in the book uses `tag: V10.5.1`), pre-release tags like 3.6.0-rc1, and build metadata like 1.3.1+patch2. The blast radius is wider than the odd package itself. The key was duplicated across get(), list_packages() and list_all_versions(), and * get(name) with no version scans every version of that package, so one such recipe breaks that package entirely; * list_packages() scans every package, and PackageResolver builds its "package not found in registry. Available: ..." message from it -- so a single unusual recipe anywhere in the registry turns an ordinary missing-package error into a ValueError traceback. Replace the three copies with one version_sort_key(). Ordering: a leading v/V is ignored; all-digit components compare numerically so 1.10.0 still sorts above 1.9.0; any other component compares as text and ranks below a numeric one; a -/+ suffix ranks below the same version without one, so 3.6.0-rc1 < 3.6.0. The order is total and never raises, which is the property that matters here -- one unusual recipe must not decide whether lookup works for the packages around it. Deliberately not a full PEP 440 / semver implementation. That would mean a dependency or a lot more code for a comparison the recipe format does not specify; this defines the rules it does need and documents them. Tests: tests/ebuild/test_package_registry.py grows from 1 case to 12, covering v-prefixes, pre-releases, build metadata, date-stamped and non-numeric versions, one odd version among good ones, and a totality check over the key. Nine of the eleven behavioural cases fail against the unfixed registry.py (checked by running them against it); the other two are the pre-existing numeric-order test and "2024.06", which parsed as ints before. Docs: the ordering rules are now in docs/book/book.md section 12.5. Note on CI: master currently fails for reasons unrelated to this change -- ebuild/build/dispatch.py has a duplicated `else:` from a merge, so the module does not parse (PRs #65/#66 address it), and ninja_backend.py is missing _object_path. This branch leaves the failure set exactly as it found it: 26 failed / 162 passed before and after, identical lists. Verified: pytest tests/ebuild/test_package_registry.py -> 12 passed. Verified: ruff check --select=E,F,W --ignore=E501 and mypy --ignore-missing-imports on both changed files -> clean. --- docs/book/book.md | 17 +++++ ebuild/packages/registry.py | 69 ++++++++++++++++---- tests/ebuild/test_package_registry.py | 92 ++++++++++++++++++++++++++- 3 files changed, 165 insertions(+), 13 deletions(-) diff --git a/docs/book/book.md b/docs/book/book.md index e03618cd..4f4251f6 100644 --- a/docs/book/book.md +++ b/docs/book/book.md @@ -935,6 +935,23 @@ Request: freertos >= 10.5, mbedtls ^3.0 +-------------------+ ``` +#### Version ordering + +A request without a version resolves to the highest version of that package in +the registry. `version` is a free-form string — the recipe format does not +require dotted integers — so the ordering is defined as follows: + +| Rule | Example | +|---|---| +| A leading `v` or `V` is ignored | `v2.9.3` ranks with `2.9.3` | +| All-digit components compare numerically | `1.10.0` > `1.9.0` | +| Any other component compares as text, below any numeric one | `1.x` < `1.0` | +| A `-` or `+` suffix ranks below the same version without one | `3.6.0-rc1` < `3.6.0` | + +Every version string has a place in this order, including ones that carry no +numbers at all (`main`), so one unusual recipe cannot break lookup for the +packages around it. + --- ## Chapter 13: SDK Generation diff --git a/ebuild/packages/registry.py b/ebuild/packages/registry.py index d63622a7..9682c154 100644 --- a/ebuild/packages/registry.py +++ b/ebuild/packages/registry.py @@ -9,11 +9,64 @@ from __future__ import annotations +import re from pathlib import Path -from typing import Dict, List, Optional +from typing import Dict, List, Optional, Tuple from ebuild.packages.recipe import PackageRecipe, RecipeError, load_recipe +# Everything from the first '-' or '+' is a suffix: a pre-release tag +# ("3.6.0-rc1") or build metadata ("1.3.1+patch2"). +_SUFFIX_SPLIT = re.compile(r"[-+]") + +_ComponentKey = Tuple[int, int, str] + + +def _component_key(component: str) -> _ComponentKey: + """Order one dot-separated component of a version string. + + Numeric components compare numerically, so 1.10.0 still sorts above + 1.9.0. Anything else compares as text and ranks below any numeric + component, which keeps the ordering total without inventing a meaning + for identifiers the recipe format does not define. + """ + if component.isdigit(): + return (1, int(component), "") + return (0, 0, component) + + +def version_sort_key(version: str) -> tuple: + """Sort key for a package version string. + + ``PackageRecipe.validate()`` accepts any non-empty version, and real + embedded recipes use more than dotted integers: a leading ``v`` + (``v2.9.3``, littlefs's own tag format), pre-release tags + (``3.6.0-rc1``) and build metadata (``1.3.1+patch2``). Ordering used to + be ``[int(x) for x in version.split('.')]``, which raised ValueError on + every one of them -- and did so from ``get()``, ``list_packages()`` and + ``list_all_versions()``, so a single such recipe anywhere in the + registry took down package lookup for the whole project. + + Ordering rules: + * an optional leading ``v`` or ``V`` is ignored; + * the release part is compared component by component, numerically + where a component is all digits; + * a version carrying a pre-release or build suffix sorts below the + otherwise-equal version without one, so 3.6.0-rc1 < 3.6.0; + * nothing raises -- any string has a place in the order. + """ + text = version.strip() + if text[:1] in ("v", "V"): + text = text[1:] + + parts = _SUFFIX_SPLIT.split(text, maxsplit=1) + release = tuple(_component_key(c) for c in parts[0].split(".")) + + if len(parts) == 1: + return (release, 1, ()) + suffix = tuple(_component_key(c) for c in re.split(r"[.\-+]", parts[1])) + return (release, 0, suffix) + class PackageRegistry: """Registry of available package recipes. @@ -74,8 +127,7 @@ def get(self, name: str, version: Optional[str] = None) -> Optional[PackageRecip if version: return versions.get(version) - latest_version = sorted(versions.keys(), key=lambda v: [int(x) for x in v.split('.')])[-1] - return versions[latest_version] + return versions[max(versions, key=version_sort_key)] def has(self, name: str, version: Optional[str] = None) -> bool: """Check if a recipe exists.""" @@ -86,20 +138,13 @@ def list_packages(self) -> List[PackageRecipe]: result = [] for name in sorted(self._recipes.keys()): versions = self._recipes[name] - latest = sorted(versions.keys(), key=lambda v: [int(x) for x in v.split('.')])[-1] - result.append(versions[latest]) + result.append(versions[max(versions, key=version_sort_key)]) return result def list_all_versions(self, name: str) -> List[PackageRecipe]: """Return all versions of a package.""" versions = self._recipes.get(name, {}) - return [ - versions[v] - for v in sorted( - versions.keys(), - key=lambda v: [int(x) for x in v.split(".")], - ) - ] + return [versions[v] for v in sorted(versions, key=version_sort_key)] @property def package_count(self) -> int: diff --git a/tests/ebuild/test_package_registry.py b/tests/ebuild/test_package_registry.py index d9fcae70..1c2f9625 100644 --- a/tests/ebuild/test_package_registry.py +++ b/tests/ebuild/test_package_registry.py @@ -1,5 +1,7 @@ +import pytest + from ebuild.packages.recipe import PackageRecipe -from ebuild.packages.registry import PackageRegistry +from ebuild.packages.registry import PackageRegistry, version_sort_key def make_recipe(version: str) -> PackageRecipe: @@ -10,6 +12,13 @@ def make_recipe(version: str) -> PackageRecipe: ) +def registry_with(*versions: str) -> PackageRegistry: + registry = PackageRegistry() + for version in versions: + registry._register(make_recipe(version)) + return registry + + def test_list_all_versions_uses_numeric_version_order(): registry = PackageRegistry() @@ -24,3 +33,84 @@ def test_list_all_versions_uses_numeric_version_order(): "1.9.0", "1.10.0", ] + + +# --- Versions that are not dotted integers ----------------------------------- +# +# PackageRecipe.validate() accepts any non-empty version string, so these all +# load and register. Ordering used to be [int(x) for x in v.split('.')], which +# raised ValueError on every one of them. + + +@pytest.mark.parametrize( + "version", + [ + "v2.9.3", # littlefs publishes its releases with a leading v + "3.6.0-rc1", # pre-release tag + "1.3.1+patch2", # build metadata + "2024.06", # date-stamped release + "main", # a branch, not a release + "", # degenerate, but reachable through _register() + ], +) +def test_lookup_survives_a_non_numeric_version(version): + registry = registry_with(version) + + assert registry.get("demo").version == version + assert [r.version for r in registry.list_packages()] == [version] + assert [r.version for r in registry.list_all_versions("demo")] == [version] + + +def test_one_odd_version_does_not_break_lookup_of_the_rest(): + """A single unparseable version used to take down the whole registry. + + get() with no version scans every version of the package, and + list_packages() scans every package -- which the resolver calls to build + its 'package not found' message. One recipe with a 'v' prefix therefore + turned an ordinary lookup anywhere in the project into a ValueError. + """ + registry = registry_with("1.0.0", "v9.9.9", "1.2.0") + + assert registry.get("demo").version == "v9.9.9" + assert registry.get("demo", "1.2.0").version == "1.2.0" + assert len(registry.list_all_versions("demo")) == 3 + + +def test_leading_v_does_not_change_precedence(): + registry = registry_with("v2.9.3", "2.10.0") + + assert registry.get("demo").version == "2.10.0" + + +def test_prerelease_sorts_below_its_release(): + registry = registry_with("3.6.0", "3.6.0-rc1", "3.6.0-rc2") + + assert [r.version for r in registry.list_all_versions("demo")] == [ + "3.6.0-rc1", + "3.6.0-rc2", + "3.6.0", + ] + assert registry.get("demo").version == "3.6.0" + + +def test_build_metadata_does_not_outrank_the_next_release(): + registry = registry_with("1.3.1+patch2", "1.3.2") + + assert registry.get("demo").version == "1.3.2" + + +def test_version_ordering_is_total_and_never_raises(): + """Every pair must be comparable, in both directions, without raising.""" + versions = [ + "1.0.0", "1.0", "1.0.1", "v1.0.1", "2024.06", "1.0.0-rc1", + "1.0.0+meta", "main", "", "1.0.0-alpha.1", "10.0.0", + ] + keys = [version_sort_key(v) for v in versions] + + for left in keys: + for right in keys: + assert (left < right) or (left >= right) + + assert sorted(versions, key=version_sort_key) == sorted( + versions, key=version_sort_key + )