From 4f5e325e337957ebea139dc52a00027acbbb572f Mon Sep 17 00:00:00 2001 From: Googler Date: Tue, 7 Jun 2022 12:05:52 -0700 Subject: [PATCH] Clean up null handling in `BuildStartingEvent` and use `AutoValue` for 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 --- .../build/lib/buildtool/BuildTool.java | 2 +- .../buildevent/BuildStartingEvent.java | 90 +++++++++---------- .../build/lib/runtime/BuildEventStreamer.java | 2 +- .../lib/runtime/TargetSummaryPublisher.java | 4 +- .../build/lib/worker/WorkerModule.java | 2 +- .../lib/runtime/BuildEventStreamerTest.java | 4 +- .../UiEventHandlerStdOutAndStdErrTest.java | 4 +- .../OutputsInvalidationIntegrationTest.java | 1 + .../build/lib/worker/WorkerModuleTest.java | 46 +++++----- 9 files changed, 73 insertions(+), 82 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/buildtool/BuildTool.java b/src/main/java/com/google/devtools/build/lib/buildtool/BuildTool.java index c7c7a8edc58ec8..ecde1383b9205b 100644 --- a/src/main/java/com/google/devtools/build/lib/buildtool/BuildTool.java +++ b/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()); diff --git a/src/main/java/com/google/devtools/build/lib/buildtool/buildevent/BuildStartingEvent.java b/src/main/java/com/google/devtools/build/lib/buildtool/buildevent/BuildStartingEvent.java index f0ea90f17a750e..f739e9362ea1f0 100644 --- a/src/main/java/com/google/devtools/build/lib/buildtool/buildevent/BuildStartingEvent.java +++ b/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,17 +29,26 @@ 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 @@ -45,44 +56,29 @@ public final class BuildStartingEvent implements BuildEvent { * @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 getChildrenEvents() { + public final ImmutableList getChildrenEvents() { return ImmutableList.of( ProgressEvent.INITIAL_PROGRESS_UPDATE, BuildEventIdUtil.unstructuredCommandlineId(), @@ -92,26 +88,24 @@ public Collection 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(); } diff --git a/src/main/java/com/google/devtools/build/lib/runtime/BuildEventStreamer.java b/src/main/java/com/google/devtools/build/lib/runtime/BuildEventStreamer.java index 26c6b4371719ae..79224cd3d5f9ac 100644 --- a/src/main/java/com/google/devtools/build/lib/runtime/BuildEventStreamer.java +++ b/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()); diff --git a/src/main/java/com/google/devtools/build/lib/runtime/TargetSummaryPublisher.java b/src/main/java/com/google/devtools/build/lib/runtime/TargetSummaryPublisher.java index fbf8f30b64d225..3f72a311ebab05 100644 --- a/src/main/java/com/google/devtools/build/lib/runtime/TargetSummaryPublisher.java +++ b/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); diff --git a/src/main/java/com/google/devtools/build/lib/worker/WorkerModule.java b/src/main/java/com/google/devtools/build/lib/worker/WorkerModule.java index 0fc95320134d36..9ec34a16560f67 100644 --- a/src/main/java/com/google/devtools/build/lib/worker/WorkerModule.java +++ b/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); } diff --git a/src/test/java/com/google/devtools/build/lib/runtime/BuildEventStreamerTest.java b/src/test/java/com/google/devtools/build/lib/runtime/BuildEventStreamerTest.java index fc451d3203ab22..ee504e59d04d2e 100644 --- a/src/test/java/com/google/devtools/build/lib/runtime/BuildEventStreamerTest.java +++ b/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 diff --git a/src/test/java/com/google/devtools/build/lib/runtime/UiEventHandlerStdOutAndStdErrTest.java b/src/test/java/com/google/devtools/build/lib/runtime/UiEventHandlerStdOutAndStdErrTest.java index 183d3decf7e254..b24d0748ec5a61 100644 --- a/src/test/java/com/google/devtools/build/lib/runtime/UiEventHandlerStdOutAndStdErrTest.java +++ b/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 diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/OutputsInvalidationIntegrationTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/OutputsInvalidationIntegrationTest.java index f019857d89e2d4..bb4441ff70b97a 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/OutputsInvalidationIntegrationTest.java +++ b/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); } diff --git a/src/test/java/com/google/devtools/build/lib/worker/WorkerModuleTest.java b/src/test/java/com/google/devtools/build/lib/worker/WorkerModuleTest.java index c0ed65e0e0cc38..1e691bd7a1fc73 100644 --- a/src/test/java/com/google/devtools/build/lib/worker/WorkerModuleTest.java +++ b/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,13 +254,13 @@ 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"); @@ -273,8 +268,7 @@ public void buildStarting_restartsOnNumWorkersChanges() throws IOException, Abru } @Test - public void buildStarting_survivesNoWorkerDir() - throws IOException, AbruptExitException, InterruptedException { + public void buildStarting_survivesNoWorkerDir() throws Exception { WorkerModule module = new WorkerModule(); WorkerOptions options = WorkerOptions.DEFAULTS; @@ -282,11 +276,10 @@ public void buildStarting_survivesNoWorkerDir() 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"); } }