--show_config_fragments logic cleanup.
Let rule implementations manage their implementation-specific requirements.
Also add test coverage for android_binary, android_test, android_local_test
(they set feature flags with a custom attribute, therefore "require" them).
PiperOrigin-RevId: 307859878
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/RuleConfiguredTargetBuilder.java b/src/main/java/com/google/devtools/build/lib/analysis/RuleConfiguredTargetBuilder.java
index ba3fcff..3d827cc 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/RuleConfiguredTargetBuilder.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/RuleConfiguredTargetBuilder.java
@@ -25,7 +25,6 @@
import com.google.devtools.build.lib.actions.Actions.GeneratingActions;
import com.google.devtools.build.lib.actions.Artifact;
import com.google.devtools.build.lib.actions.MutableActionGraph.ActionConflictException;
-import com.google.devtools.build.lib.analysis.config.ConfigMatchingProvider;
import com.google.devtools.build.lib.analysis.config.CoreOptions;
import com.google.devtools.build.lib.analysis.config.CoreOptions.IncludeConfigFragmentsEnum;
import com.google.devtools.build.lib.analysis.configuredtargets.RuleConfiguredTarget;
@@ -48,7 +47,6 @@
import com.google.devtools.build.lib.collect.nestedset.Order;
import com.google.devtools.build.lib.packages.Attribute;
import com.google.devtools.build.lib.packages.BuildSetting;
-import com.google.devtools.build.lib.packages.BuildType;
import com.google.devtools.build.lib.packages.Info;
import com.google.devtools.build.lib.packages.Provider;
import com.google.devtools.build.lib.packages.Rule;
@@ -57,11 +55,12 @@
import com.google.devtools.build.lib.packages.Type.LabelClass;
import com.google.devtools.build.lib.syntax.EvalException;
import com.google.devtools.build.lib.syntax.Location;
+import java.util.Collection;
import java.util.LinkedHashMap;
+import java.util.LinkedHashSet;
import java.util.List;
import java.util.Map;
import java.util.TreeMap;
-import java.util.stream.Collectors;
import javax.annotation.Nullable;
/**
@@ -89,6 +88,8 @@
private Runfiles persistentTestRunnerRunfiles;
private Artifact executable;
private ImmutableSet<ActionAnalysisMetadata> actionsWithoutExtraAction = ImmutableSet.of();
+ private final LinkedHashSet<String> ruleImplSpecificRequiredConfigFragments =
+ new LinkedHashSet<>();
public RuleConfiguredTargetBuilder(RuleContext ruleContext) {
this.ruleContext = ruleContext;
@@ -256,11 +257,8 @@
* CoreOptions.IncludeConfigFragmentsEnum#OFF}.
*
* <p>See {@link ConfiguredTargetFactory#getRequiredConfigFragments} for a description of the
- * meaning of this provider's content. That method populates {@link
- * RuleContext#getRequiredConfigFragments}, which we read here. We add to that any additional
- * config state that is only known to be required after the rule's analysis function has finished.
- * In particular, if the current rule is a {@code config_setting}, we add as a direct requirement
- * the config state that it matches.
+ * meaning of this provider's content. That method populates the results of {@link
+ * RuleContext#getRequiredConfigFragments} and {@link #ruleImplSpecificRequiredConfigFragments}.
*/
private void maybeAddRequiredConfigFragmentsProvider() {
if (ruleContext
@@ -268,36 +266,14 @@
.getOptions()
.get(CoreOptions.class)
.includeRequiredConfigFragmentsProvider
- == IncludeConfigFragmentsEnum.OFF) {
- return;
+ != IncludeConfigFragmentsEnum.OFF) {
+ addProvider(
+ new RequiredConfigFragmentsProvider(
+ ImmutableSet.<String>builder()
+ .addAll(ruleContext.getRequiredConfigFragments())
+ .addAll(ruleImplSpecificRequiredConfigFragments)
+ .build()));
}
-
- ImmutableSet.Builder<String> requiredFragments = ImmutableSet.builder();
- requiredFragments.addAll(ruleContext.getRequiredConfigFragments());
-
- if (providersBuilder.contains(ConfigMatchingProvider.class)) {
- // config_setting discovers extra requirements through its "values = {'some_option': ...}"
- // references. Make sure those are included here.
- requiredFragments.addAll(
- providersBuilder.getProvider(ConfigMatchingProvider.class).getRequiredFragmentOptions());
- }
-
- if (ruleContext.getRule().isAttrDefined("feature_flags", BuildType.LABEL_KEYED_STRING_DICT)) {
- // Sad hack to make android_binary rules list the feature flags they set.
- // TODO(gregce): move this to AndroidBinary.java. This requires some more coordination to
- // make sure we don't add a RequiredConfigFragmentsProvider both there and here, which is
- // technically a violation of TransitiveInfoProviderMapBuilder.put.
- requiredFragments.addAll(
- ruleContext
- .attributes()
- .get("feature_flags", BuildType.LABEL_KEYED_STRING_DICT)
- .keySet()
- .stream()
- .map(label -> label.toString())
- .collect(Collectors.toList()));
- }
-
- addProvider(new RequiredConfigFragmentsProvider(requiredFragments.build()));
}
private NestedSet<Label> transitiveLabels() {
@@ -635,11 +611,11 @@
}
/**
- * Set the extra action pseudo actions.
+ * Supplements {@link #maybeAddRequiredConfigFragmentsProvider} with rule implementation-specific
+ * requirements.
*/
- public RuleConfiguredTargetBuilder setActionsWithoutExtraAction(
- ImmutableSet<ActionAnalysisMetadata> actions) {
- this.actionsWithoutExtraAction = actions;
+ public RuleConfiguredTargetBuilder addRequiredConfigFragments(Collection<String> fragments) {
+ ruleImplSpecificRequiredConfigFragments.addAll(fragments);
return this;
}
diff --git a/src/main/java/com/google/devtools/build/lib/rules/android/AndroidBinary.java b/src/main/java/com/google/devtools/build/lib/rules/android/AndroidBinary.java
index d2add04..771fb98 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/android/AndroidBinary.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/android/AndroidBinary.java
@@ -690,6 +690,11 @@
.addNativeDeclaredProvider(
AndroidFeatureFlagSetProvider.create(
AndroidFeatureFlagSetProvider.getAndValidateFlagMapFromRuleContext(ruleContext)))
+ // Report set feature flags as required "config fragments".
+ // While these aren't technically fragments, in practice they're user-defined settings with
+ // the same meaning: pieces of configuration the rule requires to work properly. So it makes
+ // sense to treat them equivalently for "requirements" reporting purposes.
+ .addRequiredConfigFragments(AndroidFeatureFlagSetProvider.getFlagNames(ruleContext))
.addOutputGroup("android_deploy_info", deployInfo);
}
diff --git a/src/main/java/com/google/devtools/build/lib/rules/android/AndroidFeatureFlagSetProvider.java b/src/main/java/com/google/devtools/build/lib/rules/android/AndroidFeatureFlagSetProvider.java
index 69a8f25..4323d26 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/android/AndroidFeatureFlagSetProvider.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/android/AndroidFeatureFlagSetProvider.java
@@ -16,6 +16,7 @@
import com.google.common.base.Optional;
import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableSet;
import com.google.devtools.build.lib.analysis.AliasProvider;
import com.google.devtools.build.lib.analysis.RuleContext;
import com.google.devtools.build.lib.analysis.RuleDefinitionEnvironment;
@@ -125,6 +126,17 @@
return Optional.of(ImmutableMap.copyOf(expectedValues));
}
+ /** Returns the feature flags this rule sets as user-friendly strings. */
+ public static ImmutableSet<String> getFlagNames(RuleContext ruleContext) {
+ return ruleContext
+ .attributes()
+ .get(AndroidFeatureFlagSetProvider.FEATURE_FLAG_ATTR, BuildType.LABEL_KEYED_STRING_DICT)
+ .keySet()
+ .stream()
+ .map(label -> label.toString())
+ .collect(ImmutableSet.toImmutableSet());
+ }
+
/**
* Returns whether it is acceptable to have a dependency with flags {@code depFlags} if the target
* has flags {@code targetFlags}.
diff --git a/src/main/java/com/google/devtools/build/lib/rules/android/AndroidLocalTestBase.java b/src/main/java/com/google/devtools/build/lib/rules/android/AndroidLocalTestBase.java
index 3a00c5d..83d4287 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/android/AndroidLocalTestBase.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/android/AndroidLocalTestBase.java
@@ -338,6 +338,11 @@
// Just confirming that there are no aliases being used here.
AndroidFeatureFlagSetProvider.getAndValidateFlagMapFromRuleContext(ruleContext);
+ // Report set feature flags as required "config fragments".
+ // While these aren't technically fragments, in practice they're user-defined settings with
+ // the same meaning: pieces of configuration the rule requires to work properly. So it makes
+ // sense to treat them equivalently for "requirements" reporting purposes.
+ builder.addRequiredConfigFragments(AndroidFeatureFlagSetProvider.getFlagNames(ruleContext));
if (oneVersionOutputArtifact != null) {
builder.addOutputGroup(OutputGroupInfo.HIDDEN_TOP_LEVEL, oneVersionOutputArtifact);
diff --git a/src/main/java/com/google/devtools/build/lib/rules/config/ConfigSetting.java b/src/main/java/com/google/devtools/build/lib/rules/config/ConfigSetting.java
index dd448bb..a0bfdea 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/config/ConfigSetting.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/config/ConfigSetting.java
@@ -156,6 +156,7 @@
.addProvider(FilesToRunProvider.class, FilesToRunProvider.EMPTY)
.addProvider(LicensesProviderImpl.EMPTY)
.addProvider(ConfigMatchingProvider.class, configMatcher)
+ .addRequiredConfigFragments(configMatcher.getRequiredFragmentOptions())
.build();
}
diff --git a/src/test/java/com/google/devtools/build/lib/rules/android/AndroidBinaryTest.java b/src/test/java/com/google/devtools/build/lib/rules/android/AndroidBinaryTest.java
index 497671a..38e4202 100644
--- a/src/test/java/com/google/devtools/build/lib/rules/android/AndroidBinaryTest.java
+++ b/src/test/java/com/google/devtools/build/lib/rules/android/AndroidBinaryTest.java
@@ -41,6 +41,7 @@
import com.google.devtools.build.lib.analysis.FileProvider;
import com.google.devtools.build.lib.analysis.FilesToRunProvider;
import com.google.devtools.build.lib.analysis.OutputGroupInfo;
+import com.google.devtools.build.lib.analysis.RequiredConfigFragmentsProvider;
import com.google.devtools.build.lib.analysis.actions.FileWriteAction;
import com.google.devtools.build.lib.analysis.actions.SpawnAction;
import com.google.devtools.build.lib.cmdline.RepositoryName;
@@ -3401,6 +3402,29 @@
}
@Test
+ public void featureFlagsSetByAndroidBinaryAreInRequiredFragments() throws Exception {
+ useConfiguration("--include_config_fragments_provider=direct");
+ scratch.file(
+ "java/com/foo/BUILD",
+ "config_feature_flag(",
+ " name = 'flag1',",
+ " allowed_values = ['on', 'off'],",
+ " default_value = 'off',",
+ ")",
+ "android_binary(",
+ " name = 'foo',",
+ " manifest = 'AndroidManifest.xml',",
+ " srcs = [':FooFlags.java'],",
+ " feature_flags = {",
+ " 'flag1': 'on',",
+ " },",
+ ")");
+ ConfiguredTarget ct = getConfiguredTarget("//java/com/foo:foo");
+ assertThat(ct.getProvider(RequiredConfigFragmentsProvider.class).getRequiredConfigFragments())
+ .contains("//java/com/foo:flag1");
+ }
+
+ @Test
public void testNocompressExtensions() throws Exception {
scratch.file(
"java/r/android/BUILD",
diff --git a/src/test/java/com/google/devtools/build/lib/rules/android/AndroidLocalTestTest.java b/src/test/java/com/google/devtools/build/lib/rules/android/AndroidLocalTestTest.java
index 6998282..88ac994 100644
--- a/src/test/java/com/google/devtools/build/lib/rules/android/AndroidLocalTestTest.java
+++ b/src/test/java/com/google/devtools/build/lib/rules/android/AndroidLocalTestTest.java
@@ -22,6 +22,7 @@
import com.google.devtools.build.lib.actions.util.ActionsTestUtil;
import com.google.devtools.build.lib.analysis.ConfiguredTarget;
import com.google.devtools.build.lib.analysis.FileProvider;
+import com.google.devtools.build.lib.analysis.RequiredConfigFragmentsProvider;
import com.google.devtools.build.lib.analysis.actions.SpawnAction;
import com.google.devtools.build.lib.analysis.configuredtargets.RuleConfiguredTarget;
import com.google.devtools.build.lib.collect.nestedset.NestedSet;
@@ -277,6 +278,41 @@
assertThat(aaptArguments).contains("ar_XB");
}
+ @Test
+ public void featureFlagsSetByAndroidLocalTestAreInRequiredFragments() throws Exception {
+ useConfiguration("--include_config_fragments_provider=direct");
+ scratch.overwriteFile(
+ "tools/whitelists/config_feature_flag/BUILD",
+ "package_group(",
+ " name = 'config_feature_flag',",
+ " packages = ['//java/com/google/android/foo'])");
+ scratch.file(
+ "java/com/google/android/foo/BUILD",
+ "load('//java/bar:foo.bzl', 'extra_deps')",
+ "config_feature_flag(",
+ " name = 'flag1',",
+ " allowed_values = ['on', 'off'],",
+ " default_value = 'off',",
+ ")",
+ "android_binary(",
+ " name = 'foo_under_test',",
+ " srcs = ['Test.java'],",
+ " manifest = 'AndroidManifest.xml',",
+ ")",
+ "android_local_test(",
+ " name = 'local_test',",
+ " srcs = ['test.java'],",
+ " deps = extra_deps,",
+ " feature_flags = {",
+ " 'flag1': 'on',",
+ " },",
+ " resource_configuration_filters = ['ar_XB'])");
+
+ ConfiguredTarget ct = getConfiguredTarget("//java/com/google/android/foo:local_test");
+ assertThat(ct.getProvider(RequiredConfigFragmentsProvider.class).getRequiredConfigFragments())
+ .contains("//java/com/google/android/foo:flag1");
+ }
+
@Override
protected String getRuleName() {
return "android_local_test";
