diff options
Diffstat (limited to 'src/main')
5 files changed, 104 insertions, 115 deletions
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/ConfigurationCollectionFactory.java b/src/main/java/com/google/devtools/build/lib/analysis/ConfigurationCollectionFactory.java index 3e3b35f7b3..77c51d9e44 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/ConfigurationCollectionFactory.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/ConfigurationCollectionFactory.java @@ -39,8 +39,6 @@ public interface ConfigurationCollectionFactory { * @param loadedPackageProvider the package provider * @param buildOptions top-level build options representing the command-line * @param errorEventListener the event listener for errors - * @param performSanityCheck flag to signal about performing sanity check. Can be false only for - * tests in skyframe. Legacy mode uses loadedPackageProvider == null condition for this. * @return the top-level configuration * @throws InvalidConfigurationException */ @@ -50,8 +48,7 @@ public interface ConfigurationCollectionFactory { Cache<String, BuildConfiguration> cache, PackageProviderForConfigurations loadedPackageProvider, BuildOptions buildOptions, - EventHandler errorEventListener, - boolean performSanityCheck) throws InvalidConfigurationException; + EventHandler errorEventListener) throws InvalidConfigurationException; /** * Returns the module the given configuration should use for choosing dynamic transitions. diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfiguration.java b/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfiguration.java index 96a79209d8..9935a305b9 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfiguration.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfiguration.java @@ -148,9 +148,26 @@ public final class BuildConfiguration { * The resulting set only contains labels that were derived from command-line options; the * intention is that it can be used to sanity-check that the command-line options actually * contain these in their transitive closure. + * + * <p>This functionality only exists for legacy configuration fragments that compute labels from + * command-line option values. Don't do that! Instead, use a rule that specifies the mapping + * explicitly. */ @SuppressWarnings("unused") - public void addImplicitLabels(Multimap<String, Label> implicitLabels) { + protected void addImplicitLabels(Multimap<String, Label> implicitLabels) { + } + + /** + * Returns a multimap of all labels that should be implicitly loaded from labels that were + * specified as options, keyed by the name to be displayed to the user if something goes wrong. + * The returned set only contains labels that were derived from command-line options; the + * intention is that it can be used to sanity-check that the command-line options actually + * contain these in their transitive closure. + */ + public final ListMultimap<String, Label> getImplicitLabels() { + ListMultimap<String, Label> implicitLabels = ArrayListMultimap.create(); + addImplicitLabels(implicitLabels); + return implicitLabels; } /** @@ -1849,21 +1866,6 @@ public final class BuildConfiguration { } /** - * Returns a multimap of all labels that should be implicitly loaded from labels that were - * specified as options, keyed by the name to be displayed to the user if something goes wrong. - * The returned set only contains labels that were derived from command-line options; the - * intention is that it can be used to sanity-check that the command-line options actually contain - * these in their transitive closure. - */ - public ListMultimap<String, Label> getImplicitLabels() { - ListMultimap<String, Label> implicitLabels = ArrayListMultimap.create(); - for (Fragment fragment : fragments.values()) { - fragment.addImplicitLabels(implicitLabels); - } - return implicitLabels; - } - - /** * For an given environment, returns a subset containing all * variables in the given list if they are defined in the given * environment. diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/ConfigurationFactory.java b/src/main/java/com/google/devtools/build/lib/analysis/config/ConfigurationFactory.java index cd8e10cc93..31eb150cab 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/config/ConfigurationFactory.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/config/ConfigurationFactory.java @@ -14,7 +14,6 @@ package com.google.devtools.build.lib.analysis.config; -import com.google.common.annotations.VisibleForTesting; import com.google.common.cache.Cache; import com.google.common.collect.ImmutableList; import com.google.devtools.build.lib.analysis.BlazeDirectories; @@ -47,7 +46,6 @@ import javax.annotation.Nullable; public final class ConfigurationFactory { private final List<ConfigurationFragmentFactory> configurationFragmentFactories; private final ConfigurationCollectionFactory configurationCollectionFactory; - private boolean performSanityCheck = true; public ConfigurationFactory( ConfigurationCollectionFactory configurationCollectionFactory, @@ -63,12 +61,8 @@ public final class ConfigurationFactory { this.configurationFragmentFactories = ImmutableList.copyOf(fragmentFactories); } - @VisibleForTesting - public void setSanityCheck(boolean performSanityCheck) { - this.performSanityCheck = performSanityCheck; - } - - /** Creates a set of build configurations with top-level configuration having the given options. + /** + * Creates a set of build configurations with top-level configuration having the given options. * * <p>The rest of the configurations are created based on the set of transitions available. */ @@ -79,7 +73,7 @@ public final class ConfigurationFactory { EventHandler errorEventListener) throws InvalidConfigurationException { return configurationCollectionFactory.createConfigurations(this, cache, - loadedPackageProvider, buildOptions, errorEventListener, performSanityCheck); + loadedPackageProvider, buildOptions, errorEventListener); } /** diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelConfigurationCollection.java b/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelConfigurationCollection.java index 4452f3da2d..3564564dfa 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelConfigurationCollection.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelConfigurationCollection.java @@ -31,20 +31,11 @@ import com.google.devtools.build.lib.analysis.config.ConfigurationFactory; import com.google.devtools.build.lib.analysis.config.InvalidConfigurationException; import com.google.devtools.build.lib.analysis.config.PackageProviderForConfigurations; import com.google.devtools.build.lib.bazel.rules.cpp.BazelCppRuleClasses.CppTransition; -import com.google.devtools.build.lib.cmdline.Label; -import com.google.devtools.build.lib.events.Event; import com.google.devtools.build.lib.events.EventHandler; -import com.google.devtools.build.lib.packages.AggregatingAttributeMapper; import com.google.devtools.build.lib.packages.Attribute.ConfigurationTransition; import com.google.devtools.build.lib.packages.Attribute.SplitTransition; import com.google.devtools.build.lib.packages.Attribute.Transition; -import com.google.devtools.build.lib.packages.BuildType; -import com.google.devtools.build.lib.packages.NoSuchThingException; -import com.google.devtools.build.lib.packages.Rule; -import com.google.devtools.build.lib.packages.Target; -import java.util.Collection; -import java.util.HashSet; import java.util.LinkedHashSet; import java.util.List; import java.util.Map; @@ -63,8 +54,7 @@ public class BazelConfigurationCollection implements ConfigurationCollectionFact Cache<String, BuildConfiguration> cache, PackageProviderForConfigurations packageProvider, BuildOptions buildOptions, - EventHandler eventHandler, - boolean performSanityCheck) throws InvalidConfigurationException { + EventHandler eventHandler) throws InvalidConfigurationException { // Target configuration BuildConfiguration targetConfiguration = configurationFactory.getConfiguration( packageProvider, buildOptions, false, cache); @@ -107,29 +97,6 @@ public class BazelConfigurationCollection implements ConfigurationCollectionFact if (packageProvider.valuesMissing()) { return null; } - - // Sanity check that the implicit labels are all in the transitive closure of explicit ones. - // This also registers all targets in the cache entry and validates them on subsequent requests. - Set<Label> reachableLabels = new HashSet<>(); - if (performSanityCheck) { - // We allow the package provider to be null for testing. - for (Map.Entry<String, Label> entry : buildOptions.getAllLabels().entries()) { - Label label = entry.getValue(); - try { - collectTransitiveClosure(packageProvider, reachableLabels, label); - } catch (NoSuchThingException e) { - eventHandler.handle(Event.error(e.getMessage())); - throw new InvalidConfigurationException( - String.format("Failed to load required %s target: '%s'", entry.getKey(), label)); - } - } - if (packageProvider.valuesMissing()) { - return null; - } - sanityCheckImplicitLabels(reachableLabels, targetConfiguration); - sanityCheckImplicitLabels(reachableLabels, hostConfiguration); - } - BuildConfiguration result = setupTransitions( targetConfiguration, dataConfiguration, hostConfiguration, splitTransitionsTable); result.reportInvalidOptions(eventHandler); @@ -252,52 +219,4 @@ public class BazelConfigurationCollection implements ConfigurationCollectionFact return targetConfiguration; } - - /** - * Checks that the implicit labels are reachable from the loaded labels. The loaded labels are - * those returned from {@link BuildOptions#getAllLabels()}, and the implicit ones are those that - * need to be available for late-bound attributes. - */ - private void sanityCheckImplicitLabels(Collection<Label> reachableLabels, - BuildConfiguration config) throws InvalidConfigurationException { - for (Map.Entry<String, Label> entry : config.getImplicitLabels().entries()) { - if (!reachableLabels.contains(entry.getValue())) { - throw new InvalidConfigurationException("The required " + entry.getKey() - + " target is not transitively reachable from a command-line option: '" - + entry.getValue() + "'"); - } - } - } - - private void collectTransitiveClosure(PackageProviderForConfigurations packageProvider, - Set<Label> reachableLabels, Label from) throws NoSuchThingException { - if (!reachableLabels.add(from)) { - return; - } - Target fromTarget = packageProvider.getTarget(from); - if (fromTarget instanceof Rule) { - Rule rule = (Rule) fromTarget; - if (rule.getRuleClassObject().hasAttr("srcs", BuildType.LABEL_LIST)) { - // TODO(bazel-team): refine this. This visits "srcs" reachable under *any* configuration, - // not necessarily the configuration actually applied to the rule. We should correlate the - // two. However, doing so requires faithfully reflecting the configuration transitions that - // might happen as we traverse the dependency chain. - // TODO(bazel-team): Why don't we use AbstractAttributeMapper#visitLabels() here? - for (List<Label> labelsForConfiguration : - AggregatingAttributeMapper.of(rule).visitAttribute("srcs", BuildType.LABEL_LIST)) { - for (Label label : labelsForConfiguration) { - collectTransitiveClosure(packageProvider, reachableLabels, - from.resolveRepositoryRelative(label)); - } - } - } - - if (rule.getRuleClass().equals("bind")) { - Label actual = AggregatingAttributeMapper.of(rule).get("actual", BuildType.LABEL); - if (actual != null) { - collectTransitiveClosure(packageProvider, reachableLabels, actual); - } - } - } - } } diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ConfigurationFragmentFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/ConfigurationFragmentFunction.java index 0242f3a79c..f2a12e7b34 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/ConfigurationFragmentFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/ConfigurationFragmentFunction.java @@ -15,6 +15,7 @@ package com.google.devtools.build.lib.skyframe; import com.google.common.base.Supplier; import com.google.common.collect.ImmutableList; +import com.google.common.collect.ListMultimap; import com.google.devtools.build.lib.analysis.BlazeDirectories; import com.google.devtools.build.lib.analysis.config.BuildConfiguration.Fragment; import com.google.devtools.build.lib.analysis.config.BuildOptions; @@ -24,10 +25,16 @@ import com.google.devtools.build.lib.analysis.config.InvalidConfigurationExcepti import com.google.devtools.build.lib.analysis.config.PackageProviderForConfigurations; import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.cmdline.LabelSyntaxException; +import com.google.devtools.build.lib.events.Event; import com.google.devtools.build.lib.events.EventHandler; +import com.google.devtools.build.lib.packages.AggregatingAttributeMapper; +import com.google.devtools.build.lib.packages.Attribute; +import com.google.devtools.build.lib.packages.AttributeMap.AcceptsLabelAttribute; import com.google.devtools.build.lib.packages.NoSuchPackageException; import com.google.devtools.build.lib.packages.NoSuchTargetException; +import com.google.devtools.build.lib.packages.NoSuchThingException; import com.google.devtools.build.lib.packages.Package; +import com.google.devtools.build.lib.packages.Rule; import com.google.devtools.build.lib.packages.RuleClassProvider; import com.google.devtools.build.lib.packages.Target; import com.google.devtools.build.lib.skyframe.ConfigurationFragmentValue.ConfigurationFragmentKey; @@ -38,12 +45,15 @@ import com.google.devtools.build.skyframe.SkyKey; import com.google.devtools.build.skyframe.SkyValue; import java.io.IOException; +import java.util.HashSet; +import java.util.LinkedHashSet; +import java.util.Map; +import java.util.Set; /** * A builder for {@link ConfigurationFragmentValue}s. */ -public class ConfigurationFragmentFunction implements SkyFunction { - +public final class ConfigurationFragmentFunction implements SkyFunction { private final Supplier<ImmutableList<ConfigurationFragmentFactory>> configurationFragments; private final RuleClassProvider ruleClassProvider; @@ -66,7 +76,11 @@ public class ConfigurationFragmentFunction implements SkyFunction { new SkyframePackageLoaderWithValueEnvironment(env, ruleClassProvider); ConfigurationEnvironment confEnv = new ConfigurationBuilderEnvironment(packageProvider); Fragment fragment = factory.create(confEnv, buildOptions); - + + if (env.valuesMissing()) { + return null; + } + sanityCheck(fragment, buildOptions, packageProvider); if (env.valuesMissing()) { return null; } @@ -80,7 +94,70 @@ public class ConfigurationFragmentFunction implements SkyFunction { throw new ConfigurationFragmentFunctionException(e); } } - + + /** + * Checks that the implicit labels are reachable from the loaded labels. The loaded labels are + * those returned from {@link BuildOptions#getAllLabels()}, and the implicit ones are those that + * are returned from {@link Fragment#getImplicitLabels}. + */ + private void sanityCheck(Fragment fragment, BuildOptions buildOptions, + PackageProviderForConfigurations packageProvider) throws InvalidConfigurationException { + if (fragment == null) { + return; + } + ListMultimap<String, Label> implicitLabels = fragment.getImplicitLabels(); + if (implicitLabels.isEmpty()) { + return; + } + // Sanity check that the implicit labels are all in the transitive closure of explicit ones. + // This also registers all targets in the cache entry and validates them on subsequent requests. + Set<Label> reachableLabels = new HashSet<>(); + for (Map.Entry<String, Label> entry : buildOptions.getAllLabels().entries()) { + Label label = entry.getValue(); + try { + collectAllTransitiveLabels(packageProvider, reachableLabels, label); + } catch (NoSuchThingException e) { + packageProvider.getEventHandler().handle(Event.error(e.getMessage())); + throw new InvalidConfigurationException( + String.format("Failed to load required %s target: '%s'", entry.getKey(), label)); + } + } + if (packageProvider.valuesMissing()) { + return; + } + for (Map.Entry<String, Label> entry : implicitLabels.entries()) { + if (!reachableLabels.contains(entry.getValue())) { + throw new InvalidConfigurationException( + String.format("The required %s target is not transitively reachable from a " + + "command-line option: '%s'", entry.getKey(), entry.getValue())); + } + } + } + + private void collectAllTransitiveLabels(PackageProviderForConfigurations packageProvider, + Set<Label> reachableLabels, Label from) throws NoSuchThingException { + if (!reachableLabels.add(from)) { + return; + } + Target fromTarget = packageProvider.getTarget(from); + if (fromTarget == null) { + return; + } + if (fromTarget instanceof Rule) { + Rule rule = (Rule) fromTarget; + final Set<Label> allLabels = new LinkedHashSet<>(); + AggregatingAttributeMapper.of(rule).visitLabels(new AcceptsLabelAttribute() { + @Override + public void acceptLabelAttribute(Label label, Attribute attribute) { + allLabels.add(label); + } + }); + for (Label label : allLabels) { + collectAllTransitiveLabels(packageProvider, reachableLabels, label); + } + } + } + private ConfigurationFragmentFactory getFactory(Class<? extends Fragment> fragmentType) { for (ConfigurationFragmentFactory factory : configurationFragments.get()) { if (factory.creates().equals(fragmentType)) { |