Skip to content

Commit 638b722

Browse files
committed
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. 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 <[email protected]>
1 parent 1e5a577 commit 638b722

File tree

4 files changed

+43
-260
lines changed

4 files changed

+43
-260
lines changed

contrib/pyln-client/pyln/client/__init__.py

+1-2
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@
22
from .plugin import Plugin, monkey_patch, RpcException
33
from .gossmap import Gossmap, GossmapNode, GossmapChannel, GossmapHalfchannel, GossmapNodeId, LnFeatureBits
44
from .gossmapstats import GossmapStats
5-
from .version import NodeVersion, VersionSpec
5+
from .version import NodeVersion
66

77
__version__ = "25.02rc3"
88

@@ -22,5 +22,4 @@
2222
"LnFeatureBits",
2323
"GossmapStats",
2424
"NodeVersion",
25-
"VersionSpec",
2625
]
+26-181
Original file line numberDiff line numberDiff line change
@@ -1,16 +1,10 @@
11
from __future__ import annotations
22

3-
from dataclasses import dataclass
43
from functools import total_ordering
54
import re
6-
from typing import List, Optional, Protocol, runtime_checkable, Union
7-
8-
9-
_MODDED_PATTERN = "[0-9a-f]+-modded"
10-
5+
from typing import List, Union
116

127
@total_ordering
13-
@dataclass
148
class NodeVersion:
159
"""NodeVersion
1610
@@ -30,195 +24,46 @@ class NodeVersion:
3024
- `v23.11` < `v24.02`
3125
The oldest version is the smallest
3226
"""
33-
34-
version: str
35-
36-
def to_parts(self) -> List[_NodeVersionPart]:
37-
parts = self.version[1:].split(".")
38-
# If the first part contains a v we will ignore it
39-
if not parts[0][0].isdigit():
40-
parts[0] = parts[1:]
41-
42-
return [_NodeVersionPart.parse(p) for p in parts]
43-
44-
def strict_equal(self, other: NodeVersion) -> bool:
45-
if not isinstance(other, NodeVersion):
46-
raise TypeError(
47-
"`other` is expected to be of type `NodeVersion` but is `{type(other)}`"
48-
)
49-
else:
50-
return self.version == other.version
27+
def __init__(self, version: str):
28+
# e.g. v24.11-225-gda793e66b9
29+
if version.startswith('v'):
30+
version = version[1:]
31+
version = version.split('-')[0]
32+
parts = version.split('.')
33+
# rc is considered "close enough"
34+
if 'rc' in parts[-1]:
35+
parts[-1] = parts[-1].split('rc')[0]
36+
37+
self.parts: int = []
38+
for p in parts:
39+
self.parts.append(int(p))
5140

5241
def __eq__(self, other: Union[NodeVersion, str]) -> bool:
5342
if isinstance(other, str):
5443
other = NodeVersion(other)
5544
if not isinstance(other, NodeVersion):
5645
return False
5746

58-
if self.strict_equal(other):
59-
return True
60-
elif re.match(_MODDED_PATTERN, self.version):
47+
if len(self.parts) != len(other.parts):
6148
return False
62-
else:
63-
self_parts = [p.num for p in self.to_parts()]
64-
other_parts = [p.num for p in other.to_parts()]
65-
66-
if len(self_parts) != len(other_parts):
49+
for a, b in zip(self.parts, other.parts):
50+
if a != b:
6751
return False
68-
69-
for ps, po in zip(self_parts, other_parts):
70-
if ps != po:
71-
return False
72-
return True
52+
return True
7353

7454
def __lt__(self, other: Union[NodeVersion, str]) -> bool:
7555
if isinstance(other, str):
7656
other = NodeVersion(other)
7757
if not isinstance(other, NodeVersion):
7858
return NotImplemented
7959

80-
# If we are in CI the version will by a hex ending on modded
81-
# We will assume it is the latest version
82-
if re.match(_MODDED_PATTERN, self.version):
83-
return False
84-
elif re.match(_MODDED_PATTERN, other.version):
85-
return True
86-
else:
87-
self_parts = [p.num for p in self.to_parts()]
88-
other_parts = [p.num for p in other.to_parts()]
89-
90-
# zip truncates to shortes length
91-
for sp, op in zip(self_parts, other_parts):
92-
if sp < op:
93-
return True
94-
if sp > op:
95-
return False
96-
97-
# If the initial parts are all equal the longest version is the biggest
98-
#
99-
# self = 'v24.02'
100-
# other = 'v24.02.1'
101-
return len(self_parts) < len(other_parts)
102-
103-
def matches(self, version_spec: VersionSpecLike) -> bool:
104-
"""Returns True if the version matches the spec
105-
106-
The `version_spec` can be represented as a string and has 8 operators
107-
which are `=`, `===`, `!=`, `!===`, `<`, `<=`, `>`, `>=`.
108-
109-
The `=` is the equality operator. The verson_spec `=v24.02` matches
110-
all versions that equal `v24.02` including release candidates such as `v24.02rc1`.
111-
You can use the strict-equality operator `===` if strict equality is required.
112-
113-
Specifiers can be combined by separating the with a comma ','. The `version_spec`
114-
`>=v23.11, <v24.02" includes any version which is greater than or equal to `v23.11`
115-
and smaller than `v24.02`.
116-
"""
117-
spec = VersionSpec.parse(version_spec)
118-
return spec.matches(self)
119-
120-
121-
@dataclass
122-
class _NodeVersionPart:
123-
num: int
124-
text: Optional[str] = None
125-
126-
@classmethod
127-
def parse(cls, part: str) -> _NodeVersionPart:
128-
# We assume all parts start with a number and are followed by a text
129-
# E.g: v24.01rc2 has two parts
130-
# - "24" -> num = 24, text = None
131-
# - "01rc" -> num = 01, text = "rc"
132-
133-
number = re.search(r"\d+", part).group()
134-
text = part[len(number):]
135-
text_opt = text if text != "" else None
136-
return _NodeVersionPart(int(number), text_opt)
137-
138-
139-
@runtime_checkable
140-
class VersionSpec(Protocol):
141-
def matches(self, other: NodeVersionLike) -> bool:
142-
...
143-
144-
@classmethod
145-
def parse(cls, spec: VersionSpecLike) -> VersionSpec:
146-
if isinstance(spec, VersionSpec):
147-
return spec
148-
else:
149-
parts = [p.strip() for p in spec.split(",")]
150-
subspecs = [_CompareSpec.parse(p) for p in parts]
151-
return _AndVersionSpecifier(subspecs)
152-
153-
154-
@dataclass
155-
class _AndVersionSpecifier(VersionSpec):
156-
specs: List[VersionSpec]
157-
158-
def matches(self, other: NodeVersionLike) -> bool:
159-
for spec in self.specs:
160-
if not spec.matches(other):
60+
# We want a zero-padded zip. Pad both to make one.
61+
totlen = max(len(self.parts), len(other.parts))
62+
for a, b in zip(self.parts + [0] * totlen, other.parts + [0] * totlen):
63+
if a < b:
64+
return True
65+
if a < b:
16166
return False
162-
return True
163-
164-
165-
_OPERATORS = [
166-
"===", # Strictly equal
167-
"!===", # not strictly equal
168-
"=", # Equal
169-
">=", # Greater or equal
170-
"<=", # Less or equal
171-
"<", # less
172-
">", # greater than
173-
"!=", # not equal
174-
]
175-
176-
177-
@dataclass
178-
class _CompareSpec(VersionSpec):
179-
operator: str
180-
version: NodeVersion
181-
182-
def __post_init__(self):
183-
if self.operator not in _OPERATORS:
184-
raise ValueError(f"Invalid operator '{self.operator}'")
185-
186-
def matches(self, other: NodeVersionLike):
187-
if isinstance(other, str):
188-
other = NodeVersion(other)
189-
if self.operator == "===":
190-
return other.strict_equal(self.version)
191-
if self.operator == "!===":
192-
return not other.strict_equal(self.version)
193-
if self.operator == "=":
194-
return other == self.version
195-
if self.operator == ">=":
196-
return other >= self.version
197-
if self.operator == "<=":
198-
return other <= self.version
199-
if self.operator == "<":
200-
return other < self.version
201-
if self.operator == ">":
202-
return other > self.version
203-
if self.operator == "!=":
204-
return other != self.version
205-
else:
206-
ValueError("Unknown operator")
207-
208-
@classmethod
209-
def parse(cls, spec_string: str) -> _CompareSpec:
210-
spec_string = spec_string.strip()
211-
212-
for op in _OPERATORS:
213-
if spec_string.startswith(op):
214-
version = spec_string[len(op):]
215-
version = version.strip()
216-
return _CompareSpec(op, NodeVersion(version))
217-
218-
raise ValueError(f"Failed to parse '{spec_string}'")
219-
220-
221-
NodeVersionLike = Union[NodeVersion, str]
222-
VersionSpecLike = Union[VersionSpec, str]
67+
return False
22368

