diff options
author | 2016-07-29 19:32:38 +0000 | |
---|---|---|
committer | 2016-08-01 08:07:23 +0000 | |
commit | e49cd59a3281ca1aae539baf18dfde8627c01abe (patch) | |
tree | 19dd6851dac446b3f14e22513c7b1703b9b37044 /src | |
parent | 49dfc29a40764ae48f2343007f88f3a4938498f1 (diff) |
Allow objc rule deps to include any rule that exports an "objc" provider.
This change was motivated by a need to write pure Skylark rules that expose their own objc providers so they can be used as deps to other libraries/application targets (e.g., SceneKit/SpriteKit compiled resources, []) without having to whitelist them and wait for a Blaze release.
This CL fixes what seems to be a bug in validateRuleDependency, where the behavior in the doc comment implies that it will accept a whitelisted rule name *or* a list of mandatory providers, but as implemented today it seems to require the rule to be whitelisted even if the mandatory native providers matched.
RELNOTES: objc_* rules can now depend on any target that returns an "objc" provider.
--
MOS_MIGRATED_REVID=128835096
Diffstat (limited to 'src')
3 files changed, 55 insertions, 36 deletions
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 8965cb596a..b429606b48 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 @@ -77,6 +77,7 @@ import com.google.devtools.build.lib.syntax.EvalException; import com.google.devtools.build.lib.syntax.Type; import com.google.devtools.build.lib.util.FileTypeSet; import com.google.devtools.build.lib.util.Preconditions; +import com.google.devtools.build.lib.util.StringUtil; import com.google.devtools.build.lib.vfs.FileSystemUtils; import com.google.devtools.build.lib.vfs.PathFragment; import java.util.ArrayList; @@ -1613,19 +1614,28 @@ public final class RuleContext extends TargetContext reporter.attributeWarning(attrName, message); } - private void reportBadPrerequisite(Attribute attribute, String targetKind, - ConfiguredTarget prerequisite, String reason, boolean isWarning) { + private String badPrerequisiteMessage(String targetKind, ConfiguredTarget prerequisite, + String reason, boolean isWarning) { String msgPrefix = targetKind != null ? targetKind + " " : ""; String msgReason = reason != null ? " (" + reason + ")" : ""; if (isWarning) { - attributeWarning(attribute.getName(), String.format( + return String.format( "%s'%s'%s is unexpected here%s; continuing anyway", msgPrefix, prerequisite.getLabel(), AliasProvider.printVisibilityChain(prerequisite), - msgReason)); + msgReason); + } + return String.format( + "%s'%s'%s is misplaced here%s", msgPrefix, prerequisite.getLabel(), + AliasProvider.printVisibilityChain(prerequisite), msgReason); + } + + private void reportBadPrerequisite(Attribute attribute, String targetKind, + ConfiguredTarget prerequisite, String reason, boolean isWarning) { + String message = badPrerequisiteMessage(targetKind, prerequisite, reason, isWarning); + if (isWarning) { + attributeWarning(attribute.getName(), message); } else { - attributeError(attribute.getName(), String.format( - "%s'%s'%s is misplaced here%s", msgPrefix, prerequisite.getLabel(), - AliasProvider.printVisibilityChain(prerequisite), msgReason)); + attributeError(attribute.getName(), message); } } @@ -1803,19 +1813,20 @@ public final class RuleContext extends TargetContext /** * Because some rules still have to use allowedRuleClasses to do rule dependency validation. - * We implemented the allowedRuleClasses OR mandatoryProvidersList mechanism. Either condition - * is satisfied, we consider the dependency valid. + * A dependency is valid if it is from a rule in allowedRuledClasses, OR if all of the providers + * in mandatoryNativeProviders AND mandatoryProvidersList are provided by the target. */ private void validateRuleDependency(ConfiguredTarget prerequisite, Attribute attribute) { Target prerequisiteTarget = prerequisite.getTarget(); RuleClass ruleClass = ((Rule) prerequisiteTarget).getRuleClassObject(); - Boolean allowed = null; + HashSet<String> unfulfilledRequirements = new HashSet<>(); if (attribute.getAllowedRuleClassesPredicate() != Predicates.<RuleClass>alwaysTrue()) { - allowed = attribute.getAllowedRuleClassesPredicate().apply(ruleClass); - if (allowed) { + if (attribute.getAllowedRuleClassesPredicate().apply(ruleClass)) { return; } + unfulfilledRequirements.add(badPrerequisiteMessage(prerequisiteTarget.getTargetKind(), + prerequisite, "expected " + attribute.getAllowedRuleClassesPredicate(), false)); } if (attribute.getAllowedRuleClassesWarningPredicate() @@ -1828,27 +1839,38 @@ public final class RuleContext extends TargetContext } } - if (!attribute.getMandatoryNativeProviders().isEmpty()) { - String missing = getMissingMandatoryNativeProviders(prerequisite, attribute); - if (missing != null) { - attributeError( - attribute.getName(), - "'" + prerequisite.getLabel() + "' does not have mandatory providers: " + missing); - return; + // This condition is required; getMissingMandatory[Native]Providers() would be vacuously true + // if no providers were mandatory (thus, none are missing), which would cause an early return + // below without emitting the error message about the not-allowed rule class if that + // requirement was unfulfilled. + if (!attribute.getMandatoryNativeProviders().isEmpty() + || !attribute.getMandatoryProvidersList().isEmpty()) { + boolean hadAllMandatoryProviders = true; + + String missingNativeProviders = getMissingMandatoryNativeProviders(prerequisite, attribute); + if (missingNativeProviders != null) { + unfulfilledRequirements.add( + "'" + prerequisite.getLabel() + "' does not have mandatory providers: " + + missingNativeProviders); + hadAllMandatoryProviders = false; } - } - if (!attribute.getMandatoryProvidersList().isEmpty()) { - String missingMandatoryProviders = getMissingMandatoryProviders(prerequisite, attribute); - if (missingMandatoryProviders != null) { - attributeError( - attribute.getName(), + String missingProviders = getMissingMandatoryProviders(prerequisite, attribute); + if (missingProviders != null) { + unfulfilledRequirements.add( "'" + prerequisite.getLabel() + "' does not have mandatory provider " - + missingMandatoryProviders); + + missingProviders); + hadAllMandatoryProviders = false; + } + + if (hadAllMandatoryProviders) { + return; } - } else if (Boolean.FALSE.equals(allowed)) { - reportBadPrerequisite(attribute, prerequisiteTarget.getTargetKind(), prerequisite, - "expected " + attribute.getAllowedRuleClassesPredicate(), false); + } + + if (!unfulfilledRequirements.isEmpty()) { + attributeError( + attribute.getName(), StringUtil.joinEnglishList(unfulfilledRequirements, "and")); } } diff --git a/src/main/java/com/google/devtools/build/lib/packages/Attribute.java b/src/main/java/com/google/devtools/build/lib/packages/Attribute.java index 2ac485195d..c53b92af54 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/Attribute.java +++ b/src/main/java/com/google/devtools/build/lib/packages/Attribute.java @@ -1706,6 +1706,8 @@ public final class Attribute implements Comparable<Attribute> { builder.allowedFileTypesForLabels = allowedFileTypesForLabels; builder.allowedRuleClassesForLabels = allowedRuleClassesForLabels; builder.allowedRuleClassesForLabelsWarning = allowedRuleClassesForLabelsWarning; + builder.mandatoryNativeProviders = mandatoryNativeProviders; + builder.mandatoryProvidersList = mandatoryProvidersList; builder.validityPredicate = validityPredicate; builder.condition = condition; builder.configTransition = configTransition; diff --git a/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcRuleClasses.java b/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcRuleClasses.java index 81c8d9bf5e..e499a3b6cf 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcRuleClasses.java +++ b/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcRuleClasses.java @@ -682,15 +682,8 @@ public class ObjcRuleClasses { */ static final Iterable<String> ALLOWED_DEPS_RULE_CLASSES = ImmutableSet.of( - "objc_library", - "objc_import", - "objc_framework", - "objc_proto_library", - "j2objc_library", "cc_library", "cc_inc_library", - "ios_framework", - "swift_library", "experimental_objc_library"); @Override @@ -736,6 +729,7 @@ public class ObjcRuleClasses { attr("deps", LABEL_LIST) .direct_compile_time_input() .allowedRuleClasses(ALLOWED_DEPS_RULE_CLASSES) + .mandatoryNativeProviders(ObjcProvider.class) .allowedFileTypes()) /* <!-- #BLAZE_RULE($objc_compiling_rule).ATTRIBUTE(runtime_deps) --> The list of framework targets that are late loaded at runtime. They are included in the @@ -758,6 +752,7 @@ public class ObjcRuleClasses { attr("non_propagated_deps", LABEL_LIST) .direct_compile_time_input() .allowedRuleClasses(ALLOWED_DEPS_RULE_CLASSES) + .mandatoryNativeProviders(ObjcProvider.class) .allowedFileTypes()) /* <!-- #BLAZE_RULE($objc_compiling_rule).ATTRIBUTE(defines) --> Extra <code>-D</code> flags to pass to the compiler. They should be in |