C++ dependency pruning is broken for Windows/MSVC

[lummax]

Description of the problem:

With gcc style compiler bazel performs "dotD" dependency pruning of unused headers to limit recompilations if these unused headers were touched. As a feature this is somewhat documented in CODEBASE.md and several issues here/mails on bazel-discuss.

It is less clear if bazel does/is supposed to do the same with MSVC toolchains. It is clear that bazel uses /showIncludes to perform header inclusion checking, but unclear if it goes further. #11765 hints at header pruning on Windows being a feature.

Bugs: what's the simplest, easiest way to reproduce this bug?

• Clone https://github.com/lummax/bazel-header-recompilation-reproducer
• run bazel build :main -s
• modify dead.h which may be used by main but is never included from main.cpp
• rerun bazel build :main -s

On Linux/gcc the second build does nothing, on Windows/MSVC the compilation unit is rebuild.

What operating system are you running Bazel on?

Ubuntu 21.10 / Windows 10

What's the output of bazel info release?

release 5.0.0

Any other information, logs, or outputs that you want to share?

The following change in

bazel/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileActionBuilder.java

Line 350 in 6c955a5

if (!shouldScanIncludes && dotdFile == null) {

fixes the problem, but I am not sure if that modification is safe.

 .../devtools/build/lib/rules/cpp/CppCompileActionBuilder.java   | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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 36c81dc5..4472d771 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
@@ -347,7 +347,7 @@ public class CppCompileActionBuilder {
     if (grepIncludes != null) {
       realMandatoryInputsBuilder.add(grepIncludes);
     }
-    if (!shouldScanIncludes && dotdFile == null) {
+    if (!shouldScanIncludes && dotdFile == null && !featureConfiguration.isEnabled(CppRuleClasses.PARSE_SHOWINCLUDES)) {
       realMandatoryInputsBuilder.addTransitive(ccCompilationContext.getDeclaredIncludeSrcs());
       realMandatoryInputsBuilder.addTransitive(additionalPrunableHeaders);
     }

[konste]

@meteorcloudy
Hi Yun, it happened so that this pretty simple bug is a show stopper right at the time when we supposed to switch to Bazel build at Salesforce. Will you accept a one liner PR for it? It should be pretty safe as it only affects Windows.

[meteorcloudy]

@konste Sure, I'm happy to review a PR, this sounds like a nice improvement for Windows.

[meteorcloudy]

@konste Sorry, I meant the one line fix looks good to me, maybe extract the conditions to a function called shouldParseShowIncludes?

[meteorcloudy]

@bazel-io fork 6.2.0

[konste]

@meteorcloudy please review #17928
I also have tested it on our huge three platforms project and it does the right thing.

[konste]

@meteorcloudy - PR approved! Please merge it and mark for cherry pick to 6.2.0 AND to 7.0.0-pre.20230322.4 if possible.

[meteorcloudy]

/cc @kshyanashree Please cherry pick 788801a into 6.2 branch