Fix genrule autostamping in bazel

AlessandroPatti · copybara-github · commit ee57d991ccb5 · 2024-02-21T12:28:49.000-08:00
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 #21407. PiperOrigin-RevId: 609085052 Change-Id: I873941e9aaae3760a545c1900cd13cb60a9ada39
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
@@ -18,25 +18,15 @@
 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 final class BazelGenRule extends GenRuleBase {
 
-  @Override
-  protected boolean isStampingEnabled(RuleContext ruleContext) {
-    if (!ruleContext.attributes().has("stamp", Type.BOOLEAN)) {
-      return false;
-    }
-    return ruleContext.attributes().get("stamp", Type.BOOLEAN);
-  }
-
   @Override
   protected ImmutableMap<Label, NestedSet<Artifact>> collectSources(
       List<? extends TransitiveInfoCollection> srcs) {
diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/genrule/BazelGenRuleRule.java b/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();
   }
 
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
@@ -43,7 +43,9 @@
 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;
@@ -56,7 +58,7 @@
 
 /**
  * A base implementation of genrule, to be used by specific implementing rules which can change the
- * semantics of {@link #isStampingEnabled} and {@link #collectSources}.
+ * semantics of {@link #collectSources}.
  */
 public abstract class GenRuleBase implements RuleConfiguredTargetFactory {
 
@@ -227,20 +229,20 @@ public String toString() {
         .build();
   }
 
-  /**
-   * 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 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,
diff --git a/src/test/java/com/google/devtools/build/lib/bazel/rules/genrule/GenRuleConfiguredTargetTest.java b/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");
   }
 
