Hide fix for linkstmap inputs computation behind an option

This cl introduces a bazel option (--experimental_fix_linkstamp_inputs_bug_73447914) to control which inputs get added into C++ linkstamp compile action. When set to true (defaults to false), all inputs of relevant C++ linking action get added as inputs to the linkstamp compile action too. RELNOTES: None. PiperOrigin-RevId: 187030217
bazelbuild · Feb 26, 2018 · 1c0bc2d · 1c0bc2d
1 parent d6b4455
commit 1c0bc2d
Show file tree

Hide file tree

Showing 4 changed files with 43 additions and 9 deletions.
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppConfiguration.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppConfiguration.java
@@ -200,6 +200,8 @@ public String toString() {
 
   private final boolean shouldProvideMakeVariables;
 
+  private final boolean fixLinkstampInputsBug;
+
   /**
    * If true, the ConfiguredTarget is only used to get the necessary cross-referenced {@code
    * CcCompilationInfo}s, but registering build actions is disabled.
@@ -315,6 +317,7 @@ static CppConfiguration create(CppConfigurationParameters params)
         ImmutableList.copyOf(cppOptions.ltoindexoptList),
         cppOptions,
         params.cpuTransformer,
+        cppOptions.fixLinkstampInputsBug,
         (cppOptions.stripBinaries == StripMode.ALWAYS
             || (cppOptions.stripBinaries == StripMode.SOMETIMES
                 && compilationMode == CompilationMode.FASTBUILD)),
@@ -351,6 +354,7 @@ static CppConfiguration create(CppConfigurationParameters params)
       ImmutableList<String> ltoindexOptions,
       CppOptions cppOptions,
       CpuTransformer cpuTransformerEnum,
+      boolean fixLinkstampInputsBug,
       boolean stripBinaries,
       CompilationMode compilationMode,
       boolean shouldProvideMakeVariables,
@@ -381,6 +385,7 @@ static CppConfiguration create(CppConfigurationParameters params)
     this.ltoindexOptions = ltoindexOptions;
     this.cppOptions = cppOptions;
     this.cpuTransformerEnum = cpuTransformerEnum;
+    this.fixLinkstampInputsBug = fixLinkstampInputsBug;
     this.stripBinaries = stripBinaries;
     this.compilationMode = compilationMode;
     this.shouldProvideMakeVariables = shouldProvideMakeVariables;
@@ -421,6 +426,10 @@ public Function<String, String> getCpuTransformer() {
     return cpuTransformerEnum.getTransformer();
   }
 
+  public boolean shouldFixLinkstampInputsBug() {
+    return fixLinkstampInputsBug;
+  }
+
   /**
    * Returns the path of the crosstool.
    */

diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionBuilder.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionBuilder.java
@@ -1191,19 +1191,25 @@ public CppLinkAction build() throws InterruptedException {
 
     if (!isLtoIndexing) {
       for (Entry<Linkstamp, Artifact> linkstampEntry : linkstampMap.entrySet()) {
+        Iterable<Artifact> inputs;
+        if (cppConfiguration.shouldFixLinkstampInputsBug()) {
+          inputs = IterablesChain.<Artifact>builder()
+              .add(ImmutableSet.copyOf(nonCodeInputs))
+              // We don't want to add outputs of this linkstamp compilation action to
+              // inputsBuilder before this line, since that would introduce a cycle in the
+              // graph.
+              .add(inputsBuilder.deduplicate().build())
+              .build();
+        } else {
+          inputs = ImmutableSet.copyOf(nonCodeInputs);
+        }
         analysisEnvironment.registerAction(
             CppLinkstampCompileHelper.createLinkstampCompileAction(
                 ruleContext,
                 linkstampEntry.getKey().getArtifact(),
                 linkstampEntry.getValue(),
                 linkstampEntry.getKey().getDeclaredIncludeSrcs(),
-                NestedSetBuilder.<Artifact>stableOrder()
-                    .addAll(nonCodeInputs)
-                    // We don't want to add outputs of this linkstamp compilation action to
-                    // inputsBuilder before this line, since that would introduce a cycle in the
-                    // graph.
-                    .addAll(inputsBuilder.deduplicate().build())
-                    .build(),
+                inputs,
                 buildInfoHeaderArtifacts,
                 additionalLinkstampDefines,
                 toolchain,

diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppOptions.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppOptions.java
@@ -888,6 +888,24 @@ private boolean enableLipoSettings() {
   )
   public boolean useLLVMCoverageMapFormat;
 
+  @Option(
+      name = "experimental_fix_linkstamp_inputs_bug",
+      defaultValue = "false",
+      category = "experimental",
+      documentationCategory = OptionDocumentationCategory.UNDOCUMENTED,
+      effectTags = {
+          OptionEffectTag.CHANGES_INPUTS,
+          OptionEffectTag.AFFECTS_OUTPUTS,
+          OptionEffectTag.LOADING_AND_ANALYSIS
+      },
+      metadataTags = {OptionMetadataTag.EXPERIMENTAL},
+      help =
+          "If set to true, bazel will add all inputs of the C++ linking action as inputs to the "
+              + "C++ linkstamp compile action too. This is a migration-only flag and will be "
+              + "removed as soon as g3 is fixed."
+  )
+  public boolean fixLinkstampInputsBug;
+
   @Override
   public FragmentOptions getHost() {
     CppOptions host = (CppOptions) getDefault();

diff --git a/src/test/java/com/google/devtools/build/lib/rules/cpp/CppLinkstampCompileHelperTest.java b/src/test/java/com/google/devtools/build/lib/rules/cpp/CppLinkstampCompileHelperTest.java
@@ -38,7 +38,7 @@ public void testLinkstampCompileOptionsForExecutable() throws Exception {
     AnalysisMock.get()
         .ccSupport()
         .setupCrosstool(mockToolsConfig, "builtin_sysroot: '/usr/local/custom-sysroot'");
-    useConfiguration();
+    useConfiguration("--experimental_fix_linkstamp_inputs_bug");
     scratch.file(
         "x/BUILD",
         "cc_binary(",
@@ -92,7 +92,7 @@ public void testLinkstampCompileOptionsForSharedLibrary() throws Exception {
     AnalysisMock.get()
         .ccSupport()
         .setupCrosstool(mockToolsConfig, "builtin_sysroot: '/usr/local/custom-sysroot'");
-    useConfiguration();
+    useConfiguration("--experimental_fix_linkstamp_inputs_bug");
     scratch.file(
         "x/BUILD",
         "cc_binary(",
@@ -198,6 +198,7 @@ public void testLinkstampCompileDependsOnAllCcBinaryLinkingInputs() throws Excep
         "  srcs = [ 'bar.cc' ],",
         "  linkstamp = 'ls.cc',",
         ")");
+    useConfiguration("--experimental_fix_linkstamp_inputs_bug");
 
     ConfiguredTarget target = getConfiguredTarget("//x:foo");
     Artifact executable = getExecutable(target);