From 0b081d8593d9eeb4e31aceed0b066ce4482ece49 Mon Sep 17 00:00:00 2001 From: Yun Peng Date: Mon, 19 Aug 2019 06:50:54 -0700 Subject: [PATCH] Windows: Ignore workspace name differences while doing header checking Fixes https://github.com/bazelbuild/bazel/issues/9172 Closes #9188. PiperOrigin-RevId: 264146358 --- .../build/lib/rules/cpp/CppCompileAction.java | 7 ++---- .../lib/rules/cpp/ShowIncludesFilter.java | 24 +++++++++---------- .../lib/rules/cpp/ShowIncludesFilterTest.java | 22 +++++++++++++---- src/test/py/bazel/bazel_windows_cpp_test.py | 22 +++++++++++++++++ src/test/py/bazel/test_base.py | 17 +++++++++---- 5 files changed, 66 insertions(+), 26 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 16b5a3c03bbf8c..ec01eaaeb9d831 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 @@ -1241,11 +1241,8 @@ public ActionContinuationOrResult beginExecution(ActionExecutionContext actionEx ShowIncludesFilter showIncludesFilterForStdout; ShowIncludesFilter showIncludesFilterForStderr; if (featureConfiguration.isEnabled(CppRuleClasses.PARSE_SHOWINCLUDES)) { - String workspaceName = actionExecutionContext.getExecRoot().getBaseName(); - showIncludesFilterForStdout = - new ShowIncludesFilter(getSourceFile().getFilename(), workspaceName); - showIncludesFilterForStderr = - new ShowIncludesFilter(getSourceFile().getFilename(), workspaceName); + showIncludesFilterForStdout = new ShowIncludesFilter(getSourceFile().getFilename()); + showIncludesFilterForStderr = new ShowIncludesFilter(getSourceFile().getFilename()); FileOutErr originalOutErr = actionExecutionContext.getFileOutErr(); FileOutErr tempOutErr = originalOutErr.childOutErr(); spawnContext = actionExecutionContext.withFileOutErr(tempOutErr); diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/ShowIncludesFilter.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/ShowIncludesFilter.java index a3767652812b67..b3a43383a4de1f 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/ShowIncludesFilter.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/ShowIncludesFilter.java @@ -25,7 +25,8 @@ import java.util.ArrayList; import java.util.Collection; import java.util.Collections; -import java.util.List; +import java.util.regex.Matcher; +import java.util.regex.Pattern; /** * A Class for filtering the output of /showIncludes from MSVC compiler. @@ -40,11 +41,9 @@ public class ShowIncludesFilter { private FilterShowIncludesOutputStream filterShowIncludesOutputStream; private final String sourceFileName; - private final String workspaceName; - public ShowIncludesFilter(String sourceFileName, String workspaceName) { + public ShowIncludesFilter(String sourceFileName) { this.sourceFileName = sourceFileName; - this.workspaceName = workspaceName; } /** @@ -58,7 +57,7 @@ public static class FilterShowIncludesOutputStream extends FilterOutputStream { private static final int NEWLINE = '\n'; // "Note: including file:" in 14 languages, // cl.exe will print different prefix according to the locale configured for MSVC. - private static final List SHOW_INCLUDES_PREFIXES = + private static final ImmutableList SHOW_INCLUDES_PREFIXES = ImmutableList.of( new String( new byte[] { @@ -149,13 +148,12 @@ public static class FilterShowIncludesOutputStream extends FilterOutputStream { StandardCharsets.UTF_8) // Spanish ); private final String sourceFileName; - private final String execRootSuffix; + private static final Pattern EXECROOT_HEADER_PATTERN = + Pattern.compile(".*execroot\\\\[^\\\\]*\\\\(?.*)"); - public FilterShowIncludesOutputStream( - OutputStream out, String sourceFileName, String workspaceName) { + public FilterShowIncludesOutputStream(OutputStream out, String sourceFileName) { super(out); this.sourceFileName = sourceFileName; - this.execRootSuffix = "execroot\\" + workspaceName + "\\"; } @Override @@ -167,9 +165,9 @@ public void write(int b) throws IOException { for (String prefix : SHOW_INCLUDES_PREFIXES) { if (line.startsWith(prefix)) { line = line.substring(prefix.length()).trim(); - int index = line.indexOf(execRootSuffix); - if (index != -1) { - line = line.substring(index + execRootSuffix.length()); + Matcher m = EXECROOT_HEADER_PATTERN.matcher(line); + if (m.matches()) { + line = m.group("headerPath"); } dependencies.add(line); prefixMatched = true; @@ -214,7 +212,7 @@ public Collection getDependencies() { public FilterOutputStream getFilteredOutputStream(OutputStream outputStream) { filterShowIncludesOutputStream = - new FilterShowIncludesOutputStream(outputStream, sourceFileName, workspaceName); + new FilterShowIncludesOutputStream(outputStream, sourceFileName); return filterShowIncludesOutputStream; } diff --git a/src/test/java/com/google/devtools/build/lib/rules/cpp/ShowIncludesFilterTest.java b/src/test/java/com/google/devtools/build/lib/rules/cpp/ShowIncludesFilterTest.java index 9b4ba63a29cc7a..ee5c2ff474f40f 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/cpp/ShowIncludesFilterTest.java +++ b/src/test/java/com/google/devtools/build/lib/rules/cpp/ShowIncludesFilterTest.java @@ -38,7 +38,7 @@ public class ShowIncludesFilterTest { @Before public void setUpOutputStreams() throws IOException { - showIncludesFilter = new ShowIncludesFilter("foo.cpp", "__main__"); + showIncludesFilter = new ShowIncludesFilter("foo.cpp"); output = new ByteArrayOutputStream(); filterOutputStream = showIncludesFilter.getFilteredOutputStream(output); fs = new InMemoryFileSystem(); @@ -93,17 +93,31 @@ public void testMatchAllOfNotePrefix() throws IOException { } @Test - public void testMatchAllOfNotePrefixWithAbsolutePath() throws IOException { + // Regression tests for https://github.com/bazelbuild/bazel/issues/9172 + public void testFindHeaderFromAbsolutePathUnderExecroot() throws IOException { // "Note: including file:" is the prefix filterOutputStream.write( - getBytes("Note: including file: C:\\tmp\\xxxx\\execroot\\__main__\\bar.h")); + getBytes("Note: including file: C:\\tmp\\xxxx\\execroot\\__main__\\foo\\bar\\bar.h")); filterOutputStream.flush(); // flush to output should not work, waiting for newline assertThat(output.toString()).isEmpty(); filterOutputStream.write(getBytes("\n")); // It's a match, output should be filtered, dependency on bar.h should be found. assertThat(output.toString()).isEmpty(); - assertThat(showIncludesFilter.getDependencies()).contains("bar.h"); + assertThat(showIncludesFilter.getDependencies()).contains("foo\\bar\\bar.h"); + } + + @Test + public void testFindHeaderFromAbsolutePathOutsideExecroot() throws IOException { + // "Note: including file:" is the prefix + filterOutputStream.write(getBytes("Note: including file: C:\\system\\foo\\bar\\bar.h")); + filterOutputStream.flush(); + // flush to output should not work, waiting for newline + assertThat(output.toString()).isEmpty(); + filterOutputStream.write(getBytes("\n")); + // It's a match, output should be filtered, dependency on bar.h should be found. + assertThat(output.toString()).isEmpty(); + assertThat(showIncludesFilter.getDependencies()).contains("C:\\system\\foo\\bar\\bar.h"); } @Test diff --git a/src/test/py/bazel/bazel_windows_cpp_test.py b/src/test/py/bazel/bazel_windows_cpp_test.py index 7c502cbcfcbb87..9cb39a0ad2ec7e 100644 --- a/src/test/py/bazel/bazel_windows_cpp_test.py +++ b/src/test/py/bazel/bazel_windows_cpp_test.py @@ -639,6 +639,28 @@ def testBuildWithClangClByToolchainResolution(self): self.AssertExitCode(exit_code, 0, stderr) self.assertIn('clang-cl.exe', ''.join(stderr)) + def createSimpleCppWorkspace(self, name): + work_dir = self.ScratchDir(name) + self.ScratchFile(name + '/WORKSPACE', ['workspace(name = "%s")' % name]) + self.ScratchFile( + name + '/BUILD', + ['cc_library(name = "lib", srcs = ["lib.cc"], hdrs = ["lib.h"])']) + self.ScratchFile(name + '/lib.h', ['void hello();']) + self.ScratchFile(name + '/lib.cc', ['#include "lib.h"', 'void hello() {}']) + return work_dir + + # Regression test for https://github.com/bazelbuild/bazel/issues/9172 + def testCacheBetweenWorkspaceWithDifferentNames(self): + cache_dir = self.ScratchDir('cache') + dir_a = self.createSimpleCppWorkspace('A') + dir_b = self.createSimpleCppWorkspace('B') + exit_code, _, stderr = self.RunBazel( + ['build', '--disk_cache=' + cache_dir, ':lib'], cwd=dir_a) + self.AssertExitCode(exit_code, 0, stderr) + exit_code, _, stderr = self.RunBazel( + ['build', '--disk_cache=' + cache_dir, ':lib'], cwd=dir_b) + self.AssertExitCode(exit_code, 0, stderr) + if __name__ == '__main__': unittest.main() diff --git a/src/test/py/bazel/test_base.py b/src/test/py/bazel/test_base.py index 4c80cee5c31ee9..f7d6f0983d86d8 100644 --- a/src/test/py/bazel/test_base.py +++ b/src/test/py/bazel/test_base.py @@ -285,7 +285,7 @@ def CopyFile(self, src_path, dst_path, executable=False): os.chmod(abspath, stat.S_IRWXU) return abspath - def RunBazel(self, args, env_remove=None, env_add=None): + def RunBazel(self, args, env_remove=None, env_add=None, cwd=None): """Runs "bazel ", waits for it to exit. Args: @@ -294,6 +294,8 @@ def RunBazel(self, args, env_remove=None, env_add=None): to Bazel env_add: {string: string}; optional; environment variables to pass to Bazel, won't be removed by env_remove. + cwd: [string]; the working directory of Bazel, will be self._test_cwd if + not specified. Returns: (int, [string], [string]) tuple: exit code, stdout lines, stderr lines """ @@ -301,7 +303,7 @@ def RunBazel(self, args, env_remove=None, env_add=None): self.Rlocation('io_bazel/src/bazel'), '--bazelrc=' + self._test_bazelrc, '--nomaster_bazelrc', - ] + args, env_remove, env_add) + ] + args, env_remove, env_add, False, cwd) def StartRemoteWorker(self): """Runs a "local remote worker" to run remote builds and tests on. @@ -370,7 +372,12 @@ def StopRemoteWorker(self): print('--------------------------') print('\n'.join(stderr_lines)) - def RunProgram(self, args, env_remove=None, env_add=None, shell=False): + def RunProgram(self, + args, + env_remove=None, + env_add=None, + shell=False, + cwd=None): """Runs a program (args[0]), waits for it to exit. Args: @@ -381,6 +388,8 @@ def RunProgram(self, args, env_remove=None, env_add=None, shell=False): the program, won't be removed by env_remove. shell: {bool: bool}; optional; whether to use the shell as the program to execute + cwd: [string]; the current working dirctory, will be self._test_cwd if not + specified. Returns: (int, [string], [string]) tuple: exit code, stdout lines, stderr lines """ @@ -390,7 +399,7 @@ def RunProgram(self, args, env_remove=None, env_add=None, shell=False): args, stdout=stdout, stderr=stderr, - cwd=self._test_cwd, + cwd=(cwd if cwd else self._test_cwd), env=self._EnvMap(env_remove, env_add), shell=shell) exit_code = proc.wait()