Skip to content

Commit

Permalink
Refactor handling of Fragment and FragmentOptions classes out of …
Browse files Browse the repository at this point in the history 
…`ConfiguredRuleClassProvider` and into `FragmentRegistry`.

This is a predecessor to using `BitSet` to efficiently represent requirements in `RequiredConfigFragmentsProvider`. `FragmentRegistry` will be expanded to assign canonical indices to fragments, and it was cleaner to do this in a dedicated class since `ConfiguredRuleClassProvider` is already large.

PiperOrigin-RevId: 401086357
  • Loading branch information
@justinhorvitz @copybara-github
justinhorvitz authored and copybara-github committed Oct 5, 2021
1 parent 670af30 commit 681886d
Show file tree
Hide file tree
Showing 29 changed files with 331 additions and 268 deletions.
14 changes: 14 additions & 0 deletions src/main/java/com/google/devtools/build/lib/analysis/BUILD
View file
Original file line number Diff line number Diff line change
Expand Up @@ -292,6 +292,7 @@ java_library(
":config/fragment",
":config/fragment_class_set",
":config/fragment_options",
":config/fragment_registry",
":config/host_transition",
":config/invalid_configuration_exception",
":config/per_label_options",
Expand Down Expand Up @@ -1573,6 +1574,7 @@ java_library(
":config/fragment_options",
"//src/main/java/com/google/devtools/build/lib/cmdline",
"//src/main/java/com/google/devtools/build/lib/skyframe/serialization/autocodec",
"//src/main/java/com/google/devtools/build/lib/skyframe/serialization/autocodec:serialization-constant",
"//src/main/java/com/google/devtools/build/lib/util",
"//src/main/java/com/google/devtools/common/options",
"//third_party:guava",
Expand Down Expand Up @@ -1715,6 +1717,18 @@ java_library(
],
)

java_library(
name = "config/fragment_registry",
srcs = ["config/FragmentRegistry.java"],
deps = [
":config/build_options",
":config/fragment",
":config/fragment_class_set",
":config/fragment_options",
"//third_party:guava",
],
)

java_library(
name = "config/fragment_options",
srcs = [
Expand Down
110 changes: 12 additions & 98 deletions src/main/java/com/google/devtools/build/lib/analysis/ConfiguredRuleClassProvider.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
import com.google.devtools.build.lib.analysis.config.Fragment;
import com.google.devtools.build.lib.analysis.config.FragmentClassSet;
import com.google.devtools.build.lib.analysis.config.FragmentOptions;
import com.google.devtools.build.lib.analysis.config.FragmentRegistry;
import com.google.devtools.build.lib.analysis.config.SymlinkDefinition;
import com.google.devtools.build.lib.analysis.config.transitions.ComposingTransitionFactory;
import com.google.devtools.build.lib.analysis.config.transitions.PatchTransition;
Expand All @@ -59,19 +60,14 @@
import com.google.devtools.build.lib.vfs.PathFragment;
import com.google.devtools.build.lib.vfs.Root;
import com.google.devtools.build.lib.vfs.inmemoryfs.InMemoryFileSystem;
import com.google.devtools.common.options.Option;
import com.google.devtools.common.options.OptionDefinition;
import com.google.devtools.common.options.OptionsProvider;
import java.io.IOException;
import java.io.InputStream;
import java.io.OutputStream;
import java.lang.reflect.Constructor;
import java.lang.reflect.Field;
import java.lang.reflect.InvocationTargetException;
import java.util.ArrayList;
import java.util.HashMap;
import java.util.LinkedHashMap;
import java.util.LinkedHashSet;
import java.util.List;
import java.util.Map;
import java.util.Optional;
Expand Down Expand Up @@ -142,8 +138,7 @@ public static final class Builder implements RuleDefinitionEnvironment {
@Nullable private String builtinsBzlPackagePathInSource;
private final List<Class<? extends Fragment>> configurationFragmentClasses = new ArrayList<>();
private final List<BuildInfoFactory> buildInfoFactories = new ArrayList<>();
private final Set<Class<? extends FragmentOptions>> configurationOptions =
new LinkedHashSet<>();
private final List<Class<? extends FragmentOptions>> configurationOptions = new ArrayList<>();

private final Map<String, RuleClass> ruleClassMap = new HashMap<>();
private final Map<String, RuleDefinition> ruleDefinitionMap = new HashMap<>();
Expand Down Expand Up @@ -286,7 +281,6 @@ public Builder addNativeAspectClass(NativeAspectClass aspectFactoryClass) {
* no two different configuration fragments can share the same name.
*/
public Builder addConfigurationFragment(Class<? extends Fragment> fragmentClass) {
this.configurationOptions.addAll(Fragment.requiredOptions(fragmentClass));
configurationFragmentClasses.add(fragmentClass);
return this;
}
Expand Down Expand Up @@ -554,12 +548,11 @@ public ConfiguredRuleClassProvider build() {
ImmutableMap.copyOf(ruleClassMap),
ImmutableMap.copyOf(ruleDefinitionMap),
ImmutableMap.copyOf(nativeAspectClassMap),
FragmentRegistry.create(
configurationFragmentClasses, universalFragments, configurationOptions),
defaultWorkspaceFilePrefix.toString(),
defaultWorkspaceFileSuffix.toString(),
ImmutableList.copyOf(buildInfoFactories),
ImmutableList.copyOf(configurationOptions),
FragmentClassSet.of(configurationFragmentClasses),
FragmentClassSet.of(universalFragments),
trimmingTransitionFactory,
toolchainTaggedTrimmingTransition,
shouldInvalidateCacheForOptionDiff,
Expand Down Expand Up @@ -632,18 +625,7 @@ public Builder setNetworkAllowlistForTests(Label allowlist) {
/** Maps aspect name to the aspect factory meta class. */
private final ImmutableMap<String, NativeAspectClass> nativeAspectClassMap;

/** The configuration options that affect the behavior of the rules. */
private final ImmutableList<Class<? extends FragmentOptions>> configurationOptions;

/** The set of configuration fragment factories. */
private final FragmentClassSet configurationFragmentClasses;

/**
* Maps build option names to matching config fragments. This is used to determine correct
* fragment requirements for config_setting rules, which are unique in that their dependencies are
* triggered by string representations of option names.
*/
private final Map<String, Class<? extends Fragment>> optionsToFragmentMap;
private final FragmentRegistry fragmentRegistry;

/** The transition factory used to produce the transition that will trim targets. */
@Nullable private final TransitionFactory<RuleTransitionData> trimmingTransitionFactory;
Expand All @@ -654,12 +636,6 @@ public Builder setNetworkAllowlistForTests(Label allowlist) {
/** The predicate used to determine whether a diff requires the cache to be invalidated. */
private final OptionsDiffPredicate shouldInvalidateCacheForOptionDiff;

/**
* Configuration fragments that should be available to all rules even when they don't explicitly
* require it.
*/
private final FragmentClassSet universalFragments;

private final ImmutableList<BuildInfoFactory> buildInfoFactories;

private final PrerequisiteValidator prerequisiteValidator;
Expand Down Expand Up @@ -694,12 +670,10 @@ private ConfiguredRuleClassProvider(
ImmutableMap<String, RuleClass> ruleClassMap,
ImmutableMap<String, RuleDefinition> ruleDefinitionMap,
ImmutableMap<String, NativeAspectClass> nativeAspectClassMap,
FragmentRegistry fragmentRegistry,
String defaultWorkspaceFilePrefix,
String defaultWorkspaceFileSuffix,
ImmutableList<BuildInfoFactory> buildInfoFactories,
ImmutableList<Class<? extends FragmentOptions>> configurationOptions,
FragmentClassSet configurationFragmentClasses,
FragmentClassSet universalFragments,
@Nullable TransitionFactory<RuleTransitionData> trimmingTransitionFactory,
PatchTransition toolchainTaggedTrimmingTransition,
OptionsDiffPredicate shouldInvalidateCacheForOptionDiff,
Expand All @@ -721,13 +695,10 @@ private ConfiguredRuleClassProvider(
this.ruleClassMap = ruleClassMap;
this.ruleDefinitionMap = ruleDefinitionMap;
this.nativeAspectClassMap = nativeAspectClassMap;
this.fragmentRegistry = fragmentRegistry;
this.defaultWorkspaceFilePrefix = defaultWorkspaceFilePrefix;
this.defaultWorkspaceFileSuffix = defaultWorkspaceFileSuffix;
this.buildInfoFactories = buildInfoFactories;
this.configurationOptions = configurationOptions;
this.configurationFragmentClasses = configurationFragmentClasses;
this.optionsToFragmentMap = computeOptionsToFragmentMap(configurationFragmentClasses);
this.universalFragments = universalFragments;
this.trimmingTransitionFactory = trimmingTransitionFactory;
this.toolchainTaggedTrimmingTransition = toolchainTaggedTrimmingTransition;
this.shouldInvalidateCacheForOptionDiff = shouldInvalidateCacheForOptionDiff;
Expand All @@ -739,43 +710,12 @@ private ConfiguredRuleClassProvider(
this.symlinkDefinitions = symlinkDefinitions;
this.reservedActionMnemonics = reservedActionMnemonics;
this.actionEnvironmentProvider = actionEnvironmentProvider;
this.configurationFragmentMap = createFragmentMap(configurationFragmentClasses);
this.configurationFragmentMap = createFragmentMap(fragmentRegistry.getAllFragments());
this.constraintSemantics = constraintSemantics;
this.thirdPartyLicenseExistencePolicy = thirdPartyLicenseExistencePolicy;
this.networkAllowlistForTests = networkAllowlistForTests;
}

/**
* Computes the option name --> config fragments map. Note that this mapping is technically
* one-to-many: a single option may be required by multiple fragments (e.g. Java options are used
* by both JavaConfiguration and Jvm). In such cases, we arbitrarily choose one fragment since
* that's all that's needed to satisfy the config_setting.
*/
private static Map<String, Class<? extends Fragment>> computeOptionsToFragmentMap(
FragmentClassSet configurationFragments) {
Map<String, Class<? extends Fragment>> result = new LinkedHashMap<>();
Map<Class<? extends FragmentOptions>, Integer> visitedOptionsClasses = new HashMap<>();
for (Class<? extends Fragment> fragment : configurationFragments) {
Set<Class<? extends FragmentOptions>> requiredOpts = Fragment.requiredOptions(fragment);
for (Class<? extends FragmentOptions> optionsClass : requiredOpts) {
Integer previousBest = visitedOptionsClasses.get(optionsClass);
if (previousBest != null && previousBest <= requiredOpts.size()) {
// Multiple config fragments may require the same options class, but we only need one of
// them to guarantee that class makes it into the configuration. Pick one that depends
// on as few options classes as possible (not necessarily unique).
continue;
}
visitedOptionsClasses.put(optionsClass, requiredOpts.size());
for (Field field : optionsClass.getFields()) {
if (field.isAnnotationPresent(Option.class)) {
result.put(field.getAnnotation(Option.class).name(), fragment);
}
}
}
}
return result;
}

public PrerequisiteValidator getPrerequisiteValidator() {
return prerequisiteValidator;
}
Expand Down Expand Up @@ -822,6 +762,10 @@ public NativeAspectClass getNativeAspectClass(String key) {
return nativeAspectClassMap.get(key);
}

public FragmentRegistry getFragmentRegistry() {
return fragmentRegistry;
}

public Map<BuildInfoKey, BuildInfoFactory> getBuildInfoFactoriesAsMap() {
ImmutableMap.Builder<BuildInfoKey, BuildInfoFactory> factoryMapBuilder = ImmutableMap.builder();
for (BuildInfoFactory factory : buildInfoFactories) {
Expand All @@ -830,16 +774,6 @@ public Map<BuildInfoKey, BuildInfoFactory> getBuildInfoFactoriesAsMap() {
return factoryMapBuilder.build();
}

/** Returns the set of configuration fragments provided by this module. */
public FragmentClassSet getConfigurationFragments() {
return configurationFragmentClasses;
}

@Nullable
public Class<? extends Fragment> getConfigurationFragmentForOption(String requiredOption) {
return optionsToFragmentMap.get(requiredOption);
}

/**
* Returns the transition factory used to produce the transition to trim targets.
*
Expand Down Expand Up @@ -868,31 +802,11 @@ public boolean shouldInvalidateCacheForOptionDiff(
return shouldInvalidateCacheForOptionDiff.apply(newOptions, changedOption, oldValue, newValue);
}

/** Returns the set of configuration options that are supported in this module. */
public ImmutableList<Class<? extends FragmentOptions>> getConfigurationOptions() {
return configurationOptions;
}

/** Returns the definition of the rule class definition with the specified name. */
public RuleDefinition getRuleClassDefinition(String ruleClassName) {
return ruleDefinitionMap.get(ruleClassName);
}

/**
* Returns the configuration fragments that should be available to all rules even when not
* explicitly required.
*
* <p>This is a subset of {@link #getConfigurationFragments}.
*/
public FragmentClassSet getUniversalFragments() {
return universalFragments;
}

/** Creates a BuildOptions class for the given options taken from an optionsProvider. */
public BuildOptions createBuildOptions(OptionsProvider optionsProvider) {
return BuildOptions.of(configurationOptions, optionsProvider);
}

private static ImmutableMap<String, Object> createNativeRuleSpecificBindings(
ImmutableMap<String, Object> starlarkAccessibleTopLevels,
ImmutableList<Bootstrap> bootstraps) {
Expand Down
8 changes: 4 additions & 4 deletions src/main/java/com/google/devtools/build/lib/analysis/ConfiguredTargetFactory.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -303,15 +303,15 @@ private ConfiguredTarget createRule(
.setVisibility(convertVisibility(prerequisiteMap, env.getEventHandler(), rule))
.setPrerequisites(transformPrerequisiteMap(prerequisiteMap))
.setConfigConditions(configConditions)
.setUniversalFragments(ruleClassProvider.getUniversalFragments())
.setUniversalFragments(ruleClassProvider.getFragmentRegistry().getUniversalFragments())
.setToolchainContexts(toolchainContexts)
.setExecGroupCollectionBuilder(execGroupCollectionBuilder)
.setConstraintSemantics(ruleClassProvider.getConstraintSemantics())
.setRequiredConfigFragments(
RequiredFragmentsUtil.getRuleRequiredFragmentsIfEnabled(
rule,
configuration,
ruleClassProvider.getUniversalFragments(),
ruleClassProvider.getFragmentRegistry().getUniversalFragments(),
configConditions.asProviders(),
prerequisiteMap.values()))
.build();
Expand Down Expand Up @@ -521,7 +521,7 @@ public ConfiguredAspect createAspect(
.setPrerequisites(transformPrerequisiteMap(prerequisiteMap))
.setAspectAttributes(aspectAttributes)
.setConfigConditions(configConditions)
.setUniversalFragments(ruleClassProvider.getUniversalFragments())
.setUniversalFragments(ruleClassProvider.getFragmentRegistry().getUniversalFragments())
.setToolchainContext(toolchainContext)
// TODO(b/161222568): Implement the exec_properties attr for aspects and read its value
// here.
Expand All @@ -534,7 +534,7 @@ public ConfiguredAspect createAspect(
aspectFactory,
associatedTarget.getTarget().getAssociatedRule(),
aspectConfiguration,
ruleClassProvider.getUniversalFragments(),
ruleClassProvider.getFragmentRegistry().getUniversalFragments(),
configConditions.asProviders(),
prerequisiteMap.values()))
.build();
Expand Down
36 changes: 17 additions & 19 deletions src/main/java/com/google/devtools/build/lib/analysis/config/BuildOptions.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,20 +22,19 @@
import com.google.common.base.Strings;
import com.google.common.collect.ArrayListMultimap;
import com.google.common.collect.ImmutableCollection;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.ImmutableSortedMap;
import com.google.common.collect.Lists;
import com.google.common.collect.MapDifference;
import com.google.common.collect.Maps;
import com.google.common.collect.Multimap;
import com.google.common.collect.Ordering;
import com.google.common.collect.Sets;
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.skyframe.serialization.DeserializationContext;
import com.google.devtools.build.lib.skyframe.serialization.ObjectCodec;
import com.google.devtools.build.lib.skyframe.serialization.SerializationContext;
import com.google.devtools.build.lib.skyframe.serialization.autocodec.SerializationConstant;
import com.google.devtools.build.lib.util.Fingerprint;
import com.google.devtools.build.lib.util.OrderedSetMultimap;
import com.google.devtools.common.options.OptionDefinition;
Expand Down Expand Up @@ -67,9 +66,10 @@
// TODO(janakr): If overhead of FragmentOptions class names is too high, add constructor that just
// takes fragments and gets names from them.
public final class BuildOptions implements Cloneable, Serializable {
private static final Comparator<Class<? extends FragmentOptions>>
lexicalFragmentOptionsComparator = Comparator.comparing(Class::getName);
private static final Comparator<Label> starlarkOptionsComparator = Ordering.natural();

@SerializationConstant
static final Comparator<Class<? extends FragmentOptions>> LEXICAL_FRAGMENT_OPTIONS_COMPARATOR =
Comparator.comparing(Class::getName);

public static Map<Label, Object> labelizeStarlarkOptions(Map<String, Object> starlarkOptions) {
return starlarkOptions.entrySet().stream()
Expand All @@ -78,7 +78,7 @@ public static Map<Label, Object> labelizeStarlarkOptions(Map<String, Object> sta
}

public static BuildOptions getDefaultBuildOptionsForFragments(
List<Class<? extends FragmentOptions>> fragmentClasses) {
Iterable<Class<? extends FragmentOptions>> fragmentClasses) {
try {
return BuildOptions.of(fragmentClasses);
} catch (OptionsParsingException e) {
Expand Down Expand Up @@ -142,11 +142,11 @@ public static BuildOptions of(
* BuildOptions class that only has native options.
*/
@VisibleForTesting
public static BuildOptions of(List<Class<? extends FragmentOptions>> optionsList, String... args)
public static BuildOptions of(
Iterable<Class<? extends FragmentOptions>> optionsList, String... args)
throws OptionsParsingException {
Builder builder = builder();
OptionsParser parser =
OptionsParser.builder().optionsClasses(ImmutableList.copyOf(optionsList)).build();
OptionsParser parser = OptionsParser.builder().optionsClasses(optionsList).build();
parser.parse(args);
for (Class<? extends FragmentOptions> optionsClass : optionsList) {
builder.addFragmentOptions(parser.getOptions(optionsClass));
Expand Down Expand Up @@ -454,17 +454,15 @@ public Builder removeStarlarkOption(Label key) {

public BuildOptions build() {
return new BuildOptions(
ImmutableSortedMap.copyOf(fragmentOptions, lexicalFragmentOptionsComparator),
ImmutableSortedMap.copyOf(starlarkOptions, starlarkOptionsComparator));
ImmutableSortedMap.copyOf(fragmentOptions, LEXICAL_FRAGMENT_OPTIONS_COMPARATOR),
ImmutableSortedMap.copyOf(starlarkOptions));
}

private final Map<Class<? extends FragmentOptions>, FragmentOptions> fragmentOptions;
private final Map<Label, Object> starlarkOptions;
private final Map<Class<? extends FragmentOptions>, FragmentOptions> fragmentOptions =
new HashMap<>();
private final Map<Label, Object> starlarkOptions = new HashMap<>();

private Builder() {
fragmentOptions = new HashMap<>();
starlarkOptions = new HashMap<>();
}
private Builder() {}
}

/** Returns the difference between two BuildOptions in a new {@link BuildOptions.OptionsDiff}. */
Expand Down Expand Up @@ -530,8 +528,8 @@ public static OptionsDiff diff(OptionsDiff diff, BuildOptions first, BuildOption
}

// Compare Starlark options for the two classes.
Map<Label, Object> starlarkFirst = first.getStarlarkOptions();
Map<Label, Object> starlarkSecond = second.getStarlarkOptions();
Map<Label, Object> starlarkFirst = first.starlarkOptionsMap;
Map<Label, Object> starlarkSecond = second.starlarkOptionsMap;
for (Label buildSetting : Sets.union(starlarkFirst.keySet(), starlarkSecond.keySet())) {
if (starlarkFirst.get(buildSetting) == null) {
diff.addExtraSecondStarlarkOption(buildSetting, starlarkSecond.get(buildSetting));
Expand Down
Loading

0 comments on commit 681886d

Please sign in to comment.