diff options
author | Mark Schaller <mschaller@google.com> | 2015-07-07 22:29:28 +0000 |
---|---|---|
committer | Han-Wen Nienhuys <hanwen@google.com> | 2015-07-08 11:41:42 +0000 |
commit | 766e96f76a9c62fd1b6779588874a6329080c2c1 (patch) | |
tree | 3a294939c2492a8d928fe0c638b54a340ec4031c /src/main | |
parent | 6ab88033fc55a7344fcda528c2490ce0ea8eab93 (diff) |
Rollback of commit 4dbd628dc1384e5dce8c036e2b6bf93dd974bf04.
[]*** Reason for rollback ***
Large number of newly broken targets found by []
*** Original change description ***
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=97716849
Diffstat (limited to 'src/main')
7 files changed, 35 insertions, 165 deletions
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 1a52b5ed20..8afcbb7d55 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().deserialize(in); + deserializedPkg = new PackageDeserializer(null, null).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. */ - public Label getBuildFileLabel() { + 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 d5a612efa5..e4bc250333 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,6 +34,7 @@ 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; @@ -54,23 +55,11 @@ public class PackageDeserializer { private static final Logger LOG = Logger.getLogger(PackageDeserializer.class.getName()); - /** - * 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. + // Workaround for Java serialization not allowing to pass in a context manually. // volatile is needed to ensure that the objects are published safely. - // TODO(bazel-team): Subclass ObjectOutputStream to pass this through instead. - public static volatile PackageDeserializationEnvironment defaultPackageDeserializationEnvironment; + // TODO(bazel-team): Subclass ObjectOutputStream to pass through environment variables. + public static volatile RuleClassProvider defaultRuleClassProvider; + public static volatile FileSystem defaultDeserializerFileSystem; private class Context { private final Package.Builder packageBuilder; @@ -131,9 +120,15 @@ public class PackageDeserializer { } } - void deserializeRule(Build.Rule rulePb) throws PackageDeserializationException { - Location ruleLocation = deserializeLocation(rulePb.getParseableLocation()); - RuleClass ruleClass = packageDeserializationEnvironment.getRuleClass(rulePb, ruleLocation); + 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)); + } + Map<String, ParsedAttributeValue> attributeValues = new HashMap<>(); for (Build.Attribute attrPb : rulePb.getAttributeList()) { Type<?> type = ruleClass.getAttributeByName(attrPb.getName()).getType(); @@ -141,6 +136,7 @@ public class PackageDeserializer { } Label ruleLabel = deserializeLabel(rulePb.getName()); + Location ruleLocation = deserializeLocation(rulePb.getParseableLocation()); try { Rule rule = ruleClass.createRuleWithParsedAttributeValues( ruleLabel, packageBuilder, ruleLocation, attributeValues, @@ -154,7 +150,8 @@ public class PackageDeserializer { } } - private final PackageDeserializationEnvironment packageDeserializationEnvironment; + private final FileSystem fileSystem; + private final RuleClassProvider ruleClassProvider; @Immutable private static final class ExplicitLocation extends Location { @@ -199,16 +196,15 @@ public class PackageDeserializer { } } - /** - * Creates a {@link PackageDeserializer} using {@link #defaultPackageDeserializationEnvironment}. - */ - public PackageDeserializer() { - this.packageDeserializationEnvironment = defaultPackageDeserializationEnvironment; - } - - public PackageDeserializer(PackageDeserializationEnvironment packageDeserializationEnvironment) { - this.packageDeserializationEnvironment = - Preconditions.checkNotNull(packageDeserializationEnvironment); + 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); } /** @@ -314,7 +310,7 @@ public class PackageDeserializer { */ private void deserializeInternal(Build.Package packagePb, StoredEventHandler eventHandler, Package.Builder builder, InputStream in) throws PackageDeserializationException, IOException { - Path buildFile = packageDeserializationEnvironment.getPath(packagePb.getBuildFilePath()); + Path buildFile = fileSystem.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 4f1a7a3008..16b318cb62 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,7 +158,6 @@ 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) { @@ -166,7 +165,6 @@ 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<Integer>) value) { @@ -395,7 +393,6 @@ 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 b566ba7ba4..653e283e84 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,17 +35,11 @@ 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.Set; +import java.util.Map; /** * Shared code used in proto buffer output for rules and rule classes. @@ -54,13 +48,9 @@ public class ProtoUtils { /** * This map contains all attribute types that are recognized by the protocol * output formatter. - * - * <p>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). */ - static final ImmutableMap<Type<?>, Discriminator> TYPE_MAP = - new ImmutableMap.Builder<Type<?>, Discriminator>() + private static final Map<Type<?>, Discriminator> TYPE_MAP + = new ImmutableMap.Builder<Type<?>, Discriminator>() .put(INTEGER, Discriminator.INTEGER) .put(DISTRIBUTIONS, Discriminator.DISTRIBUTION_SET) .put(LABEL, Discriminator.LABEL) @@ -85,71 +75,11 @@ public class ProtoUtils { .put(STRING_DICT_UNARY, Discriminator.STRING_DICT_UNARY) .build(); - static final ImmutableSet<Type<?>> NODEP_TYPES = - ImmutableSet.of(NODEP_LABEL, NODEP_LABEL_LIST); - - static final ImmutableSetMultimap<Discriminator, Type<?>> 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), type); + Preconditions.checkArgument(TYPE_MAP.containsKey(type)); return TYPE_MAP.get(type); } - - /** - * Returns the appropriate set of internal attribute types from an Attribute.Discriminator value. - */ - public static ImmutableSet<Type<?>> 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<Boolean> nodeps, String ruleClassName, String attrName) { - Preconditions.checkArgument(INVERSE_TYPE_MAP.containsKey(discriminator)); - ImmutableSet<Type<?>> 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 Type<?>s 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 - // Type<?>s {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<Type<?>> 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<Type<?>> 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 e7cbf9edec..0286796470 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,7 +31,6 @@ 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; @@ -340,29 +339,6 @@ 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<String, Attribute> 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. - } }; /** @@ -714,11 +690,6 @@ 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. */ @@ -818,20 +789,6 @@ public final class RuleClass { } } - public static Builder createPlaceholderBuilder(final String name, final Location ruleLocation, - ImmutableList<RuleClass> parents) { - return new Builder(name, RuleClassType.PLACEHOLDER, /*skylark=*/true, - parents.toArray(new RuleClass[parents.size()])).factory( - new ConfiguredTargetFactory<Object, Object>() { - @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 f39d2922fd..1cb405f2a6 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,15 +118,13 @@ public class SkylarkRuleClassFunctions { }); // TODO(bazel-team): Remove the code duplication (BaseRuleClasses and this class). - /** Parent rule class for non-test Skylark rules. */ - public static final RuleClass baseRule = + private static final RuleClass baseRule = BaseRuleClasses.commonCoreAndSkylarkAttributes( new RuleClass.Builder("$base_rule", RuleClassType.ABSTRACT, true)) .add(attr("expect_failure", STRING)) .build(); - /** Parent rule class for test Skylark rules. */ - public static final RuleClass testBaseRule = + private 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 9fb59b5c6d..6780d4d844 100644 --- a/src/main/protobuf/build.proto +++ b/src/main/protobuf/build.proto @@ -136,11 +136,6 @@ 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. @@ -236,9 +231,6 @@ 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 |