Skip to content

Commit

Permalink
$ blaze cquery --show_config_fragments: include aspects
Browse files Browse the repository at this point in the history 
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 #10613.

PiperOrigin-RevId: 293669124
  • Loading branch information
@gregestren
gregestren authored and Copybara-Service committed Feb 6, 2020
1 parent 2b708fb commit 130feba
Show file tree
Hide file tree
Showing 5 changed files with 165 additions and 23 deletions.
14 changes: 14 additions & 0 deletions src/main/java/com/google/devtools/build/lib/analysis/ConfiguredAspect.java
View file
Expand Up @@ -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;
Expand Down Expand Up @@ -281,6 +283,18 @@ public ConfiguredAspect build() throws ActionConflictException {
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(),
Expand Down
33 changes: 33 additions & 0 deletions src/main/java/com/google/devtools/build/lib/analysis/ConfiguredTargetFactory.java
View file
Expand Up @@ -342,6 +342,35 @@ private static ImmutableSet<String> getRequiredConfigFragments(
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).
Expand Down Expand Up @@ -599,6 +628,10 @@ public ConfiguredAspect createAspect(
.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
Expand Down
17 changes: 17 additions & 0 deletions src/main/java/com/google/devtools/build/lib/analysis/RequiredConfigFragmentsProvider.java
View file
Expand Up @@ -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
Expand All @@ -36,4 +39,18 @@ public RequiredConfigFragmentsProvider(ImmutableSet<String> requiredConfigFragme
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());
}
}
60 changes: 37 additions & 23 deletions ...java/com/google/devtools/build/lib/analysis/configuredtargets/MergedConfiguredTarget.java
View file
Expand Up @@ -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;
Expand All @@ -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
Expand All @@ -61,18 +68,18 @@ public DuplicateException(String message) {
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);
}
Expand All @@ -85,8 +92,8 @@ protected void addExtraSkylarkKeys(Consumer<String> result) {
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);
}
Expand All @@ -96,7 +103,7 @@ protected void addExtraSkylarkKeys(Consumer<String> result) {

@Override
protected Info rawGetSkylarkProvider(Provider.Key providerKey) {
Info provider = providers.get(providerKey);
Info provider = nonBaseProviders.get(providerKey);
if (provider == null) {
provider = base.get(providerKey);
}
Expand All @@ -120,7 +127,7 @@ protected Object rawGetSkylarkProvider(String providerKey) {
}
return actions.build();
}
Object provider = providers.get(providerKey);
Object provider = nonBaseProviders.get(providerKey);
if (provider == null) {
provider = base.get(providerKey);
}
Expand All @@ -135,57 +142,64 @@ public static ConfiguredTarget of(ConfiguredTarget base, Iterable<ConfiguredAspe
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) {
TransitiveInfoProviderMap providers = aspect.getProviders();
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;
}

if (providerKey instanceof Class<?>) {
@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(
Expand Down
64 changes: 64 additions & 0 deletions src/test/shell/integration/configured_query_test.sh
View file
Expand Up @@ -649,6 +649,70 @@ EOF
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
Expand Down

0 comments on commit 130feba

Please sign in to comment.