Clean up GenRuleBase in preparation for merging #21407.

justinhorvitz · copybara-github · commit cb901b71b4be · 2024-02-21T08:14:37.000-08:00
Isolate two `@ForOverride` methods and make everything else `private` to make it clear what behavior may diverge between bazel and blaze.

PiperOrigin-RevId: 608999251
Change-Id: I16343d4a1b538d9c13544661ec421f13bf9b1c40
diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/genrule/BUILD b/src/main/java/com/google/devtools/build/lib/bazel/rules/genrule/BUILD
@@ -15,11 +15,16 @@ java_library(
     name = "genrule",
     srcs = glob(["*.java"]),
     deps = [
+        "//src/main/java/com/google/devtools/build/lib/actions:artifacts",
         "//src/main/java/com/google/devtools/build/lib/analysis:analysis_cluster",
         "//src/main/java/com/google/devtools/build/lib/analysis:config/execution_transition_factory",
+        "//src/main/java/com/google/devtools/build/lib/analysis:file_provider",
         "//src/main/java/com/google/devtools/build/lib/analysis:rule_definition_environment",
+        "//src/main/java/com/google/devtools/build/lib/analysis:transitive_info_collection",
+        "//src/main/java/com/google/devtools/build/lib/cmdline",
+        "//src/main/java/com/google/devtools/build/lib/collect/nestedset",
         "//src/main/java/com/google/devtools/build/lib/packages",
         "//src/main/java/com/google/devtools/build/lib/rules/genrule",
-        "//src/main/java/com/google/devtools/build/lib/util:filetype",
+        "//third_party:guava",
     ],
 )
diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/genrule/BazelGenRule.java b/src/main/java/com/google/devtools/build/lib/bazel/rules/genrule/BazelGenRule.java
@@ -14,14 +14,20 @@
 
 package com.google.devtools.build.lib.bazel.rules.genrule;
 
+import com.google.common.collect.ImmutableMap;
+import com.google.devtools.build.lib.actions.Artifact;
+import com.google.devtools.build.lib.analysis.AliasProvider;
+import com.google.devtools.build.lib.analysis.FileProvider;
 import com.google.devtools.build.lib.analysis.RuleContext;
+import com.google.devtools.build.lib.analysis.TransitiveInfoCollection;
+import com.google.devtools.build.lib.cmdline.Label;
+import com.google.devtools.build.lib.collect.nestedset.NestedSet;
 import com.google.devtools.build.lib.packages.Type;
 import com.google.devtools.build.lib.rules.genrule.GenRuleBase;
+import java.util.List;
 
