CppCompileAction: do not make declaredIncludeSrcs() part of the action key

[obruns]

There are two types of input discovery in Bazel:

• include scanning is hard-disabled [1]
• depfile parsing is active and the action key is updated accordingly

However, due to declaredIncludeSrcs() being part of the action key, a change of the declared include srcs, either explicitly or via a glob(), causes a CppCompileAction to be re-executed even though none of its inputs (as reported by the compiler) had changed.

When an action does not discover inputs, there is no need to distinguish between inputs and declared inputs since the inputs already go into the cache key. When an action does discover inputs, the action key gets updated after parsing the depfile.

With this change, Bazel behaves much like Ninja in terms of avoiding unnecessary recompiles of C++ translation units.

There should probably be an entry in the release notes because this change makes existing cache entries non-candidates.

[1] https://github.com/bazelbuild/bazel/commit/1df4c71a9cf52f2dd24c4a10da75bb88631e7fde

Fixes: #8319 Hint-by: Lukacs T. Berki [email protected]

[google-cla[bot]]

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

:memo: Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

---

What to do if you already signed the CLA

Individual signers

• It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

• Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
• The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
• The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

[obruns]

@googlebot I signed it!

[lberki]

@oquenchil , can you take care of this? I have a little too much to do this week and the next :(

[lberki]

I don't think this is going to work because this would probably break our internal tests for the reasons described here.

I can imagine two possible resolutions:

• @oquenchil runs our internal presubmits on this change. If they pass, I got all this wrong. They are extensive enough that in that case, I'm comfortable merging this pull request.
• You change this pull request to add getDeclaredIncludeSrcs() to the action key iff include scanning is enabled. Given that that means that internal behavior does not change, I'm fairly certain that it will not break anything at Google..

[obruns]

Thanks for your feedback @lberki. I am happy to proceed with either of your proposals.

About the failing tests on Windows/msvc: /showIncludes and parsing should be enabled, AFAICT. I would have to look into why exactly those tests are failing on msvc. Unfortunately, I currently don't have a working Windows/msvc/Bazel setup. Therefore, I could:

• Disable those test cases based on $is_windows which is already available in cpp_test.sh or
• Spent some time setting things up and proceed with properly fixing those failures (not preferred)

[oquenchil]

I agree that the original issue #8319 needs to be addressed. The priority right now is not the highest though, it doesn't happen that often that you add a new header. I will see if I have some time to properly look at this during Q3.

[obruns]

@oquenchil and @lberki, can you give a status update? Do you still plan to have a closer look during Q3/2021? Thanks.

[lberki]

@oquenchil ?

[oquenchil]

No sorry, no time this quarter.

[Yannic]

Please consider the following test-case that starts failing with this change:

function test_compile_on_header_existence_check() {
  local -r pkg=$FUNCNAME
  mkdir -p $pkg
  cat > $pkg/BUILD <<EOF
cc_binary(name="a", srcs=["a.cc"], deps=["b"])
cc_library(name="b", includes=["."], hdrs=["b.h"])
EOF

  cat > $pkg/a.cc <<EOF
#include "b.h"

int main(void) {
  return 0;
}
EOF

  cat > $pkg/b.h <<EOF
#if __has_include("unused.h")
#error unused.h shall not exist
#endif  // __has_include("unused.h")
EOF

  bazel build -s //$pkg:a >& $TEST_log || fail "build failed"
  expect_log "Compiling $pkg/a.cc"

  cat > $pkg/BUILD <<EOF
cc_binary(name="a", srcs=["a.cc"], deps=["b"])
cc_library(name="b", includes=["."], hdrs=["b.h", "unused.h"])
EOF

  cat > $pkg/unused.h <<EOF
=== BANANA ===
EOF

  bazel build -s //$pkg:a >& $TEST_log && fail "unexpected success" || true
  expect_log "Compiling $pkg/a.cc"
}

See https://github.com/bazelbuild/bazel/issues/8319#issuecomment-976693110 for more context.

Edit: only tested with clang so far, gcc/msvc may not support __has_include

[oquenchil]

Closing this PR. If still interested, please file an issue instead so that this is tackled in a different way. It should pass Yannic's test and internal tests.