From 72787a1267a6087923aca83bf161f93c0a1323e0 Mon Sep 17 00:00:00 2001 From: Philipp Schrader Date: Tue, 9 Aug 2022 10:58:49 -0700 Subject: [PATCH] Refactor Incompatible Target Skipping This patch reworks how Incompatible Target Skipping is implemented. The biggest change is that incompatibility is now checked earlier. This allows us to avoid evaluating dependencies (such as toolchains) in situations where a target is "directly" incompatible. "Direct incompatibility" refers to incompatibility due to a target's `target_compatible_with` value. Moving the incompatibility checking earlier had the undesired effect of visibility restrictions being ignored on incompatible targets. This is tracked in #16044. As per https://github.com/bazelbuild/bazel/pull/14096#issuecomment-1189589626, we will fix it in a separate patch. Fixes #12897. Closes #14096. PiperOrigin-RevId: 466407887 Change-Id: I3014390ddb95c7ff6bfaaf497a32b60c8a6e8fc9 --- site/en/docs/platforms.md | 5 + .../google/devtools/build/lib/analysis/BUILD | 34 ++- .../build/lib/analysis/BaseRuleClasses.java | 4 +- .../lib/analysis/ConfiguredTargetFactory.java | 7 - .../build/lib/analysis/RuleContext.java | 9 - .../build/lib/analysis/RunfilesSupport.java | 16 -- .../RuleConfiguredTarget.java | 23 ++ .../IncompatibleTargetChecker.java | 272 ++++++++++++++++++ .../RuleContextConstraintSemantics.java | 173 ----------- .../TopLevelConstraintSemantics.java | 5 +- .../lib/analysis/test/TestActionBuilder.java | 20 ++ .../google/devtools/build/lib/skyframe/BUILD | 1 + .../skyframe/ConfiguredTargetFunction.java | 33 ++- .../skyframe/RuleConfiguredTargetValue.java | 2 +- .../target_compatible_with_test.sh | 221 ++++++++++++++ 15 files changed, 612 insertions(+), 213 deletions(-) create mode 100644 src/main/java/com/google/devtools/build/lib/analysis/constraints/IncompatibleTargetChecker.java diff --git a/site/en/docs/platforms.md b/site/en/docs/platforms.md index 256db39d6e49d0..9284b34c939752 100644 --- a/site/en/docs/platforms.md +++ b/site/en/docs/platforms.md @@ -245,3 +245,8 @@ def format(target): $ bazel cquery //... --output=starlark --starlark:file=example.cquery ``` + +### Known Issues + +Incompatible targets [ignore visibility +restrictions](https://github.com/bazelbuild/bazel/issues/16044). 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 7410f484c6b1bf..3e40160b2ebe5a 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/BUILD +++ b/src/main/java/com/google/devtools/build/lib/analysis/BUILD @@ -384,7 +384,6 @@ java_library( "//src/main/java/com/google/devtools/build/lib/actions:localhost_capacity", "//src/main/java/com/google/devtools/build/lib/actions:package_roots", "//src/main/java/com/google/devtools/build/lib/analysis/platform", - "//src/main/java/com/google/devtools/build/lib/analysis/platform:utils", "//src/main/java/com/google/devtools/build/lib/analysis/starlark/annotations", "//src/main/java/com/google/devtools/build/lib/analysis/stringtemplate", "//src/main/java/com/google/devtools/build/lib/bugreport", @@ -2225,6 +2224,39 @@ java_library( ], ) +java_library( + name = "constraints/incompatible_target_checker", + srcs = ["constraints/IncompatibleTargetChecker.java"], + deps = [ + ":analysis_cluster", + ":file_provider", + ":incompatible_platform_provider", + ":test/test_configuration", + ":transitive_info_provider_map_builder", + "//src/main/java/com/google/devtools/build/lib/actions:artifacts", + "//src/main/java/com/google/devtools/build/lib/analysis:config/build_configuration", + "//src/main/java/com/google/devtools/build/lib/analysis:config/config_conditions", + "//src/main/java/com/google/devtools/build/lib/analysis:configured_target", + "//src/main/java/com/google/devtools/build/lib/analysis:configured_target_value", + "//src/main/java/com/google/devtools/build/lib/analysis:dependency", + "//src/main/java/com/google/devtools/build/lib/analysis:dependency_kind", + "//src/main/java/com/google/devtools/build/lib/analysis:target_and_configuration", + "//src/main/java/com/google/devtools/build/lib/analysis/platform", + "//src/main/java/com/google/devtools/build/lib/analysis/platform:utils", + "//src/main/java/com/google/devtools/build/lib/cmdline", + "//src/main/java/com/google/devtools/build/lib/collect/nestedset", + "//src/main/java/com/google/devtools/build/lib/packages", + "//src/main/java/com/google/devtools/build/lib/packages:configured_attribute_mapper", + "//src/main/java/com/google/devtools/build/lib/skyframe:configured_target_and_data", + "//src/main/java/com/google/devtools/build/lib/skyframe:configured_target_key", + "//src/main/java/com/google/devtools/build/lib/skyframe:rule_configured_target_value", + "//src/main/java/com/google/devtools/build/lib/util", + "//src/main/java/com/google/devtools/build/skyframe", + "//third_party:guava", + "//third_party:jsr305", + ], +) + # TODO(b/144899336): This should be analysis/starlark/BUILD java_library( name = "starlark/args", diff --git a/src/main/java/com/google/devtools/build/lib/analysis/BaseRuleClasses.java b/src/main/java/com/google/devtools/build/lib/analysis/BaseRuleClasses.java index fbbaec9cc72358..bc14130e9cb3fb 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/BaseRuleClasses.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/BaseRuleClasses.java @@ -403,8 +403,8 @@ public RuleClass build(RuleClass.Builder builder, RuleDefinitionEnvironment env) .add( attr("distribs", DISTRIBUTIONS) .nonconfigurable("Used in core loading phase logic with no access to configs")) - // Any rule that has provides its own meaning for the "target_compatible_with" attribute - // has to be excluded in `RuleContextConstraintSemantics.incompatibleConfiguredTarget()`. + // Any rule that provides its own meaning for the "target_compatible_with" attribute + // has to be excluded in `IncompatibleTargetChecker`. .add( attr(RuleClass.TARGET_COMPATIBLE_WITH_ATTR, LABEL_LIST) .mandatoryProviders(ConstraintValueInfo.PROVIDER.id()) diff --git a/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredTargetFactory.java b/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredTargetFactory.java index 390092c3623c04..977a65b29c9081 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredTargetFactory.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredTargetFactory.java @@ -38,7 +38,6 @@ import com.google.devtools.build.lib.analysis.configuredtargets.OutputFileConfiguredTarget; import com.google.devtools.build.lib.analysis.configuredtargets.PackageGroupConfiguredTarget; import com.google.devtools.build.lib.analysis.configuredtargets.RuleConfiguredTarget; -import com.google.devtools.build.lib.analysis.constraints.RuleContextConstraintSemantics; import com.google.devtools.build.lib.analysis.starlark.StarlarkRuleConfiguredTargetUtil; import com.google.devtools.build.lib.analysis.test.AnalysisFailure; import com.google.devtools.build.lib.analysis.test.AnalysisFailureInfo; @@ -325,12 +324,6 @@ private ConfiguredTarget createRule( return erroredConfiguredTarget(ruleContext); } - ConfiguredTarget incompatibleTarget = - RuleContextConstraintSemantics.incompatibleConfiguredTarget(ruleContext, prerequisiteMap); - if (incompatibleTarget != null) { - return incompatibleTarget; - } - try { Class missingFragmentClass = null; for (Class fragmentClass : 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 98eab47cd14c8a..4ce591a69f5318 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 @@ -53,7 +53,6 @@ import com.google.devtools.build.lib.analysis.config.FragmentCollection; import com.google.devtools.build.lib.analysis.config.transitions.ConfigurationTransition; import com.google.devtools.build.lib.analysis.config.transitions.NoTransition; -import com.google.devtools.build.lib.analysis.constraints.RuleContextConstraintSemantics; import com.google.devtools.build.lib.analysis.platform.ConstraintValueInfo; import com.google.devtools.build.lib.analysis.platform.PlatformInfo; import com.google.devtools.build.lib.analysis.starlark.StarlarkRuleContext; @@ -2180,14 +2179,6 @@ private static boolean checkRuleDependencyMandatoryProviders( private void validateDirectPrerequisite( Attribute attribute, ConfiguredTargetAndData prerequisite) { - if (RuleContextConstraintSemantics.checkForIncompatibility(prerequisite.getConfiguredTarget()) - .isIncompatible()) { - // If the prerequisite is incompatible (e.g. has an incompatible provider), we pretend that - // there is no further validation needed. Otherwise, it would be difficult to make the - // incompatible target satisfy things like required providers and file extensions. - return; - } - validateDirectPrerequisiteType(prerequisite, attribute); validateDirectPrerequisiteFileTypes(prerequisite, attribute); if (attribute.performPrereqValidatorCheck()) { diff --git a/src/main/java/com/google/devtools/build/lib/analysis/RunfilesSupport.java b/src/main/java/com/google/devtools/build/lib/analysis/RunfilesSupport.java index df040755f46277..ef668a7974a8be 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/RunfilesSupport.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/RunfilesSupport.java @@ -448,22 +448,6 @@ public static RunfilesSupport withExecutable( computeActionEnvironment(ruleContext)); } - /** - * Creates and returns a {@link RunfilesSupport} object for the given rule and executable. This - * version discards all arguments. Only use this for Incompatible Target - * Skipping. - */ - public static RunfilesSupport withExecutableButNoArgs( - RuleContext ruleContext, Runfiles runfiles, Artifact executable) { - return RunfilesSupport.create( - ruleContext, - executable, - runfiles, - CommandLine.EMPTY, - computeActionEnvironment(ruleContext)); - } - private static CommandLine computeArgs(RuleContext ruleContext, CommandLine additionalArgs) { if (!ruleContext.getRule().isAttrDefined("args", Type.STRING_LIST)) { // Some non-_binary rules create RunfilesSupport instances; it is fine to not have an args diff --git a/src/main/java/com/google/devtools/build/lib/analysis/configuredtargets/RuleConfiguredTarget.java b/src/main/java/com/google/devtools/build/lib/analysis/configuredtargets/RuleConfiguredTarget.java index eb6b28f72c3e6e..c52131d3a536a4 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/configuredtargets/RuleConfiguredTarget.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/configuredtargets/RuleConfiguredTarget.java @@ -23,6 +23,7 @@ import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.analysis.FileProvider; import com.google.devtools.build.lib.analysis.FilesToRunProvider; +import com.google.devtools.build.lib.analysis.IncompatiblePlatformProvider; import com.google.devtools.build.lib.analysis.RuleContext; import com.google.devtools.build.lib.analysis.RunfilesProvider; import com.google.devtools.build.lib.analysis.TransitiveInfoProvider; @@ -155,6 +156,28 @@ public RuleConfiguredTarget( } } + /** Use this constructor for creating incompatible ConfiguredTarget instances. */ + public RuleConfiguredTarget( + Label label, + BuildConfigurationKey configurationKey, + NestedSet visibility, + TransitiveInfoProviderMap providers, + ImmutableMap configConditions, + String ruleClassString) { + this( + label, + configurationKey, + visibility, + providers, + configConditions, + ImmutableSet.of(), + ruleClassString, + ImmutableList.of(), + ImmutableMap.of()); + + Preconditions.checkState(providers.get(IncompatiblePlatformProvider.PROVIDER) != null, label); + } + /** The configuration conditions that trigger this rule's configurable attributes. */ @Override public ImmutableMap getConfigConditions() { diff --git a/src/main/java/com/google/devtools/build/lib/analysis/constraints/IncompatibleTargetChecker.java b/src/main/java/com/google/devtools/build/lib/analysis/constraints/IncompatibleTargetChecker.java new file mode 100644 index 00000000000000..5f5a306575b8d3 --- /dev/null +++ b/src/main/java/com/google/devtools/build/lib/analysis/constraints/IncompatibleTargetChecker.java @@ -0,0 +1,272 @@ +// Copyright 2022 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.constraints; + +import static com.google.common.collect.ImmutableList.toImmutableList; + +import com.google.common.collect.ImmutableList; +import com.google.devtools.build.lib.actions.Artifact; +import com.google.devtools.build.lib.analysis.ConfiguredTarget; +import com.google.devtools.build.lib.analysis.ConfiguredTargetValue; +import com.google.devtools.build.lib.analysis.Dependency; +import com.google.devtools.build.lib.analysis.DependencyKind; +import com.google.devtools.build.lib.analysis.FileProvider; +import com.google.devtools.build.lib.analysis.FilesToRunProvider; +import com.google.devtools.build.lib.analysis.IncompatiblePlatformProvider; +import com.google.devtools.build.lib.analysis.Runfiles; +import com.google.devtools.build.lib.analysis.RunfilesProvider; +import com.google.devtools.build.lib.analysis.TargetAndConfiguration; +import com.google.devtools.build.lib.analysis.TransitiveInfoProviderMapBuilder; +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.configuredtargets.RuleConfiguredTarget; +import com.google.devtools.build.lib.analysis.platform.ConstraintValueInfo; +import com.google.devtools.build.lib.analysis.platform.PlatformInfo; +import com.google.devtools.build.lib.analysis.platform.PlatformProviderUtils; +import com.google.devtools.build.lib.analysis.test.TestActionBuilder; +import com.google.devtools.build.lib.analysis.test.TestConfiguration; +import com.google.devtools.build.lib.analysis.test.TestProvider; +import com.google.devtools.build.lib.cmdline.Label; +import com.google.devtools.build.lib.collect.nestedset.NestedSet; +import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder; +import com.google.devtools.build.lib.collect.nestedset.Order; +import com.google.devtools.build.lib.packages.BuildType; +import com.google.devtools.build.lib.packages.ConfiguredAttributeMapper; +import com.google.devtools.build.lib.packages.Package; +import com.google.devtools.build.lib.packages.PackageSpecification; +import com.google.devtools.build.lib.packages.PackageSpecification.PackageGroupContents; +import com.google.devtools.build.lib.packages.Rule; +import com.google.devtools.build.lib.packages.Target; +import com.google.devtools.build.lib.skyframe.ConfiguredTargetAndData; +import com.google.devtools.build.lib.skyframe.ConfiguredTargetKey; +import com.google.devtools.build.lib.skyframe.RuleConfiguredTargetValue; +import com.google.devtools.build.lib.util.OrderedSetMultimap; +import com.google.devtools.build.skyframe.SkyFunction.Environment; +import com.google.devtools.build.skyframe.SkyframeLookupResult; +import java.util.List; +import java.util.Optional; +import javax.annotation.Nullable; + +/** + * Helpers for creating configured targets that are incompatible. + * + *

A target is considered incompatible if any of the following applies: + * + *

    + *
  1. The target's target_compatible_with attribute specifies a constraint that is + * not present in the target platform. The target is said to be "directly incompatible". + *
  2. One or more of the target's dependencies is incompatible. The target is said to be + * "indirectly incompatible." + *
+ * + * The intent of these helpers is that they get called as early in the analysis phase as possible. + * That's why there are two helpers instead of just one. The first helper determines direct + * incompatibility very early in the analysis phase. If a target is not directly incompatible, the + * dependencies need to be analysed and then we can check for indirect incompatibility. Doing these + * checks as early as possible allows us to skip analysing unused dependencies and ignore unused + * toolchains. + * + *

See https://bazel.build/docs/platforms#skipping-incompatible-targets for more information on + * incompatible target skipping. + */ +public class IncompatibleTargetChecker { + /** + * Creates an incompatible configured target if it is "directly incompatible". + * + *

In other words, this function checks if a target is incompatible because of its + * "target_compatible_with" attribute. + * + *

This function returns a nullable {@code Optional} of a {@link RuleConfiguredTargetValue}. + * This provides three states of return values: + * + *

    + *
  • {@code null}: A skyframe restart is required. + *
  • {@code Optional.empty()}: The target is not directly incompatible. Analysis can continue. + *
  • {@code !Optional.empty()}: The target is directly incompatible. Analysis should not + * continue. + *
+ */ + @Nullable + public static Optional createDirectlyIncompatibleTarget( + TargetAndConfiguration targetAndConfiguration, + ConfigConditions configConditions, + Environment env, + @Nullable PlatformInfo platformInfo, + NestedSetBuilder transitivePackagesForPackageRootResolution) + throws InterruptedException { + Target target = targetAndConfiguration.getTarget(); + Rule rule = target.getAssociatedRule(); + + if (rule == null || rule.getRuleClass().equals("toolchain") || platformInfo == null) { + return Optional.empty(); + } + + // Retrieve the label list for the target_compatible_with attribute. + BuildConfigurationValue configuration = targetAndConfiguration.getConfiguration(); + ConfiguredAttributeMapper attrs = + ConfiguredAttributeMapper.of(rule, configConditions.asProviders(), configuration); + if (!attrs.has("target_compatible_with", BuildType.LABEL_LIST)) { + return Optional.empty(); + } + + // Resolve the constraint labels. + List