diff options
author | ulfjack <ulfjack@google.com> | 2017-07-14 22:49:55 +0200 |
---|---|---|
committer | Jakob Buchgraber <buchgr@google.com> | 2017-07-17 10:11:03 +0200 |
commit | f1c1427a78e621a9293a1a8f686f3c84516552a4 (patch) | |
tree | c7ffd860b750566aea46617b4111715ad5a32cde /src | |
parent | d5cfbfe3d7e5669191639d31c5048358ca7f0892 (diff) |
Merge handleOptions into beforeCommand
Now that we have the options before calling beforeCommand, there's no need for
a separate handleOptions method in the BlazeModule API. Remove it.
PiperOrigin-RevId: 162002300
Diffstat (limited to 'src')
9 files changed, 75 insertions, 177 deletions
diff --git a/src/main/java/com/google/devtools/build/lib/bazel/BazelRepositoryModule.java b/src/main/java/com/google/devtools/build/lib/bazel/BazelRepositoryModule.java index 23eec44981..e8d744b91d 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/BazelRepositoryModule.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/BazelRepositoryModule.java @@ -180,11 +180,12 @@ public class BazelRepositoryModule extends BlazeModule { } @Override - public void handleOptions(OptionsProvider optionsProvider) { - PackageCacheOptions pkgOptions = optionsProvider.getOptions(PackageCacheOptions.class); + public void beforeCommand(CommandEnvironment env) { + delegator.setClientEnvironment(env.getActionClientEnv()); + PackageCacheOptions pkgOptions = env.getOptions().getOptions(PackageCacheOptions.class); isFetch.set(pkgOptions != null && pkgOptions.fetch); - RepositoryOptions repoOptions = optionsProvider.getOptions(RepositoryOptions.class); + RepositoryOptions repoOptions = env.getOptions().getOptions(RepositoryOptions.class); if (repoOptions != null) { if (repoOptions.experimentalRepositoryCache != null) { Path repositoryCachePath = filesystem.getPath(repoOptions.experimentalRepositoryCache); @@ -216,11 +217,6 @@ public class BazelRepositoryModule extends BlazeModule { } @Override - public void beforeCommand(CommandEnvironment env) { - delegator.setClientEnvironment(env.getActionClientEnv()); - } - - @Override public Iterable<Class<? extends OptionsBase>> getCommandOptions(Command command) { return ImmutableSet.of("fetch", "build", "query").contains(command.name()) ? ImmutableList.<Class<? extends OptionsBase>>of(RepositoryOptions.class) 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 60df1913a4..ead187d005 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 @@ -15,7 +15,6 @@ package com.google.devtools.build.lib.buildeventservice; import static com.google.common.base.Preconditions.checkNotNull; -import static com.google.common.base.Preconditions.checkState; import static com.google.common.base.Strings.isNullOrEmpty; import static com.google.devtools.build.lib.buildeventservice.BuildEventServiceTransport.UPLOAD_FAILED_MESSAGE; import static java.lang.String.format; @@ -23,10 +22,8 @@ import static java.lang.String.format; import com.google.common.annotations.VisibleForTesting; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; -import com.google.common.eventbus.Subscribe; import com.google.devtools.build.lib.authandtls.AuthAndTLSOptions; import com.google.devtools.build.lib.buildeventservice.client.BuildEventServiceClient; -import com.google.devtools.build.lib.buildeventstream.BuildEvent; import com.google.devtools.build.lib.buildeventstream.BuildEventTransport; import com.google.devtools.build.lib.buildeventstream.PathConverter; import com.google.devtools.build.lib.buildeventstream.transports.BuildEventStreamOptions; @@ -45,8 +42,6 @@ import com.google.devtools.build.lib.util.ExitCode; import com.google.devtools.build.lib.util.io.OutErr; import com.google.devtools.common.options.OptionsBase; import com.google.devtools.common.options.OptionsProvider; -import java.util.ArrayList; -import java.util.List; import java.util.Set; import java.util.UUID; import java.util.logging.Logger; @@ -64,25 +59,7 @@ public abstract class BuildEventServiceModule<T extends BuildEventServiceOptions private static final Logger logger = Logger.getLogger(BuildEventServiceModule.class.getName()); - private CommandEnvironment commandEnvironment; - private SynchronizedOutputStream out; - private SynchronizedOutputStream err; - private boolean disabled; - - private static class BuildEventRecorder { - private final List<BuildEvent> events = new ArrayList<>(); - - @Subscribe - public void buildEvent(BuildEvent event) { - events.add(event); - } - - List<BuildEvent> getEvents() { - return events; - } - } - - private BuildEventRecorder buildEventRecorder; + private OutErr outErr; @Override public Iterable<Class<? extends OptionsBase>> getCommandOptions(Command command) { @@ -92,25 +69,15 @@ public abstract class BuildEventServiceModule<T extends BuildEventServiceOptions @Override public void beforeCommand(CommandEnvironment commandEnvironment) throws AbruptExitException { - disabled = false; + // Reset to null in case afterCommand was not called. + this.outErr = null; if (!whitelistedCommands().contains(commandEnvironment.getCommandName())) { - disabled = true; return; } - this.commandEnvironment = commandEnvironment; - this.buildEventRecorder = new BuildEventRecorder(); - commandEnvironment.getEventBus().register(buildEventRecorder); - } - @Override - public void handleOptions(OptionsProvider optionsProvider) { - if (disabled) { - return; - } - checkState(commandEnvironment != null, "Methods called out of order"); BuildEventStreamer streamer = tryCreateStreamer( - optionsProvider, + commandEnvironment.getOptions(), commandEnvironment.getReporter(), commandEnvironment.getBlazeModuleEnvironment(), commandEnvironment.getRuntime().getClock(), @@ -122,58 +89,35 @@ public abstract class BuildEventServiceModule<T extends BuildEventServiceOptions commandEnvironment.getReporter().addHandler(streamer); commandEnvironment.getEventBus().register(streamer); - final SynchronizedOutputStream theOut = this.out; - final SynchronizedOutputStream theErr = this.err; - // out and err should be non-null at this point, as getOutputListener is supposed to - // be always called before handleOptions. But let's still prefer a stream with no - // stdout/stderr over an aborted build. + final SynchronizedOutputStream out = new SynchronizedOutputStream(); + final SynchronizedOutputStream err = new SynchronizedOutputStream(); + this.outErr = OutErr.create(out, err); streamer.registerOutErrProvider( new BuildEventStreamer.OutErrProvider() { @Override public String getOut() { - if (theOut == null) { - return null; - } - return theOut.readAndReset(); + return out.readAndReset(); } @Override public String getErr() { - if (theErr == null) { - return null; - } - return theErr.readAndReset(); + return err.readAndReset(); } }); - if (theErr != null) { - theErr.registerStreamer(streamer); - } - if (theOut != null) { - theOut.registerStreamer(streamer); - } - for (BuildEvent event : buildEventRecorder.getEvents()) { - streamer.buildEvent(event); - } + err.registerStreamer(streamer); + out.registerStreamer(streamer); logger.fine("BuildEventStreamer created and registered successfully."); - } else { - // If there is no streamer to consume the output, we should not try to accumulate it. - this.out.setDiscardAll(); - this.err.setDiscardAll(); } - commandEnvironment.getEventBus().unregister(buildEventRecorder); - this.buildEventRecorder = null; - this.out = null; - this.err = null; } @Override public OutErr getOutputListener() { - if (disabled) { - return null; - } - this.out = new SynchronizedOutputStream(); - this.err = new SynchronizedOutputStream(); - return OutErr.create(this.out, this.err); + return outErr; + } + + @Override + public void afterCommand() { + this.outErr = null; } /** diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteModule.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteModule.java index d5c0575115..1a4b874db9 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteModule.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteModule.java @@ -14,8 +14,6 @@ package com.google.devtools.build.lib.remote; -import static com.google.common.base.Preconditions.checkState; - import com.google.common.annotations.VisibleForTesting; import com.google.common.collect.ImmutableList; import com.google.devtools.build.lib.authandtls.AuthAndTLSOptions; @@ -77,7 +75,6 @@ public final class RemoteModule extends BlazeModule { private final CasPathConverter converter = new CasPathConverter(); - private CommandEnvironment env; private RemoteActionContextProvider actionContextProvider; @Override @@ -89,15 +86,9 @@ public final class RemoteModule extends BlazeModule { @Override public void beforeCommand(CommandEnvironment env) { env.getEventBus().register(this); - this.env = env; - } - - @Override - public void handleOptions(OptionsProvider optionsProvider) { - checkState(env != null); - RemoteOptions remoteOptions = optionsProvider.getOptions(RemoteOptions.class); - AuthAndTLSOptions authAndTlsOptions = optionsProvider.getOptions(AuthAndTLSOptions.class); + RemoteOptions remoteOptions = env.getOptions().getOptions(RemoteOptions.class); + AuthAndTLSOptions authAndTlsOptions = env.getOptions().getOptions(AuthAndTLSOptions.class); converter.options = remoteOptions; // Quit if no remote options specified. diff --git a/src/main/java/com/google/devtools/build/lib/runtime/BlazeCommandDispatcher.java b/src/main/java/com/google/devtools/build/lib/runtime/BlazeCommandDispatcher.java index cbd8e564ec..453cd7c24b 100644 --- a/src/main/java/com/google/devtools/build/lib/runtime/BlazeCommandDispatcher.java +++ b/src/main/java/com/google/devtools/build/lib/runtime/BlazeCommandDispatcher.java @@ -367,10 +367,13 @@ public class BlazeCommandDispatcher { OptionsProvider options = optionsResult.get(); // The initCommand call also records the start time for the timestamp granularity monitor. - CommandEnvironment env = workspace.initCommand(commandAnnotation); + CommandEnvironment env = workspace.initCommand(commandAnnotation, options); // Record the command's starting time for use by the commands themselves. env.recordCommandStartTime(firstContactTime); + // Temporary: there is one module that outputs events during beforeCommand, but the reporter + // isn't setup yet. Add the stored event handler to catch those events. + env.getReporter().addHandler(eventHandler); for (BlazeModule module : runtime.getBlazeModules()) { try { module.beforeCommand(env); @@ -385,6 +388,7 @@ public class BlazeCommandDispatcher { earlyExitCode = e.getExitCode(); } } + env.getReporter().removeHandler(eventHandler); // We may only start writing to outErr once we've given the modules the chance to hook into it. for (BlazeModule module : runtime.getBlazeModules()) { @@ -502,9 +506,6 @@ public class BlazeCommandDispatcher { reporter.handle(Event.error(e.getMessage())); return e.getExitCode().getNumericExitCode(); } - for (BlazeModule module : runtime.getBlazeModules()) { - module.handleOptions(options); - } env.getEventBus().post(originalCommandLine); @@ -573,6 +574,16 @@ public class BlazeCommandDispatcher { // applied the invocation policy. AtomicReference<OptionsProvider> parsedOptions, List<String> rcfileNotes) { + OptionsParser optionsParser; + try { + optionsParser = createOptionsParser(command); + // We need to set this early so it's not null when we return. + parsedOptions.set(optionsParser); + } catch (OptionsParser.ConstructionException e) { + // This should never happen. + throw new IllegalStateException(e); + } + // The initialization code here was carefully written to parse the options early before we call // into the BlazeModule APIs, which means we must not generate any output to outErr, return, or // throw an exception. All the events happening here are instead stored in a temporary event @@ -584,8 +595,6 @@ public class BlazeCommandDispatcher { } try { - OptionsParser optionsParser = createOptionsParser(command); - // TODO(ulfjack): The second parameter is supposed to be the working directory, except that // the client passes that as part of CommonCommandOptions, and we can't know those until // after we've parsed them. @@ -624,10 +633,6 @@ public class BlazeCommandDispatcher { for (String warning : optionsParser.getWarnings()) { eventHandler.handle(Event.warn(warning)); } - parsedOptions.set(optionsParser); - } catch (OptionsParser.ConstructionException e) { - // This should never happen. - throw new IllegalStateException(e); } catch (OptionsParsingException e) { eventHandler.handle(Event.error(e.getMessage())); return ExitCode.COMMAND_LINE_ERROR; diff --git a/src/main/java/com/google/devtools/build/lib/runtime/BlazeModule.java b/src/main/java/com/google/devtools/build/lib/runtime/BlazeModule.java index 40b420e9ac..49265e0e91 100644 --- a/src/main/java/com/google/devtools/build/lib/runtime/BlazeModule.java +++ b/src/main/java/com/google/devtools/build/lib/runtime/BlazeModule.java @@ -173,14 +173,6 @@ public abstract class BlazeModule { } /** - * Does any handling of options needed by the command. - * - * <p>This method will be called at the beginning of each command (after #beforeCommand). - */ - @SuppressWarnings("unused") - public void handleOptions(OptionsProvider optionsProvider) {} - - /** * Returns extra options this module contributes to a specific command. Note that option * inheritance applies: if this method returns a non-empty list, then the returned options are * added to every command that depends on this command. diff --git a/src/main/java/com/google/devtools/build/lib/runtime/BlazeWorkspace.java b/src/main/java/com/google/devtools/build/lib/runtime/BlazeWorkspace.java index 47b0cddc9e..5877ec2331 100644 --- a/src/main/java/com/google/devtools/build/lib/runtime/BlazeWorkspace.java +++ b/src/main/java/com/google/devtools/build/lib/runtime/BlazeWorkspace.java @@ -185,22 +185,10 @@ public final class BlazeWorkspace { * <p>This method should be called from the "main" thread on which the command will execute; * that thread will receive interruptions if a module requests an early exit. */ - public CommandEnvironment initCommand(Command command) { + public CommandEnvironment initCommand(Command command, OptionsProvider options) { CommandEnvironment env = new CommandEnvironment( - runtime, this, new EventBus(eventBusExceptionHandler), Thread.currentThread(), command); - skyframeExecutor.setClientEnv(env.getClientEnv()); - return env; - } - - /** - * Same as {@code #initCommand()} but setting the command name and the options manually since - * those values are set by {@code CommandEnvironment#beforeCommand()} which is not called for - * testing. Use ONLY for testing purposes. - */ - public CommandEnvironment initCommandForTesting(Command command, OptionsProvider options) { - CommandEnvironment env = new CommandEnvironment( - runtime, this, new EventBus(eventBusExceptionHandler), Thread.currentThread(), - command, options); + runtime, this, new EventBus(eventBusExceptionHandler), Thread.currentThread(), command, + options); skyframeExecutor.setClientEnv(env.getClientEnv()); return env; } @@ -214,7 +202,7 @@ public final class BlazeWorkspace { /** * Reinitializes the Skyframe evaluator. */ - public void resetEvaluator() throws IOException { + public void resetEvaluator() { skyframeExecutor.resetEvaluator(); } diff --git a/src/main/java/com/google/devtools/build/lib/runtime/CommandEnvironment.java b/src/main/java/com/google/devtools/build/lib/runtime/CommandEnvironment.java index 43c5faa4fd..df19077a78 100644 --- a/src/main/java/com/google/devtools/build/lib/runtime/CommandEnvironment.java +++ b/src/main/java/com/google/devtools/build/lib/runtime/CommandEnvironment.java @@ -16,7 +16,6 @@ package com.google.devtools.build.lib.runtime; import static com.google.devtools.build.lib.profiler.AutoProfiler.profiled; -import com.google.common.annotations.VisibleForTesting; import com.google.common.eventbus.EventBus; import com.google.devtools.build.lib.actions.PackageRootResolver; import com.google.devtools.build.lib.actions.cache.ActionCache; @@ -80,6 +79,7 @@ public final class CommandEnvironment { private final TimestampGranularityMonitor timestampGranularityMonitor; private final Thread commandThread; private final Command command; + private final OptionsProvider options; private String[] crashData; @@ -89,8 +89,6 @@ public final class CommandEnvironment { private Path workingDirectory; private String workspaceName; - private OptionsProvider options; - private AtomicReference<AbruptExitException> pendingException = new AtomicReference<>(); private class BlazeModuleEnvironment implements BlazeModule.ModuleEnvironment { @@ -121,7 +119,7 @@ public final class CommandEnvironment { */ CommandEnvironment( BlazeRuntime runtime, BlazeWorkspace workspace, EventBus eventBus, Thread commandThread, - Command command) { + Command command, OptionsProvider options) { this.runtime = runtime; this.workspace = workspace; this.directories = workspace.getDirectories(); @@ -130,6 +128,7 @@ public final class CommandEnvironment { this.eventBus = eventBus; this.commandThread = commandThread; this.command = command; + this.options = options; this.blazeModuleEnvironment = new BlazeModuleEnvironment(); this.timestampGranularityMonitor = new TimestampGranularityMonitor(runtime.getClock()); // Record the command's starting time again, for use by @@ -144,22 +143,31 @@ public final class CommandEnvironment { this.workspaceName = null; workspace.getSkyframeExecutor().setEventBus(eventBus); - } - /** - * Same as CommandEnvironment(BlazeRuntime, BlazeWorkspace, EventBus, Thread) but with an - * explicit commandName and options. - * - * ONLY for testing. - */ - @VisibleForTesting - CommandEnvironment( - BlazeRuntime runtime, BlazeWorkspace workspace, EventBus eventBus, Thread commandThread, - Command command, OptionsProvider optionsForTesting) { - this(runtime, workspace, eventBus, commandThread, command); - // Options are normally set by beforeCommand(); however this method is not called in tests (i.e. - // tests use BlazeRuntimeWrapper). These fields should only be set for testing. - this.options = optionsForTesting; + updateClientEnv(options.getOptions(CommonCommandOptions.class).clientEnv); + + // actionClientEnv contains the environment where values from actionEnvironment are overridden. + actionClientEnv.putAll(clientEnv); + + if (command.builds()) { + // Compute the set of environment variables that are whitelisted on the commandline + // for inheritance. + for (Map.Entry<String, String> entry : + options.getOptions(BuildConfiguration.Options.class).actionEnvironment) { + if (entry.getValue() == null) { + visibleActionEnv.add(entry.getKey()); + } else { + visibleActionEnv.remove(entry.getKey()); + actionClientEnv.put(entry.getKey(), entry.getValue()); + } + } + for (Map.Entry<String, String> entry : + options.getOptions(BuildConfiguration.Options.class).testEnvironment) { + if (entry.getValue() == null) { + visibleTestEnv.add(entry.getKey()); + } + } + } } public BlazeRuntime getRuntime() { @@ -236,8 +244,7 @@ public final class CommandEnvironment { return Collections.unmodifiableMap(result); } - @VisibleForTesting - void updateClientEnv(List<Map.Entry<String, String>> clientEnvList) { + private void updateClientEnv(List<Map.Entry<String, String>> clientEnvList) { Preconditions.checkState(clientEnv.isEmpty()); Collection<Map.Entry<String, String>> env = clientEnvList; @@ -545,7 +552,6 @@ public final class CommandEnvironment { InvocationPolicy invocationPolicy) throws AbruptExitException { commandStartTime -= commonOptions.startupTime; - this.options = options; eventBus.post( new GotOptionsEvent(runtime.getStartupOptionsProvider(), options, invocationPolicy)); @@ -584,8 +590,6 @@ public final class CommandEnvironment { this.relativeWorkingDirectory = workingDirectory.relativeTo(workspace); this.workingDirectory = workingDirectory; - updateClientEnv(commonOptions.clientEnv); - // Fail fast in the case where a Blaze command forgets to install the package path correctly. skyframeExecutor.setActive(false); // Let skyframe figure out if it needs to store graph edges for this build. @@ -596,29 +600,6 @@ public final class CommandEnvironment { // Start the performance and memory profilers. runtime.beforeCommand(this, commonOptions, execStartTimeNanos); - // actionClientEnv contains the environment where values from actionEnvironment are overridden. - actionClientEnv.putAll(clientEnv); - - if (command.builds()) { - // Compute the set of environment variables that are whitelisted on the commandline - // for inheritance. - for (Map.Entry<String, String> entry : - options.getOptions(BuildConfiguration.Options.class).actionEnvironment) { - if (entry.getValue() == null) { - visibleActionEnv.add(entry.getKey()); - } else { - visibleActionEnv.remove(entry.getKey()); - actionClientEnv.put(entry.getKey(), entry.getValue()); - } - } - for (Map.Entry<String, String> entry : - options.getOptions(BuildConfiguration.Options.class).testEnvironment) { - if (entry.getValue() == null) { - visibleTestEnv.add(entry.getKey()); - } - } - } - eventBus.post(new CommandStartEvent( command.name(), getCommandId(), getClientEnv(), workingDirectory, getDirectories(), waitTimeInMs + commonOptions.waitTime)); diff --git a/src/main/java/com/google/devtools/build/lib/ssd/SsdModule.java b/src/main/java/com/google/devtools/build/lib/ssd/SsdModule.java index 00b5ad370c..2e73896516 100644 --- a/src/main/java/com/google/devtools/build/lib/ssd/SsdModule.java +++ b/src/main/java/com/google/devtools/build/lib/ssd/SsdModule.java @@ -16,8 +16,8 @@ package com.google.devtools.build.lib.ssd; import com.google.common.collect.ImmutableList; import com.google.devtools.build.lib.actions.cache.DigestUtils; import com.google.devtools.build.lib.runtime.BlazeModule; +import com.google.devtools.build.lib.runtime.CommandEnvironment; import com.google.devtools.common.options.OptionsBase; -import com.google.devtools.common.options.OptionsProvider; /** * BlazeModule that applies optimizations to Bazel's internals in order to improve performance when @@ -30,8 +30,8 @@ public final class SsdModule extends BlazeModule { } @Override - public void handleOptions(OptionsProvider optionsProvider) { - SsdOptions options = optionsProvider.getOptions(SsdOptions.class); + public void beforeCommand(CommandEnvironment env) { + SsdOptions options = env.getOptions().getOptions(SsdOptions.class); if (options.experimentalMultiThreadedDigest) { DigestUtils.setMultiThreadedDigest(options.experimentalMultiThreadedDigest); } diff --git a/src/main/java/com/google/devtools/build/lib/util/io/DelegatingOutErr.java b/src/main/java/com/google/devtools/build/lib/util/io/DelegatingOutErr.java index d5cafab2d4..233e613ef6 100644 --- a/src/main/java/com/google/devtools/build/lib/util/io/DelegatingOutErr.java +++ b/src/main/java/com/google/devtools/build/lib/util/io/DelegatingOutErr.java @@ -14,6 +14,7 @@ package com.google.devtools.build.lib.util.io; +import com.google.common.base.Preconditions; import java.io.IOException; import java.io.OutputStream; import java.util.ArrayList; @@ -68,7 +69,7 @@ public final class DelegatingOutErr extends OutErr { private final List<OutputStream> sinks = new ArrayList<>(); public void addSink(OutputStream sink) { - sinks.add(sink); + sinks.add(Preconditions.checkNotNull(sink)); } public void removeSink(OutputStream sink) { |