diff --git a/docs/book/book.md b/docs/book/book.md index cdd7391b..f162ecc3 100644 --- a/docs/book/book.md +++ b/docs/book/book.md @@ -942,6 +942,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 61830d35..9682c154 100644 --- a/ebuild/packages/registry.py +++ b/ebuild/packages/registry.py @@ -15,33 +15,57 @@ from ebuild.packages.recipe import PackageRecipe, RecipeError, load_recipe -_LEADING_DIGITS = re.compile(r"(\d*)(.*)") +# 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) -> Tuple[int, str]: - """Sort key for one dot-separated component of a version string. - - Splits the leading digits from any trailing text so "11" sorts above "2" - numerically, and "11b" sorts just above "11". - """ - digits, rest = _LEADING_DIGITS.match(component).groups() - return (int(digits) if digits else 0, rest) +def _component_key(component: str) -> _ComponentKey: + """Order one dot-separated component of a version string. -def version_key(version: str) -> Tuple[Tuple[Tuple[int, str], ...], bool, str]: - """Sort key for a version string, tolerant of non-numeric parts. + 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:] - Real-world versions are not always all digits — "3.6.0-rc1", "1.2.11b" and - "2.9.3p1" all occur upstream. Comparing them numerically raises ValueError, - and comparing them as plain strings puts "11.1.0" below "2.9.3". + parts = _SUFFIX_SPLIT.split(text, maxsplit=1) + release = tuple(_component_key(c) for c in parts[0].split(".")) - A pre-release suffix lowers precedence, so 3.6.0-rc1 sorts below 3.6.0. - """ - release, _, pre_release = version.partition("-") - components = tuple(_component_key(c) for c in release.split(".")) - # `not pre_release` is True for a final release, which sorts above any - # pre-release sharing the same components. - return (components, not pre_release, pre_release) + 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: @@ -103,8 +127,7 @@ def get(self, name: str, version: Optional[str] = None) -> Optional[PackageRecip if version: return versions.get(version) - latest_version = max(versions.keys(), key=version_key) - 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.""" @@ -115,14 +138,13 @@ def list_packages(self) -> List[PackageRecipe]: result = [] for name in sorted(self._recipes.keys()): versions = self._recipes[name] - latest = max(versions.keys(), key=version_key) - 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=version_key)] + 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 + )