diff --git a/dataset/GitHub_Java/bazelbuild.bazel/src/main/java/com/google/devtools/build/lib/analysis/DependencyResolver.java b/dataset/GitHub_Java/bazelbuild.bazel/src/main/java/com/google/devtools/build/lib/analysis/DependencyResolver.java index 5eb5d4dba2c..5d80db2ecbc 100644 --- a/dataset/GitHub_Java/bazelbuild.bazel/src/main/java/com/google/devtools/build/lib/analysis/DependencyResolver.java +++ b/dataset/GitHub_Java/bazelbuild.bazel/src/main/java/com/google/devtools/build/lib/analysis/DependencyResolver.java @@ -21,6 +21,7 @@ import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; +import com.google.common.collect.ImmutableSet; import com.google.devtools.build.lib.analysis.AspectCollection.AspectCycleOnPathException; import com.google.devtools.build.lib.analysis.DependencyKind.AttributeDependencyKind; import com.google.devtools.build.lib.analysis.DependencyKind.ToolchainDependencyKind; @@ -39,6 +40,7 @@ import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder; import com.google.devtools.build.lib.packages.Aspect; import com.google.devtools.build.lib.packages.AspectClass; +import com.google.devtools.build.lib.packages.AspectDescriptor; import com.google.devtools.build.lib.packages.Attribute; import com.google.devtools.build.lib.packages.Attribute.ComputedDefault; import com.google.devtools.build.lib.packages.Attribute.LateBoundDefault; @@ -61,7 +63,8 @@ import java.util.List; import java.util.Map; import javax.annotation.Nullable; -import net.starlark.java.syntax.Location; +import net.starlark.java.eval.EvalException; +import net.starlark.java.eval.Starlark; /** * Resolver for dependencies between configured targets. @@ -183,7 +186,7 @@ public final OrderedSetMultimap dependentNodeMap( @Nullable ToolchainCollection toolchainContexts, boolean useToolchainTransition, @Nullable TransitionFactory trimmingTransitionFactory) - throws Failure, InterruptedException, InconsistentAspectOrderException { + throws EvalException, InterruptedException, InconsistentAspectOrderException { NestedSetBuilder rootCauses = NestedSetBuilder.stableOrder(); OrderedSetMultimap outgoingEdges = dependentNodeMap( @@ -210,7 +213,7 @@ public final OrderedSetMultimap dependentNodeMap( * representing the given target and configuration. * *

Otherwise {@code aspects} represents an aspect path. The function returns dependent nodes of - * the entire path applied to given target and configuration. These are the dependent nodes of the + * the entire path applied to given target and configuration. These are the depenent nodes of the * last aspect in the path. * *

This also implements the first step of applying configuration transitions, namely, split @@ -241,7 +244,7 @@ public final OrderedSetMultimap dependentNodeMap( boolean useToolchainTransition, NestedSetBuilder rootCauses, @Nullable TransitionFactory trimmingTransitionFactory) - throws Failure, InterruptedException, InconsistentAspectOrderException { + throws EvalException, InterruptedException, InconsistentAspectOrderException { Target target = node.getTarget(); BuildConfiguration config = node.getConfiguration(); OrderedSetMultimap outgoingLabels = OrderedSetMultimap.create(); @@ -303,11 +306,11 @@ public final OrderedSetMultimap dependentNodeMap( partiallyResolveDependencies( OrderedSetMultimap outgoingLabels, @Nullable Rule fromRule, - @Nullable ConfiguredAttributeMapper attributeMap, + ConfiguredAttributeMapper attributeMap, @Nullable ToolchainCollection toolchainContexts, boolean useToolchainTransition, Iterable aspects) - throws Failure { + throws EvalException { OrderedSetMultimap partiallyResolvedDeps = OrderedSetMultimap.create(); @@ -368,19 +371,6 @@ public final OrderedSetMultimap dependentNodeMap( continue; } - // Compute the set of aspects that could be applied to a dependency. This is composed of two - // parts: - // - // 1. The aspects are visible to this aspect being evaluated, if any (if another aspect is - // visible on the configured target for this one, it should also be visible on the - // dependencies for consistency). This is the argument "aspects". - // 2. The aspects propagated by the attributes of this configured target / aspect. This is - // computed by collectPropagatingAspects(). - // - // The presence of an aspect here does not necessarily mean that it will be available on a - // dependency: it can still be filtered out because it requires a provider that the configured - // target it should be attached to it doesn't advertise. This is taken into account in - // computeAspectCollections() once the Target instances for the dependencies are known. Attribute attribute = entry.getKey().getAttribute(); ImmutableList.Builder propagatingAspects = ImmutableList.builder(); propagatingAspects.addAll(attribute.getAspects(fromRule)); @@ -393,11 +383,15 @@ public final OrderedSetMultimap dependentNodeMap( String execGroup = ((ExecutionTransitionFactory) attribute.getTransitionFactory()).getExecGroup(); if (!toolchainContexts.hasToolchainContext(execGroup)) { - throw new Failure( - fromRule != null ? fromRule.getLocation() : null, + String error = String.format( "Attr '%s' declares a transition for non-existent exec group '%s'", - attribute.getName(), execGroup)); + attribute.getName(), execGroup); + if (fromRule != null) { + throw new EvalException(fromRule.getLocation(), error); + } else { + throw Starlark.errorf("%s", error); + } } if (toolchainContexts.getToolchainContext(execGroup).executionPlatform() != null) { executionPlatformLabel = @@ -461,7 +455,7 @@ private OrderedSetMultimap fullyResolveDependenci trimmingTransitionFactory); AspectCollection requiredAspects = - computeAspectCollections(partiallyResolvedDependency.getPropagatingAspects(), toTarget); + filterPropagatingAspects(partiallyResolvedDependency.getPropagatingAspects(), toTarget); DependencyKey.Builder dependencyKeyBuilder = partiallyResolvedDependency.getDependencyKeyBuilder(); @@ -472,22 +466,6 @@ private OrderedSetMultimap fullyResolveDependenci return outgoingEdges; } - /** A DependencyResolver.Failure indicates a failure during dependency resolution. */ - public static class Failure extends Exception { - @Nullable private final Location location; - - private Failure(Location location, String message) { - super(message); - this.location = location; - } - - /** Returns the location of the error, if known. */ - @Nullable - public Location getLocation() { - return location; - } - } - private void visitRule( TargetAndConfiguration node, BuildConfiguration hostConfig, @@ -495,16 +473,12 @@ private void visitRule( ConfiguredAttributeMapper attributeMap, @Nullable ToolchainCollection toolchainContexts, OrderedSetMultimap outgoingLabels) - throws Failure { + throws EvalException { Preconditions.checkArgument(node.getTarget() instanceof Rule, node); BuildConfiguration ruleConfig = Preconditions.checkNotNull(node.getConfiguration(), node); Rule rule = (Rule) node.getTarget(); - try { - attributeMap.validateAttributes(); - } catch (ConfiguredAttributeMapper.ValidationException ex) { - throw new Failure(rule.getLocation(), ex.getMessage()); - } + attributeMap.validateAttributes(); visitTargetVisibility(node, outgoingLabels); resolveAttributes(outgoingLabels, rule, attributeMap, aspects, ruleConfig, hostConfig); @@ -743,15 +717,11 @@ private List getAttributes(Rule rule, Iterable } /** - * Compute the way aspects should be computed for the direct dependencies. - * - *

This is done by filtering the aspects that can be propagated on any attribute according to - * the providers advertised by direct dependencies and by creating the {@link AspectCollection} - * that tells how to compute the final set of providers based on the interdependencies between the - * propagating aspects. + * Filter the set of aspects that are to be propagated according to the dependency type and the + * set of advertised providers of the dependency. */ - private static AspectCollection computeAspectCollections( - ImmutableList aspects, Target toTarget) throws InconsistentAspectOrderException { + private AspectCollection filterPropagatingAspects(ImmutableList aspects, Target toTarget) + throws InconsistentAspectOrderException { if (toTarget instanceof OutputFile) { aspects = aspects.stream() @@ -766,6 +736,7 @@ private static AspectCollection computeAspectCollections( Rule toRule = (Rule) toTarget; ImmutableList.Builder filteredAspectPath = ImmutableList.builder(); + ImmutableSet.Builder visibleAspects = ImmutableSet.builder(); for (Aspect aspect : aspects) { if (aspect @@ -773,10 +744,11 @@ private static AspectCollection computeAspectCollections( .getRequiredProviders() .isSatisfiedBy(toRule.getRuleClassObject().getAdvertisedProviders())) { filteredAspectPath.add(aspect); + visibleAspects.add(aspect.getDescriptor()); } } try { - return AspectCollection.create(filteredAspectPath.build()); + return AspectCollection.create(filteredAspectPath.build(), visibleAspects.build()); } catch (AspectCycleOnPathException e) { throw new InconsistentAspectOrderException(toTarget, e); } diff --git a/dataset/GitHub_Java/bazelbuild.bazel/src/main/java/com/google/devtools/build/lib/analysis/Util.java b/dataset/GitHub_Java/bazelbuild.bazel/src/main/java/com/google/devtools/build/lib/analysis/Util.java index e40c3ea5ace..5f9c8e52c2c 100644 --- a/dataset/GitHub_Java/bazelbuild.bazel/src/main/java/com/google/devtools/build/lib/analysis/Util.java +++ b/dataset/GitHub_Java/bazelbuild.bazel/src/main/java/com/google/devtools/build/lib/analysis/Util.java @@ -14,18 +14,26 @@ package com.google.devtools.build.lib.analysis; +import com.google.common.collect.ImmutableSet; +import com.google.common.collect.ListMultimap; +import com.google.common.collect.Sets; +import com.google.devtools.build.lib.analysis.config.BuildConfiguration; import com.google.devtools.build.lib.cmdline.Label; +import com.google.devtools.build.lib.collect.compacthashset.CompactHashSet; +import com.google.devtools.build.lib.packages.AttributeMap; import com.google.devtools.build.lib.packages.Target; +import com.google.devtools.build.lib.skyframe.ConfiguredTargetAndData; +import com.google.devtools.build.lib.skyframe.ConfiguredTargetKey; import com.google.devtools.build.lib.vfs.PathFragment; +import java.util.List; +import java.util.Set; -/** - * Utility methods for use by ConfiguredTarget implementations. - */ +/** Utility methods for use by ConfiguredTarget implementations. */ public abstract class Util { private Util() {} - //---------- Label and Target related methods + // ---------- Label and Target related methods /** * Returns the workspace-relative path of the specified target (file or rule). @@ -46,8 +54,8 @@ public static PathFragment getWorkspaceRelativePath(Label label) { } /** - * Returns the workspace-relative path of the specified target (file or rule), - * prepending a prefix and appending a suffix. + * Returns the workspace-relative path of the specified target (file or rule), prepending a prefix + * and appending a suffix. * *

For example, "//foo/bar:wiz" and "//foo:bar/wiz" both result in "foo/bar/wiz". */ @@ -55,10 +63,75 @@ public static PathFragment getWorkspaceRelativePath(Target target, String prefix return target.getLabel().getPackageFragment().getRelative(prefix + target.getName() + suffix); } - /** - * Checks if a PathFragment contains a '-'. - */ + /** Checks if a PathFragment contains a '-'. */ public static boolean containsHyphen(PathFragment path) { return path.getPathString().indexOf('-') >= 0; } + + // ---------- Implicit dependency extractor + + /* + * Given a RuleContext, find all the implicit attribute deps aka deps that weren't explicitly set + * in the build file but are attached behind the scenes to some attribute. This means this + * function does *not* cover deps attached other ways e.g. toolchain-related implicit deps + * (see {@link PostAnalysisQueryEnvironment#targetifyValues} for more info on further implicit + * deps filtering). + * note: nodes that are depended on both implicitly and explicitly are considered explicit. + */ + public static ImmutableSet findImplicitDeps(RuleContext ruleContext) { + Set maybeImplicitDeps = CompactHashSet.create(); + Set explicitDeps = CompactHashSet.create(); + // Consider rule attribute dependencies. + AttributeMap attributes = ruleContext.attributes(); + ListMultimap targetMap = + ruleContext.getConfiguredTargetAndDataMap(); + for (String attrName : attributes.getAttributeNames()) { + List attrValues = targetMap.get(attrName); + if (attrValues != null && !attrValues.isEmpty()) { + if (attributes.isAttributeValueExplicitlySpecified(attrName)) { + addLabelsAndConfigs(explicitDeps, attrValues); + } else { + addLabelsAndConfigs(maybeImplicitDeps, attrValues); + } + } + } + // Consider toolchain dependencies. + ToolchainContext toolchainContext = ruleContext.getToolchainContext(); + if (toolchainContext != null) { + // This logic should stay up to date with the dep creation logic in + // DependencyResolver#partiallyResolveDependencies. + BuildConfiguration targetConfiguration = ruleContext.getConfiguration(); + BuildConfiguration hostConfiguration = ruleContext.getHostConfiguration(); + for (Label toolchain : toolchainContext.resolvedToolchainLabels()) { + if (DependencyResolver.shouldUseToolchainTransition( + targetConfiguration, ruleContext.getRule())) { + maybeImplicitDeps.add( + ConfiguredTargetKey.builder() + .setLabel(toolchain) + .setConfiguration(targetConfiguration) + .setToolchainContextKey(toolchainContext.key()) + .build()); + } else { + maybeImplicitDeps.add( + ConfiguredTargetKey.builder() + .setLabel(toolchain) + .setConfiguration(hostConfiguration) + .build()); + } + } + } + return ImmutableSet.copyOf(Sets.difference(maybeImplicitDeps, explicitDeps)); + } + + private static void addLabelsAndConfigs( + Set set, List deps) { + for (ConfiguredTargetAndData dep : deps) { + // Dereference any aliases that might be present. + set.add( + ConfiguredTargetKey.builder() + .setLabel(dep.getConfiguredTarget().getOriginalLabel()) + .setConfiguration(dep.getConfiguration()) + .build()); + } + } } diff --git a/dataset/GitHub_Java/bazelbuild.bazel/src/test/java/com/google/devtools/build/lib/query2/testutil/PostAnalysisQueryTest.java b/dataset/GitHub_Java/bazelbuild.bazel/src/test/java/com/google/devtools/build/lib/query2/testutil/PostAnalysisQueryTest.java index e658f4893a3..2798a4ee8ef 100644 --- a/dataset/GitHub_Java/bazelbuild.bazel/src/test/java/com/google/devtools/build/lib/query2/testutil/PostAnalysisQueryTest.java +++ b/dataset/GitHub_Java/bazelbuild.bazel/src/test/java/com/google/devtools/build/lib/query2/testutil/PostAnalysisQueryTest.java @@ -18,15 +18,15 @@ import static com.google.devtools.build.lib.packages.BuildType.LABEL; import static com.google.devtools.build.lib.testutil.TestConstants.PLATFORM_LABEL; -import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; import com.google.common.collect.Iterables; +import com.google.devtools.build.lib.analysis.ConfiguredRuleClassProvider; import com.google.devtools.build.lib.analysis.config.BuildConfiguration; import com.google.devtools.build.lib.analysis.config.BuildOptions; import com.google.devtools.build.lib.analysis.config.BuildOptionsView; import com.google.devtools.build.lib.analysis.config.FragmentOptions; import com.google.devtools.build.lib.analysis.config.transitions.PatchTransition; -import com.google.devtools.build.lib.analysis.util.DummyTestFragment.DummyTestOptions; +import com.google.devtools.build.lib.analysis.test.TestConfiguration.TestOptions; import com.google.devtools.build.lib.analysis.util.MockRule; import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.events.EventHandler; @@ -40,7 +40,7 @@ import com.google.devtools.build.lib.query2.engine.QueryParser; import com.google.devtools.build.lib.server.FailureDetails.ConfigurableQuery.Code; import com.google.devtools.build.lib.server.FailureDetails.FailureDetail; -import com.google.devtools.build.lib.testutil.TestConstants; +import com.google.devtools.build.lib.testutil.TestRuleClassProvider; import com.google.devtools.build.lib.util.FileTypeSet; import java.util.Collections; import java.util.HashMap; @@ -136,6 +136,15 @@ private void maybeParseUniverseScope(String query) throws Exception { protected abstract BuildConfiguration getConfiguration(T target); + protected ConfiguredRuleClassProvider.Builder setRuleClassProviders(MockRule... mockRules) { + ConfiguredRuleClassProvider.Builder builder = new ConfiguredRuleClassProvider.Builder(); + TestRuleClassProvider.addStandardRules(builder); + for (MockRule rule : mockRules) { + builder.addRuleDefinition(rule); + } + return builder; + } + @Override protected boolean testConfigurableAttributes() { // ConfiguredTargetQuery knows the actual configuration, so it doesn't falsely overapproximate. @@ -252,116 +261,10 @@ public void testNoImplicitDeps_toolchains() throws Exception { evalToListOfStrings(explicits + " + " + implicits + " + " + PLATFORM_LABEL)); helper.setQuerySettings(Setting.NO_IMPLICIT_DEPS); - ImmutableList filteredDeps = evalToListOfStrings("deps(//test:my_rule)"); - assertThat(filteredDeps).containsAtLeastElementsIn(evalToListOfStrings(explicits)); - assertThat(filteredDeps).containsNoneIn(evalToListOfStrings(implicits)); - } - - @Test - public void testNoImplicitDeps_starlark_toolchains() throws Exception { - writeFile( - "test/toolchain.bzl", - "def _impl(ctx):", - " toolchain = platform_common.ToolchainInfo()", - " return [toolchain]", - "test_toolchain = rule(", - " implementation = _impl,", - ")"); - writeFile( - "test/rule.bzl", - "def _impl(ctx):", - " return []", - "implicit_toolchain_deps_rule = rule(", - " implementation = _impl,", - " toolchains = ['//test:toolchain_type']", - ")"); - writeFile( - "test/BUILD", - "load(':toolchain.bzl', 'test_toolchain')", - "load(':rule.bzl', 'implicit_toolchain_deps_rule')", - "implicit_toolchain_deps_rule(", - " name = 'my_rule',", - ")", - "toolchain_type(name = 'toolchain_type')", - "toolchain(", - " name = 'toolchain',", - " toolchain_type = ':toolchain_type',", - " toolchain = ':toolchain_impl',", - ")", - "test_toolchain(name = 'toolchain_impl')"); - ((PostAnalysisQueryHelper) helper).useConfiguration("--extra_toolchains=//test:toolchain"); - - String implicits = "//test:toolchain_impl"; - String explicits = "//test:my_rule"; - - // Check for implicit toolchain dependencies assertThat(evalToListOfStrings("deps(//test:my_rule)")) - .containsAtLeastElementsIn(evalToListOfStrings(explicits + " + " + implicits)); - - helper.setQuerySettings(Setting.NO_IMPLICIT_DEPS); - ImmutableList filteredDeps = evalToListOfStrings("deps(//test:my_rule)"); - assertThat(filteredDeps).containsAtLeastElementsIn(evalToListOfStrings(explicits)); - assertThat(filteredDeps).containsNoneIn(evalToListOfStrings(implicits)); - } - - @Test - public void testNoImplicitDeps_cc_toolchains() throws Exception { - writeFile( - "test/toolchain/toolchain_config.bzl", - "def _impl(ctx):", - " return cc_common.create_cc_toolchain_config_info(", - " ctx = ctx,", - " toolchain_identifier = 'mock-llvm-toolchain-k8',", - " host_system_name = 'mock-system-name-for-k8',", - " target_system_name = 'mock-target-system-name-for-k8',", - " target_cpu = 'k8',", - " target_libc = 'mock-libc-for-k8',", - " compiler = 'mock-compiler-for-k8',", - " abi_libc_version = 'mock-abi-libc-for-k8',", - " abi_version = 'mock-abi-version-for-k8')", - "cc_toolchain_config = rule(", - " implementation = _impl,", - " attrs = {},", - " provides = [CcToolchainConfigInfo],", - ")"); - writeFile( - "test/toolchain/BUILD", - "load(':toolchain_config.bzl', 'cc_toolchain_config')", - "cc_toolchain_config(name = 'some-cc-toolchain-config')", - "filegroup(name = 'nothing', srcs = [])", - "cc_toolchain(", - " name = 'some_cc_toolchain_impl',", - " all_files = ':nothing',", - " as_files = ':nothing',", - " compiler_files = ':nothing',", - " dwp_files = ':nothing',", - " linker_files = ':nothing',", - " objcopy_files = ':nothing',", - " strip_files = ':nothing',", - " toolchain_config = ':some-cc-toolchain-config',", - ")", - "toolchain(", - " name = 'some_cc_toolchain',", - " toolchain = ':some_cc_toolchain_impl',", - " toolchain_type = '" + TestConstants.TOOLS_REPOSITORY + "//tools/cpp:toolchain_type',", - ")"); - writeFile( - "test/BUILD", "cc_library(", " name = 'my_rule',", " srcs = ['whatever.cpp'],", ")"); - ((PostAnalysisQueryHelper) helper) - .useConfiguration("--extra_toolchains=//test/toolchain:some_cc_toolchain"); - - String implicits = "//test/toolchain:some_cc_toolchain_impl"; - String explicits = "//test:my_rule"; - - // Check for implicit toolchain dependencies + .containsAtLeastElementsIn(evalToListOfStrings(explicits)); assertThat(evalToListOfStrings("deps(//test:my_rule)")) - .containsAtLeastElementsIn( - evalToListOfStrings(explicits + " + " + implicits + " + " + PLATFORM_LABEL)); - - helper.setQuerySettings(Setting.NO_IMPLICIT_DEPS); - ImmutableList filteredDeps = evalToListOfStrings("deps(//test:my_rule)"); - assertThat(filteredDeps).containsAtLeastElementsIn(evalToListOfStrings(explicits)); - assertThat(filteredDeps).containsNoneIn(evalToListOfStrings(implicits)); + .containsNoneIn(evalToListOfStrings(implicits)); } // Regression test for b/148550864 @@ -434,18 +337,18 @@ public void testSet() throws Exception { super.testSet(); } - /** PatchTransition on --foo */ - public static class FooPatchTransition implements PatchTransition { + /** PatchTransition on --test_arg */ + public static class TestArgPatchTransition implements PatchTransition { String toOption; String name; - public FooPatchTransition(String toOption, String name) { + public TestArgPatchTransition(String toOption, String name) { this.toOption = toOption; this.name = name; } - public FooPatchTransition(String toOption) { - this(toOption, "FooPatchTransition"); + public TestArgPatchTransition(String toOption) { + this(toOption, "TestArgPatchTransition"); } @Override @@ -455,13 +358,13 @@ public String getName() { @Override public ImmutableSet> requiresOptionFragments() { - return ImmutableSet.of(DummyTestOptions.class); + return ImmutableSet.of(TestOptions.class); } @Override public BuildOptions patch(BuildOptionsView options, EventHandler eventHandler) { BuildOptionsView result = options.clone(); - result.get(DummyTestOptions.class).foo = toOption; + result.get(TestOptions.class).testArguments = Collections.singletonList(toOption); return result.underlying(); } } @@ -472,7 +375,7 @@ public void testMultipleTopLevelConfigurations() throws Exception { () -> MockRule.define( "transitioned_rule", - (builder, env) -> builder.cfg(new FooPatchTransition("SET BY PATCH")).build()); + (builder, env) -> builder.cfg(new TestArgPatchTransition("SET BY PATCH")).build()); MockRule untransitionedRule = () -> MockRule.define("untransitioned_rule"); @@ -504,7 +407,7 @@ public void testMultipleTopLevelConfigurations_multipleConfigsPrefersTopLevel() "rule_with_transition_and_dep", (builder, env) -> builder - .cfg(new FooPatchTransition("SET BY PATCH")) + .cfg(new TestArgPatchTransition("SET BY PATCH")) .addAttribute( attr("dep", LABEL).allowedFileTypes(FileTypeSet.ANY_FILE).build()) .build());