[7.1.0] Fix genrule autostamping in bazel (bazelbuild#21512)

Genrule does not support conditional stamping based on the value of the
`--stamp` flag. This is because genrule treats the `stamp` attribute as
`boolean`, while all other rules use the `tristate` type. After this
change, the stamp attribute in genrule can be set to `-1` to request
conditional stamping.

RELNOTES: `genrule` now supports setting `stamp = -1` to request
conditional stamping (based on the value of the build's `--stamp` flag).

Closes bazelbuild#21407.

Commit
bazelbuild@cb901b7,
bazelbuild@ee57d99

PiperOrigin-RevId: 609085052
Change-Id: I873941e9aaae3760a545c1900cd13cb60a9ada39

---------

Co-authored-by: Googler <[email protected]>
Co-authored-by: Alessandro Patti <[email protected]>

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",
     ],
 )

src/main/java/com/google/devtools/build/lib/bazel/rules/genrule/BazelGenRule.java

@@ -14,20 +14,30 @@
 
 package com.google.devtools.build.lib.bazel.rules.genrule;
 
-import com.google.devtools.build.lib.analysis.RuleContext;
-import com.google.devtools.build.lib.packages.Type;
+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.TransitiveInfoCollection;
+import com.google.devtools.build.lib.cmdline.Label;
+import com.google.devtools.build.lib.collect.nestedset.NestedSet;
 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) {
-    if (!ruleContext.attributes().has("stamp", Type.BOOLEAN)) {
-      return false;
+  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 ruleContext.attributes().get("stamp", Type.BOOLEAN);
+
+    return labelMap.buildOrThrow();
   }
 }

src/main/java/com/google/devtools/build/lib/bazel/rules/genrule/BazelGenRuleRule.java

@@ -15,19 +15,20 @@
 
 import static com.google.devtools.build.lib.packages.Attribute.attr;
 import static com.google.devtools.build.lib.packages.BuildType.LABEL;
-import static com.google.devtools.build.lib.packages.Type.BOOLEAN;
+import static com.google.devtools.build.lib.packages.BuildType.TRISTATE;
 
 import com.google.devtools.build.lib.analysis.RuleDefinition;
 import com.google.devtools.build.lib.analysis.RuleDefinitionEnvironment;
 import com.google.devtools.build.lib.analysis.config.ExecutionTransitionFactory;
 import com.google.devtools.build.lib.packages.RuleClass;
+import com.google.devtools.build.lib.packages.TriState;
 import com.google.devtools.build.lib.rules.genrule.GenRuleBaseRule;
 
 /**
  * Rule definition for genrule for Bazel.
  */
 public final class BazelGenRuleRule implements RuleDefinition {
-  public static final String GENRULE_SETUP_LABEL = "//tools/genrule:genrule-setup.sh";
+  private static final String GENRULE_SETUP_LABEL = "//tools/genrule:genrule-setup.sh";
 
   @Override
   public RuleClass build(RuleClass.Builder builder, RuleDefinitionEnvironment env) {
@@ -43,9 +44,7 @@ public RuleClass build(RuleClass.Builder builder, RuleDefinitionEnvironment env)
             attr("$genrule_setup", LABEL)
                 .cfg(ExecutionTransitionFactory.createFactory())
                 .value(env.getToolsLabel(GENRULE_SETUP_LABEL)))
-
-        // TODO(bazel-team): stamping doesn't seem to work. Fix it or remove attribute.
-        .add(attr("stamp", BOOLEAN).value(false))
+        .add(attr("stamp", TRISTATE).value(TriState.NO))
         .build();
   }
 

src/main/java/com/google/devtools/build/lib/rules/genrule/GenRuleBase.java

@@ -23,7 +23,6 @@
 import com.google.devtools.build.lib.actions.CommandLines;
 import com.google.devtools.build.lib.actions.CompositeRunfilesSupplier;
 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;
@@ -46,77 +45,28 @@
 import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
 import com.google.devtools.build.lib.collect.nestedset.Order;
 import com.google.devtools.build.lib.packages.AttributeMap;
