Merge pull request #2008 from apache/tristan/prefer-better-version-matches

Prefer better version matches

Author: gtristan

src/buildstream/_frontend/cli.py

@@ -409,7 +409,7 @@ def help_command(ctx, command):
 @click.option(
     "--min-version",
     type=click.STRING,
-    default="2.4",
+    default="2.5",
     show_default=True,
     help="The required format version",
 )

src/buildstream/downloadablefilesource.py

@@ -43,12 +43,9 @@
   version string from the specified URI, in order to fill out the reported
   :attr:`~buildstream.source.SourceInfo.version_guess`.
 
-  The URI will be *searched* using this regular expression, and is allowed to
-  yield a number of *groups*. For example the value ``(\\d+)_(\\d+)_(\\d+)`` would
-  report 3 *groups* if 3 numerical values separated by underscores were found in
-  the URI.
-
-  The default value for ``version-guess-pattern`` is ``\\d+\\.\\d+(?:\\.\\d+)?``.
+  This is done using the :func:`utils.guess_version() <buildstream.utils.guess_version>`
+  utility function, please refer to that function documentation to understand how
+  the guessing mechanics works, and what kind of string you should provide here.
 
   .. note:
 
@@ -140,7 +137,7 @@ def translate_url(
 for which it reports the sha256 checksum of the remote file content as the *version*.
 
 An attempt to guess the version based on the remote filename will be made
-for the reporting of the *guess_version*. Control over how the guess is made
+for the reporting of the *version_guess*. Control over how the guess is made
 or overridden is explained above in the
 :ref:`built-in functionality documentation <core_downloadable_source_builtins>`.
 """
@@ -268,7 +265,6 @@ class DownloadableFileSource(Source):
     COMMON_CONFIG_KEYS = Source.COMMON_CONFIG_KEYS + ["url", "ref", "version-guess-pattern", "version"]
 
     __default_mirror_file = None
-    __default_guess_pattern = re.compile(r"\d+\.\d+(?:\.\d+)?")
 
     def configure(self, node):
         self.original_url = node.get_str("url")
@@ -281,9 +277,8 @@ def configure(self, node):
         self._mirror_dir = os.path.join(self.get_mirror_directory(), utils.url_directory_name(self.original_url))
 
         self._guess_pattern_string = node.get_str("version-guess-pattern", None)
-        if self._guess_pattern_string is None:
-            self._guess_pattern = self.__default_guess_pattern
-        else:
+        self._guess_pattern = None
+        if self._guess_pattern_string is not None:
             self._guess_pattern = re.compile(self._guess_pattern_string)
 
         self._version = node.get_str("version", None)
@@ -298,7 +293,7 @@ def get_unique_key(self):
         # attributes which affect SourceInfo generation.
         if self._version is not None:
             unique_key.append(self._version)
-        elif self._guess_pattern is not self.__default_guess_pattern:
+        elif self._guess_pattern_string is not None:
             unique_key.append(self._guess_pattern_string)
 
         return unique_key
@@ -352,16 +347,9 @@ def fetch(self):  # pylint: disable=arguments-differ
             )
 
     def collect_source_info(self):
-        if self._version is None:
-            version_match = self._guess_pattern.search(self.original_url)
-            if not version_match:
-                version_guess = None
-            elif self._guess_pattern.groups == 0:
-                version_guess = version_match.group(0)
-            else:
-                version_guess = ".".join(version_match.groups())
-        else:
-            version_guess = self._version
+        version_guess = self._version
+        if version_guess is None:
+            version_guess = utils.guess_version(self.original_url, pattern=self._guess_pattern)
 
         return [
             self.create_source_info(

src/buildstream/source.py

@@ -1089,7 +1089,7 @@ def collect_source_info(self) -> Iterable[SourceInfo]:
         .. note::
 
            If your plugin uses :class:`.SourceFetcher` objects, you can implement
-           :func:`Source.collect_source_info() <buildstream.source.SourceFetcher.get_source_info>` instead.
+           :func:`Source.get_source_info() <buildstream.source.SourceFetcher.get_source_info>` instead.
 
         *Since: 2.5*
         """

src/buildstream/utils.py

@@ -35,7 +35,7 @@
 import itertools
 from contextlib import contextmanager
 from pathlib import Path
-from typing import Callable, IO, Iterable, Iterator, Optional, Tuple, Union
+from typing import Callable, IO, Iterable, Iterator, Optional, Tuple, Union, Pattern
 from google.protobuf import timestamp_pb2
 
 import psutil
@@ -66,6 +66,10 @@
 # it might not work
 _USE_CP_FILE_RANGE = hasattr(os, "copy_file_range")
 
+# The default version guessing pattern for utils.guess_version()
+#
+_DEFAULT_GUESS_PATTERN = re.compile(r"(\d+)\.(\d+)(?:\.(\d+))?")
+
 
 class UtilError(BstError):
     """Raised by utility functions when system calls fail.
@@ -697,15 +701,87 @@ def cleanup_tempfile():
 
 # get_umask():
 #
-# Get the process's file mode creation mask without changing it.
+#
 #
 # Returns:
-#     (int) The process's file mode creation mask.
+#     (int)
 #
-def get_umask():
+def get_umask() -> int:
+    """
+    Get the process's file mode creation mask without changing it.
+
+    Returns: The process's file mode creation mask.
+    """
     return _UMASK
 
 
+def guess_version(string: str, *, pattern: Optional[Pattern[str]] = None) -> Optional[str]:
+    """
+    Attempt to extract a version from an arbitrary string.
+
+    This function is used by sources who implement
+    :func:`Source.get_source_info() <buildstream.source.SourceFetcher.get_source_info>`
+    in order to provide a guess at what the version is, given some domain specific
+    knowledge such as a git tag or a tarball URL.
+
+    This function will be traverse the provided string for non-overlapping matches, and
+    in the case of *optional groups* being specified in the pattern; the match with the
+    greatest amount of matched groups will be preferred, allowing for correct handling
+    of cases like: ``https://example.com/releases/1.2/release-1.2.3.tgz`` which may
+    match the *pattern* multiple times.
+
+    The resulting version will be the captured groups, separated by ``.`` characters.
+
+    Args:
+       string: The domain specific string to scan for a version
+       pattern: A compiled regex pattern to scan *string*, or None for the default ``(\\d+)\\.(\\d+)(?:\\.(\\d+))?``.
+
+    Returns:
+       The guessed version, or None if no match was found.
+
+    .. note::
+
+       **Specifying a pattern**
+
+       When specifying the pattern, any number of capture groups may be specified, and
+       the match containing the most matching groups will be selected.
+
+       The capture groups must contain only the intended result and not any separating
+       characters.
+
+       For example, you may parse a string such as ``release-1_2_3-r2`` with the pattern:
+       ``(\\d+)_(\\d+)(?:_(\\d+))?(?:\\-(r\\d+))?``, and this would produce the parsed
+       version ``1.2.3.r2``.
+
+    **Since: 2.5**.
+    """
+    version_guess: Optional[str] = None
+    version_guess_groups = 0
+
+    if pattern is None:
+        pattern = _DEFAULT_GUESS_PATTERN
+
+    # Iterate over non-overlapping matches, and prefer a match which is more qualified (i.e. 1.2.3 is better than 1.2)
+    for version_match in pattern.finditer(string):
+
+        if not version_match:
+            iter_guess = None
+            iter_n_groups = 0
+        elif pattern.groups == 0:
+            iter_guess = str(version_match.group(0))
+            iter_n_groups = 1
+        else:
+            iter_groups = [group for group in version_match.groups() if group is not None]
+            iter_n_groups = len(iter_groups)
+            iter_guess = ".".join(iter_groups)
+
+        if version_guess is None or iter_n_groups > version_guess_groups:
+            version_guess = iter_guess
+            version_guess_groups = iter_n_groups
+
+    return version_guess
+
+
 # _get_host_tool_internal():
 #
 # Get the full path of a host tool, including tools bundled inside the Python package.

tests/frontend/show.py

@@ -594,7 +594,7 @@ def test_invalid_alias(cli, tmpdir, datafiles):
         (
             "tar.bst",
             "tar",
-            "https://flying-ponies.com/releases/pony-flight-1.2.3.tgz",
+            "https://flying-ponies.com/releases/1.2/pony-flight-1.2.3.tgz",
             "remote-file",
             "sha256",
             "9d0c936c78d0dfe3a67cae372c9a2330476ea87a2eec16b2daada64a664ca501",

tests/frontend/source-info/elements/tar.bst

@@ -2,5 +2,5 @@ kind: import
 
 sources:
 - kind: tar
-  url: https://flying-ponies.com/releases/pony-flight-1.2.3.tgz
+  url: https://flying-ponies.com/releases/1.2/pony-flight-1.2.3.tgz
   ref: 9d0c936c78d0dfe3a67cae372c9a2330476ea87a2eec16b2daada64a664ca501