224-
__all__ = [NodeVersion, NodeVersionLike, VersionSpec, VersionSpecLike]
69+
__all__ = [NodeVersion]

contrib/pyln-client/tests/test_version.py

+7-60
Original file line numberDiff line numberDiff line change
@@ -3,40 +3,18 @@
33

44
def test_create_version():
55
# These are the strings returned by `lightningd --version`
6-
_ = NodeVersion("v24.02")
7-
_ = NodeVersion("23.08.1")
8-
9-
10-
def test_parse_parts():
11-
assert _NodeVersionPart.parse("2rc2") == _NodeVersionPart(2, "rc2")
12-
assert _NodeVersionPart.parse("0rc1") == _NodeVersionPart(0, "rc1")
13-
assert _NodeVersionPart.parse("2") == _NodeVersionPart(2, None)
14-
assert _NodeVersionPart.parse("2").text is None
15-
16-
17-
def test_version_to_parts():
18-
19-
assert NodeVersion("v24.02rc1").to_parts() == [
20-
_NodeVersionPart(24),
21-
_NodeVersionPart(2, "rc1"),
22-
]
23-
24-
assert NodeVersion("v24.02.1").to_parts() == [
25-
_NodeVersionPart(24),
26-
_NodeVersionPart(2),
27-
_NodeVersionPart(1),
28-
]
6+
_ = NodeVersion("v24.11-232-g5a76c7a")
7+
_ = NodeVersion("v24.11-225-gda793e6-modded")
8+
_ = NodeVersion("v24.11")
299

3010

3111
def test_equality_classes_in_node_versions():
3212
assert NodeVersion("v24.02") == NodeVersion("v24.02")
33-
assert NodeVersion("v24.02") == NodeVersion("v24.02rc1")
34-
assert NodeVersion("v24.02rc1") == NodeVersion("v24.02")
35-
assert NodeVersion("v24.11-217-g77989b1-modded") == NodeVersion("v24.11")
13+
assert NodeVersion("v24.02") == NodeVersion("24.02")
14+
assert NodeVersion("v24.02-225") == NodeVersion("v24.02")
3615

37-
assert NodeVersion("v24.02") != NodeVersion("v24.02.1")
38-
assert NodeVersion("v24.02rc1") != NodeVersion("v24.02.1")
39-
assert NodeVersion("v23.10") != NodeVersion("v23.02")
16+
assert NodeVersion("v24.02") != NodeVersion("v24.02rc1")
17+
assert NodeVersion("v24.11-217-g77989b1-modded") == NodeVersion("v24.11")
4018

4119

