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

) (#18609) * Change --memory_profile_stable_heap_parameters to accept more than one GC specification Currently memory_profile_stable_heap_parameters expects 2 ints and runs that many GCs with pauses between them 2nd param. This CL doesn't change that, but allows any arbitrary number of pairs to be provided that will run the same logic for each pair. This allows experimenting with forcing longer pauses on that thread before doing the quick GCs that allow for cleaner memory measurement. PiperOrigin-RevId: 485646588 Change-Id: Iff4f17cdaae409854f99397b4271bb5f87c4c404 * Let `common` ignore unsupported options Fixes #3054 RELNOTES: Options specified on the pseudo-command `common` in `.rc` files are now ignored by commands that do not support them as long as they are valid options for *any* Bazel command. Previously, commands that did not support all options given for `common` would fail to run. These previous semantics of `common` are now available via the new `always` pseudo-command. Closes #18130. PiperOrigin-RevId: 534940403 Change-Id: I2ae268ae84de3f2b07ff3d1e36bf382bce714968 --------- Co-authored-by: Googler <[email protected]> Co-authored-by: Fabian Meumertzheim <[email protected]>
bazelbuild · Jun 20, 2023 · e13c3c7 · e13c3c7
1 parent 092ef1c
commit e13c3c7
Show file tree

Hide file tree

Showing 20 changed files with 936 additions and 233 deletions.
diff --git a/site/en/run/bazelrc.md b/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`.)

diff --git a/src/main/java/com/google/devtools/build/lib/profiler/BUILD b/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",

diff --git a/src/main/java/com/google/devtools/build/lib/profiler/MemoryProfiler.java b/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();
     }
   }
 

diff --git a/src/main/java/com/google/devtools/build/lib/runtime/BlazeOptionHandler.java b/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 '"

diff --git a/src/main/java/com/google/devtools/build/lib/runtime/BlazeRuntime.java b/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;
   }
 

diff --git a/src/main/java/com/google/devtools/build/lib/runtime/CommonCommandOptions.java b/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(