cquery --show_config_fragments: report fragments used by transitions
Also:
1 Move collection logic into its own utility class to declutter a
hard-enough-to-understand core class
2 Change the signature on Starlark transition requirements from fragment classes
to the user-friendly strings they ultimately get transformed to. We do this
early because transitions report options, not fragments. Trying to reduce them
to fragments involves a way more complicated API with no real benefit
(particularly for Starlark flags and CoreOptions, which have no fragment).
This change has some quirks, which is why I changed some existing test
signatures. But it's far preferable IMO to overly complicating Blaze's core APIs
One of 2's particular quirks is that the final "required fragments" string isn't consistent. For example, if a rule definition requires the C++ fragment it returns "CppConfiguration", whereas if a select() or transition requires a C++ option it returns "CppOptions". This is arguably confusing. But the alternative of forcing everything into a proper Fragment (CppConfiguration) requires passing a FragmentOptions -> Fragment map to all code that computes FragmentOptions, which really complicates Blaze's APIs.
Furthermore, "blaze config" outputs a mapping of fragments to FragmentOptions, which means consumers have all the info they need to connect and deduplicate.
The result is a minor inconvenience that's not too invasive to Blaze and doesn't compromise ultimate correctness.
In service of https://github.com/bazelbuild/bazel/issues/11258.
PiperOrigin-RevId: 319306418
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/BUILD b/src/main/java/com/google/devtools/build/lib/analysis/BUILD
index afc5ee7..4a976bc 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/BUILD
+++ b/src/main/java/com/google/devtools/build/lib/analysis/BUILD
@@ -233,6 +233,7 @@
"config/ConvenienceSymlinks.java",
"config/DependencyEvaluationException.java",
"config/FragmentCollection.java",
+ "config/RequiredFragmentsUtil.java",
"config/TransitionResolver.java",
"configuredtargets/AbstractConfiguredTarget.java",
"configuredtargets/EnvironmentGroupConfiguredTarget.java",
@@ -1793,6 +1794,7 @@
":config/build_options",
":config/fragment_options",
"//src/main/java/com/google/devtools/build/lib/events",
+ "//src/main/java/com/google/devtools/build/lib/util",
"//third_party:guava",
],
)
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 cddea57..dd0b0ec 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
@@ -20,7 +20,6 @@
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
-import com.google.common.collect.ImmutableSortedSet;
import com.google.devtools.build.lib.actions.Artifact;
import com.google.devtools.build.lib.actions.Artifact.SourceArtifact;
import com.google.devtools.build.lib.actions.ArtifactFactory;
@@ -29,8 +28,8 @@
import com.google.devtools.build.lib.analysis.RuleContext.InvalidExecGroupException;
import com.google.devtools.build.lib.analysis.config.BuildConfiguration;
import com.google.devtools.build.lib.analysis.config.ConfigMatchingProvider;
-import com.google.devtools.build.lib.analysis.config.CoreOptions;
import com.google.devtools.build.lib.analysis.config.Fragment;
+import com.google.devtools.build.lib.analysis.config.RequiredFragmentsUtil;
import com.google.devtools.build.lib.analysis.configuredtargets.EnvironmentGroupConfiguredTarget;
import com.google.devtools.build.lib.analysis.configuredtargets.InputFileConfiguredTarget;
import com.google.devtools.build.lib.analysis.configuredtargets.OutputFileConfiguredTarget;
@@ -69,9 +68,7 @@
import com.google.devtools.build.lib.skyframe.AspectValueKey;
import com.google.devtools.build.lib.skyframe.ConfiguredTargetAndData;
import com.google.devtools.build.lib.skyframe.ConfiguredTargetKey;
-import com.google.devtools.build.lib.util.ClassName;
import com.google.devtools.build.lib.util.OrderedSetMultimap;
-import java.util.Collection;
import java.util.LinkedHashMap;
import java.util.LinkedHashSet;
import java.util.List;
@@ -79,7 +76,6 @@
import java.util.Objects;
import java.util.Optional;
import java.util.Set;
-import java.util.TreeSet;
import java.util.stream.Collectors;
import javax.annotation.Nullable;
@@ -265,140 +261,6 @@
}
/**
- * Returns a set of user-friendly strings identifying <i>almost</i> all of the pieces of config
- * state required by a rule or aspect.
- *
- * <p>The returned config state includes things that are known to be required at the time when the
- * rule's/aspect's dependencies have already been analyzed but before it's been analyzed itself.
- * See {@link RuleConfiguredTargetBuilder#maybeAddRequiredConfigFragmentsProvider} for the
- * remaining pieces of config state.
- *
- * <p>The strings can be names of {@link Fragment}s, names of {@link
- * com.google.devtools.build.lib.analysis.config.FragmentOptions}, and labels of user-defined
- * options such as Starlark flags and Android feature flags.
- *
- * <p>If {@code configuration} is {@link CoreOptions.IncludeConfigFragmentsEnum#DIRECT}, the
- * result includes only the config state considered to be directly required by this rule/aspect.
- * If it's {@link CoreOptions.IncludeConfigFragmentsEnum#TRANSITIVE}, it also includes config
- * state needed by transitive dependencies. If it's {@link
- * CoreOptions.IncludeConfigFragmentEnum#OFF}, this method just returns an empty set.
- *
- * <p>{@code select()}s and toolchain dependencies are considered when looking at what config
- * state is required.
- *
- * <p>TODO: This doesn't yet support fragments required by either native or Starlark transitions.
- *
- * @param buildSettingLabel this rule's label if this is a rule that's a build setting, else empty
- * @param configuration the configuration for this rule/aspect
- * @param universallyRequiredFragments fragments that are always required even if not explicitly
- * specified for this rule/aspect
- * @param configurationFragmentPolicy source of truth for the fragments required by this
- * rule/aspect class definition
- * @param configConditions {@link com.google.devtools.build.lib.analysis.config.FragmentOptions}
- * required by {@code select}s on this rule (empty if this is an aspect). This is a different
- * type than the others: options and fragments are different concepts. There's some subtlety
- * to their relationship (e.g. a {@link
- * com.google.devtools.build.lib.analysis.config.FragmentOptions} can be associated with
- * multiple {@link Fragment}s). Rather than trying to merge all results into a pure set of
- * {@link Fragment}s we just allow the mix. In practice the conceptual dependencies remain
- * clear enough without trying to resolve these subtleties.
- * @param prerequisites all prerequisites of this rule/aspect
- * @return An alphabetically ordered set of required fragments, options, and labels of
- * user-defined options.
- */
- private static ImmutableSortedSet<String> getRequiredConfigFragments(
- Optional<Label> buildSettingLabel,
- BuildConfiguration configuration,
- Collection<Class<? extends Fragment>> universallyRequiredFragments,
- ConfigurationFragmentPolicy configurationFragmentPolicy,
- Collection<ConfigMatchingProvider> configConditions,
- Iterable<ConfiguredTargetAndData> prerequisites) {
- CoreOptions coreOptions = configuration.getOptions().get(CoreOptions.class);
- if (coreOptions.includeRequiredConfigFragmentsProvider
- == CoreOptions.IncludeConfigFragmentsEnum.OFF) {
- return ImmutableSortedSet.of();
- }
-
- ImmutableSortedSet.Builder<String> requiredFragments = ImmutableSortedSet.naturalOrder();
-
- // Add directly required fragments:
-
- // Fragments explicitly required by this rule via the native rule/aspect definition API:
- configurationFragmentPolicy
- .getRequiredConfigurationFragments()
- .forEach(fragment -> requiredFragments.add(ClassName.getSimpleNameWithOuter(fragment)));
- // Fragments explicitly required by this rule via the Starlark rule/aspect definition API:
- configurationFragmentPolicy
- .getRequiredStarlarkFragments()
- .forEach(
- starlarkName -> {
- requiredFragments.add(
- ClassName.getSimpleNameWithOuter(
- configuration.getStarlarkFragmentByName(starlarkName)));
- });
- // Fragments universally required by everything:
- universallyRequiredFragments.forEach(
- fragment -> requiredFragments.add(ClassName.getSimpleNameWithOuter(fragment)));
- // Fragments required by config_conditions this rule select()s on (for aspects should be empty):
- configConditions.forEach(
- configCondition -> requiredFragments.addAll(configCondition.getRequiredFragmentOptions()));
- // We consider build settings (which are both rules and configuration) to require themselves:
- if (buildSettingLabel.isPresent()) {
- requiredFragments.add(buildSettingLabel.get().toString());
- }
-
- // Optionally add transitively required fragments:
- requiredFragments.addAll(getRequiredConfigFragmentsFromDeps(configuration, prerequisites));
- return requiredFragments.build();
- }
-
- /**
- * 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).
- BuildSettingProvider buildSettingProvider =
- prereq.getConfiguredTarget().getProvider(BuildSettingProvider.class);
- if (buildSettingProvider != null) {
- requiredFragments.add(buildSettingProvider.getLabel().toString());
- }
- if (coreOptions.includeRequiredConfigFragmentsProvider
- == CoreOptions.IncludeConfigFragmentsEnum.TRANSITIVE) {
- // Add fragments only required because the rule's transitive deps need them.
- RequiredConfigFragmentsProvider depProvider =
- prereq.getConfiguredTarget().getProvider(RequiredConfigFragmentsProvider.class);
- if (depProvider != null) {
- requiredFragments.addAll(depProvider.getRequiredConfigFragments());
- }
- }
- }
-
- return ImmutableSet.copyOf(requiredFragments);
- }
-
- /**
* Factory method: constructs a RuleConfiguredTarget of the appropriate class, based on the rule
* class. May return null if an error occurred.
*/
@@ -433,12 +295,12 @@
.setToolchainContexts(toolchainContexts)
.setConstraintSemantics(ruleClassProvider.getConstraintSemantics())
.setRequiredConfigFragments(
- getRequiredConfigFragments(
- rule.isBuildSetting() ? Optional.of(rule.getLabel()) : Optional.empty(),
+ RequiredFragmentsUtil.getRequiredFragments(
+ rule,
configuration,
ruleClassProvider.getUniversalFragments(),
configurationFragmentPolicy,
- configConditions.values(),
+ configConditions,
prerequisiteMap.values()))
.build();
@@ -634,12 +496,13 @@
.setToolchainContext(toolchainContext)
.setConstraintSemantics(ruleClassProvider.getConstraintSemantics())
.setRequiredConfigFragments(
- getRequiredConfigFragments(
- /*buildSettingLabel=*/ Optional.empty(),
+ RequiredFragmentsUtil.getRequiredFragments(
+ aspect,
+ associatedTarget.getTarget().getAssociatedRule(),
aspectConfiguration,
ruleClassProvider.getUniversalFragments(),
aspect.getDefinition().getConfigurationFragmentPolicy(),
- /*configConditions=*/ ImmutableList.of(),
+ configConditions,
prerequisiteMap.values()))
.build();
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/RequiredFragmentsUtil.java b/src/main/java/com/google/devtools/build/lib/analysis/config/RequiredFragmentsUtil.java
new file mode 100644
index 0000000..7ea8ca9
--- /dev/null
+++ b/src/main/java/com/google/devtools/build/lib/analysis/config/RequiredFragmentsUtil.java
@@ -0,0 +1,283 @@
+// Copyright 2020 The Bazel Authors. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.devtools.build.lib.analysis.config;
+
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.ImmutableSortedSet;
+import com.google.devtools.build.lib.analysis.BuildSettingProvider;
+import com.google.devtools.build.lib.analysis.RequiredConfigFragmentsProvider;
+import com.google.devtools.build.lib.analysis.RuleConfiguredTargetBuilder;
+import com.google.devtools.build.lib.analysis.config.transitions.ConfigurationTransition;
+import com.google.devtools.build.lib.analysis.config.transitions.TransitionFactory;
+import com.google.devtools.build.lib.cmdline.Label;
+import com.google.devtools.build.lib.packages.Aspect;
+import com.google.devtools.build.lib.packages.Attribute;
+import com.google.devtools.build.lib.packages.AttributeMap;
+import com.google.devtools.build.lib.packages.AttributeTransitionData;
+import com.google.devtools.build.lib.packages.ConfigurationFragmentPolicy;
+import com.google.devtools.build.lib.packages.ConfiguredAttributeMapper;
+import com.google.devtools.build.lib.packages.Rule;
+import com.google.devtools.build.lib.skyframe.ConfiguredTargetAndData;
+import com.google.devtools.build.lib.util.ClassName;
+import java.util.Collection;
+import java.util.Optional;
+import java.util.TreeSet;
+
+/**
+ * Utility methods for determining what {@link Fragment}s are required to analyze targets.
+ *
+ * <p>For example if a target reads <code>--copt</code> as part of its analysis logic, it requires
+ * the {@link com.google.devtools.build.lib.rules.cpp.CppConfiguration} fragment.
+ *
+ * <p>Used by {@link
+ * com.google.devtools.build.lib.query2.cquery.CqueryOptions#showRequiredConfigFragments}.
+ */
+public class RequiredFragmentsUtil {
+
+ /**
+ * Returns a set of user-friendly strings identifying all pieces of configuration a target
+ * requires.
+ *
+ * <p>The returned config state includes things that are known to be required at the time when the
+ * target's dependencies have already been analyzed but before it's been analyzed itself. See
+ * {@link RuleConfiguredTargetBuilder#maybeAddRequiredConfigFragmentsProvider} for the remaining
+ * pieces of config state.
+ *
+ * <p>The strings can be names of {@link Fragment}s, names of {@link FragmentOptions}, and labels
+ * of user-defined options such as Starlark flags and Android feature flags.
+ *
+ * <p>If {@code configuration} is {@link CoreOptions.IncludeConfigFragmentsEnum#DIRECT}, the
+ * result includes only the config state considered to be directly required by this target. If
+ * it's {@link CoreOptions.IncludeConfigFragmentsEnum#TRANSITIVE}, it also includes config state
+ * needed by transitive dependencies. If it's {@link CoreOptions.IncludeConfigFragmentsEnum#OFF},
+ * this method just returns an empty set.
+ *
+ * <p>{@code select()}s and toolchain dependencies are considered when looking at what config
+ * state is required.
+ *
+ * @param target the target
+ * @param configuration the configuration for this target
+ * @param universallyRequiredFragments fragments that are always required even if not explicitly
+ * specified for this target
+ * @param configurationFragmentPolicy source of truth for the fragments required by this target
+ * class definition
+ * @param configConditions <code>config_settings</code> required by this target's <code>select
+ * </code>s. Used for a) figuring out which options <code>select</code>s read and b) figuring
+ * out which transitions are attached to the target. {@link TransitionFactory}, which
+ * determines the transitions, may read the target's attributes.
+ * @param prerequisites all prerequisites of this aspect
+ * @return An alphabetically ordered set of required fragments, options, and labels of
+ * user-defined options.
+ */
+ public static ImmutableSortedSet<String> getRequiredFragments(
+ Rule target,
+ BuildConfiguration configuration,
+ Collection<Class<? extends Fragment>> universallyRequiredFragments,
+ ConfigurationFragmentPolicy configurationFragmentPolicy,
+ ImmutableMap<Label, ConfigMatchingProvider> configConditions,
+ Iterable<ConfiguredTargetAndData> prerequisites) {
+ return getRequiredFragments(
+ target.isBuildSetting() ? Optional.of(target.getLabel()) : Optional.empty(),
+ configuration,
+ universallyRequiredFragments,
+ configurationFragmentPolicy,
+ configConditions.values(),
+ getTransitions(target, configConditions),
+ prerequisites);
+ }
+
+ /**
+ * Variation of {@link #getRequiredFragments} for aspects.
+ *
+ * @param aspect the aspect
+ * @param associatedTarget the target this aspect is attached to
+ * @param configuration the configuration for this aspect
+ * @param universallyRequiredFragments fragments that are always required even if not explicitly
+ * specified for this aspect
+ * @param configurationFragmentPolicy source of truth for the fragments required by this aspect
+ * class definition
+ * @param configConditions <code>config_settings</code> required by <code>select</code>s on the
+ * associated target. Used for figuring out which transitions are attached to the target.
+ * @param prerequisites all prerequisites of this aspect
+ * @return An alphabetically ordered set of required fragments, options, and labels of
+ * user-defined options.
+ */
+ public static ImmutableSortedSet<String> getRequiredFragments(
+ Aspect aspect,
+ Rule associatedTarget,
+ BuildConfiguration configuration,
+ Collection<Class<? extends Fragment>> universallyRequiredFragments,
+ ConfigurationFragmentPolicy configurationFragmentPolicy,
+ ImmutableMap<Label, ConfigMatchingProvider> configConditions,
+ Iterable<ConfiguredTargetAndData> prerequisites) {
+ return getRequiredFragments(
+ /*buildSettingLabel=*/ Optional.empty(),
+ configuration,
+ universallyRequiredFragments,
+ configurationFragmentPolicy,
+ configConditions.values(),
+ getTransitions(aspect, ConfiguredAttributeMapper.of(associatedTarget, configConditions)),
+ prerequisites);
+ }
+
+ /** Internal implementation that handles any source (target or aspect). */
+ private static ImmutableSortedSet<String> getRequiredFragments(
+ Optional<Label> buildSettingLabel,
+ BuildConfiguration configuration,
+ Collection<Class<? extends Fragment>> universallyRequiredFragments,
+ ConfigurationFragmentPolicy configurationFragmentPolicy,
+ Collection<ConfigMatchingProvider> configConditions,
+ Collection<ConfigurationTransition> associatedTransitions,
+ Iterable<ConfiguredTargetAndData> prerequisites) {
+ CoreOptions coreOptions = configuration.getOptions().get(CoreOptions.class);
+ if (coreOptions.includeRequiredConfigFragmentsProvider
+ == CoreOptions.IncludeConfigFragmentsEnum.OFF) {
+ return ImmutableSortedSet.of();
+ }
+ ImmutableSortedSet.Builder<String> requiredFragments = ImmutableSortedSet.naturalOrder();
+ // Add directly required fragments:
+
+ // Fragments explicitly required by the native target/aspect definition API:
+ configurationFragmentPolicy
+ .getRequiredConfigurationFragments()
+ .forEach(fragment -> requiredFragments.add(ClassName.getSimpleNameWithOuter(fragment)));
+ // Fragments explicitly required by the Starlark target/aspect definition API:
+ configurationFragmentPolicy
+ .getRequiredStarlarkFragments()
+ .forEach(
+ starlarkName -> {
+ requiredFragments.add(
+ ClassName.getSimpleNameWithOuter(
+ configuration.getStarlarkFragmentByName(starlarkName)));
+ });
+ // Fragments universally required by everything:
+ universallyRequiredFragments.forEach(
+ fragment -> requiredFragments.add(ClassName.getSimpleNameWithOuter(fragment)));
+ // Fragments required by attached select()s.
+ configConditions.forEach(
+ configCondition -> requiredFragments.addAll(configCondition.getRequiredFragmentOptions()));
+ // We consider build settings (which are both targets and configuration) to require themselves:
+ if (buildSettingLabel.isPresent()) {
+ requiredFragments.add(buildSettingLabel.get().toString());
+ }
+ // Fragments required by attached configuration transitions.
+ for (ConfigurationTransition transition : associatedTransitions) {
+ requiredFragments.addAll(transition.requiresOptionFragments(configuration.getOptions()));
+ }
+
+ // Optionally add transitively required fragments (only if --show_config_fragments=transitive):
+ requiredFragments.addAll(getRequiredFragmentsFromDeps(configuration, prerequisites));
+ return requiredFragments.build();
+ }
+
+ /**
+ * Returns the {@link ConfigurationTransition}s "attached" to a target.
+ *
+ * <p>"Attached" means the transition is attached to the target itself or one of its attributes.
+ *
+ * <p>These are the transitions required for a target to successfully analyze. Technically,
+ * transitions attached to the target are evaluated during its parent's analysis, which is where
+ * the configuration for the child is determined. We still consider these the child's requirements
+ * because the child's properties determine that dependency.
+ */
+ private static ImmutableList<ConfigurationTransition> getTransitions(
+ Rule target, ImmutableMap<Label, ConfigMatchingProvider> configConditions) {
+ ImmutableList.Builder<ConfigurationTransition> transitions = ImmutableList.builder();
+ if (target.getRuleClassObject().getTransitionFactory() != null) {
+ transitions.add(target.getRuleClassObject().getTransitionFactory().create(target));
+ }
+ // We don't set the execution platform in this data because a) that doesn't affect which
+ // fragments are required and b) it's one less parameter we have to pass to
+ // RequiredFragmenstUtil's public interface.
+ AttributeTransitionData attributeTransitionData =
+ AttributeTransitionData.builder()
+ .attributes(ConfiguredAttributeMapper.of(target, configConditions))
+ .build();
+ for (Attribute attribute : target.getRuleClassObject().getAttributes()) {
+ if (attribute.getTransitionFactory() != null) {
+ transitions.add(attribute.getTransitionFactory().create(attributeTransitionData));
+ }
+ }
+ return transitions.build();
+ }
+
+ /**
+ * Returns the {@link ConfigurationTransition}s "attached" to an aspect.
+ *
+ * <p>"Attached" means the transition is attached to one of the aspect's attributes. Transitions
+ * can'be attached directly to aspects themselves.
+ */
+ private static ImmutableList<ConfigurationTransition> getTransitions(
+ Aspect aspect, AttributeMap attributeMap) {
+ ImmutableList.Builder<ConfigurationTransition> transitions = ImmutableList.builder();
+ AttributeTransitionData attributeTransitionData =
+ AttributeTransitionData.builder().attributes(attributeMap).build();
+ for (Attribute attribute : aspect.getDefinition().getAttributes().values()) {
+ if (attribute.getTransitionFactory() != null) {
+ transitions.add(attribute.getTransitionFactory().create(attributeTransitionData));
+ }
+ }
+ return transitions.build();
+ }
+
+ /**
+ * 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> getRequiredFragmentsFromDeps(
+ 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 target depends on a Starlark build setting, conceptually that means it directly
+ // requires that as an option (even though it's technically a dependency).
+ BuildSettingProvider buildSettingProvider =
+ prereq.getConfiguredTarget().getProvider(BuildSettingProvider.class);
+ if (buildSettingProvider != null) {
+ requiredFragments.add(buildSettingProvider.getLabel().toString());
+ }
+ if (coreOptions.includeRequiredConfigFragmentsProvider
+ == CoreOptions.IncludeConfigFragmentsEnum.TRANSITIVE) {
+ // Add fragments only required because transitive deps need them.
+ RequiredConfigFragmentsProvider depProvider =
+ prereq.getConfiguredTarget().getProvider(RequiredConfigFragmentsProvider.class);
+ if (depProvider != null) {
+ requiredFragments.addAll(depProvider.getRequiredConfigFragments());
+ }
+ }
+ }
+
+ return ImmutableSet.copyOf(requiredFragments);
+ }
+
+ private RequiredFragmentsUtil() {}
+}
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/transitions/ComposingTransition.java b/src/main/java/com/google/devtools/build/lib/analysis/config/transitions/ComposingTransition.java
index 33ed134..b0f9996 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/config/transitions/ComposingTransition.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/config/transitions/ComposingTransition.java
@@ -19,7 +19,6 @@
import com.google.common.collect.ImmutableSet;
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.events.EventHandler;
import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec;
import java.util.Map;
@@ -52,10 +51,16 @@
}
@Override
- public ImmutableSet<Class<? extends FragmentOptions>> requiresOptionFragments() {
- return ImmutableSet.<Class<? extends FragmentOptions>>builder()
- .addAll(transition1.requiresOptionFragments())
- .addAll(transition2.requiresOptionFragments())
+ public ImmutableSet<String> requiresOptionFragments(BuildOptions options) {
+ // At first glance this code looks wrong. A composing transition applies transition2 over
+ // transition1's outputs, not the original options. We don't have to worry about that here
+ // because the reason we pass the options is so Starlark transitions can map individual flags
+ // like "//command_line_option:copts" to the fragments that own them. This doesn't depend on the
+ // flags' values. This is fortunate, because it producers simpler, faster code and cleaner
+ // interfaces.
+ return ImmutableSet.<String>builder()
+ .addAll(transition1.requiresOptionFragments(options))
+ .addAll(transition2.requiresOptionFragments(options))
.build();
}
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/transitions/ConfigurationTransition.java b/src/main/java/com/google/devtools/build/lib/analysis/config/transitions/ConfigurationTransition.java
index 4b62ec5..227839d 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/config/transitions/ConfigurationTransition.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/config/transitions/ConfigurationTransition.java
@@ -14,11 +14,14 @@
package com.google.devtools.build.lib.analysis.config.transitions;
+import static com.google.common.collect.ImmutableSet.toImmutableSet;
+
import com.google.common.collect.ImmutableSet;
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.events.EventHandler;
+import com.google.devtools.build.lib.util.ClassName;
import java.util.Map;
/**
@@ -45,13 +48,18 @@
* {@link #requiresOptionFragments()} variation for Starlark transitions, which need a {@link
* BuildOptions} instance to map required options to their {@link FragmentOptions}.
*
+ * <p>This version pre-translates the fragments to their final string representations. This is
+ * because Starlark transitions can depend on both native fragments and Starlark flags. The latter
+ * are reported directly, not as part of a larger fragment.
+ *
* <p>Non-Starlark transitions should override {@link #requiresOptionFragments()} and ignore this.
*
- * <p>Callers may also ignore this if they know they're not calling into a Starlark transition.
+ * <p>Callers may ignore this method if they know they're not calling into a Starlark transition.
*/
- default ImmutableSet<Class<? extends FragmentOptions>> requiresOptionFragments(
- BuildOptions options) {
- return requiresOptionFragments();
+ default ImmutableSet<String> requiresOptionFragments(BuildOptions options) {
+ return requiresOptionFragments().stream()
+ .map(ClassName::getSimpleNameWithOuter)
+ .collect(toImmutableSet());
}
/**
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/transitions/TransitionUtil.java b/src/main/java/com/google/devtools/build/lib/analysis/config/transitions/TransitionUtil.java
index 059e53c..60a20ea 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/config/transitions/TransitionUtil.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/config/transitions/TransitionUtil.java
@@ -26,6 +26,6 @@
*/
public static BuildOptionsView restrict(
ConfigurationTransition transition, BuildOptions options) {
- return new BuildOptionsView(options, transition.requiresOptionFragments(options));
+ return new BuildOptionsView(options, transition.requiresOptionFragments());
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkTransition.java b/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkTransition.java
index 9a8d47c..8011566 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkTransition.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkTransition.java
@@ -23,7 +23,6 @@
import com.google.common.collect.Maps;
import com.google.common.collect.Sets;
import com.google.devtools.build.lib.analysis.config.BuildOptions;
-import com.google.devtools.build.lib.analysis.config.FragmentOptions;
import com.google.devtools.build.lib.analysis.config.StarlarkDefinedConfigTransition;
import com.google.devtools.build.lib.analysis.config.transitions.ConfigurationTransition;
import com.google.devtools.build.lib.analysis.starlark.FunctionTransitionUtil.OptionInfo;
@@ -35,6 +34,7 @@
import com.google.devtools.build.lib.packages.Target;
import com.google.devtools.build.lib.packages.Type.ConversionException;
import com.google.devtools.build.lib.skyframe.PackageValue;
+import com.google.devtools.build.lib.util.ClassName;
import com.google.devtools.build.skyframe.SkyFunction;
import com.google.devtools.build.skyframe.SkyKey;
import com.google.devtools.build.skyframe.SkyValue;
@@ -81,22 +81,22 @@
}
@Override
- public ImmutableSet<Class<? extends FragmentOptions>> requiresOptionFragments(
- BuildOptions buildOptions) {
+ public ImmutableSet<String> requiresOptionFragments(BuildOptions buildOptions) {
// TODO(bazel-team): complexity cleanup: merge buildOptionInfo with TransitiveOptionDetails.
Map<String, OptionInfo> optionToFragment = FunctionTransitionUtil.buildOptionInfo(buildOptions);
- ImmutableSet.Builder<Class<? extends FragmentOptions>> fragments = ImmutableSet.builder();
+ ImmutableSet.Builder<String> fragments = ImmutableSet.builder();
for (String optionStarlarkName : Iterables.concat(getInputs(), getOutputs())) {
- // TODO(bazel-team): support Starlark flags.
if (!optionStarlarkName.startsWith(COMMAND_LINE_OPTION_PREFIX)) {
- continue;
- }
- String optionNativeName = optionStarlarkName.substring(COMMAND_LINE_OPTION_PREFIX.length());
- OptionInfo optionInfo = optionToFragment.get(optionNativeName);
- // A null optionInfo means the flag is invalid. Starlark transitions independently catch and
- // report that (search the code for "do not correspond to valid settings").
- if (optionInfo != null) {
- fragments.add(optionInfo.getOptionClass());
+ // Starlark flags don't belong to any fragment.
+ fragments.add(optionStarlarkName);
+ } else {
+ String optionNativeName = optionStarlarkName.substring(COMMAND_LINE_OPTION_PREFIX.length());
+ OptionInfo optionInfo = optionToFragment.get(optionNativeName);
+ // A null optionInfo means the flag is invalid. Starlark transitions independently catch and
+ // report that (search the code for "do not correspond to valid settings").
+ if (optionInfo != null) {
+ fragments.add(ClassName.getSimpleNameWithOuter(optionInfo.getOptionClass()));
+ }
}
}
return fragments.build();
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/RequiredConfigFragmentsTest.java b/src/test/java/com/google/devtools/build/lib/analysis/RequiredConfigFragmentsTest.java
index ef0997b..0996e08 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/RequiredConfigFragmentsTest.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/RequiredConfigFragmentsTest.java
@@ -141,9 +141,7 @@
getConfiguredTarget("//a:simple")
.getProvider(RequiredConfigFragmentsProvider.class)
.getRequiredConfigFragments();
- assertThat(requiredFragments)
- .containsExactlyElementsIn(genRuleFragments("--define:myvar"))
- .inOrder();
+ assertThat(requiredFragments).contains("--define:myvar");
}
/**
@@ -256,4 +254,132 @@
assertThat(requiredFragments).contains("JavaConfiguration");
assertThat(requiredFragments).contains("--define:myvar");
}
+
+ private void writeStarlarkTransitionsAndAllowList() throws Exception {
+ scratch.file(
+ "tools/allowlists/function_transition_allowlist/BUILD",
+ "package_group(",
+ " name = 'function_transition_allowlist',",
+ " packages = [",
+ " '//a/...',",
+ " ],",
+ ")");
+ scratch.file(
+ "transitions/defs.bzl",
+ "def _java_write_transition_impl(settings, attr):",
+ " return {'//command_line_option:javacopt': ['foo'] }",
+ "java_write_transition = transition(",
+ " implementation = _java_write_transition_impl,",
+ " inputs = [],",
+ " outputs = ['//command_line_option:javacopt'],",
+ ")",
+ "def _cpp_read_transition_impl(settings, attr):",
+ " return {}",
+ "cpp_read_transition = transition(",
+ " implementation = _cpp_read_transition_impl,",
+ " inputs = ['//command_line_option:copt'],",
+ " outputs = [],",
+ ")");
+ scratch.file("transitions/BUILD");
+ }
+
+ @Test
+ public void starlarkRuleTransitionReadsFragment() throws Exception {
+ writeStarlarkTransitionsAndAllowList();
+ scratch.file(
+ "a/defs.bzl",
+ "load('//transitions:defs.bzl', 'cpp_read_transition')",
+ "def _impl(ctx):",
+ " pass",
+ "has_cpp_aware_rule_transition = rule(",
+ " implementation = _impl,",
+ " cfg = cpp_read_transition,",
+ " attrs = {",
+ " '_allowlist_function_transition': attr.label(",
+ " default = '//tools/allowlists/function_transition_allowlist',",
+ " ),",
+ " })");
+ scratch.file(
+ "a/BUILD",
+ "load('//a:defs.bzl', 'has_cpp_aware_rule_transition')",
+ "has_cpp_aware_rule_transition(name = 'cctarget')");
+ useConfiguration("--include_config_fragments_provider=direct");
+ ImmutableSet<String> requiredFragments =
+ getConfiguredTarget("//a:cctarget")
+ .getProvider(RequiredConfigFragmentsProvider.class)
+ .getRequiredConfigFragments();
+ assertThat(requiredFragments).contains("CppOptions");
+ assertThat(requiredFragments).doesNotContain("JavaOptions");
+ }
+
+ @Test
+ public void starlarkRuleTransitionWritesFragment() throws Exception {
+ writeStarlarkTransitionsAndAllowList();
+ scratch.file(
+ "a/defs.bzl",
+ "load('//transitions:defs.bzl', 'java_write_transition')",
+ "def _impl(ctx):",
+ " pass",
+ "has_java_aware_rule_transition = rule(",
+ " implementation = _impl,",
+ " cfg = java_write_transition,",
+ " attrs = {",
+ " '_allowlist_function_transition': attr.label(",
+ " default = '//tools/allowlists/function_transition_allowlist',",
+ " ),",
+ " })");
+ scratch.file(
+ "a/BUILD",
+ "load('//a:defs.bzl', 'has_java_aware_rule_transition')",
+ "has_java_aware_rule_transition(name = 'javatarget')");
+ useConfiguration("--include_config_fragments_provider=direct");
+ ImmutableSet<String> requiredFragments =
+ getConfiguredTarget("//a:javatarget")
+ .getProvider(RequiredConfigFragmentsProvider.class)
+ .getRequiredConfigFragments();
+ assertThat(requiredFragments).contains("JavaOptions");
+ assertThat(requiredFragments).doesNotContain("CppOptions");
+ }
+
+ @Test
+ public void starlarkAttrTransition() throws Exception {
+ writeStarlarkTransitionsAndAllowList();
+ scratch.file(
+ "a/defs.bzl",
+ "load('//transitions:defs.bzl', 'cpp_read_transition', 'java_write_transition')",
+ "def _impl(ctx):",
+ " pass",
+ "has_java_aware_attr_transition = rule(",
+ " implementation = _impl,",
+ " attrs = {",
+ " 'deps': attr.label_list(cfg = java_write_transition),",
+ " '_allowlist_function_transition': attr.label(",
+ " default = '//tools/allowlists/function_transition_allowlist',",
+ " ),",
+ " })",
+ "has_cpp_aware_rule_transition = rule(",
+ " implementation = _impl,",
+ " cfg = cpp_read_transition,",
+ " attrs = {",
+ " '_allowlist_function_transition': attr.label(",
+ " default = '//tools/allowlists/function_transition_allowlist',",
+ " ),",
+ " })");
+ scratch.file(
+ "a/BUILD",
+ "load('//a:defs.bzl', 'has_cpp_aware_rule_transition', 'has_java_aware_attr_transition')",
+ "has_cpp_aware_rule_transition(name = 'ccchild')",
+ "has_java_aware_attr_transition(",
+ " name = 'javaparent',",
+ " deps = [':ccchild'])");
+ useConfiguration("--include_config_fragments_provider=direct");
+ ImmutableSet<String> requiredFragments =
+ getConfiguredTarget("//a:javaparent")
+ .getProvider(RequiredConfigFragmentsProvider.class)
+ .getRequiredConfigFragments();
+ // We consider the attribute transition over the parent -> child edge a property of the parent.
+ assertThat(requiredFragments).contains("JavaOptions");
+ // But not the child's rule transition.
+ assertThat(requiredFragments).doesNotContain("CppOptions");
+ }
}
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/StarlarkAttrTransitionProviderTest.java b/src/test/java/com/google/devtools/build/lib/analysis/StarlarkAttrTransitionProviderTest.java
index aa66e0b..9e35fde 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/StarlarkAttrTransitionProviderTest.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/StarlarkAttrTransitionProviderTest.java
@@ -37,7 +37,6 @@
import com.google.devtools.build.lib.packages.StarlarkProvider;
import com.google.devtools.build.lib.packages.StructImpl;
import com.google.devtools.build.lib.packages.util.BazelMockAndroidSupport;
-import com.google.devtools.build.lib.rules.cpp.CppOptions;
import com.google.devtools.build.lib.skyframe.ConfiguredTargetAndData;
import com.google.devtools.build.lib.syntax.Starlark;
import com.google.devtools.build.lib.testutil.TestRuleClassProvider;
@@ -1839,7 +1838,7 @@
.getTransitionFactory()
.create(AttributeTransitionData.builder().attributes(attributes).build());
assertThat(attrTransition.requiresOptionFragments(ct.getConfiguration().getOptions()))
- .containsExactly(CppOptions.class);
+ .containsExactly("CppOptions");
}
@Test
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/StarlarkRuleTransitionProviderTest.java b/src/test/java/com/google/devtools/build/lib/analysis/StarlarkRuleTransitionProviderTest.java
index a051d8f..4a73eb4 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/StarlarkRuleTransitionProviderTest.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/StarlarkRuleTransitionProviderTest.java
@@ -1406,6 +1406,6 @@
ConfigurationTransition ruleTransition =
testTarget.getRuleClassObject().getTransitionFactory().create(testTarget);
assertThat(ruleTransition.requiresOptionFragments(ct.getConfiguration().getOptions()))
- .containsExactly(CppOptions.class);
+ .containsExactly("CppOptions");
}
}
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/config/transitions/ComposingTransitionTest.java b/src/test/java/com/google/devtools/build/lib/analysis/config/transitions/ComposingTransitionTest.java
index dd6cd04..bc7756d 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/config/transitions/ComposingTransitionTest.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/config/transitions/ComposingTransitionTest.java
@@ -234,7 +234,7 @@
ComposingTransition.of(
new TransitionWithCustomFragments(ImmutableSet.of(CppOptions.class)),
new TransitionWithCustomFragments(ImmutableSet.of(JavaOptions.class)));
- assertThat(composed.requiresOptionFragments())
- .containsExactly(CppOptions.class, JavaOptions.class);
+ assertThat(composed.requiresOptionFragments(BuildOptions.builder().build()))
+ .containsExactly("CppOptions", "JavaOptions");
}
}
