From 4dbd628dc1384e5dce8c036e2b6bf93dd974bf04 Mon Sep 17 00:00:00 2001 From: Mark Schaller Date: Tue, 7 Jul 2015 16:42:48 +0000 Subject: Provide placeholder rule class for deserialized Skylark rules At this time, Skylark-defined rule classes don't get serialized, and aren't available at package deserialization time. To allow packages with Skylark-defined rule classes to deserialize, we provide a placeholder rule class implementation for deserialized Skylark rules. -- MOS_MIGRATED_REVID=97679963 --- .../devtools/build/lib/packages/Package.java | 4 +- .../build/lib/packages/PackageDeserializer.java | 58 ++++++++-------- .../build/lib/packages/PackageSerializer.java | 3 + .../devtools/build/lib/packages/ProtoUtils.java | 78 ++++++++++++++++++++-- .../devtools/build/lib/packages/RuleClass.java | 43 ++++++++++++ .../build/lib/rules/SkylarkRuleClassFunctions.java | 6 +- src/main/protobuf/build.proto | 8 +++ 7 files changed, 165 insertions(+), 35 deletions(-) (limited to 'src') diff --git a/src/main/java/com/google/devtools/build/lib/packages/Package.java b/src/main/java/com/google/devtools/build/lib/packages/Package.java index 8afcbb7d55..1a52b5ed20 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/Package.java +++ b/src/main/java/com/google/devtools/build/lib/packages/Package.java @@ -241,7 +241,7 @@ public class Package implements Serializable { private void readObject(ObjectInputStream in) throws IOException { try { - deserializedPkg = new PackageDeserializer(null, null).deserialize(in); + deserializedPkg = new PackageDeserializer().deserialize(in); } catch (PackageDeserializationException e) { throw new IllegalStateException(e); } @@ -450,7 +450,7 @@ public class Package implements Serializable { * though not necessarily: data in a subdirectory of a test package may use a * different filename to avoid inadvertently creating a new package. */ - Label getBuildFileLabel() { + public Label getBuildFileLabel() { return buildFile.getLabel(); } diff --git a/src/main/java/com/google/devtools/build/lib/packages/PackageDeserializer.java b/src/main/java/com/google/devtools/build/lib/packages/PackageDeserializer.java index e4bc250333..d5a612efa5 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/PackageDeserializer.java +++ b/src/main/java/com/google/devtools/build/lib/packages/PackageDeserializer.java @@ -34,7 +34,6 @@ import com.google.devtools.build.lib.syntax.GlobCriteria; import com.google.devtools.build.lib.syntax.GlobList; import com.google.devtools.build.lib.syntax.Label; import com.google.devtools.build.lib.syntax.Label.SyntaxException; -import com.google.devtools.build.lib.vfs.FileSystem; import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.PathFragment; @@ -55,11 +54,23 @@ public class PackageDeserializer { private static final Logger LOG = Logger.getLogger(PackageDeserializer.class.getName()); - // Workaround for Java serialization not allowing to pass in a context manually. + /** + * Provides the deserializer with tools it needs to build a package from its serialized form. + */ + public interface PackageDeserializationEnvironment { + + /** Converts the serialized package's path string into a {@link Path} object. */ + Path getPath(String buildFilePath); + + /** Returns a {@link RuleClass} object for the serialized rule. */ + RuleClass getRuleClass(Build.Rule rulePb, Location ruleLocation); + } + + // Workaround for Java serialization making it tough to pass in a deserialization environment + // manually. // volatile is needed to ensure that the objects are published safely. - // TODO(bazel-team): Subclass ObjectOutputStream to pass through environment variables. - public static volatile RuleClassProvider defaultRuleClassProvider; - public static volatile FileSystem defaultDeserializerFileSystem; + // TODO(bazel-team): Subclass ObjectOutputStream to pass this through instead. + public static volatile PackageDeserializationEnvironment defaultPackageDeserializationEnvironment; private class Context { private final Package.Builder packageBuilder; @@ -120,15 +131,9 @@ public class PackageDeserializer { } } - void deserializeRule(Build.Rule rulePb) - throws PackageDeserializationException { - String ruleClassName = rulePb.getRuleClass(); - RuleClass ruleClass = ruleClassProvider.getRuleClassMap().get(ruleClassName); - if (ruleClass == null) { - throw new PackageDeserializationException( - String.format("Invalid rule class '%s'", ruleClassName)); - } - + void deserializeRule(Build.Rule rulePb) throws PackageDeserializationException { + Location ruleLocation = deserializeLocation(rulePb.getParseableLocation()); + RuleClass ruleClass = packageDeserializationEnvironment.getRuleClass(rulePb, ruleLocation); Map attributeValues = new HashMap<>(); for (Build.Attribute attrPb : rulePb.getAttributeList()) { Type type = ruleClass.getAttributeByName(attrPb.getName()).getType(); @@ -136,7 +141,6 @@ public class PackageDeserializer { } Label ruleLabel = deserializeLabel(rulePb.getName()); - Location ruleLocation = deserializeLocation(rulePb.getParseableLocation()); try { Rule rule = ruleClass.createRuleWithParsedAttributeValues( ruleLabel, packageBuilder, ruleLocation, attributeValues, @@ -150,8 +154,7 @@ public class PackageDeserializer { } } - private final FileSystem fileSystem; - private final RuleClassProvider ruleClassProvider; + private final PackageDeserializationEnvironment packageDeserializationEnvironment; @Immutable private static final class ExplicitLocation extends Location { @@ -196,15 +199,16 @@ public class PackageDeserializer { } } - public PackageDeserializer(FileSystem fileSystem, RuleClassProvider ruleClassProvider) { - if (fileSystem == null) { - fileSystem = defaultDeserializerFileSystem; - } - this.fileSystem = Preconditions.checkNotNull(fileSystem); - if (ruleClassProvider == null) { - ruleClassProvider = defaultRuleClassProvider; - } - this.ruleClassProvider = Preconditions.checkNotNull(ruleClassProvider); + /** + * Creates a {@link PackageDeserializer} using {@link #defaultPackageDeserializationEnvironment}. + */ + public PackageDeserializer() { + this.packageDeserializationEnvironment = defaultPackageDeserializationEnvironment; + } + + public PackageDeserializer(PackageDeserializationEnvironment packageDeserializationEnvironment) { + this.packageDeserializationEnvironment = + Preconditions.checkNotNull(packageDeserializationEnvironment); } /** @@ -310,7 +314,7 @@ public class PackageDeserializer { */ private void deserializeInternal(Build.Package packagePb, StoredEventHandler eventHandler, Package.Builder builder, InputStream in) throws PackageDeserializationException, IOException { - Path buildFile = fileSystem.getPath(packagePb.getBuildFilePath()); + Path buildFile = packageDeserializationEnvironment.getPath(packagePb.getBuildFilePath()); Preconditions.checkNotNull(buildFile); Context context = new Context(buildFile, builder); builder.setFilename(buildFile); diff --git a/src/main/java/com/google/devtools/build/lib/packages/PackageSerializer.java b/src/main/java/com/google/devtools/build/lib/packages/PackageSerializer.java index 16b318cb62..4f1a7a3008 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/PackageSerializer.java +++ b/src/main/java/com/google/devtools/build/lib/packages/PackageSerializer.java @@ -158,6 +158,7 @@ public class PackageSerializer { if (singleAttributeValue != null) { attrPb.setStringValue(singleAttributeValue.toString()); } + attrPb.setNodep(type == NODEP_LABEL); } else if (type == STRING_LIST || type == LABEL_LIST || type == NODEP_LABEL_LIST || type == OUTPUT_LIST || type == DISTRIBUTIONS) { for (Object value : values) { @@ -165,6 +166,7 @@ public class PackageSerializer { attrPb.addStringListValue(entry.toString()); } } + attrPb.setNodep(type == NODEP_LABEL_LIST); } else if (type == INTEGER_LIST) { for (Object value : values) { for (Integer entry : (Collection) value) { @@ -393,6 +395,7 @@ public class PackageSerializer { builder.setName(rule.getLabel().toString()); builder.setRuleClass(rule.getRuleClass()); builder.setParseableLocation(serializeLocation(rule.getLocation())); + builder.setPublicByDefault(rule.getRuleClassObject().isPublicByDefault()); for (Attribute attribute : rule.getAttributes()) { PackageSerializer.addAttributeToProto(builder, attribute, getAttributeValues(rule, attribute), rule.getAttributeLocation(attribute.getName()), diff --git a/src/main/java/com/google/devtools/build/lib/packages/ProtoUtils.java b/src/main/java/com/google/devtools/build/lib/packages/ProtoUtils.java index 653e283e84..b566ba7ba4 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/ProtoUtils.java +++ b/src/main/java/com/google/devtools/build/lib/packages/ProtoUtils.java @@ -35,11 +35,17 @@ import static com.google.devtools.build.lib.packages.Type.STRING_LIST; import static com.google.devtools.build.lib.packages.Type.STRING_LIST_DICT; import static com.google.devtools.build.lib.packages.Type.TRISTATE; +import com.google.common.base.Optional; import com.google.common.base.Preconditions; +import com.google.common.base.Predicates; import com.google.common.collect.ImmutableMap; +import com.google.common.collect.ImmutableSet; +import com.google.common.collect.ImmutableSetMultimap; +import com.google.common.collect.Iterables; +import com.google.common.collect.Sets; import com.google.devtools.build.lib.query2.proto.proto2api.Build.Attribute.Discriminator; -import java.util.Map; +import java.util.Set; /** * Shared code used in proto buffer output for rules and rule classes. @@ -48,9 +54,13 @@ public class ProtoUtils { /** * This map contains all attribute types that are recognized by the protocol * output formatter. + * + *

If you modify this map, please ensure that {@link #getTypeFromDiscriminator} can still + * resolve a {@link Discriminator} value to exactly one {@link Type} (using an optional nodep + * hint, as described below). */ - private static final Map, Discriminator> TYPE_MAP - = new ImmutableMap.Builder, Discriminator>() + static final ImmutableMap, Discriminator> TYPE_MAP = + new ImmutableMap.Builder, Discriminator>() .put(INTEGER, Discriminator.INTEGER) .put(DISTRIBUTIONS, Discriminator.DISTRIBUTION_SET) .put(LABEL, Discriminator.LABEL) @@ -75,11 +85,71 @@ public class ProtoUtils { .put(STRING_DICT_UNARY, Discriminator.STRING_DICT_UNARY) .build(); + static final ImmutableSet> NODEP_TYPES = + ImmutableSet.of(NODEP_LABEL, NODEP_LABEL_LIST); + + static final ImmutableSetMultimap> INVERSE_TYPE_MAP = + TYPE_MAP.asMultimap().inverse(); + /** * Returns the appropriate Attribute.Discriminator value from an internal attribute type. */ public static Discriminator getDiscriminatorFromType(Type type) { - Preconditions.checkArgument(TYPE_MAP.containsKey(type)); + Preconditions.checkArgument(TYPE_MAP.containsKey(type), type); return TYPE_MAP.get(type); } + + /** + * Returns the appropriate set of internal attribute types from an Attribute.Discriminator value. + */ + public static ImmutableSet> getTypesFromDiscriminator(Discriminator discriminator) { + Preconditions.checkArgument(INVERSE_TYPE_MAP.containsKey(discriminator), discriminator); + return INVERSE_TYPE_MAP.get(discriminator); + } + + /** + * Returns the appropriate internal attribute type from an Attribute.Discriminator value, given + * an optional nodeps hint. + */ + public static Type getTypeFromDiscriminator(Discriminator discriminator, + Optional nodeps, String ruleClassName, String attrName) { + Preconditions.checkArgument(INVERSE_TYPE_MAP.containsKey(discriminator)); + ImmutableSet> possibleTypes = ProtoUtils.getTypesFromDiscriminator(discriminator); + Type preciseType; + if (possibleTypes.size() == 1) { + preciseType = Iterables.getOnlyElement(possibleTypes); + } else { + // If there is more than one possible type associated with the discriminator, then the + // discriminator must be either Discriminator.STRING or Discriminator.STRING_LIST. + // + // If it is Discriminator.STRING, then its possible Types are {NODEP_LABEL, STRING}. The + // nodeps hint must be present in order to distinguish between them. If nodeps is true, + // then the Type must be NODEP_LABEL, and if false, it must be STRING. + // + // A similar relation holds for the Discriminator value STRING_LIST, and its possible + // Types {NODEP_LABEL_LIST, STRING_LIST}. + + Preconditions.checkArgument(nodeps.isPresent(), + "Nodeps hint is required when discriminator is associated with more than one type." + + " Discriminator: \"%s\", Rule class: \"%s\", Attr: \"%s\"", discriminator, + ruleClassName, attrName); + if (nodeps.get()) { + Set> nodepType = Sets.filter(possibleTypes, Predicates.in(NODEP_TYPES)); + Preconditions.checkState(nodepType.size() == 1, + "There should be exactly one NODEP type associated with discriminator \"%s\"" + + ", but found these: %s. Rule class: \"%s\", Attr: \"%s\"", discriminator, + nodepType, ruleClassName, attrName); + preciseType = Iterables.getOnlyElement(nodepType); + } else { + Set> notNodepType = + Sets.filter(possibleTypes, Predicates.not(Predicates.in(NODEP_TYPES))); + Preconditions.checkState(notNodepType.size() == 1, + "There should be exactly one non-NODEP type associated with discriminator \"%s\"" + + ", but found these: %s. Rule class: \"%s\", Attr: \"%s\"", discriminator, + notNodepType, ruleClassName, attrName); + preciseType = Iterables.getOnlyElement(notNodepType); + } + } + return preciseType; + } } 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 0286796470..e7cbf9edec 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 @@ -31,6 +31,7 @@ import com.google.common.collect.ImmutableSet; import com.google.common.collect.Ordering; import com.google.devtools.build.lib.events.EventHandler; import com.google.devtools.build.lib.events.Location; +import com.google.devtools.build.lib.packages.RuleClass.Builder.RuleClassType; import com.google.devtools.build.lib.syntax.Argument; import com.google.devtools.build.lib.syntax.BaseFunction; import com.google.devtools.build.lib.syntax.Environment; @@ -339,6 +340,29 @@ public final class RuleClass { attribute.getName(), attribute.getType()); } } + }, + + /** + * Placeholder rules are only instantiated when packages which refer to non-native rule + * classes are deserialized. At this time, non-native rule classes can't be serialized. To + * prevent crashes on deserialization, when a package containing a rule with a non-native rule + * class is deserialized, the rule is assigned a placeholder rule class. This is compatible + * with our limited set of package serialization use cases. + * + * Placeholder rule class names obey the rule for identifiers. + */ + PLACEHOLDER { + @Override + public void checkName(String name) { + Preconditions.checkArgument(RULE_NAME_PATTERN.matcher(name).matches(), name); + } + + @Override + public void checkAttributes(Map attributes) { + // No required attributes; this rule class cannot have the wrong set of attributes now + // because, if it did, the rule class would have failed to build before the package + // referring to it was serialized. + } }; /** @@ -690,6 +714,11 @@ public final class RuleClass { } } + /** True if the rule class contains an attribute named {@code name}. */ + public boolean contains(String name) { + return attributes.containsKey(name); + } + /** * Sets the rule implementation function. Meant for Skylark usage. */ @@ -789,6 +818,20 @@ public final class RuleClass { } } + public static Builder createPlaceholderBuilder(final String name, final Location ruleLocation, + ImmutableList parents) { + return new Builder(name, RuleClassType.PLACEHOLDER, /*skylark=*/true, + parents.toArray(new RuleClass[parents.size()])).factory( + new ConfiguredTargetFactory() { + @Override + public Object create(Object ruleContext) throws InterruptedException { + throw new IllegalStateException( + "Cannot create configured targets from rule with placeholder class named \"" + name + + "\" at " + ruleLocation); + } + }); + } + private final String name; // e.g. "cc_library" /** diff --git a/src/main/java/com/google/devtools/build/lib/rules/SkylarkRuleClassFunctions.java b/src/main/java/com/google/devtools/build/lib/rules/SkylarkRuleClassFunctions.java index 924bd04c9c..55c65ee4e3 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/SkylarkRuleClassFunctions.java +++ b/src/main/java/com/google/devtools/build/lib/rules/SkylarkRuleClassFunctions.java @@ -118,13 +118,15 @@ public class SkylarkRuleClassFunctions { }); // TODO(bazel-team): Remove the code duplication (BaseRuleClasses and this class). - private static final RuleClass baseRule = + /** Parent rule class for non-test Skylark rules. */ + public static final RuleClass baseRule = BaseRuleClasses.commonCoreAndSkylarkAttributes( new RuleClass.Builder("$base_rule", RuleClassType.ABSTRACT, true)) .add(attr("expect_failure", STRING)) .build(); - private static final RuleClass testBaseRule = + /** Parent rule class for test Skylark rules. */ + public static final RuleClass testBaseRule = new RuleClass.Builder("$test_base_rule", RuleClassType.ABSTRACT, true, baseRule) .add(attr("size", STRING).value("medium").taggable() .nonconfigurable("used in loading phase rule validation logic")) diff --git a/src/main/protobuf/build.proto b/src/main/protobuf/build.proto index 6780d4d844..9fb59b5c6d 100644 --- a/src/main/protobuf/build.proto +++ b/src/main/protobuf/build.proto @@ -136,6 +136,11 @@ message Attribute { // Whether the attribute was explicitly specified optional bool explicitly_specified = 13; + // If this attribute has a string value or a string list value, then this + // may be set to indicate that the value may be treated as a label that + // isn't a dependency of this attribute's rule. + optional bool nodep = 20; + // The type of attribute. This message is used for all of the different // attribute types so the discriminator helps for figuring out what is // stored in the message. @@ -231,6 +236,9 @@ message Rule { // The location of the target in the BUILD file in a machine-parseable form. optional Location parseable_location = 8; + + // The rule's class's public by default value. + optional bool public_by_default = 9; } // Summary of all transitive dependencies of 'rule,' where each dependent -- cgit v1.2.3