Remove module file flags from a CppCompileAction's cache key.

Depending on input discovery, we remove some of the unused module file (.pcm)
flags. If they are part of the cache key, this can lead to spurious recompiles.
As all of actually required modules are determined by other files and the
module maps, removing the module files from the cache key does not affect
correctness.

RELNOTES: Prevent spurious recompiles, e.g. with changing --test_arg.

--
MOS_MIGRATED_REVID=139076906

1 files changed, 15 insertions, 2 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java
index df75e41fa3..bc428c05bb 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java
@@ -52,6 +52,7 @@ import com.google.devtools.build.lib.events.EventHandler;
 import com.google.devtools.build.lib.profiler.Profiler;
 import com.google.devtools.build.lib.profiler.ProfilerTask;
 import com.google.devtools.build.lib.rules.cpp.CcToolchainFeatures.FeatureConfiguration;
+import com.google.devtools.build.lib.rules.cpp.CcToolchainFeatures.Variables;
 import com.google.devtools.build.lib.rules.cpp.CppCompileActionContext.Reply;
 import com.google.devtools.build.lib.rules.cpp.CppConfiguration.Tool;
 import com.google.devtools.build.lib.util.DependencySet;
@@ -1139,9 +1140,21 @@ public class CppCompileAction extends AbstractAction
     Fingerprint f = new Fingerprint();
     f.addUUID(actionClassId);
     f.addStringMap(getEnvironment());
-    f.addStrings(getArgv());
     f.addStrings(executionRequirements);
 
+    // For the argv part of the cache key, ignore all compiler flags that explicitly denote module
+    // file (.pcm) inputs. Depending on input discovery, some of the unused ones are removed from
+    // the command line. However, these actually don't have an influence on the compile itself and
+    // so ignoring them for the cache key calculation does not affect correctness. The compile
+    // itself is fully determined by the input source files and module maps.
+    // A better long-term solution would be to make the compiler to find them automatically and
+    // never hand in the .pcm files explicitly on the command line in the first place.
+    Variables overwrittenVariables = this.overwrittenVariables;
+    this.overwrittenVariables = getOverwrittenVariables(ImmutableList.<Artifact>of());
+    // TODO(djasper): Make getArgv() accept a variables parameter.
+    f.addStrings(getArgv());
+    this.overwrittenVariables = overwrittenVariables;
+
     /*
      * getArgv() above captures all changes which affect the compilation
      * command and hence the contents of the object file.  But we need to
@@ -1425,7 +1438,7 @@ public class CppCompileAction extends AbstractAction
       // configuration; on the other hand toolchains switch off warnings for the layering check
       // that will be re-added by the feature flags.
       CcToolchainFeatures.Variables updatedVariables = variables;
-      if (overwrittenVariables != null) {
+      if (variables != null && overwrittenVariables != null) {
         CcToolchainFeatures.Variables.Builder variablesBuilder =
             new CcToolchainFeatures.Variables.Builder();
         variablesBuilder.addAll(variables);