Skip to content

Commit

Permalink
Remove Event's BUILD dependencies on filesystem things
Browse files Browse the repository at this point in the history
This winds up pulling in all sorts of dependencies (filesystem, profiler) for
what's otherwise a low-level library.

Also, this makes it much more clear that the whole withOutErr thing exists
purely for ferrying action output around the system. Having events be
semi-silently backed by files has been a source of several issues in the past,
hopefully this rephrasing / indirection discourages future uses.

I feel like the code in this area is still somewhat awkward - I tried to look
past that for the most part and stay focused on cutting dependencies.

PiperOrigin-RevId: 393846789
  • Loading branch information
michajlo authored and copybara-github committed Aug 30, 2021
1 parent 26354a0 commit 19ee1aa
Show file tree
Hide file tree
Showing 5 changed files with 149 additions and 75 deletions.
2 changes: 0 additions & 2 deletions src/main/java/com/google/devtools/build/lib/events/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,7 @@ java_library(
name = "events",
srcs = glob(["*.java"]),
deps = [
"//src/main/java/com/google/devtools/build/lib/util/io",
"//src/main/java/com/google/devtools/build/lib/util/io:out-err",
"//src/main/java/com/google/devtools/build/lib/vfs:pathfragment",
"//src/main/java/net/starlark/java/eval",
"//src/main/java/net/starlark/java/syntax",
"//third_party:guava",
Expand Down
89 changes: 44 additions & 45 deletions src/main/java/com/google/devtools/build/lib/events/Event.java
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,6 @@
import static java.util.Comparator.comparing;

import com.google.common.collect.ImmutableClassToInstanceMap;
import com.google.devtools.build.lib.util.io.FileOutErr;
import com.google.devtools.build.lib.vfs.PathFragment;
import java.io.IOException;
import java.io.Serializable;
import java.util.Arrays;
Expand Down Expand Up @@ -173,9 +171,12 @@ public Event withTag(@Nullable String tag) {
return withProperty(String.class, tag);
}

/** Like {@link #withProperty(Class, Object)}, with {@code type.equals(FileOutErr.class)}. */
public Event withStdoutStderr(FileOutErr outErr) {
return withProperty(FileOutErr.class, outErr);
/**
* Returns a new event with the provided {@link ProcessOutput} property. See {@link #withProperty}
* for more specifics.
*/
public Event withProcessOutput(ProcessOutput processOutput) {
return withProperty(ProcessOutput.class, processOutput);
}

/**
Expand All @@ -189,61 +190,29 @@ public String getTag() {
return getProperty(String.class);
}

/** Indicates if a {@link FileOutErr} property is associated with this event. */
public boolean hasStdoutStderr() {
return getProperty(FileOutErr.class) != null;
}

/**
* Gets the path to the stdout associated with this event (which the caller must not access), or
* null if there is no such path.
*/
@Nullable
public PathFragment getStdOutPathFragment() {
FileOutErr outErr = getProperty(FileOutErr.class);
return outErr == null ? null : outErr.getOutputPathFragment();
}

/** Gets the size of the stdout associated with this event without reading it. */
public long getStdOutSize() throws IOException {
FileOutErr outErr = getProperty(FileOutErr.class);
return outErr == null ? 0 : outErr.outSize();
public ProcessOutput getProcessOutput() {
return getProperty(ProcessOutput.class);
}

/** Returns the stdout bytes associated with this event if any, and {@code null} otherwise. */
@Nullable
public byte[] getStdOut() {
FileOutErr outErr = getProperty(FileOutErr.class);
if (outErr == null) {
ProcessOutput processOutput = getProperty(ProcessOutput.class);
if (processOutput == null) {
return null;
}
return outErr.outAsBytes();
}

/**
* Gets the path to the stderr associated with this event (which the caller must not access), or
* null if there is no such path.
*/
@Nullable
public PathFragment getStdErrPathFragment() {
FileOutErr outErr = getProperty(FileOutErr.class);
return outErr == null ? null : outErr.getErrorPathFragment();
}

/** Gets the size of the stderr associated with this event without reading it. */
public long getStdErrSize() throws IOException {
FileOutErr outErr = getProperty(FileOutErr.class);
return outErr == null ? 0 : outErr.errSize();
return processOutput.getStdOut();
}

/** Returns the stderr bytes associated with this event if any, and {@code null} otherwise. */
@Nullable
public byte[] getStdErr() {
FileOutErr outErr = getProperty(FileOutErr.class);
if (outErr == null) {
ProcessOutput processOutput = getProperty(ProcessOutput.class);
if (processOutput == null) {
return null;
}
return outErr.errAsBytes();
return processOutput.getStdErr();
}

/**
Expand Down Expand Up @@ -463,4 +432,34 @@ public static <T> void replayEventsOn(
public static StarlarkThread.PrintHandler makeDebugPrintHandler(EventHandler h) {
return (thread, msg) -> h.handle(Event.debug(thread.getCallerLocation(), msg));
}

/**
* Process output associated with an event. The contents is just-about-certainly on disk, so
* special care should be taken when accessing it.
*
* <p>Note that this indirection exists partially for documentation sake, but also to keep the
* event library lightweight and broadly usable by avoiding bringing in all of the dependencies
* that come with dealing with process output (specifically the filesystem library).
*/
public interface ProcessOutput {
/**
* Returns the string representation of the path containing the process's stdout for
* logging/debugging purposes.
*/
String getStdOutPath();

long getStdOutSize() throws IOException;

byte[] getStdOut();

/**
* Returns the string representation of the path containing the process's stderr for
* logging/debugging purposes.
*/
String getStdErrPath();

long getStdErrSize() throws IOException;

byte[] getStdErr();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@
import com.google.devtools.build.lib.buildtool.buildevent.TestFilteringCompleteEvent;
import com.google.devtools.build.lib.clock.Clock;
import com.google.devtools.build.lib.events.Event;
import com.google.devtools.build.lib.events.Event.ProcessOutput;
import com.google.devtools.build.lib.events.EventHandler;
import com.google.devtools.build.lib.events.EventKind;
import com.google.devtools.build.lib.events.ExtendedEventHandler.FetchProgress;
Expand Down Expand Up @@ -385,7 +386,7 @@ private void handleLocked(Event event, @Nullable byte[] stdout, @Nullable byte[]

@Nullable
private byte[] getContentIfSmallEnough(
String name, long size, Supplier<byte[]> getContent, Supplier<PathFragment> getPath) {
String name, long size, Supplier<byte[]> getContent, Supplier<String> getPath) {
if (size == 0) {
// Avoid any possible I/O when we know it'll be empty anyway.
return null;
Expand All @@ -412,13 +413,20 @@ private void handleInternal(Event event) {
// much memory.
byte[] stdout = null;
byte[] stderr = null;
if (event.hasStdoutStderr()) {
ProcessOutput processOutput = event.getProcessOutput();
if (processOutput != null) {
stdout =
getContentIfSmallEnough(
"stdout", event.getStdOutSize(), event::getStdOut, event::getStdOutPathFragment);
"stdout",
processOutput.getStdOutSize(),
processOutput::getStdOut,
processOutput::getStdOutPath);
stderr =
getContentIfSmallEnough(
"stderr", event.getStdErrSize(), event::getStdErr, event::getStdErrPathFragment);
"stderr",
processOutput.getStdErrSize(),
processOutput::getStdErr,
processOutput::getStdErrPath);
}

if (debugAllEvents) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1645,7 +1645,7 @@ private boolean dumpRecordedOutErr(
}
if (outErrBuffer != null && outErrBuffer.hasRecordedOutput()) {
// Bind the output to the prefix event.
eventHandler.handle(prefixEvent.withStdoutStderr(outErrBuffer));
eventHandler.handle(prefixEvent.withProcessOutput(new ActionOutputEventData(outErrBuffer)));
} else {
eventHandler.handle(prefixEvent);
}
Expand Down Expand Up @@ -1736,4 +1736,43 @@ public ActionInput getInput(String execPath) {
return input != null ? input : perBuildFileCache.getInput(execPath);
}
}

/** Adapts a {@link FileOutErr} to an {@link Event.ProcessOutput}. */
private static class ActionOutputEventData implements Event.ProcessOutput {
private final FileOutErr fileOutErr;

private ActionOutputEventData(FileOutErr fileOutErr) {
this.fileOutErr = fileOutErr;
}

@Override
public String getStdOutPath() {
return fileOutErr.getOutputPathFragment().getPathString();
}

@Override
public long getStdOutSize() throws IOException {
return fileOutErr.outSize();
}

@Override
public byte[] getStdOut() {
return fileOutErr.outAsBytes();
}

@Override
public String getStdErrPath() {
return fileOutErr.getErrorPathFragment().getPathString();
}

@Override
public long getStdErrSize() throws IOException {
return fileOutErr.errSize();
}

@Override
public byte[] getStdErr() {
return fileOutErr.errAsBytes();
}
}
}
76 changes: 53 additions & 23 deletions src/test/java/com/google/devtools/build/lib/events/EventTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@

import com.google.common.collect.ImmutableList;
import com.google.common.testing.EqualsTester;
import com.google.devtools.build.lib.testutil.TestFileOutErr;
import com.google.devtools.build.lib.events.Event.ProcessOutput;
import net.starlark.java.eval.Mutability;
import net.starlark.java.eval.StarlarkSemantics;
import net.starlark.java.eval.StarlarkThread;
Expand Down Expand Up @@ -180,29 +180,59 @@ public void tagIsSameAsStringProperty() {
}

@Test
public void stdoutStderr() throws Exception {
public void testWithProcessOutput() throws Exception {
String stdoutPath = "/stdout";
byte[] stdout = "some stdout output".getBytes(UTF_8);
String stderrPath = "/stderr";
byte[] stderr = "some stderr error".getBytes(UTF_8);

ProcessOutput testProcessOutput =
new ProcessOutput() {
@Override
public String getStdOutPath() {
return stdoutPath;
}

@Override
public long getStdOutSize() {
return stdout.length;
}

@Override
public byte[] getStdOut() {
return stdout;
}

@Override
public String getStdErrPath() {
return stderrPath;
}

@Override
public long getStdErrSize() {
return stderr.length;
}

@Override
public byte[] getStdErr() {
return stderr;
}
};

Event event = Event.of(EventKind.WARNING, "myMessage");
TestFileOutErr testFileOutErr = new TestFileOutErr();
String stdoutText = "some stdout output";
String stderrText = "some stderr error";
testFileOutErr.printOut(stdoutText);
testFileOutErr.printErr(stderrText);
Event eventWithStdoutStderr = event.withStdoutStderr(testFileOutErr);

assertThat(event.hasStdoutStderr()).isFalse();
assertThat(eventWithStdoutStderr.hasStdoutStderr()).isTrue();

byte[] stdoutBytes = stdoutText.getBytes(UTF_8);
int stdoutLength = stdoutBytes.length;
assertThat(eventWithStdoutStderr.getStdOut()).isEqualTo(stdoutBytes);
assertThat(eventWithStdoutStderr.getStdOutSize()).isEqualTo(stdoutLength);
assertThat(eventWithStdoutStderr.getStdOut()).isEqualTo(stdoutBytes);

byte[] stderrBytes = stderrText.getBytes(UTF_8);
int stderrLength = stderrBytes.length;
assertThat(eventWithStdoutStderr.getStdErr()).isEqualTo(stderrBytes);
assertThat(eventWithStdoutStderr.getStdErrSize()).isEqualTo(stderrLength);
assertThat(eventWithStdoutStderr.getStdErr()).isEqualTo(stderrBytes);
Event eventWithProcessOutput = event.withProcessOutput(testProcessOutput);

assertThat(eventWithProcessOutput).isNotEqualTo(event);
assertThat(event.getProcessOutput()).isNull();
assertThat(eventWithProcessOutput.getProcessOutput()).isNotNull();

assertThat(eventWithProcessOutput.getStdOut()).isEqualTo(stdout);
assertThat(eventWithProcessOutput.getProcessOutput().getStdOut()).isEqualTo(stdout);
assertThat(eventWithProcessOutput.getProcessOutput().getStdOutSize()).isEqualTo(stdout.length);

assertThat(eventWithProcessOutput.getStdErr()).isEqualTo(stderr);
assertThat(eventWithProcessOutput.getProcessOutput().getStdErr()).isEqualTo(stderr);
assertThat(eventWithProcessOutput.getProcessOutput().getStdErrSize()).isEqualTo(stderr.length);
}

@Test
Expand Down

0 comments on commit 19ee1aa

Please sign in to comment.