$ blaze cquery --show_config_fragments: include aspects
Aspects can add new rule dependencies via implicit label
attributes (see
https://docs.bazel.build/versions/2.0.0/skylark/aspects.html#aspect-definition-1).
We need to make sure these are included in whatever the rule the aspect
attaches to.
Note that *native* aspects can also declare *direct* fragment dependencies (
Starlark aspects can't). This change doesn't handle that. TBD.
Supports https://github.com/bazelbuild/bazel/issues/10613.
PiperOrigin-RevId: 293669124
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredAspect.java b/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredAspect.java
index 0e07c6d..696e930 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredAspect.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredAspect.java
@@ -26,6 +26,8 @@
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.CoreOptions;
+import com.google.devtools.build.lib.analysis.config.CoreOptions.IncludeConfigFragmentsEnum;
import com.google.devtools.build.lib.analysis.skylark.SkylarkApiProvider;
import com.google.devtools.build.lib.collect.nestedset.NestedSet;
import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
@@ -281,6 +283,18 @@
ruleContext.getOwner(),
/*outputFiles=*/ null);
+ if (ruleContext
+ .getConfiguration()
+ .getOptions()
+ .get(CoreOptions.class)
+ .includeRequiredConfigFragmentsProvider
+ != IncludeConfigFragmentsEnum.OFF) {
+ // This guarantees aspects pass through the requirements of their dependencies. But native
+ // aspects can also declare direct requirements.
+ // TODO(gregce): support native aspect direct requirements.
+ addProvider(new RequiredConfigFragmentsProvider(ruleContext.getRequiredConfigFragments()));
+ }
+
return new ConfiguredAspect(
descriptor,
generatingActions.getActions(),
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredTargetFactory.java b/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredTargetFactory.java
index c85ebe2..bba38e4 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredTargetFactory.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredTargetFactory.java
@@ -342,6 +342,35 @@
requiredFragments.add(rule.getLabel().toString());
}
+ // Optionally add transitively required fragments:
+ requiredFragments.addAll(getRequiredConfigFragmentsFromDeps(configuration, prerequisites));
+ return ImmutableSet.copyOf(requiredFragments);
+ }
+
+ /**
+ * Subset of {@link #getRequiredConfigFragments} that only returns fragments required by deps.
+ * This includes:
+ *
+ * <ul>
+ * <li>Requirements transitively required by deps iff {@link
+ * CoreOptions#includeRequiredConfigFragmentsProvider} is {@link
+ * CoreOptions.IncludeConfigFragmentsEnum#TRANSITIVE},
+ * <li>Dependencies on Starlark build settings iff {@link
+ * CoreOptions#includeRequiredConfigFragmentsProvider} is not {@link
+ * CoreOptions.IncludeConfigFragmentsEnum#OFF}. These are considered direct requirements on
+ * the rule.
+ * </ul>
+ */
+ private static ImmutableSet<String> getRequiredConfigFragmentsFromDeps(
+ BuildConfiguration configuration, Iterable<ConfiguredTargetAndData> prerequisites) {
+
+ TreeSet<String> requiredFragments = new TreeSet<>();
+ CoreOptions coreOptions = configuration.getOptions().get(CoreOptions.class);
+ if (coreOptions.includeRequiredConfigFragmentsProvider
+ == CoreOptions.IncludeConfigFragmentsEnum.OFF) {
+ return ImmutableSet.of();
+ }
+
for (ConfiguredTargetAndData prereq : prerequisites) {
// If the rule depends on a Starlark build setting, conceptually that means the rule directly
// requires that as an option (even though it's technically a dependency).
@@ -599,6 +628,10 @@
.setUniversalFragments(ruleClassProvider.getUniversalFragments())
.setToolchainContext(toolchainContext)
.setConstraintSemantics(ruleClassProvider.getConstraintSemantics())
+ .setRequiredConfigFragments(
+ // Aspects have no direct fragment requirements: all requirements come from implicit
+ // label dependencies.
+ getRequiredConfigFragmentsFromDeps(aspectConfiguration, prerequisiteMap.values()))
.build();
// If allowing analysis failures, targets should be created as normal as possible, and errors
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/RequiredConfigFragmentsProvider.java b/src/main/java/com/google/devtools/build/lib/analysis/RequiredConfigFragmentsProvider.java
index dc0f53c..1143bc4 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/RequiredConfigFragmentsProvider.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/RequiredConfigFragmentsProvider.java
@@ -14,10 +14,13 @@
package com.google.devtools.build.lib.analysis;
+import static com.google.common.base.Preconditions.checkArgument;
+
import com.google.common.collect.ImmutableSet;
import com.google.devtools.build.lib.analysis.config.BuildConfiguration;
import com.google.devtools.build.lib.analysis.config.FragmentOptions;
import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
+import java.util.List;
/**
* Provides a user-friendly list of the {@link BuildConfiguration.Fragment}s and
@@ -36,4 +39,18 @@
public ImmutableSet<String> getRequiredConfigFragments() {
return requiredConfigFragments;
}
+
+ /** Merges the values of multiple {@link RequiredConfigFragmentsProvider}s. */
+ public static RequiredConfigFragmentsProvider merge(
+ List<RequiredConfigFragmentsProvider> providers) {
+ checkArgument(!providers.isEmpty());
+ if (providers.size() == 1) {
+ return providers.get(0);
+ }
+ ImmutableSet.Builder<String> merged = ImmutableSet.builder();
+ for (RequiredConfigFragmentsProvider provider : providers) {
+ merged.addAll(provider.getRequiredConfigFragments());
+ }
+ return new RequiredConfigFragmentsProvider(merged.build());
+ }
}
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/configuredtargets/MergedConfiguredTarget.java b/src/main/java/com/google/devtools/build/lib/analysis/configuredtargets/MergedConfiguredTarget.java
index ee6eb34..31d4745 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/configuredtargets/MergedConfiguredTarget.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/configuredtargets/MergedConfiguredTarget.java
@@ -22,6 +22,7 @@
import com.google.devtools.build.lib.analysis.ConfiguredTarget;
import com.google.devtools.build.lib.analysis.ExtraActionArtifactsProvider;
import com.google.devtools.build.lib.analysis.OutputGroupInfo;
+import com.google.devtools.build.lib.analysis.RequiredConfigFragmentsProvider;
import com.google.devtools.build.lib.analysis.TransitiveInfoProvider;
import com.google.devtools.build.lib.analysis.TransitiveInfoProviderMap;
import com.google.devtools.build.lib.analysis.TransitiveInfoProviderMapBuilder;
@@ -45,7 +46,13 @@
public final class MergedConfiguredTarget extends AbstractConfiguredTarget {
private final ConfiguredTarget base;
private final ImmutableList<ConfiguredAspect> aspects;
- private final TransitiveInfoProviderMap providers;
+ /**
+ * Providers that come from any source that isn't a pure pointer to the base rule's providers.
+ *
+ * <p>Examples include providers from aspects and merged providers that appear in both the base
+ * rule and aspects.
+ */
+ private final TransitiveInfoProviderMap nonBaseProviders;
/**
* This exception is thrown when configured targets and aspects
@@ -61,18 +68,18 @@
private MergedConfiguredTarget(
ConfiguredTarget base,
Iterable<ConfiguredAspect> aspects,
- TransitiveInfoProviderMap providers) {
+ TransitiveInfoProviderMap nonBaseProviders) {
super(base.getLabel(), base.getConfigurationKey());
this.base = base;
this.aspects = ImmutableList.copyOf(aspects);
- this.providers = providers;
+ this.nonBaseProviders = nonBaseProviders;
}
@Override
public <P extends TransitiveInfoProvider> P getProvider(Class<P> providerClass) {
AnalysisUtils.checkProvider(providerClass);
- P provider = providers.getProvider(providerClass);
+ P provider = nonBaseProviders.getProvider(providerClass);
if (provider == null) {
provider = base.getProvider(providerClass);
}
@@ -85,8 +92,8 @@
if (base instanceof AbstractConfiguredTarget) {
((AbstractConfiguredTarget) base).addExtraSkylarkKeys(result);
}
- for (int i = 0; i < providers.getProviderCount(); i++) {
- Object classAt = providers.getProviderKeyAt(i);
+ for (int i = 0; i < nonBaseProviders.getProviderCount(); i++) {
+ Object classAt = nonBaseProviders.getProviderKeyAt(i);
if (classAt instanceof String) {
result.accept((String) classAt);
}
@@ -96,7 +103,7 @@
@Override
protected Info rawGetSkylarkProvider(Provider.Key providerKey) {
- Info provider = providers.get(providerKey);
+ Info provider = nonBaseProviders.get(providerKey);
if (provider == null) {
provider = base.get(providerKey);
}
@@ -120,7 +127,7 @@
}
return actions.build();
}
- Object provider = providers.get(providerKey);
+ Object provider = nonBaseProviders.get(providerKey);
if (provider == null) {
provider = base.get(providerKey);
}
@@ -135,20 +142,27 @@
return base;
}
+ TransitiveInfoProviderMapBuilder nonBaseProviders = new TransitiveInfoProviderMapBuilder();
+
// Merge output group providers.
OutputGroupInfo mergedOutputGroupInfo =
OutputGroupInfo.merge(getAllOutputGroupProviders(base, aspects));
+ if (mergedOutputGroupInfo != null) {
+ nonBaseProviders.put(mergedOutputGroupInfo);
+ }
// Merge extra-actions provider.
ExtraActionArtifactsProvider mergedExtraActionProviders = ExtraActionArtifactsProvider.merge(
getAllProviders(base, aspects, ExtraActionArtifactsProvider.class));
-
- TransitiveInfoProviderMapBuilder aspectProviders = new TransitiveInfoProviderMapBuilder();
- if (mergedOutputGroupInfo != null) {
- aspectProviders.put(mergedOutputGroupInfo);
- }
if (mergedExtraActionProviders != null) {
- aspectProviders.add(mergedExtraActionProviders);
+ nonBaseProviders.add(mergedExtraActionProviders);
+ }
+
+ // Merge required config fragments provider.
+ List<RequiredConfigFragmentsProvider> requiredConfigFragmentProviders =
+ getAllProviders(base, aspects, RequiredConfigFragmentsProvider.class);
+ if (!requiredConfigFragmentProviders.isEmpty()) {
+ nonBaseProviders.add(RequiredConfigFragmentsProvider.merge(requiredConfigFragmentProviders));
}
for (ConfiguredAspect aspect : aspects) {
@@ -156,7 +170,8 @@
for (int i = 0; i < providers.getProviderCount(); ++i) {
Object providerKey = providers.getProviderKeyAt(i);
if (OutputGroupInfo.SKYLARK_CONSTRUCTOR.getKey().equals(providerKey)
- || ExtraActionArtifactsProvider.class.equals(providerKey)) {
+ || ExtraActionArtifactsProvider.class.equals(providerKey)
+ || RequiredConfigFragmentsProvider.class.equals(providerKey)) {
continue;
}
@@ -164,28 +179,27 @@
@SuppressWarnings("unchecked")
Class<? extends TransitiveInfoProvider> providerClass =
(Class<? extends TransitiveInfoProvider>) providerKey;
- if (base.getProvider(providerClass) != null
- || aspectProviders.contains(providerClass)) {
+ if (base.getProvider(providerClass) != null || nonBaseProviders.contains(providerClass)) {
throw new DuplicateException("Provider " + providerKey + " provided twice");
}
- aspectProviders.put(
+ nonBaseProviders.put(
providerClass, (TransitiveInfoProvider) providers.getProviderInstanceAt(i));
} else if (providerKey instanceof String) {
String legacyId = (String) providerKey;
- if (base.get(legacyId) != null || aspectProviders.contains(legacyId)) {
+ if (base.get(legacyId) != null || nonBaseProviders.contains(legacyId)) {
throw new DuplicateException("Provider " + legacyId + " provided twice");
}
- aspectProviders.put(legacyId, providers.getProviderInstanceAt(i));
+ nonBaseProviders.put(legacyId, providers.getProviderInstanceAt(i));
} else if (providerKey instanceof Provider.Key) {
Provider.Key key = (Key) providerKey;
- if (base.get(key) != null || aspectProviders.contains(key)) {
+ if (base.get(key) != null || nonBaseProviders.contains(key)) {
throw new DuplicateException("Provider " + key + " provided twice");
}
- aspectProviders.put((Info) providers.getProviderInstanceAt(i));
+ nonBaseProviders.put((Info) providers.getProviderInstanceAt(i));
}
}
}
- return new MergedConfiguredTarget(base, aspects, aspectProviders.build());
+ return new MergedConfiguredTarget(base, aspects, nonBaseProviders.build());
}
private static ImmutableList<OutputGroupInfo> getAllOutputGroupProviders(
diff --git a/src/test/shell/integration/configured_query_test.sh b/src/test/shell/integration/configured_query_test.sh
index 018143c..481554a 100755
--- a/src/test/shell/integration/configured_query_test.sh
+++ b/src/test/shell/integration/configured_query_test.sh
@@ -649,6 +649,70 @@
assert_contains "//$pkg:buildme .*JavaConfiguration" output
}
+# Starlark aspects can't directly require configuration fragments. But they can
+# have implicit label dependencies on rules that do. This test ensures that
+# gets factored in.
+#
+# Note that cquery doesn't support queries over aspects: `deps(//foo)` won't
+# list aspects or traverse their dependencies. This makes it hard to understand
+# *why* a rule requires a fragment if only through an aspect. That's an argument
+# for making cquery generally aspect-aware.
+function test_show_config_fragments_includes_starlark_aspects() {
+ local -r pkg=$FUNCNAME
+ mkdir -p $pkg
+ cat > $pkg/defs.bzl <<EOF
+def _aspect_impl(target, ctx):
+ return []
+
+cc_depending_aspect = aspect(
+ attrs = {
+ # This creates an implicit dependency on a C++ library that
+ # only comes through the aspect.
+ "_extra_dep": attr.label(default = "//$pkg:cclib"),
+ },
+ implementation = _aspect_impl,
+)
+
+def _impl(ctx):
+ pass
+
+simple_rule = rule(
+ implementation = _impl,
+ attrs = {
+ "deps": attr.label_list(aspects = [cc_depending_aspect])
+ }
+)
+EOF
+
+ cat > $pkg/BUILD <<EOF
+load("//$pkg:defs.bzl", "simple_rule")
+
+simple_rule(
+ name = "buildme",
+ deps = [":simple_dep"],
+)
+
+simple_rule(
+ name = "simple_dep",
+)
+
+cc_library(
+ name = "cclib",
+ srcs = ["cclib.cc"],
+)
+EOF
+
+ # No direct requirement:
+ bazel cquery "//$pkg:buildme" --show_config_fragments=direct \
+ > output 2>"$TEST_log" || fail "Expected success"
+ assert_not_contains "//$pkg:buildme .*CppConfiguration" output
+
+ # But there is a transitive requirement through the aspect:
+ bazel cquery "//$pkg:buildme" --show_config_fragments=transitive \
+ > output 2>"$TEST_log" || fail "Expected success"
+ assert_contains "//$pkg:buildme .*CppConfiguration" output
+}
+
function test_manual_tagged_targets_always_included_for_queries() {
local -r pkg=$FUNCNAME
mkdir -p $pkg