4220
def test_inequality_of_node_versions():
@@ -59,34 +37,3 @@ def test_inequality_of_node_versions():
5937
assert not NodeVersion("v24.02.1") < NodeVersion("v24.02")
6038
assert not NodeVersion("v24.02.1") < NodeVersion("v24.02rc1")
6139
assert not NodeVersion("v24.02.1") < NodeVersion("v23.05")
62-
63-
64-
def test_comparision_parse():
65-
assert _CompareSpec.parse("===v24.02").operator == "==="
66-
assert _CompareSpec.parse("=v24.02").operator == "="
67-
assert _CompareSpec.parse("!===v24.02").operator == "!==="
68-
assert _CompareSpec.parse("!=v24.02").operator == "!="
69-
assert _CompareSpec.parse(">v24.02").operator == ">"
70-
assert _CompareSpec.parse("<v24.02").operator == "<"
71-
assert _CompareSpec.parse(">=v24.02").operator == ">="
72-
assert _CompareSpec.parse("<=v24.02").operator == "<="
73-
74-
75-
def test_compare_spec_from_string():
76-
assert VersionSpec.parse("=v24.02").matches("v24.02rc1")
77-
assert VersionSpec.parse("=v24.02").matches("v24.02")
78-
assert not VersionSpec.parse("=v24.02").matches("v24.02.1")
79-
80-
# Yes, I use weird spaces here as a part of the test
81-
list_spec = VersionSpec.parse(">= v24.02, !=== v24.02rc1")
82-
assert list_spec.matches("v24.02")
83-
assert list_spec.matches("v24.02.1")
84-
85-
assert not list_spec.matches("v24.02rc1")
86-
assert not list_spec.matches("v23.11")
87-
88-
89-
def test_ci_modded_version_is_always_latest():
90-
v1 = NodeVersion("1a86e50-modded")
91-
92-
assert v1 > NodeVersion("v24.02")

contrib/pyln-testing/pyln/testing/utils.py

+9-17
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@
1010
from decimal import Decimal
1111
from pyln.client import LightningRpc
1212
from pyln.client import Millisatoshi
13-
from pyln.client import NodeVersion, VersionSpec
13+
from pyln.client import NodeVersion
1414

1515
import ephemeral_port_reserve # type: ignore
1616
import json
@@ -615,11 +615,8 @@ def __init__(
615615
cln_version_proc = subprocess.check_output([self.executable, "--version"])
616616
self.cln_version = NodeVersion(cln_version_proc.decode('ascii').strip())
617617

618-
try:
619-
if VersionSpec.parse("<=v24.02.2").matches(self.cln_version):
620-
self.opts['log-level'] = "debug"
621-
except Exception:
622-
raise ValueError(f"Invalid version {type(self.cln_version)} - {self.cln_version}")
618+
if self.cln_version <= NodeVersion("v24.02.2"):
619+
self.opts['log-level'] = "debug"
623620

624621
opts = {
625622
'lightning-dir': lightning_dir,
@@ -657,11 +654,9 @@ def __init__(
657654
# In case you want specific ordering!
658655
self.early_opts = []
659656

660-
try:
661-
if VersionSpec.parse(">=v23.11").matches(self.cln_version):
662-
self.early_opts.append('--developer')
663-
except Exception:
664-
raise ValueError(f"Invalid version {type(self.cln_version)} - {self.cln_version}")
657+
# Before this we had a developer build option.
658+
if self.cln_version >= NodeVersion("v23.11"):
659+
self.early_opts.append('--developer')
665660

666661
def cleanup(self):
667662
# To force blackhole to exit, disconnect file must be truncated!
@@ -841,11 +836,8 @@ def __init__(self, node_id, lightning_dir, bitcoind, executor, valgrind, may_fai
841836
if EXPERIMENTAL_SPLICING:
842837
self.daemon.opts["experimental-splicing"] = None
843838
# Avoid test flakes cause by this option unless explicitly set.
844-
try:
845-
if VersionSpec.parse(">=v24.11").matches(self.cln_version):
846-
self.daemon.opts.update({"autoconnect-seeker-peers": 0})
847-
except Exception:
848-
raise ValueError(f"Invalid version {type(self.cln_version)} - {self.cln_version}")
839+
if self.cln_version >= NodeVersion("v24.11"):
840+
self.daemon.opts.update({"autoconnect-seeker-peers": 0})
849841

850842
if options is not None:
851843
self.daemon.opts.update(options)
@@ -1190,7 +1182,7 @@ def is_local_channel_active(self, scid) -> bool:
11901182
#
11911183
# The field `updates`-field didn't exist prio to v24.02 and will be
11921184
# ignored for older versions of cln
1193-
if VersionSpec.parse(">=v24.02").matches(self.cln_version):
1185+
if self.cln_version >= NodeVersion("v24.02"):
11941186
if "remote" not in channel.get("updates", {}):
11951187
return False
11961188

0 commit comments

Comments
 (0)