From 19ee1aad6d5e9a747e10acdfc29e6c10ef7e946a Mon Sep 17 00:00:00 2001 From: michajlo Date: Mon, 30 Aug 2021 13:53:09 -0700 Subject: [PATCH] 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 --- .../google/devtools/build/lib/events/BUILD | 2 - .../devtools/build/lib/events/Event.java | 89 +++++++++---------- .../build/lib/runtime/UiEventHandler.java | 16 +++- .../lib/skyframe/SkyframeActionExecutor.java | 41 ++++++++- .../devtools/build/lib/events/EventTest.java | 76 +++++++++++----- 5 files changed, 149 insertions(+), 75 deletions(-) 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 cfb4d2d0d51e03..913884e6e5a7b2 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 @@ 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", 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 bbd8c526a58c23..a74c70b936fee2 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 @@ 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); } /** @@ -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(); } /** @@ -463,4 +432,34 @@ public static 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. + * + *

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 1c56e04ed90ca8..ae84347ffb3b94 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 @@ private void handleLocked(Event event, @Nullable byte[] stdout, @Nullable byte[] @Nullable private byte[] getContentIfSmallEnough( - String name, long size, Supplier getContent, Supplier getPath) { + String name, long size, Supplier getContent, Supplier getPath) { if (size == 0) { // Avoid any possible I/O when we know it'll be empty anyway. return null; @@ -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) { 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 57e85e106f49e7..5076621b1970dc 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 @@ 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); } @@ -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(); + } + } } 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 4da3d9c213fc15..9ac764551d7b7b 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 @@ 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