Remove computation of transitively required config fragments in `TransitiveTargetFunction`.
This has been replaced by `RequiredConfigFragmentsProvider`, which does a more robust computation.
PiperOrigin-RevId: 400827565
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
index fe87b52..635fb43 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
@@ -559,7 +559,7 @@
map.put(SkyFunctions.PACKAGE_ERROR, new PackageErrorFunction());
map.put(SkyFunctions.PACKAGE_ERROR_MESSAGE, new PackageErrorMessageFunction());
map.put(SkyFunctions.TARGET_PATTERN_ERROR, new TargetPatternErrorFunction());
- map.put(TransitiveTargetKey.NAME, new TransitiveTargetFunction(ruleClassProvider));
+ map.put(TransitiveTargetKey.NAME, new TransitiveTargetFunction());
map.put(Label.TRANSITIVE_TRAVERSAL, getTransitiveTraversalFunction());
map.put(
SkyFunctions.CONFIGURED_TARGET,
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/TransitiveTargetFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/TransitiveTargetFunction.java
index 08ac1dc..9003587 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/TransitiveTargetFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/TransitiveTargetFunction.java
@@ -13,47 +13,33 @@
// limitations under the License.
package com.google.devtools.build.lib.skyframe;
-import com.google.common.collect.ImmutableList;
-import com.google.devtools.build.lib.analysis.ConfiguredRuleClassProvider;
-import com.google.devtools.build.lib.analysis.config.Fragment;
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.collect.nestedset.NestedSet;
import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
import com.google.devtools.build.lib.events.Event;
import com.google.devtools.build.lib.events.EventHandler;
import com.google.devtools.build.lib.packages.AdvertisedProviderSet;
-import com.google.devtools.build.lib.packages.Attribute;
-import com.google.devtools.build.lib.packages.ConfigurationFragmentPolicy;
import com.google.devtools.build.lib.packages.NoSuchPackageException;
import com.google.devtools.build.lib.packages.NoSuchTargetException;
import com.google.devtools.build.lib.packages.NoSuchThingException;
import com.google.devtools.build.lib.packages.Package;
import com.google.devtools.build.lib.packages.Rule;
-import com.google.devtools.build.lib.packages.RuleClassProvider;
import com.google.devtools.build.lib.packages.Target;
import com.google.devtools.build.lib.packages.TargetUtils;
import com.google.devtools.build.lib.skyframe.TransitiveTargetFunction.TransitiveTargetValueBuilder;
import com.google.devtools.build.skyframe.SkyKey;
import com.google.devtools.build.skyframe.SkyValue;
import com.google.devtools.build.skyframe.ValueOrException2;
-import java.util.LinkedHashSet;
import java.util.Map;
-import java.util.Set;
import javax.annotation.Nullable;
/**
* This class builds transitive Target values such that evaluating a Target value is similar to
* running it through the LabelVisitor.
*/
-public class TransitiveTargetFunction
+final class TransitiveTargetFunction
extends TransitiveBaseTraversalFunction<TransitiveTargetValueBuilder> {
- private final ConfiguredRuleClassProvider ruleClassProvider;
-
- TransitiveTargetFunction(RuleClassProvider ruleClassProvider) {
- this.ruleClassProvider = (ConfiguredRuleClassProvider) ruleClassProvider;
- }
-
@Override
Label argumentFromKey(SkyKey key) {
return ((TransitiveTargetKey) key).getLabel();
@@ -103,92 +89,18 @@
transitiveTargetValue.getErrorLoadingTarget(), eventHandler);
}
}
-
- NestedSet<Class<? extends Fragment>> depFragments =
- transitiveTargetValue.getTransitiveConfigFragments();
- ImmutableList<Class<? extends Fragment>> depFragmentsAsList = depFragments.toList();
- // The simplest collection technique would be to unconditionally add all deps' nested
- // sets to the current target's nested set. But when there's large overlap between their
- // fragment needs, this produces unnecessarily bloated nested sets and a lot of references
- // that don't contribute anything unique to the required fragment set. So we optimize here
- // by completely skipping sets that don't offer anything new. More fine-tuned optimization
- // is possible, but this offers a good balance between simplicity and practical efficiency.
- Set<Class<? extends Fragment>> addedConfigFragments = builder.getConfigFragmentsFromDeps();
- if (!addedConfigFragments.containsAll(depFragmentsAsList)) {
- builder.getTransitiveConfigFragments().addTransitive(depFragments);
- addedConfigFragments.addAll(depFragmentsAsList);
- }
}
+
builder.setSuccessfulTransitiveLoading(successfulTransitiveLoading);
}
@Override
public SkyValue computeSkyValue(
TargetAndErrorIfAny targetAndErrorIfAny, TransitiveTargetValueBuilder builder) {
- Target target = targetAndErrorIfAny.getTarget();
NoSuchTargetException errorLoadingTarget = targetAndErrorIfAny.getErrorLoadingTarget();
-
- // Get configuration fragments directly required by this rule.
- if (target instanceof Rule) {
- Rule rule = (Rule) target;
-
- // Declared by the rule class:
- ConfigurationFragmentPolicy configurationFragmentPolicy =
- rule.getRuleClassObject().getConfigurationFragmentPolicy();
- for (Class<? extends Fragment> fragment : ruleClassProvider.getConfigurationFragments()) {
- // isLegalConfigurationFragment considers both natively declared fragments and Starlark
- // (named) fragments.
- if (configurationFragmentPolicy.isLegalConfigurationFragment(fragment)) {
- addFragmentIfNew(builder, fragment);
- }
- }
-
- // Declared by late-bound attributes:
- for (Attribute attr : rule.getAttributes()) {
- if (attr.isLateBound()) {
- Class<?> fragment = attr.getLateBoundDefault().getFragmentClass();
- if (fragment != null && Fragment.class.isAssignableFrom(fragment)) {
- addFragmentIfNew(builder, fragment.asSubclass(Fragment.class));
- }
- }
- }
-
- // config_setting rules have values like {"some_flag": "some_value"} that need the
- // corresponding fragments in their configurations to properly resolve:
- addFragmentsFromRequiredOptions(builder, rule);
-
- // Fragments to unconditionally include:
- for (Class<? extends Fragment> universalFragment :
- ruleClassProvider.getUniversalFragments()) {
- addFragmentIfNew(builder, universalFragment);
- }
- }
-
return builder.build(errorLoadingTarget);
}
- private void addFragmentsFromRequiredOptions(TransitiveTargetValueBuilder builder, Rule rule) {
- Set<String> requiredOptions =
- rule.getRuleClassObject().getOptionReferenceFunction().apply(rule);
- for (String requiredOption : requiredOptions) {
- Class<? extends Fragment> fragment =
- ruleClassProvider.getConfigurationFragmentForOption(requiredOption);
- // Null values come from CoreOptions, which is implicitly included.
- if (fragment != null) {
- addFragmentIfNew(builder, fragment);
- }
- }
- }
-
- private static void addFragmentIfNew(
- TransitiveTargetValueBuilder builder, Class<? extends Fragment> fragment) {
- // This only checks that the deps don't already use this fragment, not the parent rule itself.
- // So duplicates are still possible. We can further optimize if needed.
- if (!builder.getConfigFragmentsFromDeps().contains(fragment)) {
- builder.getTransitiveConfigFragments().add(fragment);
- }
- }
-
@Nullable
@Override
protected AdvertisedProviderSet getAdvertisedProviderSet(
@@ -255,15 +167,9 @@
static class TransitiveTargetValueBuilder {
private boolean successfulTransitiveLoading;
private final NestedSetBuilder<Label> transitiveTargets;
- private final NestedSetBuilder<Class<? extends Fragment>> transitiveConfigFragments;
- private final Set<Class<? extends Fragment>> configFragmentsFromDeps;
public TransitiveTargetValueBuilder(Target target, boolean packageLoadedSuccessfully) {
this.transitiveTargets = NestedSetBuilder.stableOrder();
- this.transitiveConfigFragments = NestedSetBuilder.stableOrder();
- // No need to store directly required fragments that are also required by deps.
- this.configFragmentsFromDeps = new LinkedHashSet<>();
-
this.successfulTransitiveLoading = packageLoadedSuccessfully;
transitiveTargets.add(target.getLabel());
}
@@ -272,14 +178,6 @@
return transitiveTargets;
}
- public NestedSetBuilder<Class<? extends Fragment>> getTransitiveConfigFragments() {
- return transitiveConfigFragments;
- }
-
- public Set<Class<? extends Fragment>> getConfigFragmentsFromDeps() {
- return configFragmentsFromDeps;
- }
-
public boolean isSuccessfulTransitiveLoading() {
return successfulTransitiveLoading;
}
@@ -290,13 +188,9 @@
public SkyValue build(@Nullable NoSuchTargetException errorLoadingTarget) {
NestedSet<Label> loadedTargets = transitiveTargets.build();
- NestedSet<Class<? extends Fragment>> configFragments = transitiveConfigFragments.build();
return successfulTransitiveLoading
- ? TransitiveTargetValue.successfulTransitiveLoading(loadedTargets, configFragments)
- : TransitiveTargetValue.unsuccessfulTransitiveLoading(
- loadedTargets,
- errorLoadingTarget,
- configFragments);
+ ? TransitiveTargetValue.successfulTransitiveLoading(loadedTargets)
+ : TransitiveTargetValue.unsuccessfulTransitiveLoading(loadedTargets, errorLoadingTarget);
}
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/TransitiveTargetValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/TransitiveTargetValue.java
index 32ea0c6..107183f 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/TransitiveTargetValue.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/TransitiveTargetValue.java
@@ -13,7 +13,6 @@
// limitations under the License.
package com.google.devtools.build.lib.skyframe;
-import com.google.devtools.build.lib.analysis.config.Fragment;
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.collect.nestedset.NestedSet;
import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
@@ -31,36 +30,25 @@
public class TransitiveTargetValue implements SkyValue {
private final NestedSet<Label> transitiveTargets;
private final boolean encounteredLoadingError;
- @Nullable private NoSuchTargetException errorLoadingTarget;
- private NestedSet<Class<? extends Fragment>> transitiveConfigFragments;
+ @Nullable private final NoSuchTargetException errorLoadingTarget;
private TransitiveTargetValue(
NestedSet<Label> transitiveTargets,
boolean encounteredLoadingError,
- @Nullable NoSuchTargetException errorLoadingTarget,
- NestedSet<Class<? extends Fragment>> transitiveConfigFragments) {
+ @Nullable NoSuchTargetException errorLoadingTarget) {
this.transitiveTargets = transitiveTargets;
this.encounteredLoadingError = encounteredLoadingError;
this.errorLoadingTarget = errorLoadingTarget;
- this.transitiveConfigFragments = transitiveConfigFragments;
}
static TransitiveTargetValue unsuccessfulTransitiveLoading(
- NestedSet<Label> transitiveTargets,
- @Nullable NoSuchTargetException errorLoadingTarget,
- NestedSet<Class<? extends Fragment>> transitiveConfigFragments) {
+ NestedSet<Label> transitiveTargets, @Nullable NoSuchTargetException errorLoadingTarget) {
return new TransitiveTargetValue(
- transitiveTargets,
- /*encounteredLoadingError=*/ true,
- errorLoadingTarget,
- transitiveConfigFragments);
+ transitiveTargets, /*encounteredLoadingError=*/ true, errorLoadingTarget);
}
- static TransitiveTargetValue successfulTransitiveLoading(
- NestedSet<Label> transitiveTargets,
- NestedSet<Class<? extends Fragment>> transitiveConfigFragments) {
- return new TransitiveTargetValue(
- transitiveTargets, /*encounteredLoadingError=*/ false, null, transitiveConfigFragments);
+ static TransitiveTargetValue successfulTransitiveLoading(NestedSet<Label> transitiveTargets) {
+ return new TransitiveTargetValue(transitiveTargets, /*encounteredLoadingError=*/ false, null);
}
/** Returns the error, if any, from loading the target. */
@@ -77,26 +65,4 @@
public boolean encounteredLoadingError() {
return encounteredLoadingError;
}
-
- /**
- * Returns the set of {@link Fragment} classes required to configure a rule's transitive closure.
- * These are used to instantiate the right {@link BuildConfigurationValue}.
- *
- * <p>This provides the basis for rule-scoped configurations. For example, Java-related build
- * flags have nothing to do with C++. So changing a Java flag shouldn't invalidate a C++ rule
- * (unless it has transitive dependencies on other Java rules). Likewise, a C++ rule shouldn't
- * fail because the Java configuration doesn't recognize the chosen architecture.
- *
- * <p>The general principle is that a rule can be influenced by the configuration parameters it
- * directly uses and the configuration parameters its transitive dependencies use (since it reads
- * its dependencies as part of analysis). So we need to 1) determine which configuration fragments
- * provide these parameters, 2) load those fragments, then 3) create a configuration from them to
- * feed the rule's configured target. This provides the first step.
- *
- * <p>See {@link
- * com.google.devtools.build.lib.packages.RuleClass.Builder#requiresConfigurationFragments}
- */
- public NestedSet<Class<? extends Fragment>> getTransitiveConfigFragments() {
- return transitiveConfigFragments;
- }
}
