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 886b5d5d2e5fd4..5a6e324bcbec5d 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,9 +36,7 @@ import com.google.devtools.common.options.OptionsParsingException; import java.util.ArrayList; import java.util.Arrays; -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; @@ -211,14 +209,7 @@ public static List getEffectivePolicy( expandedPolicies.addAll(policies); } - // Only keep that last policy for each flag. - Map effectivePolicy = new HashMap<>(); - for (FlagPolicy expandedPolicy : expandedPolicies) { - String flagName = expandedPolicy.getFlagName(); - effectivePolicy.put(flagName, expandedPolicy); - } - - return new ArrayList<>(effectivePolicy.values()); + return expandedPolicies; } /** diff --git a/src/main/protobuf/invocation_policy.proto b/src/main/protobuf/invocation_policy.proto index f14db3e6ca7a41..218b00e0ed534f 100644 --- a/src/main/protobuf/invocation_policy.proto +++ b/src/main/protobuf/invocation_policy.proto @@ -20,10 +20,18 @@ option java_package = "com.google.devtools.build.lib.runtime.proto"; // The --invocation_policy flag takes a base64-encoded binary-serialized or text // formatted InvocationPolicy message. message InvocationPolicy { - // Order matters. - // After expanding policies on expansion flags or flags with implicit - // requirements, only the final policy on a specific flag will be enforced - // onto the user's command line. + // Policies will be applied in order. Later policies will override + // previous policies if they conflict, which is important for flags + // that interact with each other. For example, if there is a flag "--foo" + // which is an expansion flag that expands into "--bar=x --baz=y", and the + // policy list first has a SetValue policy for "set --bar to z", and then has + // a SetDefault policy to set "--foo" to its default value, both --bar and + // --baz will get reset to their default values, overriding the SetValue + // operation. The UseDefault should come before the SetValue. + // + // Note that currently, if user value is lost, either cleared by UseDefault + // or by being written over by a SetValue, a later white-listing of the user's + // inputted value will not restore it. repeated FlagPolicy flag_policies = 1; } @@ -119,10 +127,8 @@ message UseDefault { // so that when the value is requested and no flag is found, the flag parser // returns the default. This is mostly relevant for expansion flags: it will // erase user values in *all* flags that the expansion flag expands to. Only - // use this on expansion flags if this is acceptable behavior. Since the last - // policy wins, later policies on this same flag will still remove the - // expanded UseDefault, so there is a way around, but it's really best not to - // use this on expansion flags at all. + // use this on expansion flags if this is acceptable behavior. Otherwise, make + // an appropriate policy for the expanded flags that you care about. } message DisallowValues { diff --git a/src/test/java/com/google/devtools/build/lib/runtime/InvocationPolicyUseDefaultTest.java b/src/test/java/com/google/devtools/build/lib/runtime/InvocationPolicyUseDefaultTest.java index dda2d319cab5cd..a37b3e45d1c048 100644 --- a/src/test/java/com/google/devtools/build/lib/runtime/InvocationPolicyUseDefaultTest.java +++ b/src/test/java/com/google/devtools/build/lib/runtime/InvocationPolicyUseDefaultTest.java @@ -101,40 +101,6 @@ public void testUseDefaultWithExpansionFlags() throws Exception { assertThat(testOptions.expandedD).isEqualTo(TestOptions.EXPANDED_D_DEFAULT); } - @Test - public void testUseDefaultWithExpansionFlagAndLaterOverride() throws Exception { - InvocationPolicy.Builder invocationPolicyBuilder = InvocationPolicy.newBuilder(); - invocationPolicyBuilder - .addFlagPoliciesBuilder() - .setFlagName("test_expansion") - .getUseDefaultBuilder(); - invocationPolicyBuilder - .addFlagPoliciesBuilder() - .setFlagName("expanded_b") - .getAllowValuesBuilder() - .addAllowedValues("false"); - - InvocationPolicyEnforcer enforcer = createOptionsPolicyEnforcer(invocationPolicyBuilder); - parser.parse("--test_expansion"); - - TestOptions testOptions = getTestOptions(); - assertThat(testOptions.expandedA).isEqualTo(TestOptions.EXPANDED_A_TEST_EXPANSION); - assertThat(testOptions.expandedB).isEqualTo(TestOptions.EXPANDED_B_TEST_EXPANSION); - assertThat(testOptions.expandedC).isEqualTo(TestOptions.EXPANDED_C_TEST_EXPANSION); - assertThat(testOptions.expandedD).isEqualTo(TestOptions.EXPANDED_D_TEST_EXPANSION); - - // If the UseDefault is run, then the value of --expanded_b is back to it's default true, which - // isn't allowed. However, the allowValues in the later policy should wipe the expansion's - // policy on --expanded_b, so that the enforcement does not fail. - enforcer.enforce(parser, BUILD_COMMAND); - - testOptions = getTestOptions(); - assertThat(testOptions.expandedA).isEqualTo(TestOptions.EXPANDED_A_DEFAULT); - assertThat(testOptions.expandedB).isEqualTo(TestOptions.EXPANDED_B_TEST_EXPANSION); - assertThat(testOptions.expandedC).isEqualTo(TestOptions.EXPANDED_C_DEFAULT); - assertThat(testOptions.expandedD).isEqualTo(TestOptions.EXPANDED_D_DEFAULT); - } - @Test public void testUseDefaultWithRecursiveExpansionFlags() throws Exception { InvocationPolicy.Builder invocationPolicyBuilder = InvocationPolicy.newBuilder();