Skip to content

Commit

Permalink
cquery --show_config_fragments: report fragments used by transitions
Browse files Browse the repository at this point in the history
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 #11258.

PiperOrigin-RevId: 319306418
  • Loading branch information
gregestren authored and copybara-github committed Jul 1, 2020
1 parent 17bedb8 commit 24e09f7
Show file tree
Hide file tree
Showing 11 changed files with 462 additions and 176 deletions.
2 changes: 2 additions & 0 deletions src/main/java/com/google/devtools/build/lib/analysis/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -233,6 +233,7 @@ java_library(
"config/ConvenienceSymlinks.java",
"config/DependencyEvaluationException.java",
"config/FragmentCollection.java",
"config/RequiredFragmentsUtil.java",
"config/TransitionResolver.java",
"configuredtargets/AbstractConfiguredTarget.java",
"configuredtargets/EnvironmentGroupConfiguredTarget.java",
Expand Down Expand Up @@ -1793,6 +1794,7 @@ java_library(
":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",
],
)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -69,17 +68,14 @@
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;
import java.util.Map;
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;

Expand Down Expand Up @@ -264,140 +260,6 @@ public final ConfiguredTarget createConfiguredTarget(
}
}

/**
* 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.
Expand Down Expand Up @@ -433,12 +295,12 @@ private ConfiguredTarget createRule(
.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();

Expand Down Expand Up @@ -634,12 +496,13 @@ public ConfiguredAspect createAspect(
.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();

Expand Down
Loading

0 comments on commit 24e09f7

Please sign in to comment.