Clean up null handling in BuildStartingEvent and use AutoValue fo…

…r it.

Specify nullable values in `BuildStartingEvent` and remove unnecessary null handling. In particular, some of the member are never null outside tests, fix the tests and remove unnecessary checks. Use `AutoValue` for the event to remove some boilerplate code.

PiperOrigin-RevId: 453493711
Change-Id: I1f4fbc8d57b53269a1e276774dd5bb2eaa3bb9c3

src/main/java/com/google/devtools/build/lib/buildtool/BuildTool.java

@@ -165,7 +165,7 @@ public void buildTargets(BuildRequest request, BuildResult result, TargetValidat
     boolean catastrophe = false;
     try {
       try (SilentCloseable c = Profiler.instance().profile("BuildStartingEvent")) {
-        env.getEventBus().post(new BuildStartingEvent(env, request));
+        env.getEventBus().post(BuildStartingEvent.create(env, request));
       }
       logger.atInfo().log("Build identifier: %s", request.getId());
 

src/main/java/com/google/devtools/build/lib/buildtool/buildevent/BuildStartingEvent.java

@@ -14,6 +14,8 @@
 
 package com.google.devtools.build.lib.buildtool.buildevent;
 
+import com.google.auto.value.AutoValue;
+import com.google.common.annotations.VisibleForTesting;
 import com.google.common.collect.ImmutableList;
 import com.google.devtools.build.lib.analysis.BlazeVersionInfo;
 import com.google.devtools.build.lib.buildeventstream.BuildEvent;
@@ -27,62 +29,56 @@
 import com.google.devtools.build.lib.runtime.CommandEnvironment;
 import com.google.devtools.build.lib.runtime.CommandLineEvent;
 import com.google.protobuf.util.Timestamps;
-import java.util.Collection;
+import javax.annotation.Nullable;
 
 /**
  * This event is fired from BuildTool#startRequest(). At this point, the set of target patters are
  * known, but have yet to be parsed.
  */
-public final class BuildStartingEvent implements BuildEvent {
-  private final String outputFileSystem;
-  private final BuildRequest request;
-  private final String workspace;
-  private final String pwd;
+@AutoValue
+public abstract class BuildStartingEvent implements BuildEvent {
+  BuildStartingEvent() {}
+
+  /** Returns the name of output file system. */
+  public abstract String outputFileSystem();
+
+  /** Returns the active BuildRequest. */
+  public abstract BuildRequest request();
+
+  @Nullable
+  abstract String workspace();
+
+  abstract String pwd();
 
   /**
    * Construct the BuildStartingEvent
    *
    * @param request the build request.
    * @param env the environment of the request invocation.
    */
-  public BuildStartingEvent(CommandEnvironment env, BuildRequest request) {
-    this.request = request;
-    if (env != null) {
-      this.outputFileSystem = env.determineOutputFileSystem();
-      if (env.getDirectories().getWorkspace() != null) {
-        this.workspace = env.getDirectories().getWorkspace().toString();
-      } else {
-        this.workspace = null;
-      }
-      this.pwd = env.getWorkingDirectory().toString();
-    } else {
-      this.workspace = null;
-      this.pwd = null;
-      this.outputFileSystem = null;
-    }
-  }
-
-  /**
-   * @return the output file system.
-   */
-  public String getOutputFileSystem() {
-    return outputFileSystem;
+  public static BuildStartingEvent create(CommandEnvironment env, BuildRequest request) {
+    return create(
+        env.determineOutputFileSystem(),
+        request,
+        env.getDirectories().getWorkspace() != null
+            ? env.getDirectories().getWorkspace().toString()
+            : null,
+        env.getWorkingDirectory().toString());
   }
 
-  /**
-   * @return the active BuildRequest.
-   */
-  public BuildRequest getRequest() {
-    return request;
+  @VisibleForTesting
+  public static BuildStartingEvent create(
+      String outputFileSystem, BuildRequest request, @Nullable String workspace, String pwd) {
+    return new AutoValue_BuildStartingEvent(outputFileSystem, request, workspace, pwd);
   }
 
   @Override
-  public BuildEventId getEventId() {
+  public final BuildEventId getEventId() {
     return BuildEventIdUtil.buildStartedId();
   }
 
   @Override
-  public Collection<BuildEventId> getChildrenEvents() {
+  public final ImmutableList<BuildEventId> getChildrenEvents() {
     return ImmutableList.of(
         ProgressEvent.INITIAL_PROGRESS_UPDATE,
         BuildEventIdUtil.unstructuredCommandlineId(),
@@ -92,26 +88,24 @@ public Collection<BuildEventId> getChildrenEvents() {
         BuildEventIdUtil.buildMetadataId(),
         BuildEventIdUtil.optionsParsedId(),
         BuildEventIdUtil.workspaceStatusId(),
-        BuildEventIdUtil.targetPatternExpanded(request.getTargets()),
+        BuildEventIdUtil.targetPatternExpanded(request().getTargets()),
         BuildEventIdUtil.buildFinished());
   }
 
   @Override
-  public BuildEventStreamProtos.BuildEvent asStreamProto(BuildEventContext converters) {
+  public final BuildEventStreamProtos.BuildEvent asStreamProto(BuildEventContext converters) {
     BuildEventStreamProtos.BuildStarted.Builder started =
         BuildEventStreamProtos.BuildStarted.newBuilder()
-            .setUuid(request.getId().toString())
-            .setStartTime(Timestamps.fromMillis(request.getStartTime()))
-            .setStartTimeMillis(request.getStartTime())
+            .setUuid(request().getId().toString())
+            .setStartTime(Timestamps.fromMillis(request().getStartTime()))
+            .setStartTimeMillis(request().getStartTime())
             .setBuildToolVersion(BlazeVersionInfo.instance().getVersion())
-            .setOptionsDescription(request.getOptionsDescription())
-            .setCommand(request.getCommandName())
-            .setServerPid(ProcessHandle.current().pid());
-    if (pwd != null) {
-      started.setWorkingDirectory(pwd);
-    }
-    if (workspace != null) {
-      started.setWorkspaceDirectory(workspace);
+            .setOptionsDescription(request().getOptionsDescription())
+            .setCommand(request().getCommandName())
+            .setServerPid(ProcessHandle.current().pid())
+            .setWorkingDirectory(pwd());
+    if (workspace() != null) {
+      started.setWorkspaceDirectory(workspace());
     }
     return GenericBuildEvent.protoChaining(this).setStarted(started.build()).build();
   }

src/main/java/com/google/devtools/build/lib/runtime/BuildEventStreamer.java

@@ -462,7 +462,7 @@ public void buildEvent(BuildEvent event) {
     }
 
     if (event instanceof BuildStartingEvent) {
-      BuildRequest buildRequest = ((BuildStartingEvent) event).getRequest();
+      BuildRequest buildRequest = ((BuildStartingEvent) event).request();
       isTestCommand =
           "test".equals(buildRequest.getCommandName())
               || "coverage".equals(buildRequest.getCommandName());

src/main/java/com/google/devtools/build/lib/runtime/TargetSummaryPublisher.java

@@ -64,9 +64,9 @@ public TargetSummaryPublisher(EventBus eventBus) {
    */
   @Subscribe
   public void buildStarting(BuildStartingEvent event) {
-    int count = event.getRequest().getAspects().size();
+    int count = event.request().getAspects().size();
     checkState(
-        aspectCount.compareAndSet(/* expect= */ -1, count),
+        aspectCount.compareAndSet(/*expectedValue=*/ -1, count),
         "Duplicate BuildStartingEvent with %s aspects but already have %s",
         count,
         aspectCount);

src/main/java/com/google/devtools/build/lib/worker/WorkerModule.java

@@ -77,7 +77,7 @@ public void cleanStarting(CleanStartingEvent event) {
    */
   @Subscribe
   public void buildStarting(BuildStartingEvent event) {
-    WorkerOptions options = event.getRequest().getOptions(WorkerOptions.class);
+    WorkerOptions options = event.request().getOptions(WorkerOptions.class);
     if (workerFactory != null) {
       workerFactory.setReporter(options.workerVerbose ? env.getReporter() : null);
     }

src/test/java/com/google/devtools/build/lib/runtime/BuildEventStreamerTest.java

@@ -1303,7 +1303,7 @@ public void testSuccessfulActionsCanBePublished() {
             /*stderr=*/ null,
             /*actionMetadataLogs=*/ ImmutableList.of(),
             ErrorTiming.BEFORE_EXECUTION,
-            /* isInMemoryFs= */ false);
+            /*isInMemoryFs=*/ false);
 
     streamer.buildEvent(SUCCESSFUL_ACTION_EXECUTED_EVENT);
     streamer.buildEvent(failedActionExecutedEvent);
@@ -1517,7 +1517,7 @@ private static ActionExecutedEvent createActionExecutedEventWithLogs(
         /*stderr=*/ null,
         metadataLogs,
         ErrorTiming.NO_ERROR,
-        /* isInMemoryFs= */ false);
+        /*isInMemoryFs=*/ false);
   }
 
   @Test

src/test/java/com/google/devtools/build/lib/runtime/UiEventHandlerStdOutAndStdErrTest.java

@@ -80,7 +80,9 @@ private void createUiEventHandler(UiOptions uiOptions) {
 
     uiEventHandler =
         new UiEventHandler(outErr, uiOptions, new ManualClock(), /*workspacePathFragment=*/ null);
-    uiEventHandler.buildStarted(new BuildStartingEvent(/*env=*/ null, mock(BuildRequest.class)));
+    uiEventHandler.buildStarted(
+        BuildStartingEvent.create(
+            "outputFileSystemType", mock(BuildRequest.class), /*workspace=*/ null, "/pwd"));
   }
 
   @Test

src/test/java/com/google/devtools/build/lib/skyframe/OutputsInvalidationIntegrationTest.java

@@ -54,6 +54,7 @@ public final class OutputsInvalidationIntegrationTest extends BuildIntegrationTe
   public void prepareOutputServiceMock()
       throws BuildFailedException, AbruptExitException, InterruptedException, IOException {
     when(outputService.actionFileSystemType()).thenReturn(ActionFileSystemType.DISABLED);
+    when(outputService.getFilesSystemName()).thenReturn("fileSystemName");
     when(outputService.startBuild(any(), any(), anyBoolean()))
         .thenReturn(ModifiedFileSet.EVERYTHING_MODIFIED);
   }

src/test/java/com/google/devtools/build/lib/worker/WorkerModuleTest.java

@@ -41,7 +41,6 @@
 import com.google.devtools.common.options.OptionsParsingException;
 import com.google.devtools.common.options.OptionsParsingResult;
 import java.io.IOException;
-import org.junit.Before;
 import org.junit.Rule;
 import org.junit.Test;
 import org.junit.runner.RunWith;
@@ -58,14 +57,10 @@ public class WorkerModuleTest {
   @Mock BuildRequest request;
   @Mock OptionsParsingResult startupOptionsProvider;
 
-  private FileSystem fs;
+  private final FileSystem fs =
+      new InMemoryFileSystem(BlazeClock.instance(), DigestHashFunction.SHA256);
   private StoredEventHandler storedEventHandler;
 
-  @Before
-  public void setUp() {
-    fs = new InMemoryFileSystem(BlazeClock.instance(), DigestHashFunction.SHA256);
-  }
-
   @Test
   public void buildStarting_createsPools()
       throws AbruptExitException, IOException, InterruptedException {
@@ -75,7 +70,7 @@ public void buildStarting_createsPools()
     setupEnvironment("/outputRoot");
 
     module.beforeCommand(env);
-    module.buildStarting(new BuildStartingEvent(env, request));
+    module.buildStarting(BuildStartingEvent.create(env, request));
 
     assertThat(storedEventHandler.getEvents()).isEmpty();
     assertThat(fs.getPath("/outputRoot/outputBase/bazel-workers").exists()).isFalse();
@@ -96,7 +91,7 @@ public void buildStarting_noRestartOnSandboxChange() throws IOException, AbruptE
     setupEnvironment("/outputRoot");
 
     module.beforeCommand(env);
-    module.buildStarting(new BuildStartingEvent(env, request));
+    module.buildStarting(BuildStartingEvent.create(env, request));
     assertThat(storedEventHandler.getEvents()).isEmpty();
 
     Path workerDir = fs.getPath("/outputRoot/outputBase/bazel-workers");
@@ -106,7 +101,7 @@ public void buildStarting_noRestartOnSandboxChange() throws IOException, AbruptE
     WorkerPool oldPool = module.workerPool;
     options.workerSandboxing = !options.workerSandboxing;
     module.beforeCommand(env);
-    module.buildStarting(new BuildStartingEvent(env, request));
+    module.buildStarting(BuildStartingEvent.create(env, request));
     assertThat(storedEventHandler.getEvents()).isEmpty();
     assertThat(module.workerPool).isSameInstanceAs(oldPool);
     assertThat(aLog.exists()).isTrue();
@@ -122,7 +117,7 @@ public void buildStarting_workersDestroyedOnRestart()
     setupEnvironment("/outputRoot");
 
     module.beforeCommand(env);
-    module.buildStarting(new BuildStartingEvent(env, request));
+    module.buildStarting(BuildStartingEvent.create(env, request));
     WorkerKey workerKey = TestUtils.createWorkerKey(JSON, fs, true);
     Worker worker = module.workerPool.borrowObject(workerKey);
     assertThat(worker.workerKey).isEqualTo(workerKey);
@@ -138,7 +133,7 @@ public void buildStarting_workersDestroyedOnRestart()
     WorkerPool oldPool = module.workerPool;
     options.highPriorityWorkers = ImmutableList.of("Foobar");
     module.beforeCommand(env);
-    module.buildStarting(new BuildStartingEvent(env, request));
+    module.buildStarting(BuildStartingEvent.create(env, request));
     assertThat(storedEventHandler.getEvents()).hasSize(1);
     assertThat(storedEventHandler.getEvents().get(0).getMessage())
         .contains("Worker pool configuration has changed");
@@ -154,7 +149,7 @@ public void buildStarting_restartsOnOutputbaseChanges() throws IOException, Abru
     setupEnvironment("/outputRoot");
 
     module.beforeCommand(env);
-    module.buildStarting(new BuildStartingEvent(env, request));
+    module.buildStarting(BuildStartingEvent.create(env, request));
     assertThat(storedEventHandler.getEvents()).isEmpty();
 
     // Log file from old root, doesn't get cleaned
@@ -166,7 +161,7 @@ public void buildStarting_restartsOnOutputbaseChanges() throws IOException, Abru
     WorkerPool oldPool = module.workerPool;
     setupEnvironment("/otherRootDir");
     module.beforeCommand(env);
-    module.buildStarting(new BuildStartingEvent(env, request));
+    module.buildStarting(BuildStartingEvent.create(env, request));
     assertThat(storedEventHandler.getEvents()).hasSize(1);
     assertThat(storedEventHandler.getEvents().get(0).getMessage())
         .contains("Worker factory configuration has changed");
@@ -190,7 +185,7 @@ public void buildStarting_clearsLogsOnFactoryCreation() throws IOException, Abru
     oldLog.createSymbolicLink(PathFragment.EMPTY_FRAGMENT);
 
     module.beforeCommand(env);
-    module.buildStarting(new BuildStartingEvent(env, request));
+    module.buildStarting(BuildStartingEvent.create(env, request));
 
     assertThat(storedEventHandler.getEvents()).isEmpty();
     assertThat(fs.getPath("/outputRoot/outputBase/bazel-workers").exists()).isTrue();
@@ -206,7 +201,7 @@ public void buildStarting_restartsOnHiPrioChanges() throws IOException, AbruptEx
 
     module.beforeCommand(env);
     // Check that new pools/factories are made with default options
-    module.buildStarting(new BuildStartingEvent(env, request));
+    module.buildStarting(BuildStartingEvent.create(env, request));
     assertThat(storedEventHandler.getEvents()).isEmpty();
 
     // Logs are only cleared on factory reset, not on pool reset, so this file should survive
@@ -218,7 +213,7 @@ public void buildStarting_restartsOnHiPrioChanges() throws IOException, AbruptEx
     WorkerPool oldPool = module.workerPool;
     options.highPriorityWorkers = ImmutableList.of("foo");
     module.beforeCommand(env);
-    module.buildStarting(new BuildStartingEvent(env, request));
+    module.buildStarting(BuildStartingEvent.create(env, request));
     assertThat(storedEventHandler.getEvents()).hasSize(1);
     assertThat(storedEventHandler.getEvents().get(0).getMessage())
         .contains("Worker pool configuration has changed");
@@ -236,13 +231,13 @@ public void buildStarting_restartsOnNumMultiplexWorkersChanges()
 
     module.beforeCommand(env);
     // Check that new pools/factories are made with default options
-    module.buildStarting(new BuildStartingEvent(env, request));
+    module.buildStarting(BuildStartingEvent.create(env, request));
     assertThat(storedEventHandler.getEvents()).isEmpty();
 
     WorkerPool oldPool = module.workerPool;
     options.workerMaxMultiplexInstances = Lists.newArrayList(Maps.immutableEntry("foo", 42));
     module.beforeCommand(env);
-    module.buildStarting(new BuildStartingEvent(env, request));
+    module.buildStarting(BuildStartingEvent.create(env, request));
     assertThat(storedEventHandler.getEvents()).hasSize(1);
     assertThat(storedEventHandler.getEvents().get(0).getMessage())
         .contains("Worker pool configuration has changed");
@@ -259,34 +254,32 @@ public void buildStarting_restartsOnNumWorkersChanges() throws IOException, Abru
 
     module.beforeCommand(env);
     // Check that new pools/factories are made with default options
-    module.buildStarting(new BuildStartingEvent(env, request));
+    module.buildStarting(BuildStartingEvent.create(env, request));
     assertThat(storedEventHandler.getEvents()).isEmpty();
 
     WorkerPool oldPool = module.workerPool;
     options.workerMaxInstances = Lists.newArrayList(Maps.immutableEntry("bar", 3));
     module.beforeCommand(env);
-    module.buildStarting(new BuildStartingEvent(env, request));
+    module.buildStarting(BuildStartingEvent.create(env, request));
     assertThat(storedEventHandler.getEvents()).hasSize(1);
     assertThat(storedEventHandler.getEvents().get(0).getMessage())
         .contains("Worker pool configuration has changed");
     assertThat(module.workerPool).isNotSameInstanceAs(oldPool);
   }
 
   @Test
-  public void buildStarting_survivesNoWorkerDir()
-      throws IOException, AbruptExitException, InterruptedException {
+  public void buildStarting_survivesNoWorkerDir() throws Exception {
     WorkerModule module = new WorkerModule();
     WorkerOptions options = WorkerOptions.DEFAULTS;
 
     when(request.getOptions(WorkerOptions.class)).thenReturn(options);
     setupEnvironment("/outputRoot");
 
     module.beforeCommand(env);
-    Path workerDir =
-        env.getOutputBase().getRelative(env.getRuntime().getProductName() + "-workers");
+    Path workerDir = fs.getPath("/outputRoot/outputBase/bazel-workers");
 
     // Check that new pools/factories can be created without a worker dir.
-    module.buildStarting(new BuildStartingEvent(env, request));
+    module.buildStarting(BuildStartingEvent.create(env, request));
 
     // But once we try to get a worker, it should fail. This forces a situation where we can't
     // have a workerDir.
@@ -322,5 +315,6 @@ private void setupEnvironment(String rootDir) throws IOException, AbruptExitExce
     EventBus eventBus = new EventBus();
     when(env.getEventBus()).thenReturn(eventBus);
     when(env.getReporter()).thenReturn(new Reporter(eventBus, storedEventHandler));
+    when(env.determineOutputFileSystem()).thenReturn("OutputFileSystem");
   }
 }