From 592327aedaaf1941573556c328610b775d6399bb Mon Sep 17 00:00:00 2001 From: Googler Date: Mon, 17 Apr 2023 06:42:32 -0700 Subject: [PATCH] Refactor "features" handling code - 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 https://github.com/bazelbuild/bazel/issues/18077 PiperOrigin-RevId: 524832614 Change-Id: Ic37b80771f1627573485e564d39944e68c6a6524 --- .../google/devtools/build/lib/analysis/BUILD | 11 ++ .../build/lib/analysis/RuleContext.java | 62 +++------- .../config/BuildConfigurationValue.java | 9 +- .../build/lib/analysis/config/FeatureSet.java | 107 ++++++++++++++++++ .../google/devtools/build/lib/packages/BUILD | 1 + .../devtools/build/lib/packages/Package.java | 15 ++- .../query/output/ProtoOutputFormatter.java | 4 +- .../query/output/XmlOutputFormatter.java | 4 +- .../google/devtools/build/lib/analysis/BUILD | 11 ++ .../lib/analysis/config/FeatureSetTest.java | 84 ++++++++++++++ .../google/devtools/build/lib/packages/BUILD | 1 + .../lib/packages/PackageFactoryTest.java | 3 +- .../google/devtools/build/lib/pkgcache/BUILD | 1 + .../lib/pkgcache/PackageLoadingTest.java | 4 +- 14 files changed, 251 insertions(+), 66 deletions(-) create mode 100644 src/main/java/com/google/devtools/build/lib/analysis/config/FeatureSet.java create mode 100644 src/test/java/com/google/devtools/build/lib/analysis/config/FeatureSetTest.java 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 62d50eea9d0a2d..4a9f909d6dbd28 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/BUILD +++ b/src/main/java/com/google/devtools/build/lib/analysis/BUILD @@ -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", @@ -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", @@ -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"], diff --git a/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java b/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java index 146f7a9402fd34..5d504d8223e992 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java @@ -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; @@ -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; @@ -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; @@ -160,8 +158,7 @@ void validate( private final ListMultimap targetMap; private final ImmutableMap configConditions; private final AspectAwareAttributeMapper attributes; - private final ImmutableSet enabledFeatures; - private final ImmutableSet disabledFeatures; + private final FeatureSet features; private final String ruleClassNameForLogging; private final ConfigurationFragmentPolicy configurationFragmentPolicy; private final ConfiguredRuleClassProvider ruleClassProvider; @@ -215,11 +212,7 @@ private RuleContext( this.targetMap = targetMap; this.configConditions = builder.configConditions.asProviders(); this.attributes = new AspectAwareAttributeMapper(attributes, builder.aspectAttributes); - Set allEnabledFeatures = new HashSet<>(); - Set 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; @@ -231,43 +224,14 @@ private RuleContext( this.starlarkThread = createStarlarkThread(builder.mutability); // uses above state } - private void getAllFeatures(Set allEnabledFeatures, Set allDisabledFeatures) { - Set globallyEnabled = new HashSet<>(); - Set globallyDisabled = new HashSet<>(); - parseFeatures(getConfiguration().getDefaultFeatures(), globallyEnabled, globallyDisabled); - Set packageEnabled = new HashSet<>(); - Set packageDisabled = new HashSet<>(); - parseFeatures(rule.getPackage().getFeatures(), packageEnabled, packageDisabled); - Set ruleEnabled = new HashSet<>(); - Set ruleDisabled = new HashSet<>(); - if (attributes().has("features", Type.STRING_LIST)) { - parseFeatures(attributes().get("features", Type.STRING_LIST), ruleEnabled, ruleDisabled); - } - - Set ruleDisabledFeatures = - Sets.union(ruleDisabled, Sets.difference(packageDisabled, ruleEnabled)); - allDisabledFeatures.addAll(Sets.union(ruleDisabledFeatures, globallyDisabled)); - - Set packageFeatures = - Sets.difference(Sets.union(globallyEnabled, packageEnabled), packageDisabled); - Set ruleFeatures = - Sets.difference(Sets.union(packageFeatures, ruleEnabled), ruleDisabled); - allEnabledFeatures.addAll(Sets.difference(ruleFeatures, globallyDisabled)); - } - - private static void parseFeatures( - Iterable features, Set enabled, Set 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() { @@ -1660,14 +1624,14 @@ public static boolean isVisible(Rule rule, TransitiveInfoCollection prerequisite * @return the set of features applicable for the current rule. */ public ImmutableSet getFeatures() { - return enabledFeatures; + return features.on(); } /** * @return the set of features that are disabled for the current rule. */ public ImmutableSet getDisabledFeatures() { - return disabledFeatures; + return features.off(); } @Override diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfigurationValue.java b/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfigurationValue.java index 5276e0e46fdd79..eb974050ec788f 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfigurationValue.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfigurationValue.java @@ -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. @@ -261,6 +263,7 @@ private static ImmutableSortedMap, Fragment> getConfig this.reservedActionMnemonics = reservedActionMnemonics; this.buildEventSupplier = Suppliers.memoize(this::createBuildEvent); this.commandLineLimits = new CommandLineLimits(options.minParamFileSize); + this.defaultFeatures = FeatureSet.parse(options.defaultFeatures); } @Override @@ -856,9 +859,9 @@ public void modifyExecutionInfo(Map executionInfo, String mnemon options.executionInfoModifier.apply(mnemonic, executionInfo); } - /** @return the list of default features used for all packages. */ - public List getDefaultFeatures() { - return options.defaultFeatures; + /** Returns the list of default features used for all packages. */ + public FeatureSet getDefaultFeatures() { + return defaultFeatures; } /** diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/FeatureSet.java b/src/main/java/com/google/devtools/build/lib/analysis/config/FeatureSet.java new file mode 100644 index 00000000000000..b237595e0044ce --- /dev/null +++ b/src/main/java/com/google/devtools/build/lib/analysis/config/FeatureSet.java @@ -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 on(); + + public abstract ImmutableSet off(); + + private static FeatureSet of(Set on, Set 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 features) { + Map 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 featureToState) { + return of( + Maps.filterValues(featureToState, Boolean.TRUE::equals).keySet(), + Maps.filterValues(featureToState, Boolean.FALSE::equals).keySet()); + } + + private static void mergeSetIntoMap( + Set features, boolean state, Map 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 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 always disabled. + */ + public static FeatureSet mergeWithGlobalFeatures(FeatureSet base, FeatureSet global) { + Map 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 toStringList() { + return Streams.concat(on().stream(), off().stream().map(s -> "-" + s)) + .collect(toImmutableList()); + } +} diff --git a/src/main/java/com/google/devtools/build/lib/packages/BUILD b/src/main/java/com/google/devtools/build/lib/packages/BUILD index 28afbd3c7c8ee8..5356ed84c451ac 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/BUILD +++ b/src/main/java/com/google/devtools/build/lib/packages/BUILD @@ -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", diff --git a/src/main/java/com/google/devtools/build/lib/packages/Package.java b/src/main/java/com/google/devtools/build/lib/packages/Package.java index 79a61864ba655e..da52d2df555268 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/Package.java +++ b/src/main/java/com/google/devtools/build/lib/packages/Package.java @@ -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; @@ -269,7 +270,7 @@ public enum ConfigSettingVisibilityPolicy { private Set