Update LL version stringification and make parsing stricter (#6334)

redbot/cogs/audio/manager.py

@@ -106,7 +106,28 @@ LAVALINK_BUILD_LINE: Final[Pattern] = re.compile(rb"^Build:\s+(?P<build>\d+)$",
 LAVALINK_VERSION_LINE_PRE35: Final[Pattern] = re.compile(
     rb"^Version:\s+(?P<version>\S+)$", re.MULTILINE | re.VERBOSE
 )
-# used for LL 3.5-rc4 and newer
+# used for LL versions >=3.5-rc4 but below 3.6.
+# Since this only applies to historical version, this regex is based only on
+# version numbers that actually existed, not ones that technically could.
+LAVALINK_VERSION_LINE_PRE36: Final[Pattern] = re.compile(
+    rb"""
+    ^
+    Version:\s+
+    (?P<version>
+        (?P<major>3)\.(?P<minor>[0-5])
+        # Before LL 3.6, when patch version == 0, it was stripped from the version string
+        (?:\.(?P<patch>[1-9]\d*))?
+        # Before LL 3.6, the dot in rc.N was optional
+        (?:-rc\.?(?P<rc>0|[1-9]\d*))?
+        # additional build metadata, can be used by our downstream Lavalink
+        # if we need to alter an upstream release
+        (?:\+red\.(?P<red>[1-9]\d*))?
+    )
+    $
+    """,
+    re.MULTILINE | re.VERBOSE,
+)
+# used for LL 3.6 and newer
 # This regex is limited to the realistic usage in the LL version number,
 # not everything that could be a part of it according to the spec.
 # We can easily release an update to this regex in the future if it ever becomes necessary.
@@ -115,11 +136,8 @@ LAVALINK_VERSION_LINE: Final[Pattern] = re.compile(
     ^
     Version:\s+
     (?P<version>
-        (?P<major>0|[1-9]\d*)\.(?P<minor>0|[1-9]\d*)
+        (?P<major>0|[1-9]\d*)\.(?P<minor>0|[1-9]\d*)\.(?P<patch>0|[1-9]\d*)
-        # Before LL 3.6, when patch version == 0, it was stripped from the version string
+        (?:-rc\.(?P<rc>0|[1-9]\d*))?
-        (?:\.(?P<patch>0|[1-9]\d*))?
-        # Before LL 3.6, the dot in rc.N was optional
-        (?:-rc\.?(?P<rc>0|[1-9]\d*))?
         # additional build metadata, can be used by our downstream Lavalink
         # if we need to alter an upstream release
         (?:\+red\.(?P<red>[1-9]\d*))?
@@ -142,10 +160,16 @@ class LavalinkOldVersion:
     def from_version_output(cls, output: bytes) -> Self:
         build_match = LAVALINK_BUILD_LINE.search(output)
         if build_match is None:
-            raise ValueError("Could not find Build line in the given `--version` output.")
+            raise ValueError(
+                "Could not find 'Build' line in the given `--version` output,"
+                " or invalid build number given."
+            )
         version_match = LAVALINK_VERSION_LINE_PRE35.search(output)
         if version_match is None:
-            raise ValueError("Could not find Version line in the given `--version` output.")
+            raise ValueError(
+                "Could not find 'Version' line in the given `--version` output,"
+                " or invalid version number given."
+            )
         return cls(
             raw_version=version_match["version"].decode(),
             build_number=int(build_match["build"]),
@@ -206,16 +230,22 @@ class LavalinkVersion:
     def __str__(self) -> None:
         version = f"{self.major}.{self.minor}.{self.patch}"
         if self.rc is not None:
-            version += f"-rc{self.rc}"
+            version += f"-rc.{self.rc}"
         if self.red:
-            version += f"_red{self.red}"
+            version += f"+red.{self.red}"
         return version
 
     @classmethod
     def from_version_output(cls, output: bytes) -> Self:
         match = LAVALINK_VERSION_LINE.search(output)
         if match is None:
-            raise ValueError("Could not find Version line in the given `--version` output.")
+            # >=3.5-rc4, <3.6
+            match = LAVALINK_VERSION_LINE_PRE36.search(output)
+        if match is None:
+            raise ValueError(
+                "Could not find 'Version' line in the given `--version` output,"
+                " or invalid version number given."
+            )
         return LavalinkVersion(
             major=int(match["major"]),
             minor=int(match["minor"]),
@@ -583,29 +613,34 @@ class ServerManager:
         stdout = (await _proc.communicate())[0]
         if (branch := LAVALINK_BRANCH_LINE.search(stdout)) is None:
             # Output is unexpected, suspect corrupted jarfile
-            return False
+            raise ValueError(
+                "Could not find 'Branch' line in the `--version` output,"
+                " or invalid branch name given."
+            )
         if (java := LAVALINK_JAVA_LINE.search(stdout)) is None:
             # Output is unexpected, suspect corrupted jarfile
-            return False
+            raise ValueError(
+                "Could not find 'JVM' line in the `--version` output,"
+                " or invalid version number given."
+            )
         if (lavaplayer := LAVALINK_LAVAPLAYER_LINE.search(stdout)) is None:
             # Output is unexpected, suspect corrupted jarfile
-            return False
+            raise ValueError(
+                "Could not find 'Lavaplayer' line in the `--version` output,"
+                " or invalid version number given."
+            )
         if (buildtime := LAVALINK_BUILD_TIME_LINE.search(stdout)) is None:
             # Output is unexpected, suspect corrupted jarfile
-            return False
+            raise ValueError(
+                "Could not find 'Build time' line in the `--version` output,"
+                " or invalid build time given."
+            )
 
-        if (build := LAVALINK_BUILD_LINE.search(stdout)) is not None:
+        self._lavalink_version = (
-            try:
+            LavalinkOldVersion.from_version_output(stdout)
-                self._lavalink_version = LavalinkOldVersion.from_version_output(stdout)
+            if LAVALINK_BUILD_LINE.search(stdout) is not None
-            except ValueError:
+            else LavalinkVersion.from_version_output(stdout)
-                # Output is unexpected, suspect corrupted jarfile
+        )
-                return False
-        else:
-            try:
-                self._lavalink_version = LavalinkVersion.from_version_output(stdout)
-            except ValueError:
-                # Output is unexpected, suspect corrupted jarfile
-                return False
         date = buildtime["build_time"].decode()
         date = date.replace(".", "/")
         self._lavalink_branch = branch["branch"].decode()
@@ -616,7 +651,24 @@ class ServerManager:
         return self._up_to_date
 
     async def maybe_download_jar(self):
-        if not (self.lavalink_jar_file.exists() and await self._is_up_to_date()):
+        if not self.lavalink_jar_file.exists():
+            log.info("Triggering first-time download of Lavalink...")
+            await self._download_jar()
+            return
+
+        try:
+            up_to_date = await self._is_up_to_date()
+        except ValueError as exc:
+            log.warning("Failed to get Lavalink version: %s\nTriggering update...", exc)
+            await self._download_jar()
+            return
+
+        if not up_to_date:
+            log.info(
+                "Lavalink version outdated, triggering update from %s to %s...",
+                self._lavalink_version,
+                self.JAR_VERSION,
+            )
             await self._download_jar()
 
     async def wait_until_ready(self, timeout: Optional[float] = None):

tests/cogs/audio/test_manager.py

@@ -1,4 +1,5 @@
 import itertools
+from typing import Optional
 
 import pytest
 
@@ -43,30 +44,88 @@ def test_old_ll_version_parsing(
     raw_version: str, raw_build_number: str, expected: LavalinkOldVersion
 ) -> None:
     line = b"Version: %b\nBuild: %b" % (raw_version.encode(), raw_build_number.encode())
-    assert LavalinkOldVersion.from_version_output(line)
+    actual = LavalinkOldVersion.from_version_output(line)
+    assert actual == expected
+    assert str(actual) == f"{raw_version}_{raw_build_number}"
+
+
+def _generate_ll_version_line(raw_version: str) -> bytes:
+    return b"Version: " + raw_version.encode()
 
 
 @pytest.mark.parametrize(
-    "raw_version,expected",
+    "raw_version,expected_str,expected",
     (
         # older version format that allowed stripped `.0` and no dot in `rc.4`, used until LL 3.6
-        ("3.5-rc4", LavalinkVersion(3, 5, rc=4)),
+        ("3.5-rc4", "3.5.0-rc.4", LavalinkVersion(3, 5, rc=4)),
-        ("3.5", LavalinkVersion(3, 5)),
+        ("3.5", "3.5.0", LavalinkVersion(3, 5)),
         # newer version format
-        ("3.6.0-rc.1", LavalinkVersion(3, 6, 0, rc=1)),
+        ("3.6.0-rc.1", None, LavalinkVersion(3, 6, 0, rc=1)),
         # downstream RC version with `+red.N` suffix
-        ("3.7.5-rc.1+red.1", LavalinkVersion(3, 7, 5, rc=1, red=1)),
+        ("3.7.5-rc.1+red.1", None, LavalinkVersion(3, 7, 5, rc=1, red=1)),
-        ("3.7.5-rc.1+red.123", LavalinkVersion(3, 7, 5, rc=1, red=123)),
+        ("3.7.5-rc.1+red.123", None, LavalinkVersion(3, 7, 5, rc=1, red=123)),
         # upstream stable version
-        ("3.7.5", LavalinkVersion(3, 7, 5)),
+        ("3.7.5", None, LavalinkVersion(3, 7, 5)),
         # downstream stable version with `+red.N` suffix
-        ("3.7.5+red.1", LavalinkVersion(3, 7, 5, red=1)),
+        ("3.7.5+red.1", None, LavalinkVersion(3, 7, 5, red=1)),
-        ("3.7.5+red.123", LavalinkVersion(3, 7, 5, red=123)),
+        ("3.7.5+red.123", None, LavalinkVersion(3, 7, 5, red=123)),
     ),
 )
-def test_ll_version_parsing(raw_version: str, expected: LavalinkVersion) -> None:
+def test_ll_version_parsing(
-    line = b"Version: " + raw_version.encode()
+    raw_version: str, expected_str: Optional[str], expected: LavalinkVersion
-    assert LavalinkVersion.from_version_output(line)
+) -> None:
+    line = _generate_ll_version_line(raw_version)
+    actual = LavalinkVersion.from_version_output(line)
+    expected_str = expected_str or raw_version
+    assert actual == expected
+    assert str(actual) == expected_str
+
+
+@pytest.mark.parametrize(
+    "raw_version",
+    (
+        # 3.5.0-rc4 is first version to not have build number
+        # 3.5 stripped `.0` from version number
+        "3.5",
+        # RC version don't need a dot for RC versions...
+        "3.5-rc4",
+        # ...but that doesn't mean they can't
+        "3.5-rc.5",
+        # regular 3.5.x version
+        "3.5.5",
+        # one more RC version with non-zero patch version
+        "3.5.5-rc1",
+    ),
+)
+def test_ll_version_accepts_less_strict_below_3_6(raw_version: str) -> None:
+    line = _generate_ll_version_line(raw_version)
+    # check that the version can be parsed
+    LavalinkVersion.from_version_output(line)
+
+
+@pytest.mark.parametrize(
+    "raw_version",
+    (
+        # `.0` releases <3.6 had their `.0` stripped so this is not valid:
+        "3.5.0-rc4",
+        # 3.6 is first to require stricter format
+        "3.6.0-rc4",
+        "3.6",
+        # another single digit minor version newer than 3.6
+        "3.7",
+        # double digit minor version
+        "3.11.3-rc1",
+        # newer major version
+        "4.0.0-rc5",
+        # double digit major version
+        "11.0.0-rc5",
+    ),
+)
+def test_ll_version_rejects_less_strict_on_3_6_and_above(raw_version: str) -> None:
+    line = _generate_ll_version_line(raw_version)
+
+    with pytest.raises(ValueError):
+        LavalinkVersion.from_version_output(line)
 
 
 def test_ll_version_comparison() -> None: