Allow conditioning flags on the _absence_ of a feature.

This allows a flag_set to emit one flag when a feature is enabled, and a
different flag when that feature is disabled.

And while I was in there, I noticed and fixed a couple other issues:

1. env_set didn't actually implement with_feature, despite having the field in
   its proto.
2. action_config implemented with_feature as an optional field, instead of
   repeated field.

RELNOTES: None
PiperOrigin-RevId: 176510960

1 files changed, 147 insertions, 43 deletions

diff --git a/src/test/java/com/google/devtools/build/lib/rules/cpp/CcToolchainFeaturesTest.java b/src/test/java/com/google/devtools/build/lib/rules/cpp/CcToolchainFeaturesTest.java
index bf0e58d7eb..9f6b64263c 100644
--- a/src/test/java/com/google/devtools/build/lib/rules/cpp/CcToolchainFeaturesTest.java
+++ b/src/test/java/com/google/devtools/build/lib/rules/cpp/CcToolchainFeaturesTest.java
@@ -94,7 +94,7 @@ public class CcToolchainFeaturesTest {
   private Set<String> getEnabledFeatures(CcToolchainFeatures features,
       String... requestedFeatures) throws Exception {
     FeatureConfiguration configuration =
-        features.getFeatureConfiguration(assumptionsFor(requestedFeatures));
+        features.getFeatureConfiguration(specificationFor(requestedFeatures));
     ImmutableSet.Builder<String> enabledFeatures = ImmutableSet.builder();
     for (String feature : features.getActivatableNames()) {
       if (configuration.isEnabled(feature)) {
@@ -104,23 +104,23 @@ public class CcToolchainFeaturesTest {
     return enabledFeatures.build();
   }
 
-  private FeatureSpecification assumptionsFor(String... requestedFeatures) {
+  private FeatureSpecification specificationFor(String... requestedFeatures) {
     return FeatureSpecification.create(
         ImmutableSet.copyOf(requestedFeatures), ImmutableSet.<String>of());
   }
 
   @Test
   public void testUnconditionalFeature() throws Exception {
-    assertThat(buildFeatures("").getFeatureConfiguration(assumptionsFor("a")).isEnabled("a"))
+    assertThat(buildFeatures("").getFeatureConfiguration(specificationFor("a")).isEnabled("a"))
         .isFalse();
     assertThat(
             buildFeatures("feature { name: 'a' }")
-                .getFeatureConfiguration(assumptionsFor("b"))
+                .getFeatureConfiguration(specificationFor("b"))
                 .isEnabled("a"))
         .isFalse();
     assertThat(
             buildFeatures("feature { name: 'a' }")
-                .getFeatureConfiguration(assumptionsFor("a"))
+                .getFeatureConfiguration(specificationFor("a"))
                 .isEnabled("a"))
         .isTrue();
   }
@@ -128,7 +128,7 @@ public class CcToolchainFeaturesTest {
   @Test
   public void testUnsupportedAction() throws Exception {
     FeatureConfiguration configuration =
-        buildFeatures("").getFeatureConfiguration(assumptionsFor());
+        buildFeatures("").getFeatureConfiguration(specificationFor());
     assertThat(configuration.getCommandLine("invalid-action", createVariables())).isEmpty();
   }
 
@@ -158,7 +158,7 @@ public class CcToolchainFeaturesTest {
                 "     flag_group { flag: '-b-link' }",
                 "  }",
                 "}")
-            .getFeatureConfiguration(assumptionsFor("a", "b"));
+            .getFeatureConfiguration(specificationFor("a", "b"));
     List<String> commandLine = configuration.getCommandLine(
         CppCompileAction.CPP_COMPILE, createVariables());
     assertThat(commandLine).containsExactly("-a-c++-compile", "-b-c++-compile").inOrder();
@@ -186,6 +186,26 @@ public class CcToolchainFeaturesTest {
                 "     action: 'c++-compile'",
                 "     env_entry { key: 'dog', value: 'woof' }",
                 "  }",
+                "  env_set {",
+                "     action: 'c++-compile'",
+                "     with_feature: { feature: 'd' }",
+                "     env_entry { key: 'withFeature', value: 'value1' }",
+                "  }",
+                "  env_set {",
+                "     action: 'c++-compile'",
+                "     with_feature: { feature: 'e' }",
+                "     env_entry { key: 'withoutFeature', value: 'value2' }",
+                "  }",
+                "  env_set {",
+                "     action: 'c++-compile'",
+                "     with_feature: { not_feature: 'f' }",
+                "     env_entry { key: 'withNotFeature', value: 'value3' }",
+                "  }",
+                "  env_set {",
+                "     action: 'c++-compile'",
+                "     with_feature: { not_feature: 'g' }",
+                "     env_entry { key: 'withoutNotFeature', value: 'value4' }",
+                "  }",
                 "}",
                 "feature {",
                 "  name: 'c'",
@@ -193,11 +213,21 @@ public class CcToolchainFeaturesTest {
                 "     action: 'c++-compile'",
                 "     env_entry { key: 'doNotInclude', value: 'doNotIncludePlease' }",
                 "  }",
-                "}")
-            .getFeatureConfiguration(assumptionsFor("a", "b"));
+                "}",
+                "feature { name: 'd' }",
+                "feature { name: 'e' }",
+                "feature { name: 'f' }",
+                "feature { name: 'g' }")
+            .getFeatureConfiguration(specificationFor("a", "b", "d", "f"));
     Map<String, String> env = configuration.getEnvironmentVariables(
         CppCompileAction.CPP_COMPILE, createVariables());
-    assertThat(env).containsExactly("foo", "bar", "cat", "meow", "dog", "woof").inOrder();
+    assertThat(env)
+        .containsExactly(
+            "foo", "bar", "cat", "meow", "dog", "woof",
+            "withFeature", "value1", "withoutNotFeature", "value4")
+        .inOrder();
+    assertThat(env).doesNotContainEntry("withoutFeature", "value2");
+    assertThat(env).doesNotContainEntry("withNotFeature", "value3");
     assertThat(env).doesNotContainEntry("doNotInclude", "doNotIncludePlease");
   }
 
@@ -216,7 +246,7 @@ public class CcToolchainFeaturesTest {
                 "    " + groups,
                 "  }",
                 "}")
-            .getFeatureConfiguration(assumptionsFor("a"));
+            .getFeatureConfiguration(specificationFor("a"));
     return configuration.getCommandLine(CppCompileAction.CPP_COMPILE, variables);
   }
 
@@ -978,7 +1008,7 @@ public class CcToolchainFeaturesTest {
                 "     flag_group { flag: 'unconditional' }",
                 "  }",
                 "}")
-            .getFeatureConfiguration(assumptionsFor("a"));
+            .getFeatureConfiguration(specificationFor("a"));
 
     assertThat(configuration.getCommandLine(CppCompileAction.CPP_COMPILE, createVariables()))
         .containsExactly("unconditional");
@@ -1006,7 +1036,7 @@ public class CcToolchainFeaturesTest {
                 "     flag_group { flag: 'unconditional' }",
                 "  }",
                 "}")
-            .getFeatureConfiguration(assumptionsFor("a"));
+            .getFeatureConfiguration(specificationFor("a"));
 
     assertThat(
             configuration.getCommandLine(CppCompileAction.CPP_COMPILE, createVariables("v", "1")))
@@ -1035,7 +1065,7 @@ public class CcToolchainFeaturesTest {
                 "     flag_group { flag: 'unconditional' }",
                 "  }",
                 "}")
-            .getFeatureConfiguration(assumptionsFor("a"));
+            .getFeatureConfiguration(specificationFor("a"));
 
     assertThat(
             configuration.getCommandLine(
@@ -1066,7 +1096,7 @@ public class CcToolchainFeaturesTest {
                 "     flag_group { flag: 'unconditional' }",
                 "  }",
                 "}")
-            .getFeatureConfiguration(assumptionsFor("a"));
+            .getFeatureConfiguration(specificationFor("a"));
 
     assertThat(
             configuration.getCommandLine(
@@ -1092,7 +1122,7 @@ public class CcToolchainFeaturesTest {
     assertThat(getEnabledFeatures(features, "b")).containsExactly("a", "b");
     assertThat(
             features
-                .getFeatureConfiguration(assumptionsFor("b"))
+                .getFeatureConfiguration(specificationFor("b"))
                 .getCommandLine(CppCompileAction.CPP_COMPILE, createVariables("v", "1")))
         .containsExactly("-f", "1");
     byte[] serialized = TestUtils.serializeObject(features);
@@ -1101,7 +1131,7 @@ public class CcToolchainFeaturesTest {
     assertThat(getEnabledFeatures(deserialized, "b")).containsExactly("a", "b");
     assertThat(
             features
-                .getFeatureConfiguration(assumptionsFor("b"))
+                .getFeatureConfiguration(specificationFor("b"))
                 .getCommandLine(CppCompileAction.CPP_COMPILE, createVariables("v", "1")))
         .containsExactly("-f", "1");
   }
@@ -1138,12 +1168,12 @@ public class CcToolchainFeaturesTest {
             "feature {name: 'b'}");
     assertThat(
             features
-                .getFeatureConfiguration(assumptionsFor("a", "b"))
+                .getFeatureConfiguration(specificationFor("a", "b"))
                 .getCommandLine(CppCompileAction.CPP_COMPILE, createVariables()))
         .containsExactly("dummy_flag");
     assertThat(
             features
-                .getFeatureConfiguration(assumptionsFor("a"))
+                .getFeatureConfiguration(specificationFor("a"))
                 .getCommandLine(CppCompileAction.CPP_COMPILE, createVariables()))
         .doesNotContain("dummy_flag");
   }
@@ -1166,17 +1196,17 @@ public class CcToolchainFeaturesTest {
             "feature {name: 'c'}");
     assertThat(
             features
-                .getFeatureConfiguration(assumptionsFor("a", "b", "c"))
+                .getFeatureConfiguration(specificationFor("a", "b", "c"))
                 .getCommandLine(CppCompileAction.CPP_COMPILE, createVariables()))
         .containsExactly("dummy_flag");
     assertThat(
             features
-                .getFeatureConfiguration(assumptionsFor("a", "b"))
+                .getFeatureConfiguration(specificationFor("a", "b"))
                 .getCommandLine(CppCompileAction.CPP_COMPILE, createVariables()))
         .doesNotContain("dummy_flag");
     assertThat(
             features
-                .getFeatureConfiguration(assumptionsFor("a"))
+                .getFeatureConfiguration(specificationFor("a"))
                 .getCommandLine(CppCompileAction.CPP_COMPILE, createVariables()))
         .doesNotContain("dummy_flag");
   }
@@ -1202,17 +1232,63 @@ public class CcToolchainFeaturesTest {
             "feature {name: 'c2'}");
     assertThat(
             features
-                .getFeatureConfiguration(assumptionsFor("a", "b1", "c1", "b2", "c2"))
+                .getFeatureConfiguration(specificationFor("a", "b1", "c1", "b2", "c2"))
+                .getCommandLine(CppCompileAction.CPP_COMPILE, createVariables()))
+        .containsExactly("dummy_flag");
+    assertThat(
+            features
+                .getFeatureConfiguration(specificationFor("a", "b1", "c1"))
+                .getCommandLine(CppCompileAction.CPP_COMPILE, createVariables()))
+        .containsExactly("dummy_flag");
+    assertThat(
+            features
+                .getFeatureConfiguration(specificationFor("a", "b1", "b2"))
+                .getCommandLine(CppCompileAction.CPP_COMPILE, createVariables()))
+        .doesNotContain("dummy_flag");
+  }
+
+  @Test
+  public void testWithFeature_NotFeature() throws Exception {
+    CcToolchainFeatures features =
+        buildFeatures(
+            "feature {",
+            "  name: 'a'",
+            "  flag_set {",
+            "    with_feature { not_feature: 'x', not_feature: 'y', feature: 'z' }",
+            "    with_feature { not_feature: 'q' }",
+            "    action: 'c++-compile'",
+            "    flag_group {",
+            "      flag: 'dummy_flag'",
+            "    }",
+            "  }",
+            "}",
+            "feature {name: 'x'}",
+            "feature {name: 'y'}",
+            "feature {name: 'z'}",
+            "feature {name: 'q'}");
+    assertThat(
+            features
+                .getFeatureConfiguration(specificationFor("a"))
                 .getCommandLine(CppCompileAction.CPP_COMPILE, createVariables()))
         .containsExactly("dummy_flag");
     assertThat(
             features
-                .getFeatureConfiguration(assumptionsFor("a", "b1", "c1"))
+                .getFeatureConfiguration(specificationFor("a", "q"))
+                .getCommandLine(CppCompileAction.CPP_COMPILE, createVariables()))
+        .doesNotContain("dummy_flag");
+    assertThat(
+            features
+                .getFeatureConfiguration(specificationFor("a", "q", "z"))
                 .getCommandLine(CppCompileAction.CPP_COMPILE, createVariables()))
         .containsExactly("dummy_flag");
     assertThat(
             features
-                .getFeatureConfiguration(assumptionsFor("a", "b1", "b2"))
+                .getFeatureConfiguration(specificationFor("a", "q", "x", "z"))
+                .getCommandLine(CppCompileAction.CPP_COMPILE, createVariables()))
+        .doesNotContain("dummy_flag");
+    assertThat(
+            features
+                .getFeatureConfiguration(specificationFor("a", "q", "x", "y", "z"))
                 .getCommandLine(CppCompileAction.CPP_COMPILE, createVariables()))
         .doesNotContain("dummy_flag");
   }
@@ -1235,7 +1311,7 @@ public class CcToolchainFeaturesTest {
             "}");
 
     FeatureConfiguration featureConfiguration =
-        toolchainFeatures.getFeatureConfiguration(assumptionsFor("activates-action-a"));
+        toolchainFeatures.getFeatureConfiguration(specificationFor("activates-action-a"));
 
     assertThat(featureConfiguration.actionIsConfigured("action-a")).isTrue();
   }
@@ -1258,11 +1334,12 @@ public class CcToolchainFeaturesTest {
             "}");
 
     FeatureConfiguration featureConfigurationWithoutAction =
-        toolchainFeatures.getFeatureConfiguration(assumptionsFor("requires-action-a"));
+        toolchainFeatures.getFeatureConfiguration(specificationFor("requires-action-a"));
     assertThat(featureConfigurationWithoutAction.isEnabled("requires-action-a")).isFalse();
 
     FeatureConfiguration featureConfigurationWithAction =
-        toolchainFeatures.getFeatureConfiguration(assumptionsFor("action-a", "requires-action-a"));
+        toolchainFeatures.getFeatureConfiguration(
+            specificationFor("action-a", "requires-action-a"));
     assertThat(featureConfigurationWithAction.isEnabled("requires-action-a")).isTrue();
   }
 
@@ -1281,7 +1358,7 @@ public class CcToolchainFeaturesTest {
                 "   name: 'activates-action-a'",
                 "   implies: 'action-a'",
                 "}")
-            .getFeatureConfiguration(assumptionsFor("activates-action-a"));
+            .getFeatureConfiguration(specificationFor("activates-action-a"));
     PathFragment crosstoolPath = PathFragment.create("crosstool/");
     PathFragment toolPath = configuration.getToolForAction("action-a").getToolPath(crosstoolPath);
     assertThat(toolPath.toString()).isEqualTo("crosstool/toolchain/a");
@@ -1302,12 +1379,16 @@ public class CcToolchainFeaturesTest {
             "     }",
             "  }",
             "  tool {",
-            "    tool_path: 'toolchain/feature-a'",
-            "    with_feature: { feature: 'feature-a' }",
+            "    tool_path: 'toolchain/feature-a-and-not-c'",
+            "    with_feature: {",
+            "      feature: 'feature-a'",
+            "      not_feature: 'feature-c'",
+            "    }",
             "  }",
             "  tool {",
-            "    tool_path: 'toolchain/feature-b'",
+            "    tool_path: 'toolchain/feature-b-or-c'",
             "    with_feature: { feature: 'feature-b' }",
+            "    with_feature: { feature: 'feature-c' }",
             "  }",
             "  tool {",
             "    tool_path: 'toolchain/default'",
@@ -1320,6 +1401,9 @@ public class CcToolchainFeaturesTest {
             "  name: 'feature-b'",
             "}",
             "feature {",
+            "  name: 'feature-c'",
+            "}",
+            "feature {",
             "  name: 'activates-action-a'",
             "  implies: 'action-a'",
             "}");
@@ -1328,27 +1412,47 @@ public class CcToolchainFeaturesTest {
 
     FeatureConfiguration featureAConfiguration =
         toolchainFeatures.getFeatureConfiguration(
-            assumptionsFor("feature-a", "activates-action-a"));
+            specificationFor("feature-a", "activates-action-a"));
     assertThat(
             featureAConfiguration
                 .getToolForAction("action-a")
                 .getToolPath(crosstoolPath)
                 .toString())
-        .isEqualTo("crosstool/toolchain/feature-a");
+        .isEqualTo("crosstool/toolchain/feature-a-and-not-c");
+
+    FeatureConfiguration featureAAndCConfiguration =
+        toolchainFeatures.getFeatureConfiguration(
+            specificationFor("feature-a", "feature-c", "activates-action-a"));
+    assertThat(
+            featureAAndCConfiguration
+                .getToolForAction("action-a")
+                .getToolPath(crosstoolPath)
+                .toString())
+        .isEqualTo("crosstool/toolchain/feature-b-or-c");
 
     FeatureConfiguration featureBConfiguration =
         toolchainFeatures.getFeatureConfiguration(
-            assumptionsFor("feature-b", "activates-action-a"));
+            specificationFor("feature-b", "activates-action-a"));
     assertThat(
             featureBConfiguration
                 .getToolForAction("action-a")
                 .getToolPath(crosstoolPath)
                 .toString())
-        .isEqualTo("crosstool/toolchain/feature-b");
+        .isEqualTo("crosstool/toolchain/feature-b-or-c");
+
+    FeatureConfiguration featureCConfiguration =
+        toolchainFeatures.getFeatureConfiguration(
+            specificationFor("feature-c", "activates-action-a"));
+    assertThat(
+            featureCConfiguration
+                .getToolForAction("action-a")
+                .getToolPath(crosstoolPath)
+                .toString())
+        .isEqualTo("crosstool/toolchain/feature-b-or-c");
 
     FeatureConfiguration featureAAndBConfiguration =
         toolchainFeatures.getFeatureConfiguration(
-            assumptionsFor("feature-a", "feature-b", "activates-action-a"));
+            specificationFor("feature-a", "feature-b", "activates-action-a"));
     assertThat(
             featureAAndBConfiguration
                 .getToolForAction("action-a")
@@ -1357,7 +1461,7 @@ public class CcToolchainFeaturesTest {
         .isEqualTo("crosstool/toolchain/features-a-and-b");
 
     FeatureConfiguration noFeaturesConfiguration =
-        toolchainFeatures.getFeatureConfiguration(assumptionsFor("activates-action-a"));
+        toolchainFeatures.getFeatureConfiguration(specificationFor("activates-action-a"));
     assertThat(
             noFeaturesConfiguration
                 .getToolForAction("action-a")
@@ -1389,7 +1493,7 @@ public class CcToolchainFeaturesTest {
     PathFragment crosstoolPath = PathFragment.create("crosstool/");
 
     FeatureConfiguration noFeaturesConfiguration =
-        toolchainFeatures.getFeatureConfiguration(assumptionsFor("activates-action-a"));
+        toolchainFeatures.getFeatureConfiguration(specificationFor("activates-action-a"));
 
     try {
       noFeaturesConfiguration.getToolForAction("action-a").getToolPath(crosstoolPath);
@@ -1415,7 +1519,7 @@ public class CcToolchainFeaturesTest {
             "}");
 
     FeatureConfiguration featureConfiguration =
-        toolchainFeatures.getFeatureConfiguration(assumptionsFor("action-a"));
+        toolchainFeatures.getFeatureConfiguration(specificationFor("action-a"));
 
     assertThat(featureConfiguration.actionIsConfigured("action-a")).isTrue();
   }
@@ -1438,7 +1542,7 @@ public class CcToolchainFeaturesTest {
             "}");
 
     FeatureConfiguration featureConfiguration =
-        toolchainFeatures.getFeatureConfiguration(assumptionsFor("action-a"));
+        toolchainFeatures.getFeatureConfiguration(specificationFor("action-a"));
 
     assertThat(featureConfiguration.isEnabled("activated-feature")).isTrue();
   }
@@ -1492,7 +1596,7 @@ public class CcToolchainFeaturesTest {
                 "    flag_group {flag: 'foo'}",
                 "  }",
                 "}")
-            .getFeatureConfiguration(assumptionsFor("c++-compile"));
+            .getFeatureConfiguration(specificationFor("c++-compile"));
     List<String> commandLine =
         featureConfiguration.getCommandLine("c++-compile", createVariables());
     assertThat(commandLine).contains("foo");
@@ -1510,7 +1614,7 @@ public class CcToolchainFeaturesTest {
               "    flag_group {flag: 'foo'}",
               "  }",
               "}")
-          .getFeatureConfiguration(assumptionsFor("c++-compile"));
+          .getFeatureConfiguration(specificationFor("c++-compile"));
       fail("Should throw InvalidConfigurationException");
     } catch (InvalidConfigurationException e) {
       assertThat(e)