Skylark rules can now declare their required configuration fragments

--
MOS_MIGRATED_REVID=100163482

5 files changed, 180 insertions, 13 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfiguration.java b/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfiguration.java
index 71f5175bb8..b2e9f1a4dd 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfiguration.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfiguration.java
@@ -49,6 +49,7 @@ import com.google.devtools.build.lib.syntax.Label;
 import com.google.devtools.build.lib.syntax.Label.SyntaxException;
 import com.google.devtools.build.lib.syntax.SkylarkCallable;
 import com.google.devtools.build.lib.syntax.SkylarkModule;
+import com.google.devtools.build.lib.syntax.SkylarkModuleNameResolver;
 import com.google.devtools.build.lib.util.CPU;
 import com.google.devtools.build.lib.util.Fingerprint;
 import com.google.devtools.build.lib.util.OS;
@@ -1121,11 +1122,12 @@ public final class BuildConfiguration {
 
   private ImmutableMap<String, Class<? extends Fragment>> buildIndexOfVisibleFragments() {
     ImmutableMap.Builder<String, Class<? extends Fragment>> builder = ImmutableMap.builder();
+    SkylarkModuleNameResolver resolver = new SkylarkModuleNameResolver();
 
     for (Class<? extends Fragment> fragmentClass : fragments.keySet()) {
-      SkylarkModule annotation = fragmentClass.getAnnotation(SkylarkModule.class);
-      if (annotation != null) {
-        builder.put(annotation.name(), fragmentClass);
+      String name = resolver.resolveName(fragmentClass);
+      if (name != null) {
+        builder.put(name, fragmentClass);
       }
     }
     return builder.build();
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 2a48d87f42..67cc9359fb 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
@@ -35,6 +35,7 @@ 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;
+import com.google.devtools.build.lib.syntax.FragmentClassNameResolver;
 import com.google.devtools.build.lib.syntax.FuncallExpression;
 import com.google.devtools.build.lib.syntax.GlobList;
 import com.google.devtools.build.lib.syntax.Label;
@@ -489,6 +490,9 @@ public final class RuleClass {
     private SkylarkEnvironment ruleDefinitionEnvironment = null;
     private Set<Class<?>> configurationFragments = new LinkedHashSet<>();
     private MissingFragmentPolicy missingFragmentPolicy = MissingFragmentPolicy.FAIL_ANALYSIS;
+    private Set<String> requiredFragmentNames = new LinkedHashSet<>();
+    private FragmentClassNameResolver fragmentNameResolver;
+
     private boolean supportsConstraintChecking = true;
 
     private final Map<String, Attribute> attributes = new LinkedHashMap<>();
@@ -574,8 +578,8 @@ public final class RuleClass {
           workspaceOnly, outputsDefaultExecutable, implicitOutputsFunction, configurator,
           configuredTargetFactory, validityPredicate, preferredDependencyPredicate,
           ImmutableSet.copyOf(advertisedProviders), configuredTargetFunction,
-          externalBindingsFunction,
-          ruleDefinitionEnvironment, configurationFragments, missingFragmentPolicy,
+          externalBindingsFunction, ruleDefinitionEnvironment, configurationFragments,
+          requiredFragmentNames, fragmentNameResolver, missingFragmentPolicy,
           supportsConstraintChecking, attributes.values().toArray(new Attribute[0]));
     }
 
@@ -600,6 +604,17 @@ public final class RuleClass {
       this.missingFragmentPolicy = missingFragmentPolicy;
       return this;
     }
+    
+    /**
+     * Does the same as {@link #requiresConfigurationFragments(Class...)}, except for taking names
+     * of fragments instead of classes.
+     */
+    public Builder requiresConfigurationFragments(
+        FragmentClassNameResolver fragmentNameResolver, String... configurationFragmentNames) {
+      Collections.addAll(requiredFragmentNames, configurationFragmentNames);
+      this.fragmentNameResolver = fragmentNameResolver;
+      return this;
+    }
 
     public Builder setUndocumented() {
       documented = false;
@@ -948,6 +963,17 @@ public final class RuleClass {
   private final ImmutableSet<Class<?>> requiredConfigurationFragments;
 
   /**
+   * Same idea as requiredConfigurationFragments, but stores fragments by name instead of by class
+   */
+  private final ImmutableSet<String> requiredConfigurationFragmentNames;
+  
+  /**
+   * Used to resolve the names of fragments in order to compare them to values in {@link
+   * #requiredConfigurationFragmentNames}
+   */
+  private final FragmentClassNameResolver fragmentNameResolver;
+  
+  /**
    * What to do during analysis if a configuration fragment is missing.
    */
   private final MissingFragmentPolicy missingFragmentPolicy;
@@ -960,6 +986,53 @@ public final class RuleClass {
   private final boolean supportsConstraintChecking;
 
   /**
+   * Helper constructor that skips allowedConfigurationFragmentNames and fragmentNameResolver
+   */
+  @VisibleForTesting
+  RuleClass(String name,
+      boolean skylarkExecutable,
+      boolean documented,
+      boolean publicByDefault,
+      boolean binaryOutput,
+      boolean workspaceOnly,
+      boolean outputsDefaultExecutable,
+      ImplicitOutputsFunction implicitOutputsFunction,
+      Configurator<?, ?> configurator,
+      ConfiguredTargetFactory<?, ?> configuredTargetFactory,
+      PredicateWithMessage<Rule> validityPredicate,
+      Predicate<String> preferredDependencyPredicate,
+      ImmutableSet<Class<?>> advertisedProviders,
+      @Nullable BaseFunction configuredTargetFunction,
+      Function<? super Rule, Map<String, Label>> externalBindingsFunction,
+      @Nullable SkylarkEnvironment ruleDefinitionEnvironment,
+      Set<Class<?>> allowedConfigurationFragments,
+      MissingFragmentPolicy missingFragmentPolicy,
+      boolean supportsConstraintChecking,
+      Attribute... attributes) {
+    this(name,
+        skylarkExecutable,
+        documented,
+        publicByDefault,
+        binaryOutput,
+        workspaceOnly,
+        outputsDefaultExecutable,
+        implicitOutputsFunction,
+        configurator,
+        configuredTargetFactory,
+        validityPredicate,
+        preferredDependencyPredicate,
+        advertisedProviders,
+        configuredTargetFunction,
+        externalBindingsFunction,
+        ruleDefinitionEnvironment,
+        allowedConfigurationFragments,
+        ImmutableSet.<String>of(), null,
+        missingFragmentPolicy,
+        supportsConstraintChecking,
+        attributes);
+  }
+
+  /**
    * Constructs an instance of RuleClass whose name is 'name', attributes
    * are 'attributes'. The {@code srcsAllowedFiles} determines which types of
    * files are allowed as parameters to the "srcs" attribute; rules are always
@@ -993,7 +1066,10 @@ public final class RuleClass {
       @Nullable BaseFunction configuredTargetFunction,
       Function<? super Rule, Map<String, Label>> externalBindingsFunction,
       @Nullable SkylarkEnvironment ruleDefinitionEnvironment,
-      Set<Class<?>> allowedConfigurationFragments, MissingFragmentPolicy missingFragmentPolicy,
+      Set<Class<?>> allowedConfigurationFragments,
+      Set<String> allowedConfigurationFragmentNames,
+      @Nullable FragmentClassNameResolver fragmentNameResolver,
+      MissingFragmentPolicy missingFragmentPolicy,
       boolean supportsConstraintChecking,
       Attribute... attributes) {
     this.name = name;
@@ -1016,6 +1092,9 @@ public final class RuleClass {
     this.workspaceOnly = workspaceOnly;
     this.outputsDefaultExecutable = outputsDefaultExecutable;
     this.requiredConfigurationFragments = ImmutableSet.copyOf(allowedConfigurationFragments);
+    this.requiredConfigurationFragmentNames =
+        ImmutableSet.copyOf(allowedConfigurationFragmentNames);
+    this.fragmentNameResolver = fragmentNameResolver;
     this.missingFragmentPolicy = missingFragmentPolicy;
     this.supportsConstraintChecking = supportsConstraintChecking;
 
@@ -1177,10 +1256,23 @@ public final class RuleClass {
   public boolean isLegalConfigurationFragment(Class<?> configurationFragment) {
     // For now, we allow all rules that don't declare allowed fragments to access any fragment.
     // TODO(bazel-team): All built-in rules declare fragments, but Skylark rules don't.
-    if (requiredConfigurationFragments.isEmpty()) {
+    if (requiredConfigurationFragments.isEmpty() && requiredConfigurationFragmentNames.isEmpty()) {
       return true;
     }
-    return requiredConfigurationFragments.contains(configurationFragment);
+    return requiredConfigurationFragments.contains(configurationFragment)
+        || hasLegalFragmentName(configurationFragment);
+  }
+
+  /**
+   * Checks whether the name of the given fragment class was declared as required fragment
+   */
+  private boolean hasLegalFragmentName(Class<?> configurationFragment) {
+    if (fragmentNameResolver == null) {
+      return false;
+    }
+
+    String name = fragmentNameResolver.resolveName(configurationFragment);
+    return (name != null && requiredConfigurationFragmentNames.contains(name));
   }
 
   /**
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 a224805da9..e128d7eec7 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
@@ -33,6 +33,7 @@ import com.google.common.cache.CacheLoader;
 import com.google.common.cache.LoadingCache;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.Iterables;
 import com.google.devtools.build.lib.analysis.BaseRuleClasses;
 import com.google.devtools.build.lib.analysis.config.BuildConfiguration;
 import com.google.devtools.build.lib.analysis.config.RunUnder;
@@ -70,6 +71,7 @@ import com.google.devtools.build.lib.syntax.Label;
 import com.google.devtools.build.lib.syntax.SkylarkCallbackFunction;
 import com.google.devtools.build.lib.syntax.SkylarkEnvironment;
 import com.google.devtools.build.lib.syntax.SkylarkList;
+import com.google.devtools.build.lib.syntax.SkylarkModuleNameResolver;
 import com.google.devtools.build.lib.syntax.SkylarkSignature;
 import com.google.devtools.build.lib.syntax.SkylarkSignature.Param;
 import com.google.devtools.build.lib.syntax.SkylarkSignatureProcessor;
@@ -220,15 +222,18 @@ public class SkylarkRuleClassFunctions {
             + "there must be an action that generates <code>ctx.outputs.executable</code>."),
         @Param(name = "output_to_genfiles", type = Boolean.class, defaultValue = "False",
             doc = "If true, the files will be generated in the genfiles directory instead of the "
-            + "bin directory. This is used for compatibility with existing rules.")},
+            + "bin directory. This is used for compatibility with existing rules."),
+        @Param(name = "fragments", type = SkylarkList.class, generic1 = String.class,
+           defaultValue = "[]",
+           doc = "List of names of configuration fragments that the rule requires.")},
       useAst = true, useEnvironment = true)
   private static final BuiltinFunction rule = new BuiltinFunction("rule") {
       @SuppressWarnings({"rawtypes", "unchecked"}) // castMap produces
       // an Attribute.Builder instead of a Attribute.Builder<?> but it's OK.
-      public BaseFunction invoke(BaseFunction implementation, Boolean test,
-          Object attrs, Object implicitOutputs, Boolean executable, Boolean outputToGenfiles,
-          FuncallExpression ast, Environment funcallEnv)
-           throws EvalException, ConversionException {
+      public BaseFunction invoke(BaseFunction implementation, Boolean test, Object attrs,
+          Object implicitOutputs, Boolean executable, Boolean outputToGenfiles,
+          SkylarkList fragments, FuncallExpression ast, Environment funcallEnv)
+          throws EvalException, ConversionException {
 
         Preconditions.checkState(funcallEnv instanceof SkylarkEnvironment);
         RuleClassType type = test ? RuleClassType.TEST : RuleClassType.NORMAL;
@@ -273,6 +278,12 @@ public class SkylarkRuleClassFunctions {
           builder.setOutputToGenfiles();
         }
 
+        if (!fragments.isEmpty()) {
+          builder.requiresConfigurationFragments(
+              new SkylarkModuleNameResolver(),
+              Iterables.toArray(castList(fragments, String.class), String.class));
+        }
+
         builder.setConfiguredTargetFunction(implementation);
         builder.setRuleDefinitionEnvironment(
             ((SkylarkEnvironment) funcallEnv).getGlobalEnvironment());
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/FragmentClassNameResolver.java b/src/main/java/com/google/devtools/build/lib/syntax/FragmentClassNameResolver.java
new file mode 100644
index 0000000000..c49a0a9ab4
--- /dev/null
+++ b/src/main/java/com/google/devtools/build/lib/syntax/FragmentClassNameResolver.java
@@ -0,0 +1,33 @@
+// Copyright 2015 Google Inc. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//    http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+package com.google.devtools.build.lib.syntax;
+
+import javax.annotation.Nullable;
+
+/**
+ * Interface for retrieving the name of a {@link
+ * com.google.devtools.build.lib.analysis.config.BuildConfiguration.Fragment} based on its
+ * class.
+ * 
+ * <p>Classes implementing this specific interface are required when doing look up operations and
+ * comparisons of configuration fragments.
+ */
+public interface FragmentClassNameResolver {
+  /**
+   * Returns the name of the configuration fragment specified by the given class or null, if this is
+   * not possible.
+   */
+  @Nullable
+  String resolveName(Class<?> clazz);
+}
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkModuleNameResolver.java b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkModuleNameResolver.java
new file mode 100644
index 0000000000..d5aa9bfa1a
--- /dev/null
+++ b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkModuleNameResolver.java
@@ -0,0 +1,29 @@
+// Copyright 2015 Google Inc. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//    http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+package com.google.devtools.build.lib.syntax;
+
+/**
+ * Helper class that resolves the name of classes with SkylarkModule annotation.
+ */
+public final class SkylarkModuleNameResolver implements FragmentClassNameResolver {
+  /**
+   * Returns the Skylark name of the given class or null, if the SkylarkModule annotation is not
+   * present.
+   */
+  @Override
+  public String resolveName(Class<?> clazz) {
+    SkylarkModule annotation = clazz.getAnnotation(SkylarkModule.class);
+    return (annotation == null) ? null : annotation.name();
+  }
+}