drivefs-uprev: Fix version comparison

Use distutils.version.LooseVersion to compare the DriveFS tags being
ingested by the PUpr builder. Tags are in the form
refs/tags/drivefs_1.2.3 and |LooseVersion| will numerically compare
major, minor then patch version.

BUG=chromium:1118212
TEST=run_pytest
TEST=https://chromeos-swarming.appspot.com/task?id=5081edbc2491c410

Change-Id: Ie34a294c1015f5fd8601cf34b7378797c7bdb3d3
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/chromite/+/2592549
Tested-by: Ben Reich <benreich@chromium.org>
Reviewed-by: Mike Frysinger <vapier@chromium.org>
Reviewed-by: Chris McDonald <cjmcdonald@chromium.org>
Commit-Queue: Chris McDonald <cjmcdonald@chromium.org>
diff --git a/service/packages.py b/service/packages.py
index 53ed741..4a152b9 100644
--- a/service/packages.py
+++ b/service/packages.py
@@ -8,12 +8,14 @@
 from __future__ import print_function
 
 import collections
+from distutils.version import LooseVersion
 import fileinput
 import functools
 import json
 import os
 import re
 import sys
+from typing import List
 
 from google.protobuf import json_format
 
@@ -294,25 +296,20 @@
     UprevVersionedPackageResult: The result of updating drivefs ebuilds.
   """
 
-  DRIVEFS_REFS = 'refs/tags/drivefs_'
-  DRIVEFS_PATH = 'src/private-overlays/chromeos-overlay/chromeos-base'
+  DRIVEFS_PATH_PREFIX = 'src/private-overlays/chromeos-overlay/chromeos-base'
 
-  # parse like the example PUpr (go/pupr#steps)
-  valid_refs = [ref.ref for ref in refs if ref.ref.startswith(DRIVEFS_REFS)]
-  if not valid_refs:
-    # False alarm, there is no new package release. So do nothing.
+  drivefs_version = get_latest_drivefs_version_from_refs(refs)
+  if not drivefs_version:
+    # No valid DriveFS version is identified.
     return None
 
-  # Take the newest release, always.
-  target_version = sorted(valid_refs,
-                          reverse=True)[0][len(DRIVEFS_REFS):]
+  logging.debug('DriveFS version determined from refs: %s', drivefs_version)
 
   result = uprev_lib.UprevVersionedPackageResult()
 
-  pkg_path = os.path.join(DRIVEFS_PATH, 'drivefs')
-
+  pkg_path = os.path.join(DRIVEFS_PATH_PREFIX, 'drivefs')
   uprev_result = uprev_lib.uprev_workon_ebuild_to_version(pkg_path,
-                                                          target_version,
+                                                          drivefs_version,
                                                           chroot)
   all_changed_files = []
 
@@ -321,10 +318,10 @@
 
   all_changed_files.extend(uprev_result.changed_files)
 
-  pkg_path = os.path.join(DRIVEFS_PATH, 'drivefs-ipc')
+  pkg_path = os.path.join(DRIVEFS_PATH_PREFIX, 'drivefs-ipc')
 
   uprev_result = uprev_lib.uprev_workon_ebuild_to_version(pkg_path,
-                                                          target_version,
+                                                          drivefs_version,
                                                           chroot)
 
   if not uprev_result:
@@ -332,7 +329,7 @@
 
   all_changed_files.extend(uprev_result.changed_files)
 
-  result.add_result(target_version, all_changed_files)
+  result.add_result(drivefs_version, all_changed_files)
 
   return result
 
@@ -499,6 +496,36 @@
   return result.add_result(chrome_version, uprev_manager.modified_ebuilds)
 
 
+def get_latest_drivefs_version_from_refs(refs: List[uprev_lib.GitRef]) -> str:
+  """Get the latest DriveFS version from refs
+
+  DriveFS versions follow the tag format of refs/tags/drivefs_1.2.3.
+  Versions are compared using |distutils.version.LooseVersion| and
+  the latest version is returned.
+
+  Args:
+    refs: The tags to parse for the latest DriveFS version.
+
+  Returns:
+    The latest DriveFS version to use.
+  """
+  DRIVEFS_REFS_PREFIX = 'refs/tags/drivefs_'
+
+  valid_refs = []
+  for gitiles in refs:
+    if gitiles.ref.startswith(DRIVEFS_REFS_PREFIX):
+      valid_refs.append(gitiles.ref)
+
+  if not valid_refs:
+    return None
+
+  # Sort by version and take the latest version.
+  target_version_ref = sorted(valid_refs,
+                              key=LooseVersion,
+                              reverse=True)[0]
+  return target_version_ref.replace(DRIVEFS_REFS_PREFIX, '')
+
+
 def _generate_platform_c_files(replication_config, chroot):
   """Generates platform C files from a platform JSON payload.
 
diff --git a/service/packages_unittest.py b/service/packages_unittest.py
index 9bcae74..ad9183e 100644
--- a/service/packages_unittest.py
+++ b/service/packages_unittest.py
@@ -1054,3 +1054,31 @@
                      return_value=['key1', 'key2'])
     result = packages.get_key_id(self.build_target, 'model')
     self.assertEqual(result, None)
+
+
+class GetLatestDrivefsVersionTest(cros_test_lib.TestCase):
+  """Tests for get_latest_drivefs_version_from_refs."""
+
+  def setUp(self):
+    # The tag ref template.
+    ref_tpl = 'refs/tags/drivefs_%s'
+
+    self.latest = '44.0.20'
+    self.versions = ['42.0.1', self.latest, '44.0.19', '39.0.15']
+    self.latest_ref = uprev_lib.GitRef('/path', ref_tpl % self.latest, 'abc123')
+    self.refs = [uprev_lib.GitRef('/path', ref_tpl % v, 'abc123')
+                 for v in self.versions]
+
+  def test_single_ref(self):
+    """Test a single ref is supplied."""
+    self.assertEqual(self.latest,
+        packages.get_latest_drivefs_version_from_refs([self.latest_ref]))
+
+  def test_multiple_ref_versions(self):
+    """Test multiple refs supplied."""
+    self.assertEqual(self.latest,
+        packages.get_latest_drivefs_version_from_refs(self.refs))
+
+  def test_no_refs_returns_none(self):
+    """Test no refs supplied."""
+    self.assertEqual(packages.get_latest_drivefs_version_from_refs([]), None)