Skip to content

Commit

Permalink
Remove the entire FileOutErr filtering logic
Browse files Browse the repository at this point in the history 
This is only used on Windows, if parse_showincludes is active. Instead
of filtering on FileOutErr, we ask the SpawnRunner to write the output
to a temporary file, and then post-process that in CppCompileAction.

The previous approach was incompatible with most SpawnRunner
implementations, which do not guarantee that the output is written
through the FileOutErr output streams, but may redirect output to the
underlying files directly. It is a design flaw that FileOutErr creates
this expectation; it would be better to only pass in a pair of Path
objects for the action.

This is in preparation for switching the LocalSpawnRunner to always
redirect output from the local process to a file. This removes the need
for pumping stdout/stderr through the Bazel process (although this was
already not always happening, e.g., when using a process wrapper), and
thereby reducing the need to create two additional threads per
subprocess to perform such pumping.

PiperOrigin-RevId: 223173023
  • Loading branch information
@ulfjack
ulfjack authored and Copybara-Service committed Nov 28, 2018
1 parent 86b4ed3 commit cbacbb9
Show file tree
Hide file tree
Showing 4 changed files with 52 additions and 66 deletions.
63 changes: 51 additions & 12 deletions src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Iterables;
import com.google.common.collect.Sets;
import com.google.common.io.ByteStreams;
import com.google.devtools.build.lib.actions.AbstractAction;
import com.google.devtools.build.lib.actions.ActionEnvironment;
import com.google.devtools.build.lib.actions.ActionExecutionContext;
Expand Down Expand Up @@ -62,6 +63,7 @@
import com.google.devtools.build.lib.util.DependencySet;
import com.google.devtools.build.lib.util.Fingerprint;
import com.google.devtools.build.lib.util.ShellEscaper;
import com.google.devtools.build.lib.util.io.FileOutErr;
import com.google.devtools.build.lib.vfs.FileSystemUtils;
import com.google.devtools.build.lib.vfs.Path;
import com.google.devtools.build.lib.vfs.PathFragment;
Expand All @@ -70,6 +72,7 @@
import com.google.devtools.build.skyframe.SkyKey;
import com.google.devtools.build.skyframe.SkyValue;
import java.io.IOException;
import java.io.InputStream;
import java.util.ArrayList;
import java.util.Collection;
import java.util.HashSet;
Expand Down Expand Up @@ -1108,34 +1111,70 @@ public ActionResult execute(ActionExecutionContext actionExecutionContext)
throws ActionExecutionException, InterruptedException {
setModuleFileFlags();
CppCompileActionContext.Reply reply;
ShowIncludesFilter showIncludesFilterForStdout = null;
ShowIncludesFilter showIncludesFilterForStderr = null;
// If parse_showincludes feature is enabled, instead of parsing dotD file we parse the output of
// cl.exe caused by /showIncludes option.
if (featureConfiguration.isEnabled(CppRuleClasses.PARSE_SHOWINCLUDES)) {
showIncludesFilterForStdout = new ShowIncludesFilter(getSourceFile().getFilename());
showIncludesFilterForStderr = new ShowIncludesFilter(getSourceFile().getFilename());
actionExecutionContext.getFileOutErr().setOutputFilter(showIncludesFilterForStdout);
actionExecutionContext.getFileOutErr().setErrorFilter(showIncludesFilterForStderr);
}

if (!shouldScanDotdFiles()) {
updateActionInputs(NestedSetBuilder.wrap(Order.STABLE_ORDER, additionalInputs));
}

ActionExecutionContext spawnContext;
ShowIncludesFilter showIncludesFilterForStdout;
ShowIncludesFilter showIncludesFilterForStderr;
if (featureConfiguration.isEnabled(CppRuleClasses.PARSE_SHOWINCLUDES)) {
showIncludesFilterForStdout = new ShowIncludesFilter(getSourceFile().getFilename());
showIncludesFilterForStderr = new ShowIncludesFilter(getSourceFile().getFilename());
FileOutErr originalOutErr = actionExecutionContext.getFileOutErr();
FileOutErr tempOutErr = originalOutErr.childOutErr();
spawnContext = actionExecutionContext.withFileOutErr(tempOutErr);
} else {
spawnContext = actionExecutionContext;
showIncludesFilterForStdout = null;
showIncludesFilterForStderr = null;
}

// We want the code to be executed in the finally clause, but we need to throw IOException if
// it failed, or add the IOException as a suppressed exception if the try block threw an
// exception in the first place. Errorprone recommends using AutoCloseable for this.
final class Defer implements AutoCloseable {
@Override
public void close() throws IOException {
// If parse_showincludes feature is enabled, instead of parsing dotD file we parse the
// output of cl.exe caused by /showIncludes option.
if (featureConfiguration.isEnabled(CppRuleClasses.PARSE_SHOWINCLUDES)) {
FileOutErr tempOutErr = spawnContext.getFileOutErr();
FileOutErr outErr = actionExecutionContext.getFileOutErr();
tempOutErr.close();
if (tempOutErr.hasRecordedStdout()) {
try (InputStream in = tempOutErr.getOutputPath().getInputStream()) {
ByteStreams.copy(
in,
showIncludesFilterForStdout.getFilteredOutputStream(outErr.getOutputStream()));
}
}
if (tempOutErr.hasRecordedStderr()) {
try (InputStream in = tempOutErr.getErrorPath().getInputStream()) {
ByteStreams.copy(
in, showIncludesFilterForStderr.getFilteredOutputStream(outErr.getErrorStream()));
}
}
}
}
}

List<SpawnResult> spawnResults;
try {
try (Defer ignored = new Defer()) {
CppCompileActionResult cppCompileActionResult =
actionExecutionContext
.getContext(CppCompileActionContext.class)
.execWithReply(this, actionExecutionContext);
.execWithReply(this, spawnContext);
reply = cppCompileActionResult.contextReply();
spawnResults = cppCompileActionResult.spawnResults();
} catch (ExecException e) {
throw e.toActionExecutionException(
"C++ compilation of rule '" + getOwner().getLabel() + "'",
actionExecutionContext.getVerboseFailures(),
this);
} catch (IOException e) {
throw new ActionExecutionException(e, this, false);
} finally {
clearAdditionalInputs();
}
Expand Down
4 changes: 1 addition & 3 deletions src/main/java/com/google/devtools/build/lib/rules/cpp/ShowIncludesFilter.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@
package com.google.devtools.build.lib.rules.cpp;

import com.google.common.annotations.VisibleForTesting;
import com.google.devtools.build.lib.util.io.FileOutErr;
import com.google.devtools.build.lib.vfs.Path;
import java.io.ByteArrayOutputStream;
import java.io.FilterOutputStream;
Expand All @@ -35,7 +34,7 @@
* <p>Also suppress the basename of source file, which is printed unconditionally by MSVC compiler,
* there is no way to turn it off.
*/
public class ShowIncludesFilter implements FileOutErr.OutputFilter {
public class ShowIncludesFilter {

private FilterShowIncludesOutputStream filterShowIncludesOutputStream;
private final String sourceFileName;
Expand Down Expand Up @@ -93,7 +92,6 @@ public Collection<String> getDependencies() {
}
}

@Override
public FilterOutputStream getFilteredOutputStream(OutputStream outputStream) {
filterShowIncludesOutputStream =
new FilterShowIncludesOutputStream(outputStream, sourceFileName);
Expand Down
36 changes: 0 additions & 36 deletions src/main/java/com/google/devtools/build/lib/util/io/FileOutErr.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@
import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe;
import com.google.devtools.build.lib.vfs.FileSystemUtils;
import com.google.devtools.build.lib.vfs.Path;
import java.io.FilterOutputStream;
import java.io.IOException;
import java.io.InputStream;
import java.io.OutputStream;
Expand Down Expand Up @@ -87,16 +86,6 @@ private FileOutErr(OutputStream stream) {
super(stream, stream);
}

// Set a filter for FileOutputStream
public void setOutputFilter(OutputFilter outputFilter) {
getFileOutputStream().setFilter(outputFilter);
}

// Set a filter for FileErrorStream
public void setErrorFilter(OutputFilter outputFilter) {
getFileErrorStream().setFilter(outputFilter);
}

/**
* Returns true if any output was recorded.
*/
Expand Down Expand Up @@ -249,13 +238,6 @@ private abstract static class AbstractFileRecordingOutputStream extends OutputSt

/** Closes and deletes the output. */
abstract void clear() throws IOException;

/**
* Set a Filter for the output
*
* @param outputFilter
*/
abstract void setFilter(OutputFilter outputFilter);
}

/**
Expand Down Expand Up @@ -301,10 +283,6 @@ void dumpOut(OutputStream out) {
public void clear() {
}

@Override
void setFilter(OutputFilter outputFilter) {}


@Override
public void write(byte[] b, int off, int len) {
}
Expand All @@ -318,11 +296,6 @@ public void write(byte[] b) {
}
}

/** An interface to get a filtered output stream from the original one. */
public interface OutputFilter {
FilterOutputStream getFilteredOutputStream(OutputStream outputStream);
}

/**
* An output stream that captures all output into a file. The file is created only if output is
* received.
Expand All @@ -346,7 +319,6 @@ protected static class FileRecordingOutputStream extends AbstractFileRecordingOu
private final Path outputFile;
private OutputStream outputStream;
private String error;
private OutputFilter outputFilter;
private boolean mightHaveOutput = false;

protected FileRecordingOutputStream(Path outputFile) {
Expand Down Expand Up @@ -379,9 +351,6 @@ private OutputStream getOutputStream() throws IOException {
// you should hold the lock before you invoke this method
if (outputStream == null) {
outputStream = outputFile.getOutputStream();
if (outputFilter != null) {
outputStream = outputFilter.getFilteredOutputStream(outputStream);
}
}
return outputStream;
}
Expand All @@ -398,11 +367,6 @@ public synchronized void clear() throws IOException {
mightHaveOutput = false;
}

@Override
void setFilter(OutputFilter outputFilter) {
this.outputFilter = outputFilter;
}

/**
* Called whenever the FileRecordingOutputStream finds an error.
*/
Expand Down
15 changes: 0 additions & 15 deletions src/test/java/com/google/devtools/build/lib/rules/cpp/ShowIncludesFilterTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@
import static com.google.common.truth.Truth.assertThat;
import static java.nio.charset.StandardCharsets.UTF_8;

import com.google.devtools.build.lib.util.io.FileOutErr;
import com.google.devtools.build.lib.vfs.FileSystem;
import com.google.devtools.build.lib.vfs.inmemoryfs.InMemoryFileSystem;
import java.io.ByteArrayOutputStream;
Expand All @@ -36,7 +35,6 @@ public class ShowIncludesFilterTest {
private ByteArrayOutputStream output;
private FilterOutputStream filterOutputStream;
private FileSystem fs;
private FileOutErr outErr;

@Before
public void setUpOutputStreams() throws IOException {
Expand Down Expand Up @@ -112,17 +110,4 @@ public void testMatchPartOfSourceFileName() throws IOException {
filterOutputStream.flush();
assertThat(output.toString()).isEqualTo("foo.h");
}

@Test
public void testOnFileOutErr() throws IOException {
outErr = new FileOutErr(fs.getPath("/out/stdout"), fs.getPath("/out/stderr"));
ShowIncludesFilter showIncludesFilterForStdout = new ShowIncludesFilter("foo.cpp");
ShowIncludesFilter showIncludesFilterForStderr = new ShowIncludesFilter("foo.cpp");
outErr.setOutputFilter(showIncludesFilterForStdout);
outErr.setErrorFilter(showIncludesFilterForStderr);
outErr.getOutputStream().write(getBytes("Note: including file: bar1.h\n"));
outErr.getErrorStream().write(getBytes("Note: including file: bar2.h\n"));
assertThat(showIncludesFilterForStdout.getDependencies()).contains("bar1.h");
assertThat(showIncludesFilterForStderr.getDependencies()).contains("bar2.h");
}
}

0 comments on commit cbacbb9

Please sign in to comment.