Allow --deleted_packages to be passed multiple times.

Previously, later instances of this option overwrote previous ones, which was
not very intuitive. Now the lists are concatenated.

RELNOTES[INC]: When multiple --deleted_packages options are passed on the command line, they will be concatenated instead of the latest one taking effect.

PiperOrigin-RevId: 494652168
Change-Id: Ic6da550cff7add95fc8c2a4db72994902e3cbd89

site/en/docs/user-manual.md

@@ -83,7 +83,8 @@ Example: Building from an empty client
 This option specifies a comma-separated list of packages which Bazel
 should consider deleted, and not attempt to load from any directory
 on the package path. This can be used to simulate the deletion of packages without
-actually deleting them.
+actually deleting them. This option can be passed multiple times, in which case
+the individual lists are concatenated.
 
 ### Error checking {:#error-checking}
 

src/main/java/com/google/devtools/build/lib/pkgcache/PackageOptions.java

@@ -91,22 +91,22 @@ public ParallelismConverter() throws OptionsParsingException {
   public boolean showLoadingProgress;
 
   @Option(
-    name = "deleted_packages",
-    defaultValue = "",
-    converter = CommaSeparatedPackageNameListConverter.class,
-    documentationCategory = OptionDocumentationCategory.UNCATEGORIZED,
-    effectTags = {OptionEffectTag.UNKNOWN},
-    help =
-        "A comma-separated list of names of packages which the "
-            + "build system will consider non-existent, even if they are "
-            + "visible somewhere on the package path.\n"
-            + "Use this option when deleting a subpackage 'x/y' of an "
-            + "existing package 'x'.  For example, after deleting x/y/BUILD "
-            + "in your client, the build system may complain if it "
-            + "encounters a label '//x:y/z' if that is still provided by another "
-            + "package_path entry.  Specifying --deleted_packages x/y avoids this "
-            + "problem."
-  )
+      name = "deleted_packages",
+      allowMultiple = true,
+      defaultValue = "null",
+      converter = CommaSeparatedPackageNameListConverter.class,
+      documentationCategory = OptionDocumentationCategory.UNCATEGORIZED,
+      effectTags = {OptionEffectTag.UNKNOWN},
+      help =
+          "A comma-separated list of names of packages which the "
+              + "build system will consider non-existent, even if they are "
+              + "visible somewhere on the package path.\n"
+              + "Use this option when deleting a subpackage 'x/y' of an "
+              + "existing package 'x'.  For example, after deleting x/y/BUILD "
+              + "in your client, the build system may complain if it "
+              + "encounters a label '//x:y/z' if that is still provided by another "
+              + "package_path entry.  Specifying --deleted_packages x/y avoids this "
+              + "problem.")
   public List<PackageIdentifier> deletedPackages;
 
   @Option(

src/test/java/com/google/devtools/build/lib/pkgcache/PackageLoadingTest.java

@@ -483,7 +483,8 @@ public void testDeletedPackages() throws Exception {
     initializeSkyframeExecutor(/*doPackageLoadingChecks=*/ false);
     reporter.removeHandler(failFastHandler);
     setUpCacheWithTwoRootLocator();
-    createBuildFile(rootDir1, "c", "d/x");
+    createBuildFile(rootDir1, "c", "d/x", "e/x");
+    createBuildFile(rootDir1, "c/e", "x");
     // Now package c exists in both roots, and c/d exists in only in the second
     // root.  It's as if we've merged c and c/d in the first root.
 
@@ -493,6 +494,7 @@ public void testDeletedPackages() throws Exception {
 
     // Subpackage labels are still valid...
     assertLabelValidity(true, "//c/d:foo.txt");
+    assertLabelValidity(true, "//c/e:x");
     // ...and this crosses package boundaries:
     assertLabelValidity(false, "//c:d/x");
     assertPackageLoadingFails(
@@ -503,7 +505,7 @@ public void testDeletedPackages() throws Exception {
     assertThat(getPackageManager().isPackage(reporter, PackageIdentifier.createInMainRepo("c/d")))
         .isTrue();
 
-    setOptions("--deleted_packages=c/d");
+    setOptions("--package_path=/workspace:/otherroot", "--deleted_packages=c/d");
     invalidatePackages();
 
     assertThat(getPackageManager().isPackage(reporter, PackageIdentifier.createInMainRepo("c/d")))
@@ -521,6 +523,25 @@ public void testDeletedPackages() throws Exception {
     assertLabelValidity(false, "//c/d:x");
     // ...and now d is just a subdirectory of c:
     assertLabelValidity(true, "//c:d/x");
+
+    // Verify that multiple --deleted_packages options are concatenated
+    setOptions(
+        "--package_path=/workspace:/otherroot", "--deleted_packages=c/d", "--deleted_packages=c/e");
+    invalidatePackages();
+
+    assertLabelValidity(false, "//c/d:x");
+    assertLabelValidity(false, "//c/e:x");
+    assertLabelValidity(true, "//c:d/x");
+    assertLabelValidity(true, "//c:e/x");
+
+    // Verify that comma-separated values work, too
+    setOptions("--package_path=/workspace:/otherroot", "--deleted_packages=c/d,c/e");
+    invalidatePackages();
+
+    assertLabelValidity(false, "//c/d:x");
+    assertLabelValidity(false, "//c/e:x");
+    assertLabelValidity(true, "//c:d/x");
+    assertLabelValidity(true, "//c:e/x");
   }
 
   @Test