llvm_tools: Fix manifest update

Usually we don't need the manifest update with llvm-next
because pgo is not default there.

Remove default manifest packages from llvm-next.
Add unit test for main.

BUG=None
TEST=unit test

Change-Id: I1ef78be8184985e047db8ae68eda2f01c989a7a5
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/third_party/toolchain-utils/+/3932864
Tested-by: Denis Nikitin <denik@chromium.org>
Commit-Queue: Denis Nikitin <denik@chromium.org>
Reviewed-by: Manoj Gupta <manojgupta@chromium.org>

2 files changed, 139 insertions, 9 deletions

diff --git a/llvm_tools/update_chromeos_llvm_hash.py b/llvm_tools/update_chromeos_llvm_hash.py
index 31a10867..c52c7328 100755
--- a/llvm_tools/update_chromeos_llvm_hash.py
+++ b/llvm_tools/update_chromeos_llvm_hash.py
@@ -17,7 +17,7 @@ import os
 from pathlib import Path
 import re
 import subprocess
-from typing import Dict, List
+from typing import Dict, Iterable
 
 import chroot
 import failure_modes
@@ -99,7 +99,7 @@ def GetCommandLineArgs():
 
     parser.add_argument(
         "--manifest_packages",
-        default=",".join(DEFAULT_MANIFEST_PACKAGES),
+        default="",
         help="Comma-separated ebuilds to update manifests for "
         "(default: %(default)s)",
     )
@@ -506,7 +506,7 @@ def StagePackagesPatchResultsForCommit(package_info_dict, commit_messages):
     return commit_messages
 
 
-def UpdateManifests(packages: List[str], chroot_path: Path):
+def UpdateManifests(packages: Iterable[str], chroot_path: Path):
     """Updates manifest files for packages.
 
     Args:
@@ -524,8 +524,8 @@ def UpdateManifests(packages: List[str], chroot_path: Path):
 
 
 def UpdatePackages(
-    packages,
-    manifest_packages: List[str],
+    packages: Iterable[str],
+    manifest_packages: Iterable[str],
     llvm_variant,
     git_hash,
     svn_version,
@@ -672,7 +672,7 @@ def EnsurePackageMaskContains(chroot_path, git_hash):
 def UpdatePackagesPatchMetadataFile(
     chroot_path: Path,
     svn_version: int,
-    packages: List[str],
+    packages: Iterable[str],
     mode: failure_modes.FailureModes,
 ) -> Dict[str, patch_utils.PatchInfo]:
     """Updates the packages metadata file.
@@ -761,8 +761,14 @@ def main():
         git_hash_source
     )
 
