From da36c56ce9b141fe474908b3d5936175f9a6a6aa Mon Sep 17 00:00:00 2001 From: Philipp Schrader Date: Sat, 13 Feb 2021 13:37:57 -0800 Subject: [PATCH 01/39] EXPERIMENT: Investigate objc_library and target_compatible_with --- .../build/lib/analysis/BaseRuleClasses.java | 2 +- .../lib/analysis/ConfiguredTargetFactory.java | 6 - .../build/lib/analysis/RuleContext.java | 9 - .../build/lib/analysis/RunfilesSupport.java | 16 -- .../RuleConfiguredTarget.java | 26 +++ .../RuleContextConstraintSemantics.java | 154 -------------- .../build/lib/analysis/test/TestProvider.java | 2 +- .../google/devtools/build/lib/skyframe/BUILD | 5 + .../skyframe/ConfiguredTargetFunction.java | 193 ++++++++++++++++++ .../target_compatible_with_test.sh | 32 +++ 10 files changed, 258 insertions(+), 187 deletions(-) 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 861206b86c550e..5e781abbf87c39 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 @@ -412,7 +412,7 @@ public RuleClass build(RuleClass.Builder builder, RuleDefinitionEnvironment env) 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()`. + // has to be excluded in `ConfiguredTargetFunction.compute()`. .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 d11739ae2b44b8..14bfb72bfca0d6 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 @@ -317,12 +317,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 ea265cfc86506a..faef82498baf7c 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; @@ -2160,14 +2159,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 d0fe247bccfb05..efed6ac2b50e1f 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 05527042ca4c4f..bedc02681311f2 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 @@ -32,6 +32,7 @@ import com.google.devtools.build.lib.analysis.config.ConfigMatchingProvider; import com.google.devtools.build.lib.analysis.config.RunUnder; import com.google.devtools.build.lib.analysis.starlark.StarlarkApiProvider; +import com.google.devtools.build.lib.analysis.IncompatiblePlatformProvider; import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.collect.nestedset.NestedSet; import com.google.devtools.build.lib.concurrent.BlazeInterners; @@ -154,6 +155,31 @@ public RuleConfiguredTarget( } } + /** + * Use this constructor for creating incompatible ConfiguredTarget instances. + */ + public RuleConfiguredTarget( + Label label, + BuildConfigurationValue.Key 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/RuleContextConstraintSemantics.java b/src/main/java/com/google/devtools/build/lib/analysis/constraints/RuleContextConstraintSemantics.java index e9615893b6737d..582c06e0cc4df8 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/constraints/RuleContextConstraintSemantics.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/constraints/RuleContextConstraintSemantics.java @@ -882,158 +882,4 @@ public static IncompatibleCheckResult checkForIncompatibility(ConfiguredTarget t return IncompatibleCheckResult.create( target.get(IncompatiblePlatformProvider.PROVIDER) != null, target); } - - /** - * Creates an incompatible {@link ConfiguredTarget} if the corresponding rule is incompatible. - * - *

Returns null if the target is not incompatible. - * - *

"Incompatible" here means either: - * - *

    - *
  1. the corresponding target_compatible_with list contains a constraint that the - * current platform doesn't satisfy, or - *
  2. a transitive dependency is incompatible. - *
- * - * @param ruleContext analysis context for the rule - * @param prerequisiteMap the dependencies of the rule - * @throws ActionConflictException if the underlying {@link RuleConfiguredTargetBuilder} - * encounters a problem when assembling a dummy action for the incompatible {@link - * ConfiguredTarget}. - */ - @Nullable - public static ConfiguredTarget incompatibleConfiguredTarget( - RuleContext ruleContext, - OrderedSetMultimap prerequisiteMap) - throws ActionConflictException, InterruptedException { - // The target (ruleContext) is incompatible if explicitly specified to be. Any rule that - // provides its own meaning for the "target_compatible_with" attribute has to be excluded here. - // For example, the "toolchain" rule uses "target_compatible_with" for bazel's toolchain - // resolution. - if (!ruleContext.getRule().getRuleClass().equals("toolchain") - && ruleContext.attributes().has("target_compatible_with")) { - ImmutableList invalidConstraintValues = - PlatformProviderUtils.constraintValues( - ruleContext.getPrerequisites("target_compatible_with")) - .stream() - .filter(cv -> !ruleContext.targetPlatformHasConstraint(cv)) - .collect(toImmutableList()); - - if (!invalidConstraintValues.isEmpty()) { - return createIncompatibleConfiguredTarget(ruleContext, null, invalidConstraintValues); - } - } - - // This is incompatible if one of the dependencies is as well. - ImmutableList incompatibleDependencies = - prerequisiteMap.values().stream() - .map(value -> checkForIncompatibility(value.getConfiguredTarget())) - .filter(IncompatibleCheckResult::isIncompatible) - .map(IncompatibleCheckResult::underlyingTarget) - .collect(toImmutableList()); - if (!incompatibleDependencies.isEmpty()) { - return createIncompatibleConfiguredTarget(ruleContext, incompatibleDependencies, null); - } - - return null; - } - - /** - * A helper function for incompatibleConfiguredTarget() to actually create the incompatible {@link - * ConfiguredTarget}. - * - * @param ruleContext analysis context for the rule - * @param targetsResponsibleForIncompatibility the targets that are responsible this target's - * incompatibility. If null, that means that target is responsible for its own - * incompatibility. I.e. it has constraints in target_compatible_with that were not satisfied - * on the target platform. This must be null if violatedConstraints is set. This must be set - * if violatedConstraints is null. - * @param violatedConstraints the constraints that the target platform doesn't satisfy. This must - * be null if targetRsesponsibleForIncompatibility is set. - * @throws ActionConflictException if the underlying {@link RuleConfiguredTargetBuilder} - * encounters a problem when assembling a dummy action for the incompatible {@link - * ConfiguredTarget}. - */ - private static ConfiguredTarget createIncompatibleConfiguredTarget( - RuleContext ruleContext, - @Nullable ImmutableList targetsResponsibleForIncompatibility, - @Nullable ImmutableList violatedConstraints) - throws ActionConflictException, InterruptedException { - // Create a dummy ConfiguredTarget that has the IncompatiblePlatformProvider set. - ImmutableList outputArtifacts = ruleContext.getOutputArtifacts(); - - if (ruleContext.isTestTarget() && outputArtifacts.isEmpty()) { - // Test targets require RunfilesSupport to be added to the RuleConfiguredTargetBuilder - // which needs an "executable". Create one here if none exist already. - // - // It would be ideal if we could avoid this. Currently the problem is that some rules like - // sh_test only declare an output artifact in the corresponding ConfiguredTarget factory - // function (see ShBinary.java). Since this code path here replaces the factory function rules - // like sh_test never get a chance to declare an output artifact. - // - // On top of that, the rest of the code base makes the assumption that test targets provide an - // instance RunfilesSupport. This can be seen in the TestProvider, the TestActionBuilder, and - // the RuleConfiguredTargetBuilder classes. There might be a way to break this assumption, but - // it's currently too heavily baked in to work around it more nicely than this. - // - // Theoretically, this hack shouldn't be an issue because the corresponding actions will never - // get executed. They cannot be queried either. - outputArtifacts = ImmutableList.of(ruleContext.createOutputArtifact()); - } - - NestedSet filesToBuild = - NestedSetBuilder.stableOrder().addAll(outputArtifacts).build(); - - Runfiles.Builder runfilesBuilder = - new Runfiles.Builder( - ruleContext.getWorkspaceName(), - ruleContext.getConfiguration().legacyExternalRunfiles()); - Runfiles runfiles = - runfilesBuilder - .addTransitiveArtifacts(filesToBuild) - .addRunfiles(ruleContext, RunfilesProvider.DEFAULT_RUNFILES) - .build(); - - RuleConfiguredTargetBuilder builder = new RuleConfiguredTargetBuilder(ruleContext); - builder.setFilesToBuild(filesToBuild); - - if (targetsResponsibleForIncompatibility != null) { - builder.addNativeDeclaredProvider( - IncompatiblePlatformProvider.incompatibleDueToTargets( - targetsResponsibleForIncompatibility)); - } else if (violatedConstraints != null) { - builder.addNativeDeclaredProvider( - IncompatiblePlatformProvider.incompatibleDueToConstraints(violatedConstraints)); - } else { - throw new IllegalArgumentException( - "Both violatedConstraints and targetsResponsibleForIncompatibility are null"); - } - - // If this is an analysis test, RuleConfiguredTargetBuilder performs some additional sanity - // checks. Satisfy them with an appropriate provider. - if (ruleContext.getRule().isAnalysisTest()) { - builder.addNativeDeclaredProvider( - new AnalysisTestResultInfo( - /*success=*/ false, - "This test is incompatible and should not have been run. Please file a bug" - + " upstream.")); - } - - builder.add(RunfilesProvider.class, RunfilesProvider.simple(runfiles)); - if (!outputArtifacts.isEmpty()) { - Artifact executable = outputArtifacts.get(0); - RunfilesSupport runfilesSupport = - RunfilesSupport.withExecutableButNoArgs(ruleContext, runfiles, executable); - builder.setRunfilesSupport(runfilesSupport, executable); - - ruleContext.registerAction( - new FailAction( - ruleContext.getActionOwner(), - outputArtifacts, - "Can't build this. This target is incompatible. Please file a bug upstream.", - Code.CANT_BUILD_INCOMPATIBLE_TARGET)); - } - return builder.build(); - } } diff --git a/src/main/java/com/google/devtools/build/lib/analysis/test/TestProvider.java b/src/main/java/com/google/devtools/build/lib/analysis/test/TestProvider.java index 3275913821f2f9..d61414ccccbdab 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/test/TestProvider.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/test/TestProvider.java @@ -68,7 +68,7 @@ public static class TestParams { * Don't call this directly. Instead use {@link * com.google.devtools.build.lib.analysis.test.TestActionBuilder}. */ - TestParams( + public TestParams( int runs, int shards, boolean runsDetectsFlakes, diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/BUILD b/src/main/java/com/google/devtools/build/lib/skyframe/BUILD index 99aa2d00801031..9db75c3461186d 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/BUILD +++ b/src/main/java/com/google/devtools/build/lib/skyframe/BUILD @@ -258,15 +258,20 @@ java_library( "//src/main/java/com/google/devtools/build/lib/analysis:duplicate_exception", "//src/main/java/com/google/devtools/build/lib/analysis:exec_group_collection", "//src/main/java/com/google/devtools/build/lib/analysis:extra_action_artifacts_provider", + "//src/main/java/com/google/devtools/build/lib/analysis:file_provider", "//src/main/java/com/google/devtools/build/lib/analysis:inconsistent_aspect_order_exception", + "//src/main/java/com/google/devtools/build/lib/analysis:incompatible_platform_provider", "//src/main/java/com/google/devtools/build/lib/analysis:platform_configuration", "//src/main/java/com/google/devtools/build/lib/analysis:platform_options", "//src/main/java/com/google/devtools/build/lib/analysis:resolved_toolchain_context", "//src/main/java/com/google/devtools/build/lib/analysis:rule_configured_object_value", "//src/main/java/com/google/devtools/build/lib/analysis:target_and_configuration", + "//src/main/java/com/google/devtools/build/lib/analysis:test/test_configuration", "//src/main/java/com/google/devtools/build/lib/analysis:toolchain_collection", "//src/main/java/com/google/devtools/build/lib/analysis:toolchain_context", "//src/main/java/com/google/devtools/build/lib/analysis:top_level_artifact_context", + "//src/main/java/com/google/devtools/build/lib/analysis:transitive_info_collection", + "//src/main/java/com/google/devtools/build/lib/analysis:transitive_info_provider_map_builder", "//src/main/java/com/google/devtools/build/lib/analysis:view_creation_failed_exception", "//src/main/java/com/google/devtools/build/lib/analysis:workspace_status_action", "//src/main/java/com/google/devtools/build/lib/analysis/platform", diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java index b3f319acc115c0..cedb3c2badd824 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java @@ -22,6 +22,9 @@ import com.google.common.collect.Iterables; import com.google.common.collect.Maps; import com.google.common.flogger.GoogleLogger; +import com.google.devtools.build.lib.actions.ActionAnalysisMetadata; +import com.google.devtools.build.lib.actions.ActionInput; +import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.actions.MutableActionGraph.ActionConflictException; import com.google.devtools.build.lib.analysis.AnalysisRootCauseEvent; import com.google.devtools.build.lib.analysis.AspectResolver; @@ -39,12 +42,19 @@ import com.google.devtools.build.lib.analysis.EmptyConfiguredTarget; import com.google.devtools.build.lib.analysis.ExecGroupCollection; import com.google.devtools.build.lib.analysis.ExecGroupCollection.InvalidExecGroupException; +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.InconsistentAspectOrderException; import com.google.devtools.build.lib.analysis.PlatformConfiguration; import com.google.devtools.build.lib.analysis.ResolvedToolchainContext; +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.ToolchainCollection; import com.google.devtools.build.lib.analysis.ToolchainContext; +import com.google.devtools.build.lib.analysis.TransitiveInfoCollection; +import com.google.devtools.build.lib.analysis.TransitiveInfoProviderMapBuilder; import com.google.devtools.build.lib.analysis.config.BuildConfiguration; import com.google.devtools.build.lib.analysis.config.BuildOptions; import com.google.devtools.build.lib.analysis.config.BuildOptionsView; @@ -54,7 +64,12 @@ import com.google.devtools.build.lib.analysis.config.DependencyEvaluationException; import com.google.devtools.build.lib.analysis.config.transitions.PatchTransition; 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.platform.PlatformInfo; +import com.google.devtools.build.lib.analysis.platform.ConstraintValueInfo; +import com.google.devtools.build.lib.analysis.platform.PlatformProviderUtils; +import com.google.devtools.build.lib.analysis.test.TestConfiguration; +import com.google.devtools.build.lib.analysis.test.TestProvider; import com.google.devtools.build.lib.causes.AnalysisFailedCause; import com.google.devtools.build.lib.causes.Cause; import com.google.devtools.build.lib.causes.LoadingFailedCause; @@ -67,15 +82,23 @@ import com.google.devtools.build.lib.events.StoredEventHandler; import com.google.devtools.build.lib.packages.Aspect; import com.google.devtools.build.lib.packages.BuildType; +import com.google.devtools.build.lib.packages.ConfiguredAttributeMapper; +import com.google.devtools.build.lib.packages.ConstantRuleVisibility; import com.google.devtools.build.lib.packages.ExecGroup; import com.google.devtools.build.lib.packages.NoSuchTargetException; import com.google.devtools.build.lib.packages.NonconfigurableAttributeMapper; +import com.google.devtools.build.lib.packages.OutputFile; import com.google.devtools.build.lib.packages.Package; +import com.google.devtools.build.lib.packages.PackageGroupsRuleVisibility; +import com.google.devtools.build.lib.packages.PackageSpecification; +import com.google.devtools.build.lib.packages.PackageSpecification.PackageGroupContents; import com.google.devtools.build.lib.packages.RawAttributeMapper; import com.google.devtools.build.lib.packages.Rule; import com.google.devtools.build.lib.packages.RuleClass; import com.google.devtools.build.lib.packages.RuleClassProvider; +import com.google.devtools.build.lib.packages.RuleVisibility; import com.google.devtools.build.lib.packages.Target; +import com.google.devtools.build.lib.packages.TestTimeout; import com.google.devtools.build.lib.server.FailureDetails.Analysis; import com.google.devtools.build.lib.server.FailureDetails.Analysis.Code; import com.google.devtools.build.lib.server.FailureDetails.FailureDetail; @@ -315,6 +338,84 @@ public SkyValue compute(SkyKey key, Environment env) getPrioritizedDetailedExitCode(causes))); } + Rule rule = null; + if (target instanceof Rule) { + rule = (Rule) target; + } + if (unloadedToolchainContexts != null) { + PlatformInfo platformInfo = unloadedToolchainContexts.getTargetPlatform(); + if (platformInfo != null && rule != null) { + if (!rule.getRuleClass().equals("toolchain")) { + ConfiguredAttributeMapper attrs = ConfiguredAttributeMapper.of(rule, configConditions.asProviders(), ""); + if (attrs.has("target_compatible_with", BuildType.LABEL_LIST)) { + List