Remove Event's BUILD dependencies on filesystem things
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
diff --git a/src/main/java/com/google/devtools/build/lib/events/BUILD b/src/main/java/com/google/devtools/build/lib/events/BUILD
index cfb4d2d..913884e 100644
--- a/src/main/java/com/google/devtools/build/lib/events/BUILD
+++ b/src/main/java/com/google/devtools/build/lib/events/BUILD
@@ -12,9 +12,7 @@
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",
diff --git a/src/main/java/com/google/devtools/build/lib/events/Event.java b/src/main/java/com/google/devtools/build/lib/events/Event.java
index bbd8c52..a74c70b 100644
--- a/src/main/java/com/google/devtools/build/lib/events/Event.java
+++ b/src/main/java/com/google/devtools/build/lib/events/Event.java
@@ -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;
@@ -173,9 +171,12 @@
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);
}
/**
@@ -189,61 +190,29 @@
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();
}
/**
@@ -463,4 +432,34 @@
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();
+ }
}
diff --git a/src/main/java/com/google/devtools/build/lib/runtime/UiEventHandler.java b/src/main/java/com/google/devtools/build/lib/runtime/UiEventHandler.java
index 1c56e04..ae84347 100644
--- a/src/main/java/com/google/devtools/build/lib/runtime/UiEventHandler.java
+++ b/src/main/java/com/google/devtools/build/lib/runtime/UiEventHandler.java
@@ -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;
@@ -385,7 +386,7 @@
@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;
@@ -412,13 +413,20 @@
// 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) {
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java
index 57e85e1..5076621 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java
@@ -1645,7 +1645,7 @@
}
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);
}
@@ -1736,4 +1736,43 @@
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();
+ }
+ }
}
diff --git a/src/test/java/com/google/devtools/build/lib/events/EventTest.java b/src/test/java/com/google/devtools/build/lib/events/EventTest.java
index 4da3d9c..9ac7645 100644
--- a/src/test/java/com/google/devtools/build/lib/events/EventTest.java
+++ b/src/test/java/com/google/devtools/build/lib/events/EventTest.java
@@ -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;
@@ -180,29 +180,59 @@
}
@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);
+ Event eventWithProcessOutput = event.withProcessOutput(testProcessOutput);
- assertThat(event.hasStdoutStderr()).isFalse();
- assertThat(eventWithStdoutStderr.hasStdoutStderr()).isTrue();
+ assertThat(eventWithProcessOutput).isNotEqualTo(event);
+ assertThat(event.getProcessOutput()).isNull();
+ assertThat(eventWithProcessOutput.getProcessOutput()).isNotNull();
- 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);
+ assertThat(eventWithProcessOutput.getStdOut()).isEqualTo(stdout);
+ assertThat(eventWithProcessOutput.getProcessOutput().getStdOut()).isEqualTo(stdout);
+ assertThat(eventWithProcessOutput.getProcessOutput().getStdOutSize()).isEqualTo(stdout.length);
- 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);
+ assertThat(eventWithProcessOutput.getStdErr()).isEqualTo(stderr);
+ assertThat(eventWithProcessOutput.getProcessOutput().getStdErr()).isEqualTo(stderr);
+ assertThat(eventWithProcessOutput.getProcessOutput().getStdErrSize()).isEqualTo(stderr.length);
}
@Test
