From 0a8da02b22e7227a2165790b3f98702ca21116f2 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Sat, 22 Feb 2025 14:27:07 +1030 Subject: [PATCH] pyln-client: reimplement NodeVersion, simply. This broke my build machine, because lightningd --version was malformed (I had no tags somehow in that branch). I dived into the code to figure out what was wrong, and I was horrified. 1. STOP. Never write this much code. 2. You just need a NodeVersion class. That's it. No others. 3. Don't throw away the entire first part if it starts with 'v'. Just remove the v. 4. Handle untagged versions cleanly. 5. Always fail on invalid strings in the constructor, NOT on the first time you use it. I have rewritten it. Signed-off-by: Rusty Russell --- contrib/pyln-client/pyln/client/__init__.py | 3 +- contrib/pyln-client/pyln/client/version.py | 212 +++----------------- contrib/pyln-client/tests/test_version.py | 77 ++----- contrib/pyln-testing/pyln/testing/utils.py | 26 +-- 4 files changed, 60 insertions(+), 258 deletions(-) diff --git a/contrib/pyln-client/pyln/client/__init__.py b/contrib/pyln-client/pyln/client/__init__.py index d9d251ade8c6..45b3fe3659e6 100644 --- a/contrib/pyln-client/pyln/client/__init__.py +++ b/contrib/pyln-client/pyln/client/__init__.py @@ -2,7 +2,7 @@ from .plugin import Plugin, monkey_patch, RpcException from .gossmap import Gossmap, GossmapNode, GossmapChannel, GossmapHalfchannel, GossmapNodeId, LnFeatureBits from .gossmapstats import GossmapStats -from .version import NodeVersion, VersionSpec +from .version import NodeVersion __version__ = "25.02" @@ -22,5 +22,4 @@ "LnFeatureBits", "GossmapStats", "NodeVersion", - "VersionSpec", ] diff --git a/contrib/pyln-client/pyln/client/version.py b/contrib/pyln-client/pyln/client/version.py index dceea2d8dce3..01345c82e77a 100644 --- a/contrib/pyln-client/pyln/client/version.py +++ b/contrib/pyln-client/pyln/client/version.py @@ -1,16 +1,10 @@ from __future__ import annotations -from dataclasses import dataclass from functools import total_ordering -import re -from typing import List, Optional, Protocol, runtime_checkable, Union - - -_MODDED_PATTERN = "[0-9a-f]+-modded" +from typing import Union @total_ordering -@dataclass class NodeVersion: """NodeVersion @@ -29,25 +23,28 @@ class NodeVersion: See `strict_equal` if an exact match is required - `v23.11` < `v24.02` The oldest version is the smallest + - `vd6fa78c` + This is an untagged version, such as in CI. This is assumed to be the latest, greater than + any test. """ - - version: str - - def to_parts(self) -> List[_NodeVersionPart]: - parts = self.version[1:].split(".") - # If the first part contains a v we will ignore it - if not parts[0][0].isdigit(): - parts[0] = parts[1:] - - return [_NodeVersionPart.parse(p) for p in parts] - - def strict_equal(self, other: NodeVersion) -> bool: - if not isinstance(other, NodeVersion): - raise TypeError( - "`other` is expected to be of type `NodeVersion` but is `{type(other)}`" - ) + def __init__(self, version: str): + # e.g. v24.11-225-gda793e66b9 + if version.startswith('v'): + version = version[1:] + version = version.split('-')[0] + parts = version.split('.') + # rc is considered "close enough" + if 'rc' in parts[-1]: + parts[-1] = parts[-1].split('rc')[0] + + self.parts: int = [] + + # Single part? It's a git version, so treat it as the future. + if len(parts) == 1: + self.parts.append(100) else: - return self.version == other.version + for p in parts: + self.parts.append(int(p)) def __eq__(self, other: Union[NodeVersion, str]) -> bool: if isinstance(other, str): @@ -55,21 +52,12 @@ def __eq__(self, other: Union[NodeVersion, str]) -> bool: if not isinstance(other, NodeVersion): return False - if self.strict_equal(other): - return True - elif re.match(_MODDED_PATTERN, self.version): + if len(self.parts) != len(other.parts): return False - else: - self_parts = [p.num for p in self.to_parts()] - other_parts = [p.num for p in other.to_parts()] - - if len(self_parts) != len(other_parts): + for a, b in zip(self.parts, other.parts): + if a != b: return False - - for ps, po in zip(self_parts, other_parts): - if ps != po: - return False - return True + return True def __lt__(self, other: Union[NodeVersion, str]) -> bool: if isinstance(other, str): @@ -77,148 +65,14 @@ def __lt__(self, other: Union[NodeVersion, str]) -> bool: if not isinstance(other, NodeVersion): return NotImplemented - # If we are in CI the version will by a hex ending on modded - # We will assume it is the latest version - if re.match(_MODDED_PATTERN, self.version): - return False - elif re.match(_MODDED_PATTERN, other.version): - return True - else: - self_parts = [p.num for p in self.to_parts()] - other_parts = [p.num for p in other.to_parts()] - - # zip truncates to shortes length - for sp, op in zip(self_parts, other_parts): - if sp < op: - return True - if sp > op: - return False - - # If the initial parts are all equal the longest version is the biggest - # - # self = 'v24.02' - # other = 'v24.02.1' - return len(self_parts) < len(other_parts) - - def matches(self, version_spec: VersionSpecLike) -> bool: - """Returns True if the version matches the spec - - The `version_spec` can be represented as a string and has 8 operators - which are `=`, `===`, `!=`, `!===`, `<`, `<=`, `>`, `>=`. - - The `=` is the equality operator. The verson_spec `=v24.02` matches - all versions that equal `v24.02` including release candidates such as `v24.02rc1`. - You can use the strict-equality operator `===` if strict equality is required. - - Specifiers can be combined by separating the with a comma ','. The `version_spec` - `>=v23.11, _NodeVersionPart: - # We assume all parts start with a number and are followed by a text - # E.g: v24.01rc2 has two parts - # - "24" -> num = 24, text = None - # - "01rc" -> num = 01, text = "rc" - - number = re.search(r"\d+", part).group() - text = part[len(number):] - text_opt = text if text != "" else None - return _NodeVersionPart(int(number), text_opt) - - -@runtime_checkable -class VersionSpec(Protocol): - def matches(self, other: NodeVersionLike) -> bool: - ... - - @classmethod - def parse(cls, spec: VersionSpecLike) -> VersionSpec: - if isinstance(spec, VersionSpec): - return spec - else: - parts = [p.strip() for p in spec.split(",")] - subspecs = [_CompareSpec.parse(p) for p in parts] - return _AndVersionSpecifier(subspecs) - - -@dataclass -class _AndVersionSpecifier(VersionSpec): - specs: List[VersionSpec] - - def matches(self, other: NodeVersionLike) -> bool: - for spec in self.specs: - if not spec.matches(other): + # We want a zero-padded zip. Pad both to make one. + totlen = max(len(self.parts), len(other.parts)) + for a, b in zip(self.parts + [0] * totlen, other.parts + [0] * totlen): + if a < b: + return True + if a > b: return False - return True - - -_OPERATORS = [ - "===", # Strictly equal - "!===", # not strictly equal - "=", # Equal - ">=", # Greater or equal - "<=", # Less or equal - "<", # less - ">", # greater than - "!=", # not equal -] - - -@dataclass -class _CompareSpec(VersionSpec): - operator: str - version: NodeVersion - - def __post_init__(self): - if self.operator not in _OPERATORS: - raise ValueError(f"Invalid operator '{self.operator}'") - - def matches(self, other: NodeVersionLike): - if isinstance(other, str): - other = NodeVersion(other) - if self.operator == "===": - return other.strict_equal(self.version) - if self.operator == "!===": - return not other.strict_equal(self.version) - if self.operator == "=": - return other == self.version - if self.operator == ">=": - return other >= self.version - if self.operator == "<=": - return other <= self.version - if self.operator == "<": - return other < self.version - if self.operator == ">": - return other > self.version - if self.operator == "!=": - return other != self.version - else: - ValueError("Unknown operator") - - @classmethod - def parse(cls, spec_string: str) -> _CompareSpec: - spec_string = spec_string.strip() - - for op in _OPERATORS: - if spec_string.startswith(op): - version = spec_string[len(op):] - version = version.strip() - return _CompareSpec(op, NodeVersion(version)) - - raise ValueError(f"Failed to parse '{spec_string}'") - + return False -NodeVersionLike = Union[NodeVersion, str] -VersionSpecLike = Union[VersionSpec, str] -__all__ = [NodeVersion, NodeVersionLike, VersionSpec, VersionSpecLike] +__all__ = [NodeVersion] diff --git a/contrib/pyln-client/tests/test_version.py b/contrib/pyln-client/tests/test_version.py index 7464e09de795..71f98b2b5dbc 100644 --- a/contrib/pyln-client/tests/test_version.py +++ b/contrib/pyln-client/tests/test_version.py @@ -1,42 +1,22 @@ -from pyln.client.version import NodeVersion, VersionSpec, _NodeVersionPart, _CompareSpec +from pyln.client.version import NodeVersion def test_create_version(): # These are the strings returned by `lightningd --version` - _ = NodeVersion("v24.02") - _ = NodeVersion("23.08.1") - - -def test_parse_parts(): - assert _NodeVersionPart.parse("2rc2") == _NodeVersionPart(2, "rc2") - assert _NodeVersionPart.parse("0rc1") == _NodeVersionPart(0, "rc1") - assert _NodeVersionPart.parse("2") == _NodeVersionPart(2, None) - assert _NodeVersionPart.parse("2").text is None - - -def test_version_to_parts(): - - assert NodeVersion("v24.02rc1").to_parts() == [ - _NodeVersionPart(24), - _NodeVersionPart(2, "rc1"), - ] - - assert NodeVersion("v24.02.1").to_parts() == [ - _NodeVersionPart(24), - _NodeVersionPart(2), - _NodeVersionPart(1), - ] + _ = NodeVersion("v24.11-232-g5a76c7a") + _ = NodeVersion("v24.11-225-gda793e6-modded") + _ = NodeVersion("v24.11") + _ = NodeVersion("vd6fa78c") def test_equality_classes_in_node_versions(): assert NodeVersion("v24.02") == NodeVersion("v24.02") + assert NodeVersion("v24.02") == NodeVersion("24.02") + assert NodeVersion("v24.02-225") == NodeVersion("v24.02") + assert NodeVersion("v24.02") == NodeVersion("v24.02rc1") - assert NodeVersion("v24.02rc1") == NodeVersion("v24.02") assert NodeVersion("v24.11-217-g77989b1-modded") == NodeVersion("v24.11") - - assert NodeVersion("v24.02") != NodeVersion("v24.02.1") - assert NodeVersion("v24.02rc1") != NodeVersion("v24.02.1") - assert NodeVersion("v23.10") != NodeVersion("v23.02") + assert NodeVersion("vd6fa78c") == NodeVersion("vabcdefg") def test_inequality_of_node_versions(): @@ -44,49 +24,26 @@ def test_inequality_of_node_versions(): assert NodeVersion("v24.02.1") > NodeVersion("v24.02") assert NodeVersion("v24.02.1") > NodeVersion("v24.02rc1") assert NodeVersion("v24.02.1") > NodeVersion("v23.05") + assert NodeVersion("v24.05") > NodeVersion("v24.02") + assert NodeVersion("vd6fa78c") > NodeVersion("v26.02") assert NodeVersion("v24.02.1") >= NodeVersion("v24.02.1") assert NodeVersion("v24.02.1") >= NodeVersion("v24.02") assert NodeVersion("v24.02.1") >= NodeVersion("v24.02rc1") assert NodeVersion("v24.02.1") >= NodeVersion("v23.05") + assert NodeVersion("v24.05") >= NodeVersion("v24.02") + assert NodeVersion("vd6fa78c") >= NodeVersion("v26.02") assert NodeVersion("v24.02.1") <= NodeVersion("v24.02.1") assert not NodeVersion("v24.02.1") <= NodeVersion("v24.02") assert not NodeVersion("v24.02.1") <= NodeVersion("v24.02rc1") assert not NodeVersion("v24.02.1") <= NodeVersion("v23.05") + assert not NodeVersion("v24.05") <= NodeVersion("v24.02") + assert not NodeVersion("vd6fa78c") <= NodeVersion("v26.02") assert not NodeVersion("v24.02.1") < NodeVersion("v24.02.1") assert not NodeVersion("v24.02.1") < NodeVersion("v24.02") assert not NodeVersion("v24.02.1") < NodeVersion("v24.02rc1") assert not NodeVersion("v24.02.1") < NodeVersion("v23.05") - - -def test_comparision_parse(): - assert _CompareSpec.parse("===v24.02").operator == "===" - assert _CompareSpec.parse("=v24.02").operator == "=" - assert _CompareSpec.parse("!===v24.02").operator == "!===" - assert _CompareSpec.parse("!=v24.02").operator == "!=" - assert _CompareSpec.parse(">v24.02").operator == ">" - assert _CompareSpec.parse("=v24.02").operator == ">=" - assert _CompareSpec.parse("<=v24.02").operator == "<=" - - -def test_compare_spec_from_string(): - assert VersionSpec.parse("=v24.02").matches("v24.02rc1") - assert VersionSpec.parse("=v24.02").matches("v24.02") - assert not VersionSpec.parse("=v24.02").matches("v24.02.1") - - # Yes, I use weird spaces here as a part of the test - list_spec = VersionSpec.parse(">= v24.02, !=== v24.02rc1") - assert list_spec.matches("v24.02") - assert list_spec.matches("v24.02.1") - - assert not list_spec.matches("v24.02rc1") - assert not list_spec.matches("v23.11") - - -def test_ci_modded_version_is_always_latest(): - v1 = NodeVersion("1a86e50-modded") - - assert v1 > NodeVersion("v24.02") + assert not NodeVersion("v24.05") < NodeVersion("v24.02") + assert not NodeVersion("vd6fa78c") < NodeVersion("v26.02") diff --git a/contrib/pyln-testing/pyln/testing/utils.py b/contrib/pyln-testing/pyln/testing/utils.py index eabe695153f3..9d1fa30ff957 100644 --- a/contrib/pyln-testing/pyln/testing/utils.py +++ b/contrib/pyln-testing/pyln/testing/utils.py @@ -10,7 +10,7 @@ from decimal import Decimal from pyln.client import LightningRpc from pyln.client import Millisatoshi -from pyln.client import NodeVersion, VersionSpec +from pyln.client import NodeVersion import ephemeral_port_reserve # type: ignore import json @@ -629,11 +629,8 @@ def __init__( cln_version_proc = subprocess.check_output([self.executable, "--version"]) self.cln_version = NodeVersion(cln_version_proc.decode('ascii').strip()) - try: - if VersionSpec.parse("<=v24.02.2").matches(self.cln_version): - self.opts['log-level'] = "debug" - except Exception: - raise ValueError(f"Invalid version {type(self.cln_version)} - {self.cln_version}") + if self.cln_version <= "v24.02.2": + self.opts['log-level'] = "debug" opts = { 'lightning-dir': lightning_dir, @@ -671,11 +668,9 @@ def __init__( # In case you want specific ordering! self.early_opts = [] - try: - if VersionSpec.parse(">=v23.11").matches(self.cln_version): - self.early_opts.append('--developer') - except Exception: - raise ValueError(f"Invalid version {type(self.cln_version)} - {self.cln_version}") + # Before this we had a developer build option. + if self.cln_version >= "v23.11": + self.early_opts.append('--developer') def cleanup(self): # To force blackhole to exit, disconnect file must be truncated! @@ -855,11 +850,8 @@ def __init__(self, node_id, lightning_dir, bitcoind, executor, valgrind, may_fai if EXPERIMENTAL_SPLICING: self.daemon.opts["experimental-splicing"] = None # Avoid test flakes cause by this option unless explicitly set. - try: - if VersionSpec.parse(">=v24.11").matches(self.cln_version): - self.daemon.opts.update({"autoconnect-seeker-peers": 0}) - except Exception: - raise ValueError(f"Invalid version {type(self.cln_version)} - {self.cln_version}") + if self.cln_version >= "v24.11": + self.daemon.opts.update({"autoconnect-seeker-peers": 0}) if options is not None: self.daemon.opts.update(options) @@ -1204,7 +1196,7 @@ def is_local_channel_active(self, scid) -> bool: # # The field `updates`-field didn't exist prio to v24.02 and will be # ignored for older versions of cln - if VersionSpec.parse(">=v24.02").matches(self.cln_version): + if self.cln_version >= "v24.02": if "remote" not in channel.get("updates", {}): return False