Skip to content

Commit 4f5e325

Browse files
alexjskicopybara-github
alexjski
authored and
copybara-github
committed
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
1 parent f9c7fdb commit 4f5e325

9 files changed

Lines changed: 73 additions & 82 deletions

File tree

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -165,7 +165,7 @@ public void buildTargets(BuildRequest request, BuildResult result, TargetValidat
165165
boolean catastrophe = false;
166166
try {
167167
try (SilentCloseable c = Profiler.instance().profile("BuildStartingEvent")) {
168-
env.getEventBus().post(new BuildStartingEvent(env, request));
168+
env.getEventBus().post(BuildStartingEvent.create(env, request));
169169
}
170170
logger.atInfo().log("Build identifier: %s", request.getId());
171171

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

Lines changed: 42 additions & 48 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,8 @@
1414

1515
package com.google.devtools.build.lib.buildtool.buildevent;
1616

17+
import com.google.auto.value.AutoValue;
18+
import com.google.common.annotations.VisibleForTesting;
1719
import com.google.common.collect.ImmutableList;
1820
import com.google.devtools.build.lib.analysis.BlazeVersionInfo;
1921
import com.google.devtools.build.lib.buildeventstream.BuildEvent;
@@ -27,62 +29,56 @@
2729
import com.google.devtools.build.lib.runtime.CommandEnvironment;
2830
import com.google.devtools.build.lib.runtime.CommandLineEvent;
2931
import com.google.protobuf.util.Timestamps;
30-
import java.util.Collection;
32+
import javax.annotation.Nullable;
3133

3234
/**
3335
* This event is fired from BuildTool#startRequest(). At this point, the set of target patters are
3436
* known, but have yet to be parsed.
3537
*/
36-
public final class BuildStartingEvent implements BuildEvent {
37-
private final String outputFileSystem;
38-
private final BuildRequest request;
39-
private final String workspace;
40-
private final String pwd;
38+
@AutoValue
39+
public abstract class BuildStartingEvent implements BuildEvent {
40+
BuildStartingEvent() {}
41+
42+
/** Returns the name of output file system. */
43+
public abstract String outputFileSystem();
44+
45+
/** Returns the active BuildRequest. */
46+
public abstract BuildRequest request();
47+
48+
@Nullable
49+
abstract String workspace();
50+
51+
abstract String pwd();
4152

4253
/**
4354
* Construct the BuildStartingEvent
4455
*
4556
* @param request the build request.
4657
* @param env the environment of the request invocation.
4758
*/
48-
public BuildStartingEvent(CommandEnvironment env, BuildRequest request) {
49-
this.request = request;
50-
if (env != null) {
51-
this.outputFileSystem = env.determineOutputFileSystem();
52-
if (env.getDirectories().getWorkspace() != null) {
53-
this.workspace = env.getDirectories().getWorkspace().toString();
54-
} else {
55-
this.workspace = null;
56-
}
57-
this.pwd = env.getWorkingDirectory().toString();
58-
} else {
59-
this.workspace = null;
60-
this.pwd = null;
61-
this.outputFileSystem = null;
62-
}
63-
}
64-
65-
/**
66-
* @return the output file system.
67-
*/
68-
public String getOutputFileSystem() {
69-
return outputFileSystem;
59+
public static BuildStartingEvent create(CommandEnvironment env, BuildRequest request) {
60+
return create(
61+
env.determineOutputFileSystem(),
62+
request,
63+
env.getDirectories().getWorkspace() != null
64+
? env.getDirectories().getWorkspace().toString()
65+
: null,
66+
env.getWorkingDirectory().toString());
7067
}
7168

72-
/**
73-
* @return the active BuildRequest.
74-
*/
75-
public BuildRequest getRequest() {
76-
return request;
69+
@VisibleForTesting
70+
public static BuildStartingEvent create(
71+
String outputFileSystem, BuildRequest request, @Nullable String workspace, String pwd) {
72+
return new AutoValue_BuildStartingEvent(outputFileSystem, request, workspace, pwd);
7773
}
7874

7975
@Override
80-
public BuildEventId getEventId() {
76+
public final BuildEventId getEventId() {
8177
return BuildEventIdUtil.buildStartedId();
8278
}
8379

8480
@Override
85-
public Collection<BuildEventId> getChildrenEvents() {
81+
public final ImmutableList<BuildEventId> getChildrenEvents() {
8682
return ImmutableList.of(
8783
ProgressEvent.INITIAL_PROGRESS_UPDATE,
8884
BuildEventIdUtil.unstructuredCommandlineId(),
@@ -92,26 +88,24 @@ public Collection<BuildEventId> getChildrenEvents() {
9288
BuildEventIdUtil.buildMetadataId(),
9389
BuildEventIdUtil.optionsParsedId(),
9490
BuildEventIdUtil.workspaceStatusId(),
95-
BuildEventIdUtil.targetPatternExpanded(request.getTargets()),
91+
BuildEventIdUtil.targetPatternExpanded(request().getTargets()),
9692
BuildEventIdUtil.buildFinished());
9793
}
9894

9995
@Override
100-
public BuildEventStreamProtos.BuildEvent asStreamProto(BuildEventContext converters) {
96+
public final BuildEventStreamProtos.BuildEvent asStreamProto(BuildEventContext converters) {
10197
BuildEventStreamProtos.BuildStarted.Builder started =
10298
BuildEventStreamProtos.BuildStarted.newBuilder()
103-
.setUuid(request.getId().toString())
104-
.setStartTime(Timestamps.fromMillis(request.getStartTime()))
105-
.setStartTimeMillis(request.getStartTime())
99+
.setUuid(request().getId().toString())
100+
.setStartTime(Timestamps.fromMillis(request().getStartTime()))
101+
.setStartTimeMillis(request().getStartTime())
106102
.setBuildToolVersion(BlazeVersionInfo.instance().getVersion())
107-
.setOptionsDescription(request.getOptionsDescription())
108-
.setCommand(request.getCommandName())
109-
.setServerPid(ProcessHandle.current().pid());
110-
if (pwd != null) {
111-
started.setWorkingDirectory(pwd);
112-
}
113-
if (workspace != null) {
114-
started.setWorkspaceDirectory(workspace);
103+
.setOptionsDescription(request().getOptionsDescription())
104+
.setCommand(request().getCommandName())
105+
.setServerPid(ProcessHandle.current().pid())
106+
.setWorkingDirectory(pwd());
107+
if (workspace() != null) {
108+
started.setWorkspaceDirectory(workspace());
115109
}
116110
return GenericBuildEvent.protoChaining(this).setStarted(started.build()).build();
117111
}

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -462,7 +462,7 @@ public void buildEvent(BuildEvent event) {
462462
}
463463

464464
if (event instanceof BuildStartingEvent) {
465-
BuildRequest buildRequest = ((BuildStartingEvent) event).getRequest();
465+
BuildRequest buildRequest = ((BuildStartingEvent) event).request();
466466
isTestCommand =
467467
"test".equals(buildRequest.getCommandName())
468468
|| "coverage".equals(buildRequest.getCommandName());

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -64,9 +64,9 @@ public TargetSummaryPublisher(EventBus eventBus) {
6464
*/
6565
@Subscribe
6666
public void buildStarting(BuildStartingEvent event) {
67-
int count = event.getRequest().getAspects().size();
67+
int count = event.request().getAspects().size();
6868
checkState(
69-
aspectCount.compareAndSet(/* expect= */ -1, count),
69+
aspectCount.compareAndSet(/*expectedValue=*/ -1, count),
7070
"Duplicate BuildStartingEvent with %s aspects but already have %s",
7171
count,
7272
aspectCount);

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -77,7 +77,7 @@ public void cleanStarting(CleanStartingEvent event) {
7777
*/
7878
@Subscribe
7979
public void buildStarting(BuildStartingEvent event) {
80-
WorkerOptions options = event.getRequest().getOptions(WorkerOptions.class);
80+
WorkerOptions options = event.request().getOptions(WorkerOptions.class);
8181
if (workerFactory != null) {
8282
workerFactory.setReporter(options.workerVerbose ? env.getReporter() : null);
8383
}

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1303,7 +1303,7 @@ public void testSuccessfulActionsCanBePublished() {
13031303
/*stderr=*/ null,
13041304
/*actionMetadataLogs=*/ ImmutableList.of(),
13051305
ErrorTiming.BEFORE_EXECUTION,
1306-
/* isInMemoryFs= */ false);
1306+
/*isInMemoryFs=*/ false);
13071307

13081308
streamer.buildEvent(SUCCESSFUL_ACTION_EXECUTED_EVENT);
13091309
streamer.buildEvent(failedActionExecutedEvent);
@@ -1517,7 +1517,7 @@ private static ActionExecutedEvent createActionExecutedEventWithLogs(
15171517
/*stderr=*/ null,
15181518
metadataLogs,
15191519
ErrorTiming.NO_ERROR,
1520-
/* isInMemoryFs= */ false);
1520+
/*isInMemoryFs=*/ false);
15211521
}
15221522

15231523
@Test

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

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -80,7 +80,9 @@ private void createUiEventHandler(UiOptions uiOptions) {
8080

8181
uiEventHandler =
8282
new UiEventHandler(outErr, uiOptions, new ManualClock(), /*workspacePathFragment=*/ null);
83-
uiEventHandler.buildStarted(new BuildStartingEvent(/*env=*/ null, mock(BuildRequest.class)));
83+
uiEventHandler.buildStarted(
84+
BuildStartingEvent.create(
85+
"outputFileSystemType", mock(BuildRequest.class), /*workspace=*/ null, "/pwd"));
8486
}
8587

8688
@Test

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -54,6 +54,7 @@ public final class OutputsInvalidationIntegrationTest extends BuildIntegrationTe
5454
public void prepareOutputServiceMock()
5555
throws BuildFailedException, AbruptExitException, InterruptedException, IOException {
5656
when(outputService.actionFileSystemType()).thenReturn(ActionFileSystemType.DISABLED);
57+
when(outputService.getFilesSystemName()).thenReturn("fileSystemName");
5758
when(outputService.startBuild(any(), any(), anyBoolean()))
5859
.thenReturn(ModifiedFileSet.EVERYTHING_MODIFIED);
5960
}

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

Lines changed: 20 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,6 @@
4141
import com.google.devtools.common.options.OptionsParsingException;
4242
import com.google.devtools.common.options.OptionsParsingResult;
4343
import java.io.IOException;
44-
import org.junit.Before;
4544
import org.junit.Rule;
4645
import org.junit.Test;
4746
import org.junit.runner.RunWith;
@@ -58,14 +57,10 @@ public class WorkerModuleTest {
5857
@Mock BuildRequest request;
5958
@Mock OptionsParsingResult startupOptionsProvider;
6059

61-
private FileSystem fs;
60+
private final FileSystem fs =
61+
new InMemoryFileSystem(BlazeClock.instance(), DigestHashFunction.SHA256);
6262
private StoredEventHandler storedEventHandler;
6363

64-
@Before
65-
public void setUp() {
66-
fs = new InMemoryFileSystem(BlazeClock.instance(), DigestHashFunction.SHA256);
67-
}
68-
6964
@Test
7065
public void buildStarting_createsPools()
7166
throws AbruptExitException, IOException, InterruptedException {
@@ -75,7 +70,7 @@ public void buildStarting_createsPools()
7570
setupEnvironment("/outputRoot");
7671

7772
module.beforeCommand(env);
78-
module.buildStarting(new BuildStartingEvent(env, request));
73+
module.buildStarting(BuildStartingEvent.create(env, request));
7974

8075
assertThat(storedEventHandler.getEvents()).isEmpty();
8176
assertThat(fs.getPath("/outputRoot/outputBase/bazel-workers").exists()).isFalse();
@@ -96,7 +91,7 @@ public void buildStarting_noRestartOnSandboxChange() throws IOException, AbruptE
9691
setupEnvironment("/outputRoot");
9792

9893
module.beforeCommand(env);
99-
module.buildStarting(new BuildStartingEvent(env, request));
94+
module.buildStarting(BuildStartingEvent.create(env, request));
10095
assertThat(storedEventHandler.getEvents()).isEmpty();
10196

10297
Path workerDir = fs.getPath("/outputRoot/outputBase/bazel-workers");
@@ -106,7 +101,7 @@ public void buildStarting_noRestartOnSandboxChange() throws IOException, AbruptE
106101
WorkerPool oldPool = module.workerPool;
107102
options.workerSandboxing = !options.workerSandboxing;
108103
module.beforeCommand(env);
109-
module.buildStarting(new BuildStartingEvent(env, request));
104+
module.buildStarting(BuildStartingEvent.create(env, request));
110105
assertThat(storedEventHandler.getEvents()).isEmpty();
111106
assertThat(module.workerPool).isSameInstanceAs(oldPool);
112107
assertThat(aLog.exists()).isTrue();
@@ -122,7 +117,7 @@ public void buildStarting_workersDestroyedOnRestart()
122117
setupEnvironment("/outputRoot");
123118

124119
module.beforeCommand(env);
125-
module.buildStarting(new BuildStartingEvent(env, request));
120+
module.buildStarting(BuildStartingEvent.create(env, request));
126121
WorkerKey workerKey = TestUtils.createWorkerKey(JSON, fs, true);
127122
Worker worker = module.workerPool.borrowObject(workerKey);
128123
assertThat(worker.workerKey).isEqualTo(workerKey);
@@ -138,7 +133,7 @@ public void buildStarting_workersDestroyedOnRestart()
138133
WorkerPool oldPool = module.workerPool;
139134
options.highPriorityWorkers = ImmutableList.of("Foobar");
140135
module.beforeCommand(env);
141-
module.buildStarting(new BuildStartingEvent(env, request));
136+
module.buildStarting(BuildStartingEvent.create(env, request));
142137
assertThat(storedEventHandler.getEvents()).hasSize(1);
143138
assertThat(storedEventHandler.getEvents().get(0).getMessage())
144139
.contains("Worker pool configuration has changed");
@@ -154,7 +149,7 @@ public void buildStarting_restartsOnOutputbaseChanges() throws IOException, Abru
154149
setupEnvironment("/outputRoot");
155150

156151
module.beforeCommand(env);
157-
module.buildStarting(new BuildStartingEvent(env, request));
152+
module.buildStarting(BuildStartingEvent.create(env, request));
158153
assertThat(storedEventHandler.getEvents()).isEmpty();
159154

160155
// Log file from old root, doesn't get cleaned
@@ -166,7 +161,7 @@ public void buildStarting_restartsOnOutputbaseChanges() throws IOException, Abru
166161
WorkerPool oldPool = module.workerPool;
167162
setupEnvironment("/otherRootDir");
168163
module.beforeCommand(env);
169-
module.buildStarting(new BuildStartingEvent(env, request));
164+
module.buildStarting(BuildStartingEvent.create(env, request));
170165
assertThat(storedEventHandler.getEvents()).hasSize(1);
171166
assertThat(storedEventHandler.getEvents().get(0).getMessage())
172167
.contains("Worker factory configuration has changed");
@@ -190,7 +185,7 @@ public void buildStarting_clearsLogsOnFactoryCreation() throws IOException, Abru
190185
oldLog.createSymbolicLink(PathFragment.EMPTY_FRAGMENT);
191186

192187
module.beforeCommand(env);
193-
module.buildStarting(new BuildStartingEvent(env, request));
188+
module.buildStarting(BuildStartingEvent.create(env, request));
194189

195190
assertThat(storedEventHandler.getEvents()).isEmpty();
196191
assertThat(fs.getPath("/outputRoot/outputBase/bazel-workers").exists()).isTrue();
@@ -206,7 +201,7 @@ public void buildStarting_restartsOnHiPrioChanges() throws IOException, AbruptEx
206201

207202
module.beforeCommand(env);
208203
// Check that new pools/factories are made with default options
209-
module.buildStarting(new BuildStartingEvent(env, request));
204+
module.buildStarting(BuildStartingEvent.create(env, request));
210205
assertThat(storedEventHandler.getEvents()).isEmpty();
211206

212207
// 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
218213
WorkerPool oldPool = module.workerPool;
219214
options.highPriorityWorkers = ImmutableList.of("foo");
220215
module.beforeCommand(env);
221-
module.buildStarting(new BuildStartingEvent(env, request));
216+
module.buildStarting(BuildStartingEvent.create(env, request));
222217
assertThat(storedEventHandler.getEvents()).hasSize(1);
223218
assertThat(storedEventHandler.getEvents().get(0).getMessage())
224219
.contains("Worker pool configuration has changed");
@@ -236,13 +231,13 @@ public void buildStarting_restartsOnNumMultiplexWorkersChanges()
236231

237232
module.beforeCommand(env);
238233
// Check that new pools/factories are made with default options
239-
module.buildStarting(new BuildStartingEvent(env, request));
234+
module.buildStarting(BuildStartingEvent.create(env, request));
240235
assertThat(storedEventHandler.getEvents()).isEmpty();
241236

242237
WorkerPool oldPool = module.workerPool;
243238
options.workerMaxMultiplexInstances = Lists.newArrayList(Maps.immutableEntry("foo", 42));
244239
module.beforeCommand(env);
245-
module.buildStarting(new BuildStartingEvent(env, request));
240+
module.buildStarting(BuildStartingEvent.create(env, request));
246241
assertThat(storedEventHandler.getEvents()).hasSize(1);
247242
assertThat(storedEventHandler.getEvents().get(0).getMessage())
248243
.contains("Worker pool configuration has changed");
@@ -259,34 +254,32 @@ public void buildStarting_restartsOnNumWorkersChanges() throws IOException, Abru
259254

260255
module.beforeCommand(env);
261256
// Check that new pools/factories are made with default options
262-
module.buildStarting(new BuildStartingEvent(env, request));
257+
module.buildStarting(BuildStartingEvent.create(env, request));
263258
assertThat(storedEventHandler.getEvents()).isEmpty();
264259

265260
WorkerPool oldPool = module.workerPool;
266261
options.workerMaxInstances = Lists.newArrayList(Maps.immutableEntry("bar", 3));
267262
module.beforeCommand(env);
268-
module.buildStarting(new BuildStartingEvent(env, request));
263+
module.buildStarting(BuildStartingEvent.create(env, request));
269264
assertThat(storedEventHandler.getEvents()).hasSize(1);
270265
assertThat(storedEventHandler.getEvents().get(0).getMessage())
271266
.contains("Worker pool configuration has changed");
272267
assertThat(module.workerPool).isNotSameInstanceAs(oldPool);
273268
}
274269

275270
@Test
276-
public void buildStarting_survivesNoWorkerDir()
277-
throws IOException, AbruptExitException, InterruptedException {
271+
public void buildStarting_survivesNoWorkerDir() throws Exception {
278272
WorkerModule module = new WorkerModule();
279273
WorkerOptions options = WorkerOptions.DEFAULTS;
280274

281275
when(request.getOptions(WorkerOptions.class)).thenReturn(options);
282276
setupEnvironment("/outputRoot");
283277

284278
module.beforeCommand(env);
285-
Path workerDir =
286-
env.getOutputBase().getRelative(env.getRuntime().getProductName() + "-workers");
279+
Path workerDir = fs.getPath("/outputRoot/outputBase/bazel-workers");
287280

288281
// Check that new pools/factories can be created without a worker dir.
289-
module.buildStarting(new BuildStartingEvent(env, request));
282+
module.buildStarting(BuildStartingEvent.create(env, request));
290283

291284
// But once we try to get a worker, it should fail. This forces a situation where we can't
292285
// have a workerDir.
@@ -322,5 +315,6 @@ private void setupEnvironment(String rootDir) throws IOException, AbruptExitExce
322315
EventBus eventBus = new EventBus();
323316
when(env.getEventBus()).thenReturn(eventBus);
324317
when(env.getReporter()).thenReturn(new Reporter(eventBus, storedEventHandler));
318+
when(env.determineOutputFileSystem()).thenReturn("OutputFileSystem");
325319
}
326320
}

0 commit comments

Comments
 (0)