Rollforward of commit aa7f9307636d38cbb93a03acac8f4c59adfa0ee8.

*** Reason for rollback ***

Filter out conflicting flag policies for applicable commands. Only enforce the filtered, applicable policy.

*** Original change description ***

Automated g4 rollback of commit aa7f9307636d38cbb93a03acac8f4c59adfa0ee8.

*** Reason for rollback ***

Broke --experimental_inmemory_dotd_files

*** Original change description ***

Filter out conflicting flag policies before invocation policy enforcement.

This is to minimize the likelihood of obscure policy conflict. Now, the last policy on a flag (after policy expansion) will be the only one in the "canonical" invocation policy. There should be no reason for explicitly setting multiple policies on a single flag, but if an expansion flag is policy'd and one of its children has a more specific policy on it, make sure that the policy on the child flag is after the policy on the expansion flag.

Note that this restriction (only the last policy gets applied) also applies for repeatable flags. Make sure all values being set to a repeatable flag are set in a single SetValue operation, with multiple flagValues set.

PiperOrigin-RevId: 154969189

2 files changed, 39 insertions, 24 deletions

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 5a6e324bcb..c25f100f6d 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
@@ -36,7 +36,10 @@ import com.google.devtools.common.options.OptionsParser.OptionValueDescription;
 import com.google.devtools.common.options.OptionsParsingException;
 import java.util.ArrayList;
 import java.util.Arrays;
+import java.util.Collections;
+import java.util.HashMap;
 import java.util.List;
+import java.util.Map;
 import java.util.Set;
 import java.util.logging.Level;
 import java.util.logging.Logger;
@@ -96,30 +99,14 @@ public final class InvocationPolicyEnforcer {
     if (invocationPolicy == null || invocationPolicy.getFlagPoliciesCount() == 0) {
       return;
     }
-    List<FlagPolicy> effectivePolicy = getEffectivePolicy(invocationPolicy, parser);
 
-    ImmutableSet<String> commandAndParentCommands =
-        command == null
-            ? ImmutableSet.<String>of()
-            : CommandNameCache.CommandNameCacheInstance.INSTANCE.get(command);
+    // The effective policy returned is expanded, filtered for applicable commands, and cleaned of
+    // redundancies and conflicts.
+    List<FlagPolicy> effectivePolicy = getEffectivePolicy(invocationPolicy, parser, command);
+
     for (FlagPolicy flagPolicy : effectivePolicy) {
       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() && !commandAndParentCommands.isEmpty()) {
-        boolean flagApplies = false;
-        for (String policyCommand : flagPolicy.getCommandsList()) {
-          if (commandAndParentCommands.contains(policyCommand)) {
-            flagApplies = true;
-            break;
-          }
-        }
-        if (!flagApplies) {
-          continue;
-        }
-      }
-
       OptionValueDescription valueDescription;
       try {
         valueDescription = parser.getOptionValueDescription(flagName);
@@ -191,25 +178,52 @@ public final class InvocationPolicyEnforcer {
     }
   }
 
+  private static boolean policyApplies(FlagPolicy policy, ImmutableSet<String> applicableCommands) {
+    // 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 (policy.getCommandsList().isEmpty() || applicableCommands.isEmpty()) {
+      return true;
+    }
+
+    return !Collections.disjoint(policy.getCommandsList(), applicableCommands);
+  }
+
   /**
    * Takes the provided policy and processes it to the form that can be used on the user options.
    *
    * <p>Expands any policies on expansion flags.
    */
   public static List<FlagPolicy> getEffectivePolicy(
-      InvocationPolicy invocationPolicy, OptionsParser parser) throws OptionsParsingException {
+      InvocationPolicy invocationPolicy, OptionsParser parser, String command)
+      throws OptionsParsingException {
     if (invocationPolicy == null) {
       return ImmutableList.of();
     }
 
+    ImmutableSet<String> commandAndParentCommands =
+        command == null
+            ? ImmutableSet.<String>of()
+            : CommandNameCache.CommandNameCacheInstance.INSTANCE.get(command);
+
     // Expand all policies to transfer policies on expansion flags to policies on the child flags.
     List<FlagPolicy> expandedPolicies = new ArrayList<>();
     for (FlagPolicy policy : invocationPolicy.getFlagPoliciesList()) {
+      if (!policyApplies(policy, commandAndParentCommands)) {
+        // Only keep and expand policies that are applicable to the current command.
+        continue;
+      }
       List<FlagPolicy> policies = expandPolicy(policy, parser);
       expandedPolicies.addAll(policies);
     }
 
-    return expandedPolicies;
+    // Only keep that last policy for each flag.
+    Map<String, FlagPolicy> effectivePolicy = new HashMap<>();
+    for (FlagPolicy expandedPolicy : expandedPolicies) {
+      String flagName = expandedPolicy.getFlagName();
+      effectivePolicy.put(flagName, expandedPolicy);
+    }
+
+    return new ArrayList<>(effectivePolicy.values());
   }
 
   /**
diff --git a/src/main/java/com/google/devtools/build/lib/runtime/commands/CanonicalizeCommand.java b/src/main/java/com/google/devtools/build/lib/runtime/commands/CanonicalizeCommand.java
index 2831e6babe..8033509309 100644
--- a/src/main/java/com/google/devtools/build/lib/runtime/commands/CanonicalizeCommand.java
+++ b/src/main/java/com/google/devtools/build/lib/runtime/commands/CanonicalizeCommand.java
@@ -70,7 +70,8 @@ public final class CanonicalizeCommand implements BlazeCommand {
       help =
           "Output the canonical policy, after expansion and filtering. To keep the output "
               + "clean, the canonicalized command arguments will NOT be shown when this option is "
-              + "set to true."
+              + "set to true. Note that the command specified by --for_command affects the "
+              + "filtered policy, and if none is specified, the default command is 'build'."
     )
     public boolean canonicalizePolicy;
 
@@ -148,7 +149,7 @@ public final class CanonicalizeCommand implements BlazeCommand {
       // Print out the canonical invocation policy if requested.
       if (canonicalizeOptions.canonicalizePolicy) {
         List<FlagPolicy> effectiveFlagPolicies =
-            InvocationPolicyEnforcer.getEffectivePolicy(policy, parser);
+            InvocationPolicyEnforcer.getEffectivePolicy(policy, parser, commandName);
         InvocationPolicy effectivePolicy =
             InvocationPolicy.newBuilder().addAllFlagPolicies(effectiveFlagPolicies).build();
         env.getReporter().getOutErr().printOutLn(effectivePolicy.toString());