Skip to content

Commit

Permalink
Error on potential unsupported /showIncludes lines
Browse files Browse the repository at this point in the history
This error ensures that removing the English language pack for MSVC
without also refetching `@local_config_cc` doesn't result in silently
incorrect incremental builds.

I considered making this a warning instead, but that would either result
in warning spam (if a false positive) or drown in the unfiltered MSVC
output (if a true positive). Making this an error is better for
correctness and will lead users to report any false positive matches.
  • Loading branch information
fmeum committed Sep 21, 2023
1 parent 480b234 commit b00f9c6
Show file tree
Hide file tree
Showing 3 changed files with 66 additions and 0 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -1710,6 +1710,22 @@ private NestedSet<Artifact> discoverInputsFromShowIncludesFilters(
throws ActionExecutionException {
Collection<Path> stdoutDeps = showIncludesFilterForStdout.getDependencies(execRoot);
Collection<Path> stderrDeps = showIncludesFilterForStderr.getDependencies(execRoot);
if ((stdoutDeps.isEmpty()
&& showIncludesFilterForStdout.sawPotentialUnsupportedShowIncludesLine())
|| (stderrDeps.isEmpty()
&& showIncludesFilterForStderr.sawPotentialUnsupportedShowIncludesLine())) {
// /showIncludes parsing didn't result in any headers being found (unusual) *and* also
// encountered a line that looked like /showIncludes output in an unsupported encoding.
String message =
"While parsing the C++ compiler output for information about included headers, Bazel "
+ "failed to find any headers but encountered a line that appears to be "
+ "/showIncludes output in an unsupported encoding. This can result in incorrect "
+ "incremental builds. If you are using the default Windows MSVC toolchain that "
+ "ships with Bazel, ensure that the English language pack for Visual Studio is "
+ "installed and then execute 'bazel clean --expunge'.";
DetailedExitCode code = createDetailedExitCode(message, Code.FIND_USED_HEADERS_IO_EXCEPTION);
throw new ActionExecutionException(message, this, /* catastrophe= */ false, code);
}
return HeaderDiscovery.discoverInputsFromDependencies(
this,
getSourceFile(),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -148,10 +148,19 @@ public static class FilterShowIncludesOutputStream extends FilterOutputStream {
StandardCharsets.UTF_8) // Spanish
);
private final String sourceFileName;
private boolean sawPotentialUnsupportedShowIncludesLine;
// Grab everything under the execroot base so that external repository header files are covered
// in the sibling repository layout.
private static final Pattern EXECROOT_BASE_HEADER_PATTERN =
Pattern.compile(".*execroot\\\\(?<headerPath>.*)");
// Match a line of the form "fooo: bar: C:\some\path\file.h". As this is relatively generic,
// we require the line to include an absolute path with drive letter. If remote workers rewrite
// the path to a relative one, we won't match it, but it is unlikely that such setups use an
// unsupported encoding. We also exclude any matches that contain numbers: MSVC warnings and
// errors always contain numbers, but the /showIncludes output doesn't in any encoding since all
// codepages are ASCII-compatible.
private static final Pattern POTENTIAL_UNSUPPORTED_SHOW_INCLUDES_LINE =
Pattern.compile("[^:0-9]+:\\s+[^:0-9]+:\\s+[A-Za-z]:\\\\.*");

public FilterShowIncludesOutputStream(OutputStream out, String sourceFileName) {
super(out);
Expand Down Expand Up @@ -182,6 +191,15 @@ public void write(int b) throws IOException {
}
// cl.exe also prints out the file name unconditionally, we need to also filter it out.
if (!prefixMatched && !line.trim().equals(sourceFileName)) {
// When the toolchain definition failed to force an English locale, /showIncludes lines
// can use non-UTF8 encodings, which the checks above fail to detect. As this results in
// incorrect incremental builds, we emit a warning if the raw byte sequence comprising the
// line looks like it could be a /showIncludes line.
if (POTENTIAL_UNSUPPORTED_SHOW_INCLUDES_LINE
.matcher(buffer.toString(StandardCharsets.ISO_8859_1).trim())
.matches()) {
sawPotentialUnsupportedShowIncludesLine = true;
}
buffer.writeTo(out);
}
buffer.reset();
Expand Down Expand Up @@ -214,6 +232,10 @@ public void flush() throws IOException {
public Collection<String> getDependencies() {
return this.dependencies;
}

public boolean sawPotentialUnsupportedShowIncludesLine() {
return sawPotentialUnsupportedShowIncludesLine;
}
}

public FilterOutputStream getFilteredOutputStream(OutputStream outputStream) {
Expand All @@ -232,6 +254,11 @@ public Collection<Path> getDependencies(Path root) {
return Collections.unmodifiableCollection(dependenciesInPath);
}

public boolean sawPotentialUnsupportedShowIncludesLine() {
return filterShowIncludesOutputStream != null
&& filterShowIncludesOutputStream.sawPotentialUnsupportedShowIncludesLine();
}

@VisibleForTesting
Collection<String> getDependencies() {
return filterShowIncludesOutputStream.getDependencies();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ public void testNotMatch() throws IOException {
// Normal output message with newline
filterOutputStream.write(getBytes("I am compiling\n"));
assertThat(output.toString()).isEqualTo("I am compiling\n");
assertThat(showIncludesFilter.sawPotentialUnsupportedShowIncludesLine()).isFalse();
}

@Test
Expand All @@ -65,6 +66,7 @@ public void testNotMatchThenFlushing() throws IOException {
filterOutputStream.flush();
// flush to output should succeed
assertThat(output.toString()).isEqualTo("Still compiling");
assertThat(showIncludesFilter.sawPotentialUnsupportedShowIncludesLine()).isFalse();
}

@Test
Expand All @@ -78,6 +80,7 @@ public void testMatchPartOfNotePrefix() throws IOException {
filterOutputStream.write(getBytes("other info"));
filterOutputStream.flush();
assertThat(output.toString()).isEqualTo("Note: other info");
assertThat(showIncludesFilter.sawPotentialUnsupportedShowIncludesLine()).isFalse();
}

@Test
Expand All @@ -91,6 +94,7 @@ public void testMatchAllOfNotePrefix() throws IOException {
// 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.sawPotentialUnsupportedShowIncludesLine()).isFalse();
}

@Test
Expand All @@ -106,6 +110,7 @@ public void testFindHeaderFromAbsolutePathUnderExecrootBase() throws IOException
// It's a match, output should be filtered, dependency on bar.h should be found.
assertThat(output.toString()).isEmpty();
assertThat(showIncludesFilter.getDependencies()).contains("..\\__main__\\foo\\bar\\bar.h");
assertThat(showIncludesFilter.sawPotentialUnsupportedShowIncludesLine()).isFalse();
}

@Test
Expand All @@ -119,6 +124,7 @@ public void testFindHeaderFromAbsolutePathOutsideExecroot() throws IOException {
// 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");
assertThat(showIncludesFilter.sawPotentialUnsupportedShowIncludesLine()).isFalse();
}

@Test
Expand All @@ -127,6 +133,7 @@ public void testMatchSourceFileName() throws IOException {
// It's a match, output should be filtered, no dependency found.
assertThat(output.toString()).isEmpty();
assertThat(showIncludesFilter.getDependencies()).isEmpty();
assertThat(showIncludesFilter.sawPotentialUnsupportedShowIncludesLine()).isFalse();
}

@Test
Expand All @@ -138,5 +145,21 @@ public void testMatchPartOfSourceFileName() throws IOException {
filterOutputStream.write(getBytes(".h"));
filterOutputStream.flush();
assertThat(output.toString()).isEqualTo("foo.h");
assertThat(showIncludesFilter.sawPotentialUnsupportedShowIncludesLine()).isFalse();
}

@Test
public void testSawPotentialUnsupportedShowIncludesLine() throws IOException {
// MSVC output with French non-UTF-8 locale.
filterOutputStream.write(getBytes("Remarque"));
filterOutputStream.write(0xFF);
filterOutputStream.write(getBytes(": inclusion du fichier"));
filterOutputStream.write(0xFF);
filterOutputStream.write(getBytes(": C:\\\\foo.h\n"));
filterOutputStream.flush();

assertThat(output.toString()).isNotEmpty();
assertThat(showIncludesFilter.getDependencies()).isEmpty();
assertThat(showIncludesFilter.sawPotentialUnsupportedShowIncludesLine()).isTrue();
}
}

0 comments on commit b00f9c6

Please sign in to comment.