Don't stamp binaries built in tool configuration

Link-stamping should only be used for binaries built for the target. This change unifies the logic for cc_common.link actions defined in Starlark with the logic used for native rules (in AnalysisUtils.isStampingEnabled), which is otherwise the same. RELNOTES: Don't stamp cc_common.link actions for tool dependencies. PiperOrigin-RevId: 438027624
bazelbuild · Mar 29, 2022 · 1799842 · 1799842
1 parent 10a22e3
commit 1799842
Show file tree

Hide file tree

Showing 4 changed files with 73 additions and 22 deletions.
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/AnalysisUtils.java b/src/main/java/com/google/devtools/build/lib/analysis/AnalysisUtils.java
@@ -50,26 +50,36 @@ private AnalysisUtils() {
   }
 
   /**
-   * Returns whether link stamping is enabled for a rule.
+   * Returns whether link stamping is enabled for a TriState stamp attribute.
    *
    * <p>This returns false for unstampable rule classes and for rules used to build tools. Otherwise
    * it returns the value of the stamp attribute, or of the stamp option if the attribute value is
-   * -1.
+   * AUTO.
    */
-  public static boolean isStampingEnabled(RuleContext ruleContext, BuildConfigurationValue config) {
+  public static boolean isStampingEnabled(TriState stamp, BuildConfigurationValue config) {
     if (config.isToolConfiguration()) {
       return false;
     }
-    TriState stamp;
+    return stamp.equals(TriState.YES) || (stamp.equals(TriState.AUTO) && config.stampBinaries());
+  }
+
+  /**
+   * Returns whether link stamping is enabled for a rule.
+   *
+   * <p>This returns false for unstampable rule classes and for rules used to build tools. Otherwise
+   * it returns the value of the stamp attribute, or of the stamp option if the attribute value is
+   * -1.
+   */
+  public static boolean isStampingEnabled(RuleContext ruleContext, BuildConfigurationValue config) {
     if (ruleContext.attributes().has("stamp", BuildType.TRISTATE)) {
-      stamp = ruleContext.attributes().get("stamp", BuildType.TRISTATE);
-    } else if (ruleContext.attributes().has("stamp", Type.INTEGER)) {
-      int value = ruleContext.attributes().get("stamp", Type.INTEGER).toIntUnchecked();
-      stamp = TriState.fromInt(value);
-    } else {
-      return false;
+      TriState stamp = ruleContext.attributes().get("stamp", BuildType.TRISTATE);
+      return isStampingEnabled(stamp, config);
+    }
+    if (ruleContext.attributes().has("stamp", Type.INTEGER)) {
+      int stamp = ruleContext.attributes().get("stamp", Type.INTEGER).toIntUnchecked();
+      return isStampingEnabled(TriState.fromInt(stamp), config);
     }
-    return stamp == TriState.YES || (stamp == TriState.AUTO && config.stampBinaries());
+    return false;
   }
 
   public static boolean isStampingEnabled(RuleContext ruleContext) {

diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcModule.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcModule.java
@@ -25,6 +25,7 @@
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
 import com.google.devtools.build.lib.actions.Artifact;
+import com.google.devtools.build.lib.analysis.AnalysisUtils;
 import com.google.devtools.build.lib.analysis.RuleContext;
 import com.google.devtools.build.lib.analysis.config.BuildConfigurationValue;
 import com.google.devtools.build.lib.analysis.config.BuildOptions;
@@ -45,6 +46,7 @@
 import com.google.devtools.build.lib.packages.RuleClass.ConfiguredTargetFactory.RuleErrorException;
 import com.google.devtools.build.lib.packages.StarlarkInfo;
 import com.google.devtools.build.lib.packages.TargetUtils;
+import com.google.devtools.build.lib.packages.TriState;
 import com.google.devtools.build.lib.packages.semantics.BuildLanguageOptions;
 import com.google.devtools.build.lib.rules.cpp.CcCommon.CoptsFilter;
 import com.google.devtools.build.lib.rules.cpp.CcCompilationHelper.CompilationInfo;
@@ -1954,17 +1956,12 @@ protected void validateOutputType(String outputType) throws EvalException {
 
   private static boolean isStampingEnabled(int stamp, BuildConfigurationValue config)
       throws EvalException {
-    if (stamp == 0) {
-      return false;
-    } else if (stamp == 1) {
-      return true;
-    } else if (stamp == -1) {
-      return config.stampBinaries();
-    } else {
-      throw Starlark.errorf(
-          "stamp value %d is not supported, must be 0 (disabled), 1 (enabled), or -1 (default)",
-          stamp);
+    if (stamp == 0 || stamp == 1 || stamp == -1) {
+      return AnalysisUtils.isStampingEnabled(TriState.fromInt(stamp), config);
     }
+    throw Starlark.errorf(
+        "stamp value %d is not supported, must be 0 (disabled), 1 (enabled), or -1 (default)",
+        stamp);
   }
 
   protected Label getCallerLabel(StarlarkActionFactory actions, String name) throws EvalException {

diff --git a/src/test/java/com/google/devtools/build/lib/analysis/AnalysisUtilsTest.java b/src/test/java/com/google/devtools/build/lib/analysis/AnalysisUtilsTest.java
@@ -19,12 +19,14 @@
 import static org.junit.Assert.assertThrows;
 
 import com.google.auto.value.AutoValue;
+import com.google.devtools.build.lib.analysis.util.ConfigurationTestCase;
+import com.google.devtools.build.lib.packages.TriState;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.junit.runners.JUnit4;
 
 @RunWith(JUnit4.class)
-public class AnalysisUtilsTest {
+public class AnalysisUtilsTest extends ConfigurationTestCase {
 
   @Test
   public void checkProviderSucceedsOnClassAnnotatedWithAutoValue() {
@@ -46,4 +48,44 @@ public void checkProviderFailsOnClassGeneratedByAutoValue() {
   abstract static class AutoValuedClass implements TransitiveInfoProvider {
     abstract int foo();
   }
+
+  @Test
+  public void stampingEnabled_yesIgnoresNostamp() throws Exception {
+    assertThat(AnalysisUtils.isStampingEnabled(TriState.YES, create("--nostamp"))).isTrue();
+  }
+
+  @Test
+  public void stampingEnabled_yesPlusStamp() throws Exception {
+    assertThat(AnalysisUtils.isStampingEnabled(TriState.YES, create("--stamp"))).isTrue();
+  }
+
+  @Test
+  public void stampingEnabled_noPlusNostamp() throws Exception {
+    assertThat(AnalysisUtils.isStampingEnabled(TriState.NO, create("--nostamp"))).isFalse();
+  }
+
+  @Test
+  public void stampingEnabled_noIgnoresStamp() throws Exception {
+    assertThat(AnalysisUtils.isStampingEnabled(TriState.NO, create("--stamp"))).isFalse();
+  }
+
+  @Test
+  public void stampingEnabled_autoUsesNostamp() throws Exception {
+    assertThat(AnalysisUtils.isStampingEnabled(TriState.AUTO, create("--nostamp"))).isFalse();
+  }
+
+  @Test
+  public void stampingEnabled_autoUsesStamp() throws Exception {
+    assertThat(AnalysisUtils.isStampingEnabled(TriState.AUTO, create("--stamp"))).isTrue();
+  }
+
+  @Test
+  public void stampingEnabled_stampDisabledInToolConfig_attributeYes() throws Exception {
+    assertThat(AnalysisUtils.isStampingEnabled(TriState.YES, createHost("--stamp"))).isFalse();
+  }
+
+  @Test
+  public void stampingEnabled_stampDisabledInToolConfig_attributeAuto() throws Exception {
+    assertThat(AnalysisUtils.isStampingEnabled(TriState.AUTO, createHost("--stamp"))).isFalse();
+  }
 }
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/BUILD b/src/test/java/com/google/devtools/build/lib/analysis/BUILD
@@ -219,6 +219,8 @@ java_test(
     deps = [
         "//src/main/java/com/google/devtools/build/lib/analysis:analysis_cluster",
         "//src/main/java/com/google/devtools/build/lib/analysis:transitive_info_provider",
+        "//src/main/java/com/google/devtools/build/lib/packages",
+        "//src/test/java/com/google/devtools/build/lib/analysis/util",
         "//third_party:auto_value",
         "//third_party:junit4",
         "//third_party:truth",