Skip to content

Commit

Permalink
Start the process of getting Target out of ConfiguredTarget: add a ne…
Browse files Browse the repository at this point in the history
…w container, ConfiguredTargetAndTarget, that can be used to access Targets, and deprecate ConfiguredTarget#getTarget. ConfiguredAndTargetObjects are intended to be limited in scope, not being persisted to Skyframe.

The eventual plan is to remove the target field from ConfiguredTarget.

This CL is mostly straightforward, except for dealing with AliasConfiguredTargets, which cause some complications.

A significant cleanup is still needed before #getTarget can be removed, but I don't see any impossible blockers. We will may still need to store a Target-like object in ConfiguredTarget (that has the RuleClass, or at least a string representation of it, for instance), but that will let us avoid storing a full Target together with its associated Package.

PiperOrigin-RevId: 182371566
  • Loading branch information
janakdr authored and Copybara-Service committed Jan 18, 2018
1 parent aad6304 commit f3e6f25
Show file tree
Hide file tree
Showing 16 changed files with 467 additions and 215 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
import com.google.common.collect.ImmutableList;
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
import com.google.devtools.build.lib.skyframe.ConfiguredTargetAndTarget;

/**
* A provider that gives information about the aliases a rule was resolved through.
Expand Down Expand Up @@ -65,12 +66,12 @@ public ImmutableList<Label> getAliasChain() {
return aliasChain;
}

public static String printLabelWithAliasChain(ConfiguredTarget target) {
AliasProvider aliasProvider = target.getProvider(AliasProvider.class);
public static String printLabelWithAliasChain(ConfiguredTargetAndTarget target) {
AliasProvider aliasProvider = target.getConfiguredTarget().getProvider(AliasProvider.class);
String suffix = aliasProvider == null
? ""
: " (aliased through '" + Joiner.on("' -> '").join(aliasProvider.getAliasChain()) + "')";

return "'" + target.getLabel() + "'" + suffix;
return "'" + target.getTarget().getLabel() + "'" + suffix;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
import com.google.devtools.build.lib.packages.Rule;
import com.google.devtools.build.lib.skyframe.AspectFunction;
import com.google.devtools.build.lib.skyframe.AspectValue;
import com.google.devtools.build.lib.skyframe.ConfiguredTargetAndTarget;
import com.google.devtools.build.lib.skyframe.ConfiguredTargetValue;
import com.google.devtools.build.lib.util.OrderedSetMultimap;
import com.google.devtools.build.skyframe.SkyFunction;
Expand All @@ -49,7 +50,7 @@ public final class AspectResolver {
@Nullable
public static OrderedSetMultimap<Dependency, ConfiguredAspect> resolveAspectDependencies(
SkyFunction.Environment env,
Map<SkyKey, ConfiguredTarget> configuredTargetMap,
Map<SkyKey, ConfiguredTargetAndTarget> configuredTargetMap,
Iterable<Dependency> deps,
@Nullable NestedSetBuilder<Package> transitivePackages)
throws AspectFunction.AspectCreationException, InterruptedException {
Expand Down Expand Up @@ -89,8 +90,9 @@ public static OrderedSetMultimap<Dependency, ConfiguredAspect> resolveAspectDepe
}

// Validate that aspect is applicable to "bare" configured target.
ConfiguredTarget associatedTarget = configuredTargetMap
.get(ConfiguredTargetValue.key(dep.getLabel(), dep.getConfiguration()));
ConfiguredTargetAndTarget associatedTarget =
configuredTargetMap.get(
ConfiguredTargetValue.key(dep.getLabel(), dep.getConfiguration()));
if (!aspectMatchesConfiguredTarget(associatedTarget, aspectValue.getAspect())) {
continue;
}
Expand All @@ -113,20 +115,23 @@ public static OrderedSetMultimap<Dependency, ConfiguredAspect> resolveAspectDepe
* combinations of aspects for a particular configured target, so it would result in a
* combinatorial explosion of Skyframe nodes.
*/
public static OrderedSetMultimap<Attribute, ConfiguredTarget> mergeAspects(
public static OrderedSetMultimap<Attribute, ConfiguredTargetAndTarget> mergeAspects(
OrderedSetMultimap<Attribute, Dependency> depValueNames,
Map<SkyKey, ConfiguredTarget> depConfiguredTargetMap,
Map<SkyKey, ConfiguredTargetAndTarget> depConfiguredTargetMap,
OrderedSetMultimap<Dependency, ConfiguredAspect> depAspectMap)
throws MergedConfiguredTarget.DuplicateException {
OrderedSetMultimap<Attribute, ConfiguredTarget> result = OrderedSetMultimap.create();
OrderedSetMultimap<Attribute, ConfiguredTargetAndTarget> result = OrderedSetMultimap.create();

for (Map.Entry<Attribute, Dependency> entry : depValueNames.entries()) {
Dependency dep = entry.getValue();
SkyKey depKey = ConfiguredTargetValue.key(dep.getLabel(), dep.getConfiguration());
ConfiguredTarget depConfiguredTarget = depConfiguredTargetMap.get(depKey);
ConfiguredTargetAndTarget depConfiguredTarget = depConfiguredTargetMap.get(depKey);

result.put(entry.getKey(),
MergedConfiguredTarget.of(depConfiguredTarget, depAspectMap.get(dep)));
result.put(
entry.getKey(),
depConfiguredTarget.fromConfiguredTarget(
MergedConfiguredTarget.of(
depConfiguredTarget.getConfiguredTarget(), depAspectMap.get(dep))));
}

return result;
Expand Down Expand Up @@ -160,14 +165,15 @@ private static AspectValue.AspectKey buildAspectKey(AspectCollection.AspectDeps
return aspectKey;
}

public static boolean aspectMatchesConfiguredTarget(final ConfiguredTarget dep, Aspect aspect) {
public static boolean aspectMatchesConfiguredTarget(
ConfiguredTargetAndTarget dep, Aspect aspect) {
if (!aspect.getDefinition().applyToFiles() && !(dep.getTarget() instanceof Rule)) {
return false;
}
if (dep.getTarget().getAssociatedRule() == null) {
// even aspects that 'apply to files' cannot apply to input files.
return false;
}
return dep.satisfies(aspect.getDefinition().getRequiredProviders());
return dep.getConfiguredTarget().satisfies(aspect.getDefinition().getRequiredProviders());
}
}
38 changes: 21 additions & 17 deletions src/main/java/com/google/devtools/build/lib/analysis/BuildView.java
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
import com.google.common.base.Preconditions;
import com.google.common.base.Predicate;
import com.google.common.collect.ArrayListMultimap;
import com.google.common.collect.Collections2;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableMultimap;
Expand Down Expand Up @@ -80,6 +81,7 @@
import com.google.devtools.build.lib.skyframe.AspectValue;
import com.google.devtools.build.lib.skyframe.AspectValue.AspectKey;
import com.google.devtools.build.lib.skyframe.AspectValue.AspectValueKey;
import com.google.devtools.build.lib.skyframe.ConfiguredTargetAndTarget;
import com.google.devtools.build.lib.skyframe.ConfiguredTargetKey;
import com.google.devtools.build.lib.skyframe.CoverageReportValue;
import com.google.devtools.build.lib.skyframe.SkyframeAnalysisResult;
Expand Down Expand Up @@ -894,13 +896,15 @@ public Iterable<ConfiguredTarget> getDirectPrerequisitesForTesting(
BuildConfigurationCollection configurations)
throws EvalException, InvalidConfigurationException,
InterruptedException, InconsistentAspectOrderException {
return skyframeExecutor.getConfiguredTargets(
eventHandler,
ct.getConfiguration(),
ImmutableSet.copyOf(
getDirectPrerequisiteDependenciesForTesting(
eventHandler, ct, configurations, /*toolchainContext=*/ null)
.values()));
return Collections2.transform(
skyframeExecutor.getConfiguredTargets(
eventHandler,
ct.getConfiguration(),
ImmutableSet.copyOf(
getDirectPrerequisiteDependenciesForTesting(
eventHandler, ct, configurations, /*toolchainContext=*/ null)
.values())),
ConfiguredTargetAndTarget::getConfiguredTarget);
}

@VisibleForTesting
Expand Down Expand Up @@ -1002,7 +1006,7 @@ private ImmutableMap<Label, ConfigMatchingProvider> getConfigurableAttributeKeys
return ImmutableMap.copyOf(keys);
}

private OrderedSetMultimap<Attribute, ConfiguredTarget> getPrerequisiteMapForTesting(
private OrderedSetMultimap<Attribute, ConfiguredTargetAndTarget> getPrerequisiteMapForTesting(
final ExtendedEventHandler eventHandler,
ConfiguredTarget target,
BuildConfigurationCollection configurations,
Expand All @@ -1013,11 +1017,11 @@ private OrderedSetMultimap<Attribute, ConfiguredTarget> getPrerequisiteMapForTes
getDirectPrerequisiteDependenciesForTesting(
eventHandler, target, configurations, toolchainContext);

ImmutableMultimap<Dependency, ConfiguredTarget> cts = skyframeExecutor.getConfiguredTargetMap(
eventHandler,
target.getConfiguration(), ImmutableSet.copyOf(depNodeNames.values()));
ImmutableMultimap<Dependency, ConfiguredTargetAndTarget> cts =
skyframeExecutor.getConfiguredTargetMap(
eventHandler, target.getConfiguration(), ImmutableSet.copyOf(depNodeNames.values()));

OrderedSetMultimap<Attribute, ConfiguredTarget> result = OrderedSetMultimap.create();
OrderedSetMultimap<Attribute, ConfiguredTargetAndTarget> result = OrderedSetMultimap.create();
for (Map.Entry<Attribute, Dependency> entry : depNodeNames.entries()) {
result.putAll(entry.getKey(), cts.get(entry.getValue()));
}
Expand Down Expand Up @@ -1111,7 +1115,7 @@ public RuleContext getRuleContextForTesting(
ToolchainContext toolchainContext =
skyframeExecutor.getToolchainContextForTesting(
requiredToolchains, targetConfig, eventHandler);
OrderedSetMultimap<Attribute, ConfiguredTarget> prerequisiteMap =
OrderedSetMultimap<Attribute, ConfiguredTargetAndTarget> prerequisiteMap =
getPrerequisiteMapForTesting(eventHandler, target, configurations, toolchainContext);
toolchainContext.resolveToolchains(prerequisiteMap);

Expand Down Expand Up @@ -1147,13 +1151,13 @@ public ConfiguredTarget getPrerequisiteConfiguredTargetForTesting(
BuildConfigurationCollection configurations)
throws EvalException, InvalidConfigurationException, InterruptedException,
InconsistentAspectOrderException {
Collection<ConfiguredTarget> configuredTargets =
Collection<ConfiguredTargetAndTarget> configuredTargets =
getPrerequisiteMapForTesting(
eventHandler, dependentTarget, configurations, /*toolchainContext=*/ null)
.values();
for (ConfiguredTarget ct : configuredTargets) {
if (ct.getLabel().equals(desiredTarget)) {
return ct;
for (ConfiguredTargetAndTarget ct : configuredTargets) {
if (ct.getTarget().getLabel().equals(desiredTarget)) {
return ct.getConfiguredTarget();
}
}
return null;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@
import com.google.devtools.build.lib.packages.RuleErrorConsumer;
import com.google.devtools.build.lib.packages.Target;
import com.google.devtools.build.lib.runtime.proto.InvocationPolicyOuterClass.InvocationPolicy;
import com.google.devtools.build.lib.skyframe.ConfiguredTargetAndTarget;
import com.google.devtools.build.lib.skylarkinterface.SkylarkModule;
import com.google.devtools.build.lib.syntax.Environment;
import com.google.devtools.build.lib.syntax.Environment.Extension;
Expand Down Expand Up @@ -85,22 +86,26 @@ public class ConfiguredRuleClassProvider implements RuleClassProvider {
*/
public interface PrerequisiteValidator {
/**
* Checks whether the rule in {@code contextBuilder} is allowed to depend on
* {@code prerequisite} through the attribute {@code attribute}.
* Checks whether the rule in {@code contextBuilder} is allowed to depend on {@code
* prerequisite} through the attribute {@code attribute}.
*
* <p>Can be used for enforcing any organization-specific policies about the layout of the
* workspace.
*/
void validate(
RuleContext.Builder contextBuilder, ConfiguredTarget prerequisite, Attribute attribute);
RuleContext.Builder contextBuilder,
ConfiguredTargetAndTarget prerequisite,
Attribute attribute);
}

/** Validator to check for and warn on the deprecation of dependencies. */
public static final class DeprecationValidator implements PrerequisiteValidator {
/** Checks if the given prerequisite is deprecated and prints a warning if so. */
@Override
public void validate(
RuleContext.Builder contextBuilder, ConfiguredTarget prerequisite, Attribute attribute) {
RuleContext.Builder contextBuilder,
ConfiguredTargetAndTarget prerequisite,
Attribute attribute) {
validateDirectPrerequisiteForDeprecation(
contextBuilder, contextBuilder.getRule(), prerequisite, contextBuilder.forAspect());
}
Expand Down Expand Up @@ -148,7 +153,10 @@ private static boolean shouldEmitDeprecationWarningFor(

/** Checks if the given prerequisite is deprecated and prints a warning if so. */
public static void validateDirectPrerequisiteForDeprecation(
RuleErrorConsumer errors, Rule rule, ConfiguredTarget prerequisite, boolean forAspect) {
RuleErrorConsumer errors,
Rule rule,
ConfiguredTargetAndTarget prerequisite,
boolean forAspect) {
Target prerequisiteTarget = prerequisite.getTarget();
Label prerequisiteLabel = prerequisiteTarget.getLabel();
PackageIdentifier thatPackage = prerequisiteLabel.getPackageIdentifier();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,10 +45,13 @@ public interface ConfiguredTarget extends TransitiveInfoCollection, ClassObject,
*/
String FILES_FIELD = "files";


/**
* Returns the Target with which this {@link ConfiguredTarget} is associated.
* Returns the {@link Target} with which this {@link ConfiguredTarget} is associated.
*
* <p>Do not add new usages if possible. Prefer {@link #getLabel}, or use {@code
* ConfiguredTargetAndTarget} objects.
*/
@Deprecated
Target getTarget();

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@
import com.google.devtools.build.lib.packages.Target;
import com.google.devtools.build.lib.profiler.memory.CurrentRuleTracker;
import com.google.devtools.build.lib.skyframe.BuildConfigurationValue;
import com.google.devtools.build.lib.skyframe.ConfiguredTargetAndTarget;
import com.google.devtools.build.lib.skyframe.ConfiguredTargetKey;
import com.google.devtools.build.lib.util.OrderedSetMultimap;
import com.google.devtools.build.lib.vfs.PathFragment;
Expand Down Expand Up @@ -98,7 +99,7 @@ public ConfiguredTargetFactory(ConfiguredRuleClassProvider ruleClassProvider) {
* to the {@code AnalysisEnvironment}.
*/
private NestedSet<PackageGroupContents> convertVisibility(
OrderedSetMultimap<Attribute, ConfiguredTarget> prerequisiteMap,
OrderedSetMultimap<Attribute, ConfiguredTargetAndTarget> prerequisiteMap,
EventHandler reporter,
Target target,
BuildConfiguration packageGroupConfiguration) {
Expand All @@ -116,7 +117,7 @@ private NestedSet<PackageGroupContents> convertVisibility(
NestedSetBuilder<PackageGroupContents> result = NestedSetBuilder.stableOrder();
for (Label groupLabel : packageGroupsVisibility.getPackageGroups()) {
// PackageGroupsConfiguredTargets are always in the package-group configuration.
ConfiguredTarget group =
TransitiveInfoCollection group =
findPrerequisite(prerequisiteMap, groupLabel, packageGroupConfiguration);
PackageSpecificationProvider provider = null;
// group == null can only happen if the package group list comes
Expand Down Expand Up @@ -145,12 +146,14 @@ private NestedSet<PackageGroupContents> convertVisibility(
}
}

private ConfiguredTarget findPrerequisite(
OrderedSetMultimap<Attribute, ConfiguredTarget> prerequisiteMap, Label label,
private TransitiveInfoCollection findPrerequisite(
OrderedSetMultimap<Attribute, ConfiguredTargetAndTarget> prerequisiteMap,
Label label,
BuildConfiguration config) {
for (ConfiguredTarget prerequisite : prerequisiteMap.get(null)) {
if (prerequisite.getLabel().equals(label) && (prerequisite.getConfiguration() == config)) {
return prerequisite;
for (ConfiguredTargetAndTarget prerequisite : prerequisiteMap.get(null)) {
if (prerequisite.getTarget().getLabel().equals(label)
&& (prerequisite.getConfiguredTarget().getConfiguration() == config)) {
return prerequisite.getConfiguredTarget();
}
}
return null;
Expand Down Expand Up @@ -227,7 +230,7 @@ public final ConfiguredTarget createConfiguredTarget(
Target target,
BuildConfiguration config,
BuildConfiguration hostConfig,
OrderedSetMultimap<Attribute, ConfiguredTarget> prerequisiteMap,
OrderedSetMultimap<Attribute, ConfiguredTargetAndTarget> prerequisiteMap,
ImmutableMap<Label, ConfigMatchingProvider> configConditions,
@Nullable ToolchainContext toolchainContext)
throws InterruptedException {
Expand Down Expand Up @@ -301,7 +304,7 @@ private ConfiguredTarget createRule(
Rule rule,
BuildConfiguration configuration,
BuildConfiguration hostConfiguration,
OrderedSetMultimap<Attribute, ConfiguredTarget> prerequisiteMap,
OrderedSetMultimap<Attribute, ConfiguredTargetAndTarget> prerequisiteMap,
ImmutableMap<Label, ConfigMatchingProvider> configConditions,
@Nullable ToolchainContext toolchainContext)
throws InterruptedException {
Expand Down Expand Up @@ -406,11 +409,11 @@ public AspectDescriptor apply(Aspect aspect) {
*/
public ConfiguredAspect createAspect(
AnalysisEnvironment env,
ConfiguredTarget associatedTarget,
ConfiguredTargetAndTarget associatedTarget,
ImmutableList<Aspect> aspectPath,
ConfiguredAspectFactory aspectFactory,
Aspect aspect,
OrderedSetMultimap<Attribute, ConfiguredTarget> prerequisiteMap,
OrderedSetMultimap<Attribute, ConfiguredTargetAndTarget> prerequisiteMap,
ImmutableMap<Label, ConfigMatchingProvider> configConditions,
@Nullable ToolchainContext toolchainContext,
BuildConfiguration aspectConfiguration,
Expand Down Expand Up @@ -449,8 +452,9 @@ public ConfiguredAspect createAspect(
return null;
}

ConfiguredAspect configuredAspect = aspectFactory
.create(associatedTarget, ruleContext, aspect.getParameters());
ConfiguredAspect configuredAspect =
aspectFactory.create(
associatedTarget.getConfiguredTarget(), ruleContext, aspect.getParameters());
if (configuredAspect != null) {
validateAdvertisedProviders(
configuredAspect, aspect.getDefinition().getAdvertisedProviders(),
Expand Down
Loading

0 comments on commit f3e6f25

Please sign in to comment.