+import com.google.devtools.build.lib.packages.BuildType;
 import com.google.devtools.build.lib.packages.TargetUtils;
+import com.google.devtools.build.lib.packages.TriState;
 import com.google.devtools.build.lib.packages.Type;
 import com.google.devtools.build.lib.util.FileTypeSet;
 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 #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());
@@ -145,7 +95,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())))
@@ -167,7 +119,7 @@ public ConfiguredTarget create(RuleContext ruleContext)
             ruleContext,
             resolvedSrcs,
             filesToBuild,
-            /* makeVariableSuppliers = */ ImmutableList.of(),
+            /* makeVariableSuppliers= */ ImmutableList.of(),
             expandToWindowsPath);
     String command =
         ruleContext
@@ -281,10 +233,49 @@ public String toString() {
         .build();
   }
 
-  protected CommandHelper.Builder commandHelperBuilder(RuleContext ruleContext) {
-    return CommandHelper.builder(ruleContext)
-        .addToolDependencies("tools")
-        .addToolDependencies("toolchains");
+  /** Collects sources from src attribute. */
+  @ForOverride
+  protected abstract ImmutableMap<Label, NestedSet<Artifact>> collectSources(
+      List<? extends TransitiveInfoCollection> srcs) throws RuleErrorException;
+
+  private static boolean isStampingEnabled(RuleContext ruleContext) {
+    // This intentionally does not call AnalysisUtils.isStampingEnabled(). That method returns false
+    // in the exec configuration (regardless of the attribute value), which is the behavior for
+    // binaries, but not genrules.
+    TriState stamp = ruleContext.attributes().get("stamp", BuildType.TRISTATE);
+    return stamp == TriState.YES
+        || (stamp == TriState.AUTO && ruleContext.getConfiguration().stampBinaries());
+  }
+
+  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;
   }
 
   /**
@@ -303,14 +294,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,
@@ -327,10 +318,6 @@ public CommandResolverContext(
       this.windowsPath = windowsPath;
     }
 
-    public RuleContext getRuleContext() {
-      return ruleContext;
-    }
-
     @Override
     public String lookupVariable(String variableName)
         throws ExpansionException, InterruptedException {
@@ -397,15 +384,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();
     }

src/test/java/com/google/devtools/build/lib/bazel/rules/genrule/GenRuleConfiguredTargetTest.java

@@ -530,6 +530,7 @@ private void createStampingTargets() throws Exception {
         "u/BUILD",
         "genrule(name='foo_stamp', srcs=[], outs=['uu'], stamp=1, cmd='')",
         "genrule(name='foo_nostamp', srcs=[], outs=['vv'], stamp=0, cmd='')",
+        "genrule(name='foo_autostamp', srcs=[], outs=['aa'], stamp=-1, cmd='')",
         "genrule(name='foo_default', srcs=[], outs=['xx'], cmd='')");
   }
 
@@ -562,6 +563,8 @@ public void testStampingWithNoStamp() throws Exception {
     assertStamped(getExecConfiguredTarget("//u:foo_stamp"));
     assertNotStamped("//u:foo_nostamp");
     assertNotStamped(getExecConfiguredTarget("//u:foo_nostamp"));
+    assertNotStamped("//u:foo_autostamp");
+    assertNotStamped(getExecConfiguredTarget("//u:foo_autostamp"));
     assertNotStamped("//u:foo_default");
   }
 
@@ -571,8 +574,10 @@ public void testStampingWithStamp() throws Exception {
     createStampingTargets();
     assertStamped("//u:foo_stamp");
     assertStamped(getExecConfiguredTarget("//u:foo_stamp"));
-    // assertStamped("//u:foo_nostamp");
+    assertNotStamped("//u:foo_nostamp");
     assertNotStamped(getExecConfiguredTarget("//u:foo_nostamp"));
+    assertStamped("//u:foo_autostamp");
+    assertNotStamped(getExecConfiguredTarget("//u:foo_autostamp"));
     assertNotStamped("//u:foo_default");
   }