Skip to content

Commit

Permalink
Refactor "features" handling code
Browse files Browse the repository at this point in the history
- Added a FeatureSet type to represent a set of "on" features and a set of "off" features
- This type supports merging different feature sets with different granularity (so package-level defaults get overridden by rule-level features, for example)
  - This'll allow repo-level defaults to be overridden by package-level defaults in the future
- Global (flag-level) features are handled specially (with `mergeWithGlobalFeatures`)
- This gets rid of the frankly ridiculously complex logic in RuleContext; no behavior change is introduced.

Work towards bazelbuild#18077

PiperOrigin-RevId: 524832614
Change-Id: Ic37b80771f1627573485e564d39944e68c6a6524
  • Loading branch information
Wyverald authored and fweikert committed May 25, 2023
1 parent 6356c50 commit 89351cc
Show file tree
Hide file tree
Showing 14 changed files with 251 additions and 66 deletions.
11 changes: 11 additions & 0 deletions src/main/java/com/google/devtools/build/lib/analysis/BUILD
Expand Up @@ -288,6 +288,7 @@ java_library(
":config/config_matching_provider",
":config/core_options",
":config/execution_transition_factory",
":config/feature_set",
":config/fragment",
":config/fragment_class_set",
":config/fragment_options",
Expand Down Expand Up @@ -1620,6 +1621,7 @@ java_library(
":config/build_options",
":config/compilation_mode",
":config/core_options",
":config/feature_set",
":config/fragment",
":config/fragment_class_set",
":config/fragment_factory",
Expand Down Expand Up @@ -1788,6 +1790,15 @@ java_library(
],
)

java_library(
name = "config/feature_set",
srcs = ["config/FeatureSet.java"],
deps = [
"//third_party:auto_value",
"//third_party:guava",
],
)

java_library(
name = "config/fragment",
srcs = ["config/Fragment.java"],
Expand Down
Expand Up @@ -29,13 +29,11 @@
import com.google.common.collect.ImmutableListMultimap;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.ImmutableSortedSet;
import com.google.common.collect.Iterables;
import com.google.common.collect.ListMultimap;
import com.google.common.collect.Lists;
import com.google.common.collect.Maps;
import com.google.common.collect.Multimaps;
import com.google.common.collect.Sets;
import com.google.common.collect.Streams;
import com.google.devtools.build.lib.actions.ActionAnalysisMetadata;
import com.google.devtools.build.lib.actions.ActionLookupKey;
Expand All @@ -51,6 +49,7 @@
import com.google.devtools.build.lib.analysis.config.BuildConfigurationValue;
import com.google.devtools.build.lib.analysis.config.ConfigConditions;
import com.google.devtools.build.lib.analysis.config.ConfigMatchingProvider;
import com.google.devtools.build.lib.analysis.config.FeatureSet;
import com.google.devtools.build.lib.analysis.config.Fragment;
import com.google.devtools.build.lib.analysis.platform.ConstraintValueInfo;
import com.google.devtools.build.lib.analysis.platform.PlatformInfo;
Expand Down Expand Up @@ -99,7 +98,6 @@
import java.util.ArrayList;
import java.util.Collection;
import java.util.HashMap;
import java.util.HashSet;
import java.util.LinkedHashSet;
import java.util.List;
import java.util.Map;
Expand Down Expand Up @@ -160,8 +158,7 @@ void validate(
private final ListMultimap<String, ConfiguredTargetAndData> targetMap;
private final ImmutableMap<Label, ConfigMatchingProvider> configConditions;
private final AspectAwareAttributeMapper attributes;
private final ImmutableSet<String> enabledFeatures;
private final ImmutableSet<String> disabledFeatures;
private final FeatureSet features;
private final String ruleClassNameForLogging;
private final ConfigurationFragmentPolicy configurationFragmentPolicy;
private final ConfiguredRuleClassProvider ruleClassProvider;
Expand Down Expand Up @@ -215,11 +212,7 @@ private RuleContext(
this.targetMap = targetMap;
this.configConditions = builder.configConditions.asProviders();
this.attributes = new AspectAwareAttributeMapper(attributes, builder.aspectAttributes);
Set<String> allEnabledFeatures = new HashSet<>();
Set<String> allDisabledFeatures = new HashSet<>();
getAllFeatures(allEnabledFeatures, allDisabledFeatures);
this.enabledFeatures = ImmutableSortedSet.copyOf(allEnabledFeatures);
this.disabledFeatures = ImmutableSortedSet.copyOf(allDisabledFeatures);
this.features = computeFeatures();
this.ruleClassNameForLogging = builder.getRuleClassNameForLogging();
this.actionOwnerSymbolGenerator = new SymbolGenerator<>(builder.actionOwnerSymbol);
this.reporter = builder.reporter;
Expand All @@ -231,43 +224,14 @@ private RuleContext(
this.starlarkThread = createStarlarkThread(builder.mutability); // uses above state
}

private void getAllFeatures(Set<String> allEnabledFeatures, Set<String> allDisabledFeatures) {
Set<String> globallyEnabled = new HashSet<>();
Set<String> globallyDisabled = new HashSet<>();
parseFeatures(getConfiguration().getDefaultFeatures(), globallyEnabled, globallyDisabled);
Set<String> packageEnabled = new HashSet<>();
Set<String> packageDisabled = new HashSet<>();
parseFeatures(rule.getPackage().getFeatures(), packageEnabled, packageDisabled);
Set<String> ruleEnabled = new HashSet<>();
Set<String> ruleDisabled = new HashSet<>();
if (attributes().has("features", Type.STRING_LIST)) {
parseFeatures(attributes().get("features", Type.STRING_LIST), ruleEnabled, ruleDisabled);
}

Set<String> ruleDisabledFeatures =
Sets.union(ruleDisabled, Sets.difference(packageDisabled, ruleEnabled));
allDisabledFeatures.addAll(Sets.union(ruleDisabledFeatures, globallyDisabled));

Set<String> packageFeatures =
Sets.difference(Sets.union(globallyEnabled, packageEnabled), packageDisabled);
Set<String> ruleFeatures =
Sets.difference(Sets.union(packageFeatures, ruleEnabled), ruleDisabled);
allEnabledFeatures.addAll(Sets.difference(ruleFeatures, globallyDisabled));
}

private static void parseFeatures(
Iterable<String> features, Set<String> enabled, Set<String> disabled) {
for (String feature : features) {
if (feature.startsWith("-")) {
disabled.add(feature.substring(1));
} else if (feature.equals("no_layering_check")) {
// TODO(bazel-team): Remove once we do not have BUILD files left that contain
// 'no_layering_check'.
disabled.add("layering_check");
} else {
enabled.add(feature);
}
}
private FeatureSet computeFeatures() {
FeatureSet pkg = rule.getPackage().getFeatures();
FeatureSet rule =
attributes().has("features", Type.STRING_LIST)
? FeatureSet.parse(attributes().get("features", Type.STRING_LIST))
: FeatureSet.EMPTY;
return FeatureSet.mergeWithGlobalFeatures(
FeatureSet.merge(pkg, rule), getConfiguration().getDefaultFeatures());
}

public boolean isAllowTagsPropagation() {
Expand Down Expand Up @@ -1660,14 +1624,14 @@ public static boolean isVisible(Rule rule, TransitiveInfoCollection prerequisite
* @return the set of features applicable for the current rule.
*/
public ImmutableSet<String> getFeatures() {
return enabledFeatures;
return features.on();
}

/**
* @return the set of features that are disabled for the current rule.
*/
public ImmutableSet<String> getDisabledFeatures() {
return disabledFeatures;
return features.off();
}

@Override
Expand Down
Expand Up @@ -130,6 +130,8 @@ public interface GlobalStateProvider {

private final boolean siblingRepositoryLayout;

private final FeatureSet defaultFeatures;

/**
* Validates the options for this BuildConfigurationValue. Issues warnings for the use of
* deprecated options, and warnings or errors for any option settings that conflict.
Expand Down Expand Up @@ -261,6 +263,7 @@ private static ImmutableSortedMap<Class<? extends Fragment>, Fragment> getConfig
this.reservedActionMnemonics = reservedActionMnemonics;
this.buildEventSupplier = Suppliers.memoize(this::createBuildEvent);
this.commandLineLimits = new CommandLineLimits(options.minParamFileSize);
this.defaultFeatures = FeatureSet.parse(options.defaultFeatures);
}

@Override
Expand Down Expand Up @@ -856,9 +859,9 @@ public void modifyExecutionInfo(Map<String, String> executionInfo, String mnemon
options.executionInfoModifier.apply(mnemonic, executionInfo);
}

/** @return the list of default features used for all packages. */
public List<String> getDefaultFeatures() {
return options.defaultFeatures;
/** Returns the list of default features used for all packages. */
public FeatureSet getDefaultFeatures() {
return defaultFeatures;
}

/**
Expand Down
@@ -0,0 +1,107 @@
// Copyright 2023 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 static com.google.common.collect.ImmutableList.toImmutableList;

import com.google.auto.value.AutoValue;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.ImmutableSortedSet;
import com.google.common.collect.Maps;
import com.google.common.collect.Streams;
import java.util.HashMap;
import java.util.Map;
import java.util.Set;

/**
* Represents a set of "on" features and a set of "off" features. The two sets are guaranteed not to
* intersect.
*/
@AutoValue
public abstract class FeatureSet {
public static final FeatureSet EMPTY = of(ImmutableSet.of(), ImmutableSet.of());

public abstract ImmutableSet<String> on();

public abstract ImmutableSet<String> off();

private static FeatureSet of(Set<String> on, Set<String> off) {
return new AutoValue_FeatureSet(ImmutableSortedSet.copyOf(on), ImmutableSortedSet.copyOf(off));
}

/** Parses a {@link FeatureSet} instance from a list of strings. */
public static FeatureSet parse(Iterable<String> features) {
Map<String, Boolean> featureToState = new HashMap<>();
for (String feature : features) {
if (feature.startsWith("-")) {
featureToState.put(feature.substring(1), false);
} else if (feature.equals("no_layering_check")) {
// TODO(bazel-team): Remove once we do not have BUILD files left that contain
// 'no_layering_check'.
featureToState.put("layering_check", false);
} else {
// -X always trumps X.
featureToState.putIfAbsent(feature, true);
}
}
return fromMap(featureToState);
}

private static FeatureSet fromMap(Map<String, Boolean> featureToState) {
return of(
Maps.filterValues(featureToState, Boolean.TRUE::equals).keySet(),
Maps.filterValues(featureToState, Boolean.FALSE::equals).keySet());
}

private static void mergeSetIntoMap(
Set<String> features, boolean state, Map<String, Boolean> featureToState) {
for (String feature : features) {
featureToState.put(feature, state);
}
}

/**
* Merges two {@link FeatureSet}s into one, with {@code coarse} being the coarser-grained set
* (e.g. the package default feature set), and {@code fine} being the finer-grained set (e.g. the
* rule-level feature set). Note that this operation is not commutative.
*/
public static FeatureSet merge(FeatureSet coarse, FeatureSet fine) {
Map<String, Boolean> featureToState = new HashMap<>();
mergeSetIntoMap(coarse.on(), true, featureToState);
mergeSetIntoMap(coarse.off(), false, featureToState);
mergeSetIntoMap(fine.on(), true, featureToState);
mergeSetIntoMap(fine.off(), false, featureToState);
return fromMap(featureToState);
}

/**
* Merges a {@link FeatureSet} with the global feature set. This differs from {@link #merge} in
* that the globally disabled features are <strong>always</strong> disabled.
*/
public static FeatureSet mergeWithGlobalFeatures(FeatureSet base, FeatureSet global) {
Map<String, Boolean> featureToState = new HashMap<>();
mergeSetIntoMap(global.on(), true, featureToState);
mergeSetIntoMap(base.on(), true, featureToState);
mergeSetIntoMap(base.off(), false, featureToState);
mergeSetIntoMap(global.off(), false, featureToState);
return fromMap(featureToState);
}

public final ImmutableList<String> toStringList() {
return Streams.concat(on().stream(), off().stream().map(s -> "-" + s))
.collect(toImmutableList());
}
}
1 change: 1 addition & 0 deletions src/main/java/com/google/devtools/build/lib/packages/BUILD
Expand Up @@ -59,6 +59,7 @@ java_library(
"//src/main/java/com/google/devtools/build/docgen/annot",
"//src/main/java/com/google/devtools/build/lib/actions:execution_requirements",
"//src/main/java/com/google/devtools/build/lib/actions:thread_state_receiver",
"//src/main/java/com/google/devtools/build/lib/analysis:config/feature_set",
"//src/main/java/com/google/devtools/build/lib/analysis:config/fragment",
"//src/main/java/com/google/devtools/build/lib/analysis:config/fragment_class_set",
"//src/main/java/com/google/devtools/build/lib/analysis:config/toolchain_type_requirement",
Expand Down
Expand Up @@ -29,6 +29,7 @@
import com.google.common.collect.Iterables;
import com.google.common.collect.Lists;
import com.google.common.collect.Maps;
import com.google.devtools.build.lib.analysis.config.FeatureSet;
import com.google.devtools.build.lib.bugreport.BugReport;
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.cmdline.LabelConstants;
Expand Down Expand Up @@ -269,7 +270,7 @@ public enum ConfigSettingVisibilityPolicy {
private Set<Label> defaultCompatibleWith = ImmutableSet.of();
private Set<Label> defaultRestrictedTo = ImmutableSet.of();

private ImmutableSet<String> features;
private FeatureSet features;

private ImmutableList<TargetPattern> registeredExecutionPlatforms;
private ImmutableList<TargetPattern> registeredToolchains;
Expand Down Expand Up @@ -483,7 +484,7 @@ private void finishInit(Builder builder) {
this.defaultLicense = builder.defaultLicense;
this.defaultDistributionSet = builder.defaultDistributionSet;
this.defaultPackageMetadata = ImmutableSortedSet.copyOf(builder.defaultPackageMetadata);
this.features = ImmutableSortedSet.copyOf(builder.features);
this.features = builder.features;
this.registeredExecutionPlatforms = ImmutableList.copyOf(builder.registeredExecutionPlatforms);
this.registeredToolchains = ImmutableList.copyOf(builder.registeredToolchains);
this.repositoryMapping = Preconditions.checkNotNull(builder.repositoryMapping);
Expand Down Expand Up @@ -669,10 +670,8 @@ public String getWorkspaceName() {
return workspaceName;
}

/**
* Returns the features specified in the <code>package()</code> declaration.
*/
public ImmutableSet<String> getFeatures() {
/** Returns the features specified in the <code>package()</code> declaration. */
public FeatureSet getFeatures() {
return features;
}

Expand Down Expand Up @@ -1005,7 +1004,7 @@ public boolean recordLoadedModules() {
private RuleVisibility defaultVisibility = RuleVisibility.PRIVATE;
private ConfigSettingVisibilityPolicy configSettingVisibilityPolicy;
private boolean defaultVisibilitySet;
private final List<String> features = new ArrayList<>();
private FeatureSet features = FeatureSet.EMPTY;
private final List<Event> events = Lists.newArrayList();
private final List<Postable> posts = Lists.newArrayList();
@Nullable private String ioExceptionMessage = null;
Expand Down Expand Up @@ -1358,7 +1357,7 @@ public Builder setDefaultHdrsCheck(String hdrsCheck) {

@CanIgnoreReturnValue
public Builder addFeatures(Iterable<String> features) {
Iterables.addAll(this.features, features);
this.features = FeatureSet.merge(this.features, FeatureSet.parse(features));
return this;
}

Expand Down
Expand Up @@ -238,7 +238,7 @@ public Build.Target toTargetProtoBuffer(Target target, Object extraDataForAttrHa
.distinct()
.forEach(output -> rulePb.addRuleOutput(output.getLabel().toString()));
}
for (String feature : rule.getPackage().getFeatures()) {
for (String feature : rule.getPackage().getFeatures().toStringList()) {
rulePb.addDefaultSetting(feature);
}

Expand Down Expand Up @@ -297,7 +297,7 @@ public Build.Target toTargetProtoBuffer(Target target, Object extraDataForAttrHa
input.addSubinclude(starlarkLoadLabel.toString());
}

for (String feature : inputFile.getPackage().getFeatures()) {
for (String feature : inputFile.getPackage().getFeatures().toStringList()) {
input.addFeature(feature);
}

Expand Down
Expand Up @@ -192,7 +192,7 @@ private Element createTargetElement(Document doc, Target target)
outputElem.setAttribute("name", outputFile.getLabel().toString());
elem.appendChild(outputElem);
}
for (String feature : rule.getPackage().getFeatures()) {
for (String feature : rule.getPackage().getFeatures().toStringList()) {
Element outputElem = doc.createElement("rule-default-setting");
outputElem.setAttribute("name", feature);
elem.appendChild(outputElem);
Expand Down Expand Up @@ -277,7 +277,7 @@ private static void addPackageGroupsToElement(Document doc, Element parent, Targ
}

private static void addFeaturesToElement(Document doc, Element parent, InputFile inputFile) {
for (String feature : inputFile.getPackage().getFeatures()) {
for (String feature : inputFile.getPackage().getFeatures().toStringList()) {
Element elem = doc.createElement("feature");
elem.setAttribute("name", feature);
parent.appendChild(elem);
Expand Down

0 comments on commit 89351cc

Please sign in to comment.