-    packages = args_output.update_packages.split(",")
-    manifest_packages = args_output.manifest_packages.split(",")
+    # Filter out empty strings. For example "".split{",") returns [""].
+    packages = set(p for p in args_output.update_packages.split(",") if p)
+    manifest_packages = set(
+        p for p in args_output.manifest_packages.split(",") if p
+    )
+    if not manifest_packages and not args_output.is_llvm_next:
+        # Set default manifest packages only for the current llvm.
+        manifest_packages = set(DEFAULT_MANIFEST_PACKAGES)
     change_list = UpdatePackages(
         packages=packages,
         manifest_packages=manifest_packages,
diff --git a/llvm_tools/update_chromeos_llvm_hash_unittest.py b/llvm_tools/update_chromeos_llvm_hash_unittest.py
index 9bed2712..b758538c 100755
--- a/llvm_tools/update_chromeos_llvm_hash_unittest.py
+++ b/llvm_tools/update_chromeos_llvm_hash_unittest.py
@@ -12,6 +12,7 @@ import datetime
 import os
 from pathlib import Path
 import subprocess
+import sys
 import unittest
 import unittest.mock as mock
 
@@ -19,7 +20,6 @@ import chroot
 import failure_modes
 import get_llvm_hash
 import git
-import subprocess_helpers
 import test_helpers
 import update_chromeos_llvm_hash
 
@@ -1017,6 +1017,130 @@ class UpdateLLVMHashTest(unittest.TestCase):
 
         mock_delete_repo.assert_called_once_with(path_to_package_dir, branch)
 
+    @mock.patch.object(chroot, "VerifyOutsideChroot")
+    @mock.patch.object(get_llvm_hash, "GetLLVMHashAndVersionFromSVNOption")
+    @mock.patch.object(update_chromeos_llvm_hash, "UpdatePackages")
+    def testMainDefaults(
+        self, mock_update_packages, mock_gethash, mock_outside_chroot
+    ):
+        git_hash = "1234abcd"
+        svn_version = 5678
+        mock_gethash.return_value = (git_hash, svn_version)
+        argv = [
+            "./update_chromeos_llvm_hash_unittest.py",
+            "--llvm_version",
+            "google3",
+        ]
+
+        with mock.patch.object(sys, "argv", argv) as mock.argv:
+            update_chromeos_llvm_hash.main()
+
+        expected_packages = set(update_chromeos_llvm_hash.DEFAULT_PACKAGES)
+        expected_manifest_packages = set(
+            update_chromeos_llvm_hash.DEFAULT_MANIFEST_PACKAGES,
+        )
+        expected_llvm_variant = update_chromeos_llvm_hash.LLVMVariant.current
+        expected_chroot = update_chromeos_llvm_hash.defaultCrosRoot()
+        mock_update_packages.assert_called_once_with(
+            packages=expected_packages,
+            manifest_packages=expected_manifest_packages,
+            llvm_variant=expected_llvm_variant,
+            git_hash=git_hash,
+            svn_version=svn_version,
+            chroot_path=expected_chroot,
+            mode=failure_modes.FailureModes.FAIL,
+            git_hash_source="google3",
+            extra_commit_msg=None,
+        )
+        mock_outside_chroot.assert_called()
+
+    @mock.patch.object(chroot, "VerifyOutsideChroot")
+    @mock.patch.object(get_llvm_hash, "GetLLVMHashAndVersionFromSVNOption")
+    @mock.patch.object(update_chromeos_llvm_hash, "UpdatePackages")
+    def testMainLlvmNext(
+        self, mock_update_packages, mock_gethash, mock_outside_chroot
+    ):
+        git_hash = "1234abcd"
+        svn_version = 5678
+        mock_gethash.return_value = (git_hash, svn_version)
+        argv = [
+            "./update_chromeos_llvm_hash_unittest.py",
+            "--llvm_version",
+            "google3",
+            "--is_llvm_next",
+        ]
+
+        with mock.patch.object(sys, "argv", argv) as mock.argv:
+            update_chromeos_llvm_hash.main()
+
+        expected_packages = set(update_chromeos_llvm_hash.DEFAULT_PACKAGES)
+        expected_llvm_variant = update_chromeos_llvm_hash.LLVMVariant.next
+        expected_chroot = update_chromeos_llvm_hash.defaultCrosRoot()
+        # llvm-next upgrade does not update manifest by default.
+        mock_update_packages.assert_called_once_with(
+            packages=expected_packages,
+            manifest_packages=set(),
+            llvm_variant=expected_llvm_variant,
+            git_hash=git_hash,
+            svn_version=svn_version,
+            chroot_path=expected_chroot,
+            mode=failure_modes.FailureModes.FAIL,
+            git_hash_source="google3",
+            extra_commit_msg=None,
+        )
+        mock_outside_chroot.assert_called()
+
+    @mock.patch.object(chroot, "VerifyOutsideChroot")
+    @mock.patch.object(get_llvm_hash, "GetLLVMHashAndVersionFromSVNOption")
+    @mock.patch.object(update_chromeos_llvm_hash, "UpdatePackages")
+    def testMainAllArgs(
+        self, mock_update_packages, mock_gethash, mock_outside_chroot
+    ):
+        packages_to_update = "test-packages/package1,test-libs/lib1"
+        manifest_packages = "test-libs/lib1,test-libs/lib2"
+        failure_mode = failure_modes.FailureModes.REMOVE_PATCHES
+        chroot_path = Path("/some/path/to/chroot")
+        llvm_ver = 435698
+        git_hash = "1234abcd"
+        svn_version = 5678
+        mock_gethash.return_value = (git_hash, svn_version)
+
+        argv = [
+            "./update_chromeos_llvm_hash_unittest.py",
+            "--llvm_version",
+            str(llvm_ver),
+            "--is_llvm_next",
+            "--chroot_path",
+            str(chroot_path),
+            "--update_packages",
+            packages_to_update,
+            "--manifest_packages",
+            manifest_packages,
+            "--failure_mode",
+            failure_mode.value,
+            "--patch_metadata_file",
+            "META.json",
+        ]
+
+        with mock.patch.object(sys, "argv", argv) as mock.argv:
+            update_chromeos_llvm_hash.main()
+
+        expected_packages = {"test-packages/package1", "test-libs/lib1"}
+        expected_manifest_packages = {"test-libs/lib1", "test-libs/lib2"}
+        expected_llvm_variant = update_chromeos_llvm_hash.LLVMVariant.next
+        mock_update_packages.assert_called_once_with(
+            packages=expected_packages,
+            manifest_packages=expected_manifest_packages,
+            llvm_variant=expected_llvm_variant,
+            git_hash=git_hash,
+            svn_version=svn_version,
+            chroot_path=chroot_path,
+            mode=failure_mode,
+            git_hash_source=llvm_ver,
+            extra_commit_msg=None,
+        )
+        mock_outside_chroot.assert_called()
+
     @mock.patch.object(subprocess, "check_output", return_value=None)
     @mock.patch.object(get_llvm_hash, "GetLLVMMajorVersion")
     def testEnsurePackageMaskContainsExisting(