Unify the options initialization and error handling in BuildEventServiceModule.
The error checking for the obtaining the options classes from the command environment
were scattered all over the class. Use a single method to initialize them and reset
them with every command.
Unifying these options uncovered a brittle dependency between BuildEventServiceModule and BazelBuildEventServiceModuleTest since it was relying on the streamer creation directly. The test now exercises the logic from BuildEventServiceModule.
PiperOrigin-RevId: 234489667
diff --git a/src/main/java/com/google/devtools/build/lib/buildeventservice/BuildEventServiceModule.java b/src/main/java/com/google/devtools/build/lib/buildeventservice/BuildEventServiceModule.java
index b880da6..5d347c7 100644
--- a/src/main/java/com/google/devtools/build/lib/buildeventservice/BuildEventServiceModule.java
+++ b/src/main/java/com/google/devtools/build/lib/buildeventservice/BuildEventServiceModule.java
@@ -11,18 +11,14 @@
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
-
package com.google.devtools.build.lib.buildeventservice;
-import static com.google.common.base.Preconditions.checkNotNull;
-import static com.google.common.base.Strings.isNullOrEmpty;
-import static java.lang.String.format;
-
import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Preconditions;
import com.google.common.base.Strings;
import com.google.common.base.Supplier;
import com.google.common.base.Suppliers;
+import com.google.common.base.Throwables;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableSet;
import com.google.devtools.build.lib.authandtls.AuthAndTLSOptions;
@@ -59,14 +55,18 @@
* Module responsible for the Build Event Transport (BEP) and Build Event Service (BES)
* functionality.
*/
-public abstract class BuildEventServiceModule<T extends BuildEventServiceOptions>
+public abstract class BuildEventServiceModule<BESOptionsT extends BuildEventServiceOptions>
extends BlazeModule {
private static final Logger logger = Logger.getLogger(BuildEventServiceModule.class.getName());
private OutErr outErr;
private BuildEventStreamer streamer;
- private boolean keepClient;
+ private BESOptionsT besOptions;
+ private BuildEventProtocolOptions bepOptions;
+ private AuthAndTLSOptions authTlsOptions;
+ private BuildEventStreamOptions besStreamOptions;
+ private ImmutableSet<BuildEventTransport> bepTransports;
/** Whether an error in the Build Event Service upload causes the build to fail. */
protected boolean errorsShouldFailTheBuild() {
@@ -77,9 +77,16 @@
protected void reportError(
EventHandler commandLineReporter,
ModuleEnvironment moduleEnvironment,
- AbruptExitException exception) {
+ String msg,
+ Exception exception,
+ ExitCode exitCode) {
+ // Don't hide unchecked exceptions as part of the error reporting.
+ Throwables.throwIfUnchecked(exception);
+
+ logger.log(Level.SEVERE, msg, exception);
+ AbruptExitException abruptException = new AbruptExitException(msg, exitCode, exception);
commandLineReporter.handle(Event.error(exception.getMessage()));
- moduleEnvironment.exit(exception);
+ moduleEnvironment.exit(abruptException);
}
@Override
@@ -93,7 +100,13 @@
@Override
public void beforeCommand(CommandEnvironment commandEnvironment) {
- T besOptions = commandEnvironment.getOptions().getOptions(optionsClass());
+ OptionsParsingResult parsingResult = commandEnvironment.getOptions();
+ besOptions = Preconditions.checkNotNull(parsingResult.getOptions(optionsClass()));
+ bepOptions =
+ Preconditions.checkNotNull(parsingResult.getOptions(BuildEventProtocolOptions.class));
+ authTlsOptions = Preconditions.checkNotNull(parsingResult.getOptions(AuthAndTLSOptions.class));
+ besStreamOptions =
+ Preconditions.checkNotNull(parsingResult.getOptions(BuildEventStreamOptions.class));
// Reset to null in case afterCommand was not called.
this.outErr = null;
@@ -125,7 +138,6 @@
});
err.registerStreamer(streamer);
out.registerStreamer(streamer);
- logger.fine("BuildEventStreamer created and registered successfully.");
}
}
@@ -156,30 +168,23 @@
this.streamer = null;
}
- if (!keepClient) {
+ if (!besStreamOptions.keepBackendConnections) {
clearBesClient();
}
this.outErr = null;
+ this.bepTransports = null;
}
/** Returns {@code null} if no stream could be created. */
@Nullable
@VisibleForTesting
BuildEventStreamer tryCreateStreamer(CommandEnvironment env) {
- BuildEventStreamOptions buildEventStreamOptions =
- env.getOptions().getOptions(BuildEventStreamOptions.class);
- this.keepClient = buildEventStreamOptions.keepBackendConnections;
-
- BuildEventProtocolOptions protocolOptions =
- checkNotNull(
- env.getOptions().getOptions(BuildEventProtocolOptions.class),
- "Could not get BuildEventProtocolOptions.");
Supplier<BuildEventArtifactUploader> uploaderSupplier =
Suppliers.memoize(
() ->
env.getRuntime()
.getBuildEventArtifactUploaderFactoryMap()
- .select(protocolOptions.buildEventUploadStrategy)
+ .select(bepOptions.buildEventUploadStrategy)
.create(env));
CountingArtifactGroupNamer namer = new CountingArtifactGroupNamer();
@@ -188,40 +193,41 @@
try {
besTransport = tryCreateBesTransport(env, uploaderSupplier, namer);
} catch (IOException | OptionsParsingException e) {
- String message = "Failed to create BuildEventTransport: " + e;
- logger.log(Level.WARNING, message, e);
- ExitCode exitCode =
- (e instanceof OptionsParsingException)
- ? ExitCode.COMMAND_LINE_ERROR
- : ExitCode.TRANSIENT_BUILD_EVENT_SERVICE_UPLOAD_ERROR;
reportError(
env.getReporter(),
env.getBlazeModuleEnvironment(),
- new AbruptExitException(message, exitCode, e));
+ "Failed to create BuildEventTransport: " + e,
+ e,
+ (e instanceof OptionsParsingException)
+ ? ExitCode.COMMAND_LINE_ERROR
+ : ExitCode.TRANSIENT_BUILD_EVENT_SERVICE_UPLOAD_ERROR);
clearBesClient();
return null;
}
- ImmutableSet<BuildEventTransport> bepTransports =
+ ImmutableSet<BuildEventTransport> bepFileTransports =
BuildEventTransportFactory.createFromOptions(
- env, env.getBlazeModuleEnvironment()::exit, protocolOptions, uploaderSupplier, namer);
+ env, env.getBlazeModuleEnvironment()::exit, bepOptions, uploaderSupplier, namer);
ImmutableSet.Builder<BuildEventTransport> transportsBuilder =
- ImmutableSet.<BuildEventTransport>builder().addAll(bepTransports);
+ ImmutableSet.<BuildEventTransport>builder().addAll(bepFileTransports);
if (besTransport != null) {
transportsBuilder.add(besTransport);
}
- ImmutableSet<BuildEventTransport> transports = transportsBuilder.build();
- if (!transports.isEmpty()) {
- return new BuildEventStreamer(
- transports, env.getReporter(), buildEventStreamOptions, namer);
+ bepTransports = transportsBuilder.build();
+ if (!bepTransports.isEmpty()) {
+ return new BuildEventStreamer(bepTransports, env.getReporter(), besStreamOptions, namer);
}
} catch (Exception e) {
+ // TODO(lpino): This generic catch with Exception shouldn't exist, remove it once the code
+ // above is re-structured.
reportError(
env.getReporter(),
env.getBlazeModuleEnvironment(),
- new AbruptExitException(ExitCode.LOCAL_ENVIRONMENTAL_ERROR, e));
+ e.getMessage(),
+ e,
+ ExitCode.LOCAL_ENVIRONMENTAL_ERROR);
}
return null;
}
@@ -233,27 +239,11 @@
ArtifactGroupNamer namer)
throws IOException, OptionsParsingException {
OptionsParsingResult optionsProvider = env.getOptions();
- T besOptions =
- checkNotNull(
- optionsProvider.getOptions(optionsClass()), "Could not get BuildEventServiceOptions.");
- AuthAndTLSOptions authTlsOptions =
- checkNotNull(
- optionsProvider.getOptions(AuthAndTLSOptions.class),
- "Could not get AuthAndTLSOptions.");
- BuildEventProtocolOptions protocolOptions =
- checkNotNull(
- optionsProvider.getOptions(BuildEventProtocolOptions.class),
- "Could not get BuildEventProtocolOptions.");
- if (isNullOrEmpty(besOptions.besBackend)) {
- logger.fine("BuildEventServiceTransport is disabled.");
+ if (Strings.isNullOrEmpty(besOptions.besBackend)) {
clearBesClient();
return null;
} else {
- logger.fine(
- format(
- "Will create BuildEventServiceTransport streaming to '%s'", besOptions.besBackend));
-
String invocationId = env.getCommandId().toString();
final String besResultsUrl;
if (!Strings.isNullOrEmpty(besOptions.besResultsUrl)) {
@@ -267,7 +257,7 @@
env.getReporter()
.handle(
Event.info(
- format(
+ String.format(
"Streaming Build Event Protocol to %s build_request_id: %s "
+ "invocation_id: %s",
besOptions.besBackend, env.getBuildRequestId(), invocationId)));
@@ -292,29 +282,28 @@
.build(
client,
artifactUploader,
- protocolOptions,
+ bepOptions,
besProtoUtil,
env.getRuntime().getClock(),
bazelExitFunction(
env.getReporter(), env.getBlazeModuleEnvironment(), besResultsUrl),
namer);
- logger.fine("BuildEventServiceTransport was created successfully");
return besTransport;
}
}
- protected abstract Class<T> optionsClass();
+ protected abstract Class<BESOptionsT> optionsClass();
protected abstract BuildEventServiceClient getBesClient(
- T besOptions, AuthAndTLSOptions authAndTLSOptions)
+ BESOptionsT besOptions, AuthAndTLSOptions authAndTLSOptions)
throws IOException, OptionsParsingException;
protected abstract void clearBesClient();
- protected abstract Set<String> whitelistedCommands(T besOptions);
+ protected abstract Set<String> whitelistedCommands(BESOptionsT besOptions);
protected Set<String> keywords(
- T besOptions, @Nullable OptionsParsingResult startupOptionsProvider) {
+ BESOptionsT besOptions, @Nullable OptionsParsingResult startupOptionsProvider) {
return besOptions.besKeywords.stream()
.map(keyword -> "user_keyword=" + keyword)
.collect(ImmutableSet.toImmutableSet());
@@ -348,4 +337,11 @@
}
};
}
+
+ // TODO(lpino): This method shouldn exist. It only does because some tests are relying on the
+ // transport creation logic of this module directly.
+ @VisibleForTesting
+ ImmutableSet<BuildEventTransport> getBepTransports() {
+ return bepTransports;
+ }
}
