[6.3.0] Let common ignore unsupported options (with added commit 3dc6951)

site/en/run/bazelrc.md

@@ -103,7 +103,14 @@ line specifies when these defaults are applied:
 
 -   `startup`: startup options, which go before the command, and are described
     in `bazel help startup_options`.
--   `common`: options that apply to all Bazel commands.
+-   `common`: options that should be applied to all Bazel commands that support
+    them. If a command does not support an option specified in this way, the
+    option is ignored so long as it is valid for *some* other Bazel command.
+    Note that this only applies to option names: If the current command accepts
+    an option with the specified name, but doesn't support the specified value,
+    it will fail.
+-   `always`: options that apply to all Bazel commands. If a command does not
+    support an option specified in this way, it will fail.
 -   _`command`_: Bazel command, such as `build` or `query` to which the options
     apply. These options also apply to all commands that inherit from the
     specified command. (For example, `test` inherits from `build`.)

src/main/java/com/google/devtools/build/lib/profiler/BUILD

@@ -34,6 +34,7 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/concurrent",
         "//src/main/java/com/google/devtools/build/lib/unix:procmeminfo_parser",
         "//src/main/java/com/google/devtools/build/lib/util:os",
+        "//src/main/java/com/google/devtools/build/lib/util:pair",
         "//src/main/java/com/google/devtools/build/lib/worker:worker_metric",
         "//src/main/java/com/google/devtools/common/options",
         "//third_party:auto_value",

src/main/java/com/google/devtools/build/lib/profiler/MemoryProfiler.java

@@ -18,14 +18,16 @@
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.MoreObjects;
 import com.google.common.base.Splitter;
+import com.google.devtools.build.lib.util.Pair;
 import com.google.devtools.common.options.OptionsParsingException;
 import java.io.OutputStream;
 import java.io.PrintStream;
 import java.lang.management.ManagementFactory;
 import java.lang.management.MemoryMXBean;
 import java.lang.management.MemoryUsage;
 import java.time.Duration;
-import java.util.Iterator;
+import java.util.ArrayList;
+import java.util.List;
 import java.util.NoSuchElementException;
 import javax.annotation.Nullable;
 
