In InvocationPolicyEnforcer#enforce, instead of just checking the direct command to see if a flag policy applies, check whether the flag applies by seeing if its list of commands matches one of the commands in the hierarchy.
This avoids the tedious and brittle specification of all commands that may use a flag, while providing better filtering out of inapplicable flags.
RELNOTES: A FlagPolicy specified via the --invocation_policy flag will now match the current command if any of its commands matches any of the commands the current command inherits from, as opposed to just the current command.
--
MOS_MIGRATED_REVID=121159131
diff --git a/src/main/java/com/google/devtools/build/lib/BUILD b/src/main/java/com/google/devtools/build/lib/BUILD
index 0faafca..e1b65493 100644
--- a/src/main/java/com/google/devtools/build/lib/BUILD
+++ b/src/main/java/com/google/devtools/build/lib/BUILD
@@ -111,6 +111,7 @@
]),
deps = [
":io",
+ ":preconditions",
"//src/main/java/com/google/devtools/common/options",
"//src/main/protobuf:invocation_policy_java_proto",
"//third_party:guava",
diff --git a/src/main/java/com/google/devtools/build/lib/flags/CommandNameCache.java b/src/main/java/com/google/devtools/build/lib/flags/CommandNameCache.java
new file mode 100644
index 0000000..9c3b90a
--- /dev/null
+++ b/src/main/java/com/google/devtools/build/lib/flags/CommandNameCache.java
@@ -0,0 +1,41 @@
+// Copyright 2016 The Bazel Authors. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.package com.google.devtools.build.lib.flags;
+package com.google.devtools.build.lib.flags;
+
+import com.google.common.collect.ImmutableSet;
+
+/** Cache mapping a command to the names of all commands it inherits from, including itself. */
+public interface CommandNameCache {
+ /** Class that exists only to expose a static instance variable that can be set and retrieved. */
+ class CommandNameCacheInstance implements CommandNameCache {
+ public static final CommandNameCacheInstance INSTANCE = new CommandNameCacheInstance();
+ private CommandNameCache delegate;
+
+ private CommandNameCacheInstance() {}
+
+ /** Only for use by {@code BlazeRuntime}. */
+ public void setCommandNameCache(CommandNameCache cache) {
+ // Can be set multiple times in tests.
+ this.delegate = cache;
+ }
+
+ @Override
+ public ImmutableSet<String> get(String commandName) {
+ return delegate.get(commandName);
+ }
+ }
+
+ /** Returns the names of all commands {@code commandName} inherits from, including itself. */
+ ImmutableSet<String> get(String commandName);
+}
diff --git a/src/main/java/com/google/devtools/build/lib/flags/InvocationPolicyEnforcer.java b/src/main/java/com/google/devtools/build/lib/flags/InvocationPolicyEnforcer.java
index abeaa59..a61c432 100644
--- a/src/main/java/com/google/devtools/build/lib/flags/InvocationPolicyEnforcer.java
+++ b/src/main/java/com/google/devtools/build/lib/flags/InvocationPolicyEnforcer.java
@@ -19,6 +19,7 @@
import com.google.common.base.Strings;
import com.google.common.base.Verify;
import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Sets;
import com.google.common.io.BaseEncoding;
import com.google.devtools.build.lib.runtime.proto.InvocationPolicyOuterClass.AllowValues;
@@ -110,7 +111,7 @@
* @throws OptionsParsingException if any flag policy is invalid.
*/
public void enforce(OptionsParser parser) throws OptionsParsingException {
- enforce(parser, "");
+ enforce(parser, null);
}
/**
@@ -118,29 +119,36 @@
*
* @param parser The OptionsParser to enforce policy on.
* @param command The current blaze command, for flag policies that apply to only specific
- * commands.
+ * commands. Such policies will be enforced only if they contain this command or a command
+ * they inherit from
* @throws OptionsParsingException if any flag policy is invalid.
*/
- public void enforce(OptionsParser parser, String command) throws OptionsParsingException {
+ public void enforce(OptionsParser parser, @Nullable String command)
+ throws OptionsParsingException {
if (invocationPolicy == null || invocationPolicy.getFlagPoliciesCount() == 0) {
return;
}
+ ImmutableSet<String> commandAndParentCommands =
+ command == null
+ ? ImmutableSet.<String>of()
+ : CommandNameCache.CommandNameCacheInstance.INSTANCE.get(command);
for (FlagPolicy flagPolicy : invocationPolicy.getFlagPoliciesList()) {
String flagName = flagPolicy.getFlagName();
// Skip the flag policy if it doesn't apply to this command. If the commands list is empty,
// then the policy applies to all commands.
- if (!flagPolicy.getCommandsList().isEmpty()
- && !flagPolicy.getCommandsList().contains(command)) {
- log.info(
- String.format(
- "Skipping flag policy for flag '%s' because it "
- + "applies only to commands %s and the current command is '%s'",
- flagName,
- flagPolicy.getCommandsList(),
- command));
- continue;
+ if (!flagPolicy.getCommandsList().isEmpty() && !commandAndParentCommands.isEmpty()) {
+ boolean flagApplies = false;
+ for (String policyCommand : flagPolicy.getCommandsList()) {
+ if (commandAndParentCommands.contains(policyCommand)) {
+ flagApplies = true;
+ break;
+ }
+ }
+ if (!flagApplies) {
+ continue;
+ }
}
OptionValueDescription valueDescription;
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
index 25f38d5..58f09d8 100644
--- a/src/main/java/com/google/devtools/build/lib/runtime/BlazeRuntime.java
+++ b/src/main/java/com/google/devtools/build/lib/runtime/BlazeRuntime.java
@@ -36,6 +36,7 @@
import com.google.devtools.build.lib.analysis.config.ConfigurationFactory;
import com.google.devtools.build.lib.events.Event;
import com.google.devtools.build.lib.events.OutputFilter;
+import com.google.devtools.build.lib.flags.CommandNameCache;
import com.google.devtools.build.lib.packages.PackageFactory;
import com.google.devtools.build.lib.packages.Preprocessor;
import com.google.devtools.build.lib.packages.RuleClassProvider;
@@ -185,6 +186,8 @@
this.defaultsPackageContent =
ruleClassProvider.getDefaultsPackageContent(getInvocationPolicy());
+ CommandNameCache.CommandNameCacheInstance.INSTANCE.setCommandNameCache(
+ new CommandNameCacheImpl(getCommandMap()));
}
private static InvocationPolicy createInvocationPolicyFromModules(
diff --git a/src/main/java/com/google/devtools/build/lib/runtime/CommandNameCacheImpl.java b/src/main/java/com/google/devtools/build/lib/runtime/CommandNameCacheImpl.java
new file mode 100644
index 0000000..dd40207
--- /dev/null
+++ b/src/main/java/com/google/devtools/build/lib/runtime/CommandNameCacheImpl.java
@@ -0,0 +1,74 @@
+// Copyright 2016 The Bazel Authors. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+package com.google.devtools.build.lib.runtime;
+
+import com.google.common.base.Function;
+import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.Maps;
+import com.google.devtools.build.lib.flags.CommandNameCache;
+import com.google.devtools.build.lib.util.Preconditions;
+
+import java.util.ArrayDeque;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.Map;
+import java.util.Queue;
+import java.util.Set;
+
+class CommandNameCacheImpl implements CommandNameCache {
+ private final Map<String, Command> commandMap;
+ private final Map<String, ImmutableSet<String>> cache = new HashMap<>();
+
+ CommandNameCacheImpl(Map<String, BlazeCommand> commandMap) {
+ // Note: it is important that this map is live, since the commandMap may be altered
+ // post-creation.
+ this.commandMap =
+ Maps.transformValues(
+ commandMap,
+ new Function<BlazeCommand, Command>() {
+ @Override
+ public Command apply(BlazeCommand blazeCommand) {
+ return blazeCommand.getClass().getAnnotation(Command.class);
+ }
+ });
+ }
+
+ @Override
+ public ImmutableSet<String> get(String commandName) {
+ ImmutableSet<String> cachedResult = cache.get(commandName);
+ if (cachedResult != null) {
+ return cachedResult;
+ }
+ ImmutableSet.Builder<String> builder = ImmutableSet.builder();
+
+ Command command = Preconditions.checkNotNull(commandMap.get(commandName), commandName);
+ Set<Command> visited = new HashSet<>();
+ visited.add(command);
+ Queue<Command> queue = new ArrayDeque<>();
+ queue.add(command);
+ while (!queue.isEmpty()) {
+ Command cur = queue.remove();
+ builder.add(cur.name());
+ for (Class<? extends BlazeCommand> clazz : cur.inherits()) {
+ Command parent = clazz.getAnnotation(Command.class);
+ if (visited.add(parent)) {
+ queue.add(parent);
+ }
+ }
+ }
+ cachedResult = builder.build();
+ cache.put(commandName, cachedResult);
+ return cachedResult;
+ }
+}
diff --git a/src/main/protobuf/invocation_policy.proto b/src/main/protobuf/invocation_policy.proto
index d74cc86..554c9fc 100644
--- a/src/main/protobuf/invocation_policy.proto
+++ b/src/main/protobuf/invocation_policy.proto
@@ -43,12 +43,15 @@
// the flag's full name and explicitly set it to true or false.
optional string flag_name = 1;
- // If set, this flag policy is applied only if one of the given commands is
- // being run. If empty, this flag policy is applied for all commands.
- // This allows the policy setter to add all policies to the proto without
- // having to determine which Bazel command the user is actually running.
- // Additionally, Bazel allows multiple flags to be defined by the same name,
- // and the specific flag definition is determined by the command.
+ // If set, this flag policy is applied only if one of the given commands or a
+ // command that inherits from one of the given commands is being run. For
+ // instance, if "build" is one of the commands here, then this policy will
+ // apply to any command that inherits from build, such as info, coverage, or
+ // test. If empty, this flag policy is applied for all commands. This allows
+ // the policy setter to add all policies to the proto without having to
+ // determine which Bazel command the user is actually running. Additionally,
+ // Bazel allows multiple flags to be defined by the same name, and the
+ // specific flag definition is determined by the command.
repeated string commands = 2;
oneof operation {
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java b/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java
index f65452a..22fe257 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java
@@ -284,7 +284,7 @@
InvocationPolicyEnforcer optionsPolicyEnforcer =
new InvocationPolicyEnforcer(TestConstants.TEST_INVOCATION_POLICY);
- optionsPolicyEnforcer.enforce(optionsParser, "");
+ optionsPolicyEnforcer.enforce(optionsParser);
BuildOptions buildOptions = ruleClassProvider.createBuildOptions(optionsParser);
ensureTargetsVisited(buildOptions.getAllLabels().values());
diff --git a/src/test/java/com/google/devtools/build/lib/runtime/InvocationPolicyEnforcerTest.java b/src/test/java/com/google/devtools/build/lib/runtime/InvocationPolicyEnforcerTest.java
index a783100..25b026f 100644
--- a/src/test/java/com/google/devtools/build/lib/runtime/InvocationPolicyEnforcerTest.java
+++ b/src/test/java/com/google/devtools/build/lib/runtime/InvocationPolicyEnforcerTest.java
@@ -20,7 +20,9 @@
import static org.junit.Assert.assertTrue;
import static org.junit.Assert.fail;
+import com.google.common.collect.ImmutableSet;
import com.google.common.io.BaseEncoding;
+import com.google.devtools.build.lib.flags.CommandNameCache;
import com.google.devtools.build.lib.flags.InvocationPolicyEnforcer;
import com.google.devtools.build.lib.runtime.proto.InvocationPolicyOuterClass.InvocationPolicy;
import com.google.devtools.common.options.Option;
@@ -29,6 +31,7 @@
import com.google.devtools.common.options.OptionsParsingException;
import org.junit.Before;
+import org.junit.BeforeClass;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.JUnit4;
@@ -124,6 +127,17 @@
parser = OptionsParser.newOptionsParser(TestOptions.class);
}
+ @BeforeClass
+ public static void setCommandNameCache() throws Exception {
+ CommandNameCache.CommandNameCacheInstance.INSTANCE.setCommandNameCache(
+ new CommandNameCache() {
+ @Override
+ public ImmutableSet<String> get(String commandName) {
+ return ImmutableSet.of(commandName);
+ }
+ });
+ }
+
private TestOptions getTestOptions() {
return parser.getOptions(TestOptions.class);
}
