From af0edb563fdaee2b7135b2cbd4a72148887b6982 Mon Sep 17 00:00:00 2001 From: rosica Date: Wed, 25 Nov 2020 06:14:54 -0800 Subject: [PATCH] When generating a symlink in _virtual_includes, add the original header to the 'allowed to use' set too When a cc_library 'a' makes use of strip_include_prefix, bazel creates a symlink for the hdrs at bazel-out/.../bin/_virtual_includes/a/ Later, bazel passes -I bazel-out/.../bin/_virtual_includes/a/ to the command line to tell the compiler where to look for those headers. For each header in hdrs, bazel will either put the header artifact itself in a set of 'allowed to use' headers, or, in the case of strip_include_prefix usage, bazel will add the symlink to the header under bazel-out/.../bin/_virtual_includes/a/ as 'allowed to use'. When searching for headers, the compiler will do the following (Taken from https://gcc.gnu.org/onlinedocs/gcc/Directory-Options.html): 1. For the quote form of the include directive, the directory of the current file is searched first. 2. For the quote form of the include directive, the directories specified by -iquote options are searched in left-to-right order, as they appear on the command line. 3. Directories specified with -I options are scanned in left-to-right order. ... In the case of the following cc_library: ``` cc_library( name = 'foo', hdrs = ["lib/foo.h"], srcs = ["lib/foo.cc"], strip_include_prefix = 'lib', ) ``` if foo.cc includes foo.h as #include "foo.h" the compiler will find the header in step 1 of the search, thus will use the original header and not the symlink. The compiler will note this in the .d file. Bazel however added the symlink in the list of 'allowed to use' headers, so at some point it will error out with "undeclared inclusion(s) in rule" error due to the discrepancy. This cl tells bazel to add the original header in the 'allowed to use' headers even when it generates a symlink in the _virtual_includes directory. Fixes #3828 #6337 RELNOTES:None. PiperOrigin-RevId: 344240730 --- .../lib/rules/cpp/CcCompilationHelper.java | 4 +-- .../build/lib/rules/cpp/CcCommonTest.java | 7 ++-- src/test/shell/bazel/cc_integration_test.sh | 34 ++++++++++++++++--- 3 files changed, 36 insertions(+), 9 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCompilationHelper.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCompilationHelper.java index 59c68de26631d0..c2b11c6f5d23b1 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCompilationHelper.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCompilationHelper.java @@ -949,9 +949,9 @@ private PublicHeaders computePublicHeaders() throws InterruptedException { virtualToOriginalHeaders.add( Pair.of(virtualHeader.getExecPathString(), originalHeader.getExecPathString())); } - } else { - moduleHeadersBuilder.add(originalHeader); } + + moduleHeadersBuilder.add(originalHeader); } ImmutableList moduleMapHeaders = moduleHeadersBuilder.build(); diff --git a/src/test/java/com/google/devtools/build/lib/rules/cpp/CcCommonTest.java b/src/test/java/com/google/devtools/build/lib/rules/cpp/CcCommonTest.java index 3278276609f9e8..0eeefc15980185 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/cpp/CcCommonTest.java +++ b/src/test/java/com/google/devtools/build/lib/rules/cpp/CcCommonTest.java @@ -840,7 +840,7 @@ public void testIncludeManglingSmoke() throws Exception { ConfiguredTarget lib = getConfiguredTarget("//third_party/a"); CcCompilationContext ccCompilationContext = lib.get(CcInfo.PROVIDER).getCcCompilationContext(); assertThat(ActionsTestUtil.prettyArtifactNames(ccCompilationContext.getDeclaredIncludeSrcs())) - .containsExactly("third_party/a/_virtual_includes/a/lib/b/c.h"); + .containsExactly("third_party/a/_virtual_includes/a/lib/b/c.h", "third_party/a/v1/b/c.h"); assertThat(ccCompilationContext.getIncludeDirs()) .containsExactly( getTargetConfiguration() @@ -887,9 +887,10 @@ public void testAbsoluteAndRelativeStripPrefix() throws Exception { .getCcCompilationContext(); assertThat(ActionsTestUtil.prettyArtifactNames(relative.getDeclaredIncludeSrcs())) - .containsExactly("third_party/a/_virtual_includes/relative/b.h"); + .containsExactly("third_party/a/_virtual_includes/relative/b.h", "third_party/a/v1/b.h"); assertThat(ActionsTestUtil.prettyArtifactNames(absolute.getDeclaredIncludeSrcs())) - .containsExactly("third_party/a/_virtual_includes/absolute/a/v1/b.h"); + .containsExactly( + "third_party/a/_virtual_includes/absolute/a/v1/b.h", "third_party/a/v1/b.h"); } @Test diff --git a/src/test/shell/bazel/cc_integration_test.sh b/src/test/shell/bazel/cc_integration_test.sh index b2d3c4669e10d3..7b9756ae423d2c 100755 --- a/src/test/shell/bazel/cc_integration_test.sh +++ b/src/test/shell/bazel/cc_integration_test.sh @@ -76,8 +76,8 @@ cc_binary( ) cc_binary( - name = "bad", - srcs = ["bad.cc"], + name = "still_ok", + srcs = ["still_ok.cc"], deps = ["@foo//foo"], ) EOF @@ -90,7 +90,7 @@ int main() { } EOF - cat > bad.cc < still_ok.cc < #include "foo/v1/foo.h" int main() { @@ -98,8 +98,34 @@ int main() { } EOF - bazel build :bad && fail "Should not have found include at repository-relative path" bazel build :ok || fail "Should have found include at synthetic path" + bazel build :still_ok \ + || fail "Should have found include at repository-relative path" +} + + +function test_include_validation_sandbox_disabled() { + local workspace="${FUNCNAME[0]}" + mkdir -p "${workspace}"/lib + + create_workspace_with_default_repos "${workspace}/WORKSPACE" + cat >> "${workspace}/BUILD" << EOF +cc_library( + name = "foo", + srcs = ["lib/foo.cc"], + hdrs = ["lib/foo.h"], + strip_include_prefix = "lib", +) +EOF + cat >> "${workspace}/lib/foo.cc" << EOF +#include "foo.h" +EOF + + touch "${workspace}/lib/foo.h" + + cd "${workspace}" + bazel build --spawn_strategy=standalone //:foo &>"$TEST_log" \ + || fail "Build failed but should have succeeded" } function test_tree_artifact_headers_are_invalidated() {