From 98af4c0edc17bd355be03addd9ba182e8e462f90 Mon Sep 17 00:00:00 2001 From: FaBrand <21087362+FaBrand@users.noreply.github.com> Date: Wed, 7 Aug 2024 22:15:30 -0700 Subject: [PATCH] Fix inconsistent virtual include paths So far only the generated header in the virtual includes is tested but the Include path pointing to that directory isnt This is a testcase to reproduce the suspected error reported here: - https://github.com/bazelbuild/bazel/issues/23061 Assuming that the location of the file is asserted: ```Java assertThat(artifactsToStrings(ccInfo.getCcCompilationContext().getDirectPublicHdrs())) .contains("bin third_party/bar/_virtual_includes/starlark_lib_suffix/starlark_lib.h"); ``` Adding a testcase for the include path seems reasonable. A local run of the testcase shows the error: ``` 1) testStripIncludePrefixIncludePath(com.google.devtools.build.lib.rules.cpp.StarlarkCcCommonTest) value of: getIncludeDirs().onlyElement() expected: bazel-out/k8-fastbuild/bin/third_party/bar/_virtual_includes/starlark_lib_suffix but was : bazel-out/k8-fastbuild/bin/third_party/bar/_virtual_includes/starlark_lib at com.google.devtools.build.lib.rules.cpp.StarlarkCcCommonTest.testStripIncludePrefixIncludePath(StarlarkCcCommonTest.java:5902) FAILURES!!! Tests run: 73, Failures: 1 ``` The change in behaviour was introduced in https://github.com/bazelbuild/bazel/commit/7e0df68aa4d11307dff8571ebce11d37c79c170c#diff-403c46ec3075b8e9e6d490ce955db88dae2d457b8046608884039b18b10ab6ccR774 Fixes: https://github.com/bazelbuild/bazel/issues/23061 Closes #23071. PiperOrigin-RevId: 660681269 Change-Id: Ia3f8a8a6cf8bf0e093416a247e348e6de6719584 --- .../lib/rules/cpp/CcStarlarkInternal.java | 20 ------- .../common/cc/cc_compilation_helper.bzl | 8 +-- .../lib/rules/cpp/StarlarkCcCommonTest.java | 53 +++++++++++++++++++ 3 files changed, 58 insertions(+), 23 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcStarlarkInternal.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcStarlarkInternal.java index b10c7f4d4d78d9..09d7f140a7838a 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcStarlarkInternal.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcStarlarkInternal.java @@ -529,26 +529,6 @@ public StarlarkProvider ccTestRunnerInfo() throws EvalException { return starlarkCcTestRunnerInfo; } - // This looks ugly, however it is necessary. Good thing is we are planning to get rid of genfiles - // directory altogether so this method has a bright future(of being removed). - @StarlarkMethod( - name = "bin_or_genfiles_relative_to_unique_directory", - documented = false, - parameters = { - @Param(name = "actions", positional = false, named = true), - @Param(name = "unique_directory", positional = false, named = true), - }) - public String binOrGenfilesRelativeToUniqueDirectory( - StarlarkActionFactory actions, String uniqueDirectory) { - ActionConstructionContext actionConstructionContext = actions.getActionConstructionContext(); - return actionConstructionContext - .getBinOrGenfilesDirectory() - .getExecPath() - .getRelative( - actionConstructionContext.getUniqueDirectory(PathFragment.create(uniqueDirectory))) - .getPathString(); - } - @StarlarkMethod( name = "create_umbrella_header_action", documented = false, diff --git a/src/main/starlark/builtins_bzl/common/cc/cc_compilation_helper.bzl b/src/main/starlark/builtins_bzl/common/cc/cc_compilation_helper.bzl index 1177edbbd71630..16dbf53f73d4de 100644 --- a/src/main/starlark/builtins_bzl/common/cc/cc_compilation_helper.bzl +++ b/src/main/starlark/builtins_bzl/common/cc/cc_compilation_helper.bzl @@ -53,6 +53,7 @@ def _compute_public_headers( include_prefix, strip_include_prefix, label, + binfiles_dir, non_module_map_headers, is_sibling_repository_layout): if include_prefix: @@ -106,6 +107,7 @@ def _compute_public_headers( module_map_headers = [] virtual_to_original_headers_list = [] + virtual_include_dir = paths.join(paths.join(cc_helper.package_source_root(label.workspace_name, label.package, is_sibling_repository_layout), _VIRTUAL_INCLUDES_DIR), label.name) for original_header in public_headers_artifacts: repo_relative_path = _repo_relative_path(original_header) if not repo_relative_path.startswith(strip_prefix): @@ -115,7 +117,6 @@ def _compute_public_headers( include_path = paths.get_relative(include_prefix, include_path) if not original_header.path == include_path: - virtual_include_dir = paths.join(paths.join(cc_helper.package_source_root(label.workspace_name, label.package, is_sibling_repository_layout), _VIRTUAL_INCLUDES_DIR), label.name) virtual_header = actions.declare_shareable_artifact(paths.join(virtual_include_dir, include_path)) actions.symlink( output = virtual_header, @@ -130,11 +131,10 @@ def _compute_public_headers( module_map_headers.append(original_header) virtual_headers = module_map_headers + non_module_map_headers - return struct( headers = virtual_headers, module_map_headers = module_map_headers, - virtual_include_path = cc_internal.bin_or_genfiles_relative_to_unique_directory(actions = actions, unique_directory = _VIRTUAL_INCLUDES_DIR), + virtual_include_path = paths.join(binfiles_dir, virtual_include_dir), virtual_to_original_headers = depset(virtual_to_original_headers_list), ) @@ -249,6 +249,7 @@ def _init_cc_compilation_context( include_prefix, strip_include_prefix, label, + binfiles_dir, non_module_map_headers, sibling_repo_layout, ) @@ -285,6 +286,7 @@ def _init_cc_compilation_context( include_prefix, strip_include_prefix, label, + binfiles_dir, non_module_map_headers, sibling_repo_layout, ) diff --git a/src/test/java/com/google/devtools/build/lib/rules/cpp/StarlarkCcCommonTest.java b/src/test/java/com/google/devtools/build/lib/rules/cpp/StarlarkCcCommonTest.java index dd5c542b5ba2af..33858150a6a522 100755 --- a/src/test/java/com/google/devtools/build/lib/rules/cpp/StarlarkCcCommonTest.java +++ b/src/test/java/com/google/devtools/build/lib/rules/cpp/StarlarkCcCommonTest.java @@ -34,6 +34,7 @@ import com.google.devtools.build.lib.analysis.util.BuildViewTestCase; import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.cmdline.LabelSyntaxException; +import com.google.devtools.build.lib.cmdline.RepositoryName; import com.google.devtools.build.lib.collect.nestedset.Depset; import com.google.devtools.build.lib.packages.Provider; import com.google.devtools.build.lib.packages.StarlarkInfo; @@ -5573,6 +5574,33 @@ public void testStripIncludePrefix() throws Exception { .contains("bin third_party/bar/_virtual_includes/starlark_lib_suffix/starlark_lib.h"); } + @Test + public void testStripIncludePrefixIncludePath() throws Exception { + createFilesForTestingCompilation( + scratch, "third_party/tools/build_defs/foo", "strip_include_prefix='v1'"); + scratch.file( + "third_party/bar/BUILD", + """ + load("//third_party/tools/build_defs/foo:extension.bzl", "cc_starlark_library") + + cc_starlark_library( + name = "starlark_lib", + srcs = ["starlark_lib.cc"], + private_hdrs = ["v1/private_starlark_lib.h"], + public_hdrs = ["v1/starlark_lib.h"], + ) + """); + ConfiguredTarget target = getConfiguredTarget("//third_party/bar:starlark_lib"); + assertThat(target).isNotNull(); + CcInfo ccInfo = target.get(CcInfo.PROVIDER); + + assertThat(ccInfo.getCcCompilationContext().getIncludeDirs()) + .containsExactly( + getTargetConfiguration() + .getBinFragment(RepositoryName.MAIN) + .getRelative("third_party/bar/_virtual_includes/starlark_lib_suffix")); + } + @Test public void testStripIncludePrefixAndIncludePrefix() throws Exception { createFilesForTestingCompilation( @@ -5596,6 +5624,31 @@ public void testStripIncludePrefixAndIncludePrefix() throws Exception { "bin third_party/bar/_virtual_includes/starlark_lib_suffix/prefix/starlark_lib.h"); } + @Test + public void testStripIncludePrefixAndIncludePrefixIncludePath() throws Exception { + createFilesForTestingCompilation( + scratch, + "third_party/tools/build_defs/foo", + "strip_include_prefix='v1', include_prefix='prefix'"); + scratch.file( + "third_party/bar/BUILD", + "load('//third_party/tools/build_defs/foo:extension.bzl', 'cc_starlark_library')", + "cc_starlark_library(", + " name = 'starlark_lib',", + " srcs = ['starlark_lib.cc'],", + " public_hdrs = ['v1/starlark_lib.h'],", + " private_hdrs = ['v1/private_starlark_lib.h'],", + ")"); + ConfiguredTarget target = getConfiguredTarget("//third_party/bar:starlark_lib"); + assertThat(target).isNotNull(); + CcInfo ccInfo = target.get(CcInfo.PROVIDER); + assertThat(ccInfo.getCcCompilationContext().getIncludeDirs()) + .containsExactly( + getTargetConfiguration() + .getBinFragment(RepositoryName.MAIN) + .getRelative("third_party/bar/_virtual_includes/starlark_lib_suffix")); + } + @Test public void testHeaders() throws Exception { createFilesForTestingCompilation(