Refactor the missing fragment handling code to use a policy enum. Flip the handling such that analysis failure is the default, rather than execution failure (fail action creation). -- MOS_MIGRATED_REVID=100020812
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 f582cb6..b875a9d 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
@@ -40,6 +40,7 @@ import com.google.devtools.build.lib.packages.PackageSpecification; import com.google.devtools.build.lib.packages.Rule; import com.google.devtools.build.lib.packages.RuleClass; +import com.google.devtools.build.lib.packages.RuleClass.MissingFragmentPolicy; import com.google.devtools.build.lib.packages.RuleVisibility; import com.google.devtools.build.lib.packages.Target; import com.google.devtools.build.lib.rules.SkylarkRuleConfiguredTargetBuilder; @@ -225,9 +226,12 @@ return null; } if (!rule.getRuleClassObject().getRequiredConfigurationFragments().isEmpty()) { - if (!configuration.hasAllFragments( - rule.getRuleClassObject().getRequiredConfigurationFragments())) { - if (rule.getRuleClassObject().failIfMissingConfigurationFragment()) { + MissingFragmentPolicy missingFragmentPolicy = + rule.getRuleClassObject().missingFragmentPolicy(); + if (missingFragmentPolicy != MissingFragmentPolicy.IGNORE + && !configuration.hasAllFragments( + rule.getRuleClassObject().getRequiredConfigurationFragments())) { + if (missingFragmentPolicy == MissingFragmentPolicy.FAIL_ANALYSIS) { ruleContext.ruleError(missingFragmentError(ruleContext)); return null; }
diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/cpp/BazelCppRuleClasses.java b/src/main/java/com/google/devtools/build/lib/bazel/rules/cpp/BazelCppRuleClasses.java index 63c485e..dd09bea 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/rules/cpp/BazelCppRuleClasses.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/rules/cpp/BazelCppRuleClasses.java
@@ -627,7 +627,6 @@ public RuleClass build(Builder builder, RuleDefinitionEnvironment env) { return builder .requiresConfigurationFragments(CppConfiguration.class) - .failIfMissingConfigurationFragment() /*<!-- #BLAZE_RULE(cc_binary).IMPLICIT_OUTPUTS --> <ul> <li><code><var>name</var>.stripped</code> (only built if explicitly requested): A stripped @@ -773,7 +772,6 @@ // deps, data, linkopts, defines, srcs; override here too? .requiresConfigurationFragments(CppConfiguration.class) - .failIfMissingConfigurationFragment() /*<!-- #BLAZE_RULE(cc_library).ATTRIBUTE(alwayslink) --> If 1, any binary that depends (directly or indirectly) on this C++ library will link in all the object files for the files listed in
diff --git a/src/main/java/com/google/devtools/build/lib/packages/RuleClass.java b/src/main/java/com/google/devtools/build/lib/packages/RuleClass.java index e7cbf9e..e2bd958 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/RuleClass.java +++ b/src/main/java/com/google/devtools/build/lib/packages/RuleClass.java
@@ -183,6 +183,33 @@ }; /** + * How to handle the case if the configuration is missing fragments that are required according + * to the rule class. + */ + public enum MissingFragmentPolicy { + /** + * Some rules are monolithic across languages, and we want them to continue to work even when + * individual languages are disabled. Use this policy if the rule implementation is handling + * missing fragments. + */ + IGNORE, + + /** + * Use this policy to generate fail actions for the target rather than failing the analysis + * outright. Again, this is used when rules are monolithic across languages, but we still need + * to analyze the dependent libraries. (Instead of this mechanism, consider annotating + * attributes as unused if certain fragments are unavailable.) + */ + CREATE_FAIL_ACTIONS, + + /** + * Use this policy to fail the analysis of that target with an error message; this is the + * default. + */ + FAIL_ANALYSIS; + } + + /** * For Bazel's constraint system: the attribute that declares the set of environments a rule * supports, overriding the defaults for their respective groups. */ @@ -461,7 +488,7 @@ NO_EXTERNAL_BINDINGS; private SkylarkEnvironment ruleDefinitionEnvironment = null; private Set<Class<?>> configurationFragments = new LinkedHashSet<>(); - private boolean failIfMissingConfigurationFragment; + private MissingFragmentPolicy missingFragmentPolicy = MissingFragmentPolicy.FAIL_ANALYSIS; private boolean supportsConstraintChecking = true; private final Map<String, Attribute> attributes = new LinkedHashMap<>(); @@ -489,7 +516,7 @@ setPreferredDependencyPredicate(parent.preferredDependencyPredicate); } configurationFragments.addAll(parent.requiredConfigurationFragments); - failIfMissingConfigurationFragment |= parent.failIfMissingConfigurationFragment; + missingFragmentPolicy = parent.missingFragmentPolicy; supportsConstraintChecking = parent.supportsConstraintChecking; for (Attribute attribute : parent.getAttributes()) { @@ -548,7 +575,7 @@ configuredTargetFactory, validityPredicate, preferredDependencyPredicate, ImmutableSet.copyOf(advertisedProviders), configuredTargetFunction, externalBindingsFunction, - ruleDefinitionEnvironment, configurationFragments, failIfMissingConfigurationFragment, + ruleDefinitionEnvironment, configurationFragments, missingFragmentPolicy, supportsConstraintChecking, attributes.values().toArray(new Attribute[0])); } @@ -565,8 +592,12 @@ return this; } - public Builder failIfMissingConfigurationFragment() { - this.failIfMissingConfigurationFragment = true; + /** + * Sets the policy for the case where the configuration is missing required fragments (see + * {@link #requiresConfigurationFragments}). + */ + public Builder setMissingFragmentPolicy(MissingFragmentPolicy missingFragmentPolicy) { + this.missingFragmentPolicy = missingFragmentPolicy; return this; } @@ -917,12 +948,9 @@ private final ImmutableSet<Class<?>> requiredConfigurationFragments; /** - * Whether to fail during analysis if a configuration fragment is missing. The default behavior is - * to create fail actions for all declared outputs, i.e., to fail during execution, if any of the - * outputs is actually attempted to be built. + * What to do during analysis if a configuration fragment is missing. */ - private final boolean failIfMissingConfigurationFragment; - + private final MissingFragmentPolicy missingFragmentPolicy; /** * Determines whether instances of this rule should be checked for constraint compatibility @@ -965,7 +993,7 @@ @Nullable BaseFunction configuredTargetFunction, Function<? super Rule, Map<String, Label>> externalBindingsFunction, @Nullable SkylarkEnvironment ruleDefinitionEnvironment, - Set<Class<?>> allowedConfigurationFragments, boolean failIfMissingConfigurationFragment, + Set<Class<?>> allowedConfigurationFragments, MissingFragmentPolicy missingFragmentPolicy, boolean supportsConstraintChecking, Attribute... attributes) { this.name = name; @@ -988,7 +1016,7 @@ this.workspaceOnly = workspaceOnly; this.outputsDefaultExecutable = outputsDefaultExecutable; this.requiredConfigurationFragments = ImmutableSet.copyOf(allowedConfigurationFragments); - this.failIfMissingConfigurationFragment = failIfMissingConfigurationFragment; + this.missingFragmentPolicy = missingFragmentPolicy; this.supportsConstraintChecking = supportsConstraintChecking; // create the index: @@ -1158,8 +1186,8 @@ /** * Whether to fail analysis if any of the required configuration fragments are missing. */ - public boolean failIfMissingConfigurationFragment() { - return failIfMissingConfigurationFragment; + public MissingFragmentPolicy missingFragmentPolicy() { + return missingFragmentPolicy; } /**
diff --git a/src/test/java/com/google/devtools/build/lib/packages/RuleClassTest.java b/src/test/java/com/google/devtools/build/lib/packages/RuleClassTest.java index 2432ba7..4d1e7da 100644 --- a/src/test/java/com/google/devtools/build/lib/packages/RuleClassTest.java +++ b/src/test/java/com/google/devtools/build/lib/packages/RuleClassTest.java
@@ -42,6 +42,7 @@ import com.google.devtools.build.lib.packages.Attribute.ValidityPredicate; import com.google.devtools.build.lib.packages.Package.Builder; import com.google.devtools.build.lib.packages.RuleClass.Builder.RuleClassType; +import com.google.devtools.build.lib.packages.RuleClass.MissingFragmentPolicy; import com.google.devtools.build.lib.packages.util.PackageLoadingTestCase; import com.google.devtools.build.lib.syntax.Label; import com.google.devtools.build.lib.syntax.Label.SyntaxException; @@ -81,7 +82,8 @@ ImplicitOutputsFunction.NONE, RuleClass.NO_CHANGE, DUMMY_CONFIGURED_TARGET_FACTORY, PredicatesWithMessage.<Rule>alwaysTrue(), PREFERRED_DEPENDENCY_PREDICATE, ImmutableSet.<Class<?>>of(), null, - NO_EXTERNAL_BINDINGS, null, ImmutableSet.<Class<?>>of(), false, true, + NO_EXTERNAL_BINDINGS, null, ImmutableSet.<Class<?>>of(), + MissingFragmentPolicy.FAIL_ANALYSIS, true, attr("my-string-attr", STRING).mandatory().build(), attr("my-label-attr", LABEL).mandatory().legacyAllowAnyFileType() .value(Label.parseAbsolute("//default:label")).build(), @@ -100,7 +102,7 @@ ImplicitOutputsFunction.NONE, RuleClass.NO_CHANGE, DUMMY_CONFIGURED_TARGET_FACTORY, PredicatesWithMessage.<Rule>alwaysTrue(), PREFERRED_DEPENDENCY_PREDICATE, ImmutableSet.<Class<?>>of(), null, NO_EXTERNAL_BINDINGS, null, ImmutableSet.<Class<?>>of(), - false, true, attributes.toArray(new Attribute[0])); + MissingFragmentPolicy.FAIL_ANALYSIS, true, attributes.toArray(new Attribute[0])); } public void testRuleClassBasics() throws Exception { @@ -212,7 +214,7 @@ ImplicitOutputsFunction.NONE, RuleClass.NO_CHANGE, DUMMY_CONFIGURED_TARGET_FACTORY, PredicatesWithMessage.<Rule>alwaysTrue(), PREFERRED_DEPENDENCY_PREDICATE, ImmutableSet.<Class<?>>of(), null, NO_EXTERNAL_BINDINGS, null, ImmutableSet.<Class<?>>of(), - false, true, + MissingFragmentPolicy.FAIL_ANALYSIS, true, attr("list1", LABEL_LIST).mandatory().legacyAllowAnyFileType().build(), attr("list2", LABEL_LIST).mandatory().legacyAllowAnyFileType().build(), attr("list3", LABEL_LIST).mandatory().legacyAllowAnyFileType().build()); @@ -242,7 +244,8 @@ ImplicitOutputsFunction.NONE, RuleClass.NO_CHANGE, DUMMY_CONFIGURED_TARGET_FACTORY, PredicatesWithMessage.<Rule>alwaysTrue(), PREFERRED_DEPENDENCY_PREDICATE, ImmutableSet.<Class<?>>of(), null, NO_EXTERNAL_BINDINGS, null, ImmutableSet.<Class<?>>of(), - false, true, attr("visibility", LABEL_LIST).legacyAllowAnyFileType().build()); + MissingFragmentPolicy.FAIL_ANALYSIS, true, + attr("visibility", LABEL_LIST).legacyAllowAnyFileType().build()); Map<String, Object> attributeValues = new HashMap<>(); attributeValues.put("visibility", Arrays.asList("//visibility:legacy_public")); @@ -327,7 +330,7 @@ RuleClass.NO_CHANGE, DUMMY_CONFIGURED_TARGET_FACTORY, PredicatesWithMessage.<Rule>alwaysTrue(), PREFERRED_DEPENDENCY_PREDICATE, ImmutableSet.<Class<?>>of(), null, NO_EXTERNAL_BINDINGS, null, ImmutableSet.<Class<?>>of(), - false, true, attr("outs", OUTPUT_LIST).build()); + MissingFragmentPolicy.FAIL_ANALYSIS, true, attr("outs", OUTPUT_LIST).build()); Map<String, Object> attributeValues = new HashMap<>(); attributeValues.put("outs", Collections.singletonList("explicit_out")); @@ -348,7 +351,7 @@ ImplicitOutputsFunction.fromTemplates("%{dirname}lib%{basename}.bar"), RuleClass.NO_CHANGE, DUMMY_CONFIGURED_TARGET_FACTORY, PredicatesWithMessage.<Rule>alwaysTrue(), PREFERRED_DEPENDENCY_PREDICATE, ImmutableSet.<Class<?>>of(), null, NO_EXTERNAL_BINDINGS, - null, ImmutableSet.<Class<?>>of(), false, true); + null, ImmutableSet.<Class<?>>of(), MissingFragmentPolicy.FAIL_ANALYSIS, true); Rule rule = createRule(ruleClass, "myRule", Collections.<String, Object>emptyMap(), testRuleLocation); @@ -369,7 +372,7 @@ ImplicitOutputsFunction.fromTemplates("empty"), RuleClass.NO_CHANGE, DUMMY_CONFIGURED_TARGET_FACTORY, PredicatesWithMessage.<Rule>alwaysTrue(), PREFERRED_DEPENDENCY_PREDICATE, ImmutableSet.<Class<?>>of(), null, NO_EXTERNAL_BINDINGS, - null, ImmutableSet.<Class<?>>of(), false, true, + null, ImmutableSet.<Class<?>>of(), MissingFragmentPolicy.FAIL_ANALYSIS, true, attr("condition", BOOLEAN).value(false).build(), attr("declared1", BOOLEAN).value(false).build(), attr("declared2", BOOLEAN).value(false).build(), @@ -512,7 +515,7 @@ RuleClass.NO_CHANGE, DUMMY_CONFIGURED_TARGET_FACTORY, PredicatesWithMessage.<Rule>alwaysTrue(), PREFERRED_DEPENDENCY_PREDICATE, ImmutableSet.<Class<?>>of(), null, NO_EXTERNAL_BINDINGS, null, ImmutableSet.<Class<?>>of(), - false, true, attr("outs", OUTPUT_LIST).build()); + MissingFragmentPolicy.FAIL_ANALYSIS, true, attr("outs", OUTPUT_LIST).build()); Map<String, Object> attributeValues = new HashMap<>(); attributeValues.put("outs", ImmutableList.of("third", "fourth")); @@ -535,7 +538,7 @@ ImplicitOutputsFunction.NONE, RuleClass.NO_CHANGE, DUMMY_CONFIGURED_TARGET_FACTORY, PredicatesWithMessage.<Rule>alwaysTrue(), PREFERRED_DEPENDENCY_PREDICATE, ImmutableSet.<Class<?>>of(), null, NO_EXTERNAL_BINDINGS, null, ImmutableSet.<Class<?>>of(), - false, true, + MissingFragmentPolicy.FAIL_ANALYSIS, true, attr("a", STRING_LIST).mandatory().build(), attr("b", STRING_LIST).mandatory().build(), attr("c", STRING_LIST).mandatory().build(), @@ -621,7 +624,7 @@ ImplicitOutputsFunction.NONE, RuleClass.NO_CHANGE, DUMMY_CONFIGURED_TARGET_FACTORY, PredicatesWithMessage.<Rule>alwaysTrue(), PREFERRED_DEPENDENCY_PREDICATE, ImmutableSet.<Class<?>>of(), null, NO_EXTERNAL_BINDINGS, null, ImmutableSet.<Class<?>>of(), - false, true, attributes); + MissingFragmentPolicy.FAIL_ANALYSIS, true, attributes); return mandNonEmptyRuleClass; } @@ -632,7 +635,7 @@ ImplicitOutputsFunction.NONE, RuleClass.NO_CHANGE, DUMMY_CONFIGURED_TARGET_FACTORY, PredicatesWithMessage.<Rule>alwaysTrue(), PREFERRED_DEPENDENCY_PREDICATE, ImmutableSet.<Class<?>>of(), null, NO_EXTERNAL_BINDINGS, null, ImmutableSet.<Class<?>>of(), - false, true, attr("list", LABEL_LIST) + MissingFragmentPolicy.FAIL_ANALYSIS, true, attr("list", LABEL_LIST) .nonEmpty().legacyAllowAnyFileType().value(emptyList).build()); Map<String, Object> attributeValues = new LinkedHashMap<>(); @@ -718,7 +721,8 @@ false, ImplicitOutputsFunction.NONE, RuleClass.NO_CHANGE, DUMMY_CONFIGURED_TARGET_FACTORY, PredicatesWithMessage.<Rule>alwaysTrue(), PREFERRED_DEPENDENCY_PREDICATE, ImmutableSet.<Class<?>>of(), null, NO_EXTERNAL_BINDINGS, null, - ImmutableSet.<Class<?>>of(DummyFragment.class), false, true, attr("attr", STRING).build()); + ImmutableSet.<Class<?>>of(DummyFragment.class), MissingFragmentPolicy.FAIL_ANALYSIS, true, + attr("attr", STRING).build()); return parentRuleClass; }