-/**
- * An implementation of genrule for Bazel.
- */
-public class BazelGenRule extends GenRuleBase {
+/** An implementation of genrule for Bazel. */
+public final class BazelGenRule extends GenRuleBase {
 
   @Override
   protected boolean isStampingEnabled(RuleContext ruleContext) {
@@ -30,4 +36,18 @@ protected boolean isStampingEnabled(RuleContext ruleContext) {
     }
     return ruleContext.attributes().get("stamp", Type.BOOLEAN);
   }
+
+  @Override
+  protected ImmutableMap<Label, NestedSet<Artifact>> collectSources(
+      List<? extends TransitiveInfoCollection> srcs) {
+    ImmutableMap.Builder<Label, NestedSet<Artifact>> labelMap =
+        ImmutableMap.builderWithExpectedSize(srcs.size());
+
+    for (TransitiveInfoCollection dep : srcs) {
+      NestedSet<Artifact> files = dep.getProvider(FileProvider.class).getFilesToBuild();
+      labelMap.put(AliasProvider.getDependencyLabel(dep), files);
+    }
+
+    return labelMap.buildOrThrow();
+  }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/rules/genrule/GenRuleBase.java b/src/main/java/com/google/devtools/build/lib/rules/genrule/GenRuleBase.java
@@ -22,7 +22,6 @@
 import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.actions.CommandLines;
 import com.google.devtools.build.lib.actions.MutableActionGraph.ActionConflictException;
-import com.google.devtools.build.lib.analysis.AliasProvider;
 import com.google.devtools.build.lib.analysis.CommandConstructor;
 import com.google.devtools.build.lib.analysis.CommandHelper;
 import com.google.devtools.build.lib.analysis.ConfigurationMakeVariableContext;
@@ -50,71 +49,20 @@
 import com.google.devtools.build.lib.util.OnDemandString;
 import com.google.devtools.build.lib.util.Pair;
 import com.google.devtools.build.lib.vfs.PathFragment;
+import com.google.errorprone.annotations.ForOverride;
 import java.util.List;
 import java.util.Map;
 import javax.annotation.Nullable;
 
 /**
- * A base implementation of genrule, to be used by specific implementing rules which can change some
- * of the semantics around when the execution info and inputs are changed.
+ * A base implementation of genrule, to be used by specific implementing rules which can change the
+ * semantics of {@link #isStampingEnabled} and {@link #collectSources}.
  */
 public abstract class GenRuleBase implements RuleConfiguredTargetFactory {
 
-  /**
-   * Returns {@code true} if the rule should be stamped.
-   *
-   * <p>Genrule implementations can set this based on the rule context, including by defining their
-   * own attributes over and above what is present in {@link GenRuleBaseRule}.
-   */
-  protected abstract boolean isStampingEnabled(RuleContext ruleContext);
-
-  /** Collects sources from src attribute. */
-  protected ImmutableMap<Label, NestedSet<Artifact>> collectSources(
-      List<? extends TransitiveInfoCollection> srcs) throws RuleErrorException {
-    ImmutableMap.Builder<Label, NestedSet<Artifact>> labelMap = ImmutableMap.builder();
-
-    for (TransitiveInfoCollection dep : srcs) {
-      NestedSet<Artifact> files = dep.getProvider(FileProvider.class).getFilesToBuild();
-      labelMap.put(AliasProvider.getDependencyLabel(dep), files);
-    }
-
-    return labelMap.build();
-  }
-
-  enum CommandType {
-    BASH,
-    WINDOWS_BATCH,
-    WINDOWS_POWERSHELL,
-  }
-
-  @Nullable
-  private static Pair<CommandType, String> determineCommandTypeAndAttribute(
-      RuleContext ruleContext) {
-    AttributeMap attributeMap = ruleContext.attributes();
-    if (ruleContext.isExecutedOnWindows()) {
-      if (attributeMap.isAttributeValueExplicitlySpecified("cmd_ps")) {
-        return Pair.of(CommandType.WINDOWS_POWERSHELL, "cmd_ps");
-      }
-      if (attributeMap.isAttributeValueExplicitlySpecified("cmd_bat")) {
-        return Pair.of(CommandType.WINDOWS_BATCH, "cmd_bat");
-      }
-    }
-    if (attributeMap.isAttributeValueExplicitlySpecified("cmd_bash")) {
-      return Pair.of(CommandType.BASH, "cmd_bash");
-    }
-    if (attributeMap.isAttributeValueExplicitlySpecified("cmd")) {
-      return Pair.of(CommandType.BASH, "cmd");
-    }
-    ruleContext.attributeError(
-        "cmd",
-        "missing value for `cmd` attribute, you can also set `cmd_ps` or `cmd_bat` on"
-            + " Windows and `cmd_bash` on other platforms.");
-    return null;
-  }
-
   @Override
   @Nullable
-  public ConfiguredTarget create(RuleContext ruleContext)
+  public final ConfiguredTarget create(RuleContext ruleContext)
       throws InterruptedException, RuleErrorException, ActionConflictException {
     NestedSet<Artifact> filesToBuild =
         NestedSetBuilder.wrap(Order.STABLE_ORDER, ruleContext.getOutputArtifacts());
@@ -143,7 +91,9 @@ public ConfiguredTarget create(RuleContext ruleContext)
     // The CommandHelper class makes an explicit copy of this in the constructor, so flattening
     // here should be benign.
     CommandHelper commandHelper =
-        commandHelperBuilder(ruleContext)
+        CommandHelper.builder(ruleContext)
+            .addToolDependencies("tools")
+            .addToolDependencies("toolchains")
             .addLabelMap(
                 labelMap.entrySet().stream()
                     .collect(toImmutableMap(Map.Entry::getKey, e -> e.getValue().toList())))
@@ -165,7 +115,7 @@ public ConfiguredTarget create(RuleContext ruleContext)
             ruleContext,
             resolvedSrcs,
             filesToBuild,
-            /* makeVariableSuppliers = */ ImmutableList.of(),
+            /* makeVariableSuppliers= */ ImmutableList.of(),
             expandToWindowsPath);
     String command =
         ruleContext
@@ -277,10 +227,49 @@ public String toString() {
         .build();
   }
 
-  protected CommandHelper.Builder commandHelperBuilder(RuleContext ruleContext) {
-    return CommandHelper.builder(ruleContext)
-        .addToolDependencies("tools")
-        .addToolDependencies("toolchains");
+  /**
+   * Returns {@code true} if the rule should be stamped.
+   *
+   * <p>Genrule implementations can set this based on the rule context, including by defining their
+   * own attributes over and above what is present in {@link GenRuleBaseRule}.
+   */
+  @ForOverride
+  protected abstract boolean isStampingEnabled(RuleContext ruleContext);
+
+  /** Collects sources from src attribute. */
+  @ForOverride
+  protected abstract ImmutableMap<Label, NestedSet<Artifact>> collectSources(
+      List<? extends TransitiveInfoCollection> srcs) throws RuleErrorException;
+
+  private enum CommandType {
+    BASH,
+    WINDOWS_BATCH,
+    WINDOWS_POWERSHELL,
+  }
+
+  @Nullable
+  private static Pair<CommandType, String> determineCommandTypeAndAttribute(
+      RuleContext ruleContext) {
+    AttributeMap attributeMap = ruleContext.attributes();
+    if (ruleContext.isExecutedOnWindows()) {
+      if (attributeMap.isAttributeValueExplicitlySpecified("cmd_ps")) {
+        return Pair.of(CommandType.WINDOWS_POWERSHELL, "cmd_ps");
+      }
+      if (attributeMap.isAttributeValueExplicitlySpecified("cmd_bat")) {
+        return Pair.of(CommandType.WINDOWS_BATCH, "cmd_bat");
+      }
+    }
+    if (attributeMap.isAttributeValueExplicitlySpecified("cmd_bash")) {
+      return Pair.of(CommandType.BASH, "cmd_bash");
+    }
+    if (attributeMap.isAttributeValueExplicitlySpecified("cmd")) {
+      return Pair.of(CommandType.BASH, "cmd");
+    }
+    ruleContext.attributeError(
+        "cmd",
+        "missing value for `cmd` attribute, you can also set `cmd_ps` or `cmd_bat` on"
+            + " Windows and `cmd_bash` on other platforms.");
+    return null;
   }
 
   /**
@@ -299,14 +288,14 @@ private static Artifact getExecutable(RuleContext ruleContext, NestedSet<Artifac
    * Implementation of {@link ConfigurationMakeVariableContext} used to expand variables in a
    * genrule command string.
    */
-  protected static class CommandResolverContext extends ConfigurationMakeVariableContext {
+  private static final class CommandResolverContext extends ConfigurationMakeVariableContext {
 
     private final RuleContext ruleContext;
     private final NestedSet<Artifact> resolvedSrcs;
     private final NestedSet<Artifact> filesToBuild;
     private final boolean windowsPath;
 
-    public CommandResolverContext(
+    CommandResolverContext(
         RuleContext ruleContext,
         NestedSet<Artifact> resolvedSrcs,
         NestedSet<Artifact> filesToBuild,
@@ -323,10 +312,6 @@ public CommandResolverContext(
       this.windowsPath = windowsPath;
     }
 
-    public RuleContext getRuleContext() {
-      return ruleContext;
-    }
-
     @Override
     public String lookupVariable(String variableName)
         throws ExpansionException, InterruptedException {
@@ -393,15 +378,13 @@ private String lookupVariableImpl(String variableName)
      * Returns the path of the sole element "artifacts", generating an exception with an informative
      * error message iff the set is not a singleton. Used to expand "$<", "$@".
      */
-    private static final String expandSingletonArtifact(
+    private static String expandSingletonArtifact(
         NestedSet<Artifact> artifacts, String variable, String artifactName)
         throws ExpansionException {
       if (artifacts.isEmpty()) {
-        throw new ExpansionException("variable '" + variable
-            + "' : no " + artifactName);
+        throw new ExpansionException("variable '" + variable + "' : no " + artifactName);
       } else if (!artifacts.isSingleton()) {
-        throw new ExpansionException("variable '" + variable
-            + "' : more than one " + artifactName);
+        throw new ExpansionException("variable '" + variable + "' : more than one " + artifactName);
       }
       return artifacts.getSingleton().getExecPathString();
     }