@@ -111,14 +113,29 @@ synchronized HeapAndNonHeap prepareBeanAndGetLocalMinUsage(
     bean.gc();
     MemoryUsage minHeapUsed = bean.getHeapMemoryUsage();
     MemoryUsage minNonHeapUsed = bean.getNonHeapMemoryUsage();
+
     if (nextPhase == ProfilePhase.FINISH && memoryProfileStableHeapParameters != null) {
-      for (int i = 1; i < memoryProfileStableHeapParameters.numTimesToDoGc; i++) {
-        sleeper.sleep(memoryProfileStableHeapParameters.timeToSleepBetweenGcs);
-        bean.gc();
-        MemoryUsage currentHeapUsed = bean.getHeapMemoryUsage();
-        if (currentHeapUsed.getUsed() < minHeapUsed.getUsed()) {
-          minHeapUsed = currentHeapUsed;
-          minNonHeapUsed = bean.getNonHeapMemoryUsage();
+      for (int j = 0; j < memoryProfileStableHeapParameters.gcSpecs.size(); j++) {
+        Pair<Integer, Duration> spec = memoryProfileStableHeapParameters.gcSpecs.get(j);
+
+        int numTimesToDoGc = spec.first;
+        Duration timeToSleepBetweenGcs = spec.second;
+
+        for (int i = 0; i < numTimesToDoGc; i++) {
+          // We want to skip the first cycle for the first spec, since we ran a
+          // GC at the top of this function, but all the rest should get their
+          // proper runs
+          if (j == 0 && i == 0) {
+            continue;
+          }
+
+          sleeper.sleep(timeToSleepBetweenGcs);
+          bean.gc();
+          MemoryUsage currentHeapUsed = bean.getHeapMemoryUsage();
+          if (currentHeapUsed.getUsed() < minHeapUsed.getUsed()) {
+            minHeapUsed = currentHeapUsed;
+            minNonHeapUsed = bean.getNonHeapMemoryUsage();
+          }
         }
       }
     }
@@ -130,12 +147,10 @@ synchronized HeapAndNonHeap prepareBeanAndGetLocalMinUsage(
    * build.
    */
   public static class MemoryProfileStableHeapParameters {
-    private final int numTimesToDoGc;
-    private final Duration timeToSleepBetweenGcs;
+    private final ArrayList<Pair<Integer, Duration>> gcSpecs;
 
-    private MemoryProfileStableHeapParameters(int numTimesToDoGc, Duration timeToSleepBetweenGcs) {
-      this.numTimesToDoGc = numTimesToDoGc;
-      this.timeToSleepBetweenGcs = timeToSleepBetweenGcs;
+    private MemoryProfileStableHeapParameters(ArrayList<Pair<Integer, Duration>> gcSpecs) {
+      this.gcSpecs = gcSpecs;
     }
 
     /** Converter for {@code MemoryProfileStableHeapParameters} option. */
@@ -147,40 +162,48 @@ public static class Converter
       @Override
       public MemoryProfileStableHeapParameters convert(String input)
           throws OptionsParsingException {
-        Iterator<String> values = SPLITTER.split(input).iterator();
+        List<String> values = SPLITTER.splitToList(input);
+
+        if (values.size() % 2 != 0) {
+          throw new OptionsParsingException(
+              "Expected even number of comma-separated integer values");
+        }
+
+        ArrayList<Pair<Integer, Duration>> gcSpecs = new ArrayList<>(values.size() / 2);
+
         try {
-          int numTimesToDoGc = Integer.parseInt(values.next());
-          int numSecondsToSleepBetweenGcs = Integer.parseInt(values.next());
-          if (values.hasNext()) {
-            throw new OptionsParsingException("Expected exactly 2 comma-separated integer values");
+          for (int i = 0; i < values.size(); i += 2) {
+            int numTimesToDoGc = Integer.parseInt(values.get(i));
+            int numSecondsToSleepBetweenGcs = Integer.parseInt(values.get(i + 1));
+
+            if (numTimesToDoGc <= 0) {
+              throw new OptionsParsingException("Number of times to GC must be positive");
+            }
+            if (numSecondsToSleepBetweenGcs < 0) {
+              throw new OptionsParsingException(
+                  "Number of seconds to sleep between GC's must be non-negative");
+            }
+            gcSpecs.add(Pair.of(numTimesToDoGc, Duration.ofSeconds(numSecondsToSleepBetweenGcs)));
           }
-          if (numTimesToDoGc <= 0) {
-            throw new OptionsParsingException("Number of times to GC must be positive");
-          }
-          if (numSecondsToSleepBetweenGcs < 0) {
-            throw new OptionsParsingException(
-                "Number of seconds to sleep between GC's must be non-negative");
-          }
-          return new MemoryProfileStableHeapParameters(
-              numTimesToDoGc, Duration.ofSeconds(numSecondsToSleepBetweenGcs));
+
+          return new MemoryProfileStableHeapParameters(gcSpecs);
         } catch (NumberFormatException | NoSuchElementException nfe) {
           throw new OptionsParsingException(
-              "Expected exactly 2 comma-separated integer values", nfe);
+              "Expected even number of comma-separated integer values, could not parse integer in"
+                  + " list",
+              nfe);
         }
       }
 
       @Override
       public String getTypeDescription() {
-        return "two integers, separated by a comma";
+        return "integers, separated by a comma expected in pairs";
       }
     }
 
     @Override
     public String toString() {
-      return MoreObjects.toStringHelper(this)
-          .add("numTimesToDoGc", numTimesToDoGc)
-          .add("timeToSleepBetweenGcs", timeToSleepBetweenGcs)
-          .toString();
+      return MoreObjects.toStringHelper(this).add("gcSpecs", gcSpecs).toString();
     }
   }
 

src/main/java/com/google/devtools/build/lib/runtime/BlazeOptionHandler.java

@@ -13,6 +13,8 @@
 // limitations under the License.
 package com.google.devtools.build.lib.runtime;
 
+import static com.google.common.collect.ImmutableList.toImmutableList;
+
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Joiner;
 import com.google.common.collect.ArrayListMultimap;
@@ -35,6 +37,7 @@
 import com.google.devtools.common.options.InvocationPolicyEnforcer;
 import com.google.devtools.common.options.OptionDefinition;
 import com.google.devtools.common.options.OptionPriority.PriorityCategory;
+import com.google.devtools.common.options.OptionsBase;
 import com.google.devtools.common.options.OptionsParser;
 import com.google.devtools.common.options.OptionsParsingException;
 import com.google.devtools.common.options.OptionsParsingResult;
@@ -66,6 +69,14 @@ public final class BlazeOptionHandler {
           "client_env",
           "client_cwd");
 
+  // All options set on this pseudo command are inherited by all commands, with unrecognized options
+  // resulting in an error.
+  private static final String ALWAYS_PSEUDO_COMMAND = "always";
+
+  // All options set on this pseudo command are inherited by all commands, with unrecognized options
+  // being ignored as long as they are recognized by at least one (other) command.
+  private static final String COMMON_PSEUDO_COMMAND = "common";
+
   // Marks an event to indicate a parsing error.
   static final String BAD_OPTION_TAG = "invalidOption";
   // Separates the invalid tag from the full error message for easier parsing.
@@ -78,6 +89,7 @@ public final class BlazeOptionHandler {
   private final Command commandAnnotation;
   private final InvocationPolicy invocationPolicy;
   private final List<String> rcfileNotes = new ArrayList<>();
+  private final ImmutableList<Class<? extends OptionsBase>> allOptionsClasses;
 
   BlazeOptionHandler(
       BlazeRuntime runtime,
@@ -92,6 +104,16 @@ public final class BlazeOptionHandler {
     this.commandAnnotation = commandAnnotation;
     this.optionsParser = optionsParser;
     this.invocationPolicy = invocationPolicy;
+    this.allOptionsClasses =
+        runtime.getCommandMap().values().stream()
+            .map(BlazeCommand::getClass)
+            .flatMap(
+                cmd ->
+                    BlazeCommandUtils.getOptions(
+                        cmd, runtime.getBlazeModules(), runtime.getRuleClassProvider())
+                        .stream())
+            .distinct()
+            .collect(toImmutableList());
   }
 
   /**
@@ -191,7 +213,36 @@ void parseRcOptions(
                   "%s:\n  %s'%s' options: %s",
                   source, inherited, commandToParse, Joiner.on(' ').join(rcArgs.getArgs())));
         }
-        optionsParser.parse(PriorityCategory.RC_FILE, rcArgs.getRcFile(), rcArgs.getArgs());
+        if (commandToParse.equals(COMMON_PSEUDO_COMMAND)) {
+          // Pass in options data for all commands supported by the runtime so that options that
+          // apply to some but not the current command can be ignored.
+          //
+          // Important note: The consistency checks performed by
+          // OptionsParser#getFallbackOptionsData ensure that there aren't any two options across
+          // all commands that have the same name but parse differently (e.g. because one accepts
+          // a value and the other doesn't). This means that the options available on a command
+          // limit the options available on other commands even without command inheritance. This
+          // restriction is necessary to ensure that the options specified on the "common"
+          // pseudo command can be parsed unambiguously.
+          ImmutableList<String> ignoredArgs =
+              optionsParser.parseWithSourceFunction(
+                  PriorityCategory.RC_FILE,
+                  o -> rcArgs.getRcFile(),
+                  rcArgs.getArgs(),
+                  OptionsParser.getFallbackOptionsData(allOptionsClasses));
+          if (!ignoredArgs.isEmpty()) {
+            // Append richer information to the note.
+            int index = rcfileNotes.size() - 1;
+            String note = rcfileNotes.get(index);
+            note +=
+                String.format(
+                    "\n  Ignored as unsupported by '%s': %s",
+                    commandAnnotation.name(), Joiner.on(' ').join(ignoredArgs));
+            rcfileNotes.set(index, note);
+          }
+        } else {
+          optionsParser.parse(PriorityCategory.RC_FILE, rcArgs.getRcFile(), rcArgs.getArgs());
+        }
       }
     }
   }
@@ -227,7 +278,8 @@ private void parseArgsAndConfigs(List<String> args, ExtendedEventHandler eventHa
     optionsParser.parseWithSourceFunction(
         PriorityCategory.COMMAND_LINE,
         commandOptionSourceFunction,
-        defaultOverridesAndRcSources.build());
+        defaultOverridesAndRcSources.build(),
+        /* fallbackData= */ null);
 
     // Command-specific options from .blazerc passed in via --default_override and --rc_source.
     ClientOptions rcFileOptions = optionsParser.getOptions(ClientOptions.class);
@@ -241,7 +293,10 @@ private void parseArgsAndConfigs(List<String> args, ExtendedEventHandler eventHa
 
     // Parses the remaining command-line options.
     optionsParser.parseWithSourceFunction(
-        PriorityCategory.COMMAND_LINE, commandOptionSourceFunction, remainingCmdLine.build());
+        PriorityCategory.COMMAND_LINE,
+        commandOptionSourceFunction,
+        remainingCmdLine.build(),
+        /* fallbackData= */ null);
 
     if (commandAnnotation.builds()) {
       // splits project files from targets in the traditional sense
@@ -372,14 +427,17 @@ void expandConfigOptions(
     ConfigExpander.expandConfigOptions(
         eventHandler,
         commandToRcArgs,
+        commandAnnotation.name(),
         getCommandNamesToParse(commandAnnotation),
         rcfileNotes::add,
-        optionsParser);
+        optionsParser,
+        OptionsParser.getFallbackOptionsData(allOptionsClasses));
   }
 
   private static List<String> getCommandNamesToParse(Command commandAnnotation) {
     List<String> result = new ArrayList<>();
-    result.add("common");
+    result.add(ALWAYS_PSEUDO_COMMAND);
+    result.add(COMMON_PSEUDO_COMMAND);
     getCommandNamesToParseHelper(commandAnnotation, result);
     return result;
   }
@@ -470,7 +528,9 @@ static ListMultimap<String, RcChunkOfArgs> structureRcOptionsAndConfigs(
       if (index > 0) {
         command = command.substring(0, index);
       }
-      if (!validCommands.contains(command) && !command.equals("common")) {
+      if (!validCommands.contains(command)
+          && !command.equals(ALWAYS_PSEUDO_COMMAND)
+          && !command.equals(COMMON_PSEUDO_COMMAND)) {
         eventHandler.handle(
             Event.warn(
                 "while reading option defaults file '"

src/main/java/com/google/devtools/build/lib/runtime/BlazeRuntime.java

@@ -1110,7 +1110,8 @@ private static OptionsParsingResult parseStartupOptions(
 
     // Then parse the command line again, this time with the correct option sources
     parser = OptionsParser.builder().optionsClasses(optionClasses).allowResidue(false).build();
-    parser.parseWithSourceFunction(PriorityCategory.COMMAND_LINE, sourceFunction, args);
+    parser.parseWithSourceFunction(
+        PriorityCategory.COMMAND_LINE, sourceFunction, args, /* fallbackData= */ null);
     return parser;
   }
 

src/main/java/com/google/devtools/build/lib/runtime/CommonCommandOptions.java

@@ -337,9 +337,11 @@ public String getTypeDescription() {
       effectTags = {OptionEffectTag.BAZEL_MONITORING},
       converter = MemoryProfileStableHeapParameters.Converter.class,
       help =
-          "Tune memory profile's computation of stable heap at end of build. Should be two"
-              + " integers separated by a comma. First parameter is the number of GCs to perform."
-              + " Second parameter is the number of seconds to wait between GCs.")
+          "Tune memory profile's computation of stable heap at end of build. Should be and even"
+              + " number of  integers separated by commas. In each pair the first integer is the"
+              + " number of GCs to perform. The second integer in each pair is the number of"
+              + " seconds to wait between GCs. Ex: 2,4,4,0 would 2 GCs with a 4sec pause, followed"
+              + " by 4 GCs with zero second pause")
   public MemoryProfileStableHeapParameters memoryProfileStableHeapParameters;
 
   @Option(