Disable module file pruning for the build of other modules.

Pruning here can lead to a insufficient rebuilds which in turn cause
module-out-of-date errors. The problem surfaces if a module A depends on a
module B, but the headers of module A don't actually use any of B's headers.
Then we would currently not need to rebuild A when B changes although we are
storing a dependency in it. If B changes and we then build something that uses
A (a header of it), we mark A as well as all of its transitive dependencies as
inputs. We still don't need to rebuild A, as none of its inputs have changed,
but we do rebuild B now and then the two modules are out of sync.

For now, just disable pruning of header modules while building the modules. In
the long run, it would be even better to not embed a dependency on B into A,
but that requires signficantly larger changes.

--
MOS_MIGRATED_REVID=132039754

3 files changed, 20 insertions, 4 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 a6fc92c3d8..abc54a6efd 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
@@ -164,6 +164,7 @@ public class CppCompileAction extends AbstractAction
   private final Artifact optionalSourceFile;
   private final NestedSet<Artifact> mandatoryInputs;
   private final boolean shouldScanIncludes;
+  private final boolean shouldPruneModules;
   private final boolean usePic;
   private final CppCompilationContext context;
   private final Iterable<IncludeScannable> lipoScannables;
@@ -236,6 +237,7 @@ public class CppCompileAction extends AbstractAction
       CcToolchainFeatures.Variables variables,
       Artifact sourceFile,
       boolean shouldScanIncludes,
+      boolean shouldPruneModules,
       boolean usePic,
       Label sourceLabel,
       NestedSet<Artifact> mandatoryInputs,
@@ -281,6 +283,7 @@ public class CppCompileAction extends AbstractAction
     // known after inclusion scanning. When *not* scanning includes,
     // the inputs are as declared, hence known, and remain so.
     this.shouldScanIncludes = shouldScanIncludes;
+    this.shouldPruneModules = shouldPruneModules;
     this.usePic = usePic;
     this.inputsKnown = !shouldScanIncludes;
     this.cppCompileCommandLine =
@@ -440,7 +443,7 @@ public class CppCompileAction extends AbstractAction
       return null;
     }
 
-    if (featureConfiguration.isEnabled(CppRuleClasses.PRUNE_HEADER_MODULES)) {
+    if (shouldPruneModules) {
       Set<Artifact> initialResultSet = Sets.newLinkedHashSet(initialResult);
       List<String> usedModulePaths = Lists.newArrayList();
       for (Artifact usedModule : context.getUsedModules(usePic, initialResultSet)) {
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileActionBuilder.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileActionBuilder.java
index 260cc66b37..95bac0bed5 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileActionBuilder.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileActionBuilder.java
@@ -251,9 +251,18 @@ public class CppCompileActionBuilder {
     if (tempOutputFile == null && !shouldScanIncludes) {
       realMandatoryInputsBuilder.addTransitive(context.getDeclaredIncludeSrcs());
     }
-    if (featureConfiguration.isEnabled(CppRuleClasses.USE_HEADER_MODULES)
-        && (!shouldScanIncludes
-            || !featureConfiguration.isEnabled(CppRuleClasses.PRUNE_HEADER_MODULES))) {
+    // We disable pruning header modules in CPP_MODULE_COMPILEs as that would lead to
+    // module-out-of-date errors. The problem surfaces if a module A depends on a module B, but the
+    // headers of module A don't actually use any of B's headers.  Then we would not need to rebuild
+    // A when B changes although we are storing a dependency in it. If B changes and we then build
+    // something that uses A (a header of it), we mark A and all of its transitive deps as inputs.
+    // We still don't need to rebuild A, as none of its inputs have changed, but we do rebuild B
+    // now and then the two modules are out of sync.
+    boolean shouldPruneModules =
+        shouldScanIncludes
+            && !getActionName().equals(CppCompileAction.CPP_MODULE_COMPILE)
+            && featureConfiguration.isEnabled(CppRuleClasses.PRUNE_HEADER_MODULES);
+    if (featureConfiguration.isEnabled(CppRuleClasses.USE_HEADER_MODULES) && !shouldPruneModules) {
       realMandatoryInputsBuilder.addTransitive(context.getTransitiveModules(usePic));
     }
     realMandatoryInputsBuilder.addTransitive(context.getAdditionalInputs());
@@ -279,6 +288,7 @@ public class CppCompileActionBuilder {
           variables,
           sourceFile,
           shouldScanIncludes,
+          shouldPruneModules,
           usePic,
           sourceLabel,
           realMandatoryInputsBuilder.build(),
@@ -303,6 +313,7 @@ public class CppCompileActionBuilder {
           variables,
           sourceFile,
           shouldScanIncludes,
+          shouldPruneModules,
           usePic,
           sourceLabel,
           realMandatoryInputs,
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/FakeCppCompileAction.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/FakeCppCompileAction.java
index e82f352ce9..631e697331 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/FakeCppCompileAction.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/FakeCppCompileAction.java
@@ -65,6 +65,7 @@ public class FakeCppCompileAction extends CppCompileAction {
       CcToolchainFeatures.Variables variables,
       Artifact sourceFile,
       boolean shouldScanIncludes,
+      boolean shouldPruneModules,
       boolean usePic,
       Label sourceLabel,
       NestedSet<Artifact> mandatoryInputs,
@@ -86,6 +87,7 @@ public class FakeCppCompileAction extends CppCompileAction {
         variables,
         sourceFile,
         shouldScanIncludes,
+        shouldPruneModules,
         usePic,
         sourceLabel,
         mandatoryInputs,