From e9cd0f315750a4b313768165b0b80ea1e81416d5 Mon Sep 17 00:00:00 2001 From: Janak Ramakrishnan Date: Fri, 29 Apr 2016 22:46:16 +0000 Subject: 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 --- src/main/java/com/google/devtools/build/lib/BUILD | 1 + .../devtools/build/lib/flags/CommandNameCache.java | 41 ++++++++++++ .../build/lib/flags/InvocationPolicyEnforcer.java | 34 ++++++---- .../devtools/build/lib/runtime/BlazeRuntime.java | 3 + .../build/lib/runtime/CommandNameCacheImpl.java | 74 ++++++++++++++++++++++ 5 files changed, 140 insertions(+), 13 deletions(-) create mode 100644 src/main/java/com/google/devtools/build/lib/flags/CommandNameCache.java create mode 100644 src/main/java/com/google/devtools/build/lib/runtime/CommandNameCacheImpl.java (limited to 'src/main/java/com/google/devtools') diff --git a/src/main/java/com/google/devtools/build/lib/BUILD b/src/main/java/com/google/devtools/build/lib/BUILD index 0faafca8da..e1b65493c7 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 @@ java_library( ]), 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 0000000000..9c3b90aca0 --- /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 get(String commandName) { + return delegate.get(commandName); + } + } + + /** Returns the names of all commands {@code commandName} inherits from, including itself. */ + ImmutableSet 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 abeaa59e49..a61c432f0c 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.Functions; 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 @@ public final class InvocationPolicyEnforcer { * @throws OptionsParsingException if any flag policy is invalid. */ public void enforce(OptionsParser parser) throws OptionsParsingException { - enforce(parser, ""); + enforce(parser, null); } /** @@ -118,29 +119,36 @@ public final class InvocationPolicyEnforcer { * * @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 commandAndParentCommands = + command == null + ? ImmutableSet.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 25f38d5fd2..58f09d8ce5 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.BuildOptions; 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 @@ public final class BlazeRuntime { 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 0000000000..dd40207ecb --- /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 commandMap; + private final Map> cache = new HashMap<>(); + + CommandNameCacheImpl(Map 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() { + @Override + public Command apply(BlazeCommand blazeCommand) { + return blazeCommand.getClass().getAnnotation(Command.class); + } + }); + } + + @Override + public ImmutableSet get(String commandName) { + ImmutableSet cachedResult = cache.get(commandName); + if (cachedResult != null) { + return cachedResult; + } + ImmutableSet.Builder builder = ImmutableSet.builder(); + + Command command = Preconditions.checkNotNull(commandMap.get(commandName), commandName); + Set visited = new HashSet<>(); + visited.add(command); + Queue queue = new ArrayDeque<>(); + queue.add(command); + while (!queue.isEmpty()) { + Command cur = queue.remove(); + builder.add(cur.name()); + for (Class 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; + } +} -- cgit v1.2.3