Make options parsing in BlazeRuntimeWrapper more realistic
Actually select the options for the command and apply the command-specific
invocation policy. Previously everything was considered "build" and the
command passed to the invocation policy was "null", which is not what happens
in production code.
Making this work also required registering commands so that the invocation
policy's command-inheritance logic actually worked. The list of commands isn't
exhaustive, but should do the trick, easy enough to add more as we need them
(I don't think we need _all_ the commands for now at least).
There's still plenty to clean up around options handling in RuntimeWrapper et
al - for example we should be able to pluck options classes from the registered
modules instead of explicitly providing them, but that's for another change.
PiperOrigin-RevId: 455110823
Change-Id: I6223f7508c785e52a160ff41739b704da5354b12
diff --git a/src/main/java/com/google/devtools/build/lib/buildtool/AqueryProcessor.java b/src/main/java/com/google/devtools/build/lib/buildtool/AqueryProcessor.java
index 3e78649..20a9505 100644
--- a/src/main/java/com/google/devtools/build/lib/buildtool/AqueryProcessor.java
+++ b/src/main/java/com/google/devtools/build/lib/buildtool/AqueryProcessor.java
@@ -63,11 +63,10 @@
}
/** Outputs the current action graph from Skyframe. */
- public BlazeCommandResult dumpActionGraphFromSkyframe(
- CommandEnvironment env, BuildRequest request) {
+ public BlazeCommandResult dumpActionGraphFromSkyframe(CommandEnvironment env) {
try (QueryRuntimeHelper queryRuntimeHelper =
env.getRuntime().getQueryRuntimeHelperFactory().create(env)) {
- AqueryOptions aqueryOptions = request.getOptions(AqueryOptions.class);
+ AqueryOptions aqueryOptions = env.getOptions().getOptions(AqueryOptions.class);
PrintStream printStream =
queryRuntimeHelper.getOutputStreamForQueryOutput() == null
diff --git a/src/main/java/com/google/devtools/build/lib/runtime/commands/AqueryCommand.java b/src/main/java/com/google/devtools/build/lib/runtime/commands/AqueryCommand.java
index b4df82e..750ecb5 100644
--- a/src/main/java/com/google/devtools/build/lib/runtime/commands/AqueryCommand.java
+++ b/src/main/java/com/google/devtools/build/lib/runtime/commands/AqueryCommand.java
@@ -131,7 +131,7 @@
}
if (queryCurrentSkyframeState) {
- return aqueryBuildTool.dumpActionGraphFromSkyframe(env, request);
+ return aqueryBuildTool.dumpActionGraphFromSkyframe(env);
}
try {
return BlazeCommandResult.detailedExitCode(
diff --git a/src/test/java/com/google/devtools/build/lib/buildtool/ActionListenerIntegrationTest.java b/src/test/java/com/google/devtools/build/lib/buildtool/ActionListenerIntegrationTest.java
index 1a0c1df..3069e97 100644
--- a/src/test/java/com/google/devtools/build/lib/buildtool/ActionListenerIntegrationTest.java
+++ b/src/test/java/com/google/devtools/build/lib/buildtool/ActionListenerIntegrationTest.java
@@ -14,6 +14,7 @@
package com.google.devtools.build.lib.buildtool;
import static com.google.common.truth.Truth.assertThat;
+import static org.junit.Assert.assertThrows;
import com.google.common.collect.Iterables;
import com.google.common.collect.Sets;
@@ -439,22 +440,20 @@
write("nobuild/BUILD",
"java_library(name= 'javalib',",
" srcs=[])");
- try {
- createOptionsParser().parse("--experimental_action_listener='this is \\not\\ a valid label'");
- Assert.fail("expected failure");
- } catch (OptionsParsingException ope) {
- assertThat(ope)
- .hasMessageThat()
- .isEqualTo(
- String.format(
- "While parsing option %s='%s': invalid package name ''%s'': "
- + "package names may contain "
- + "A-Z, a-z, 0-9, or any of ' !\"#$%%&'()*+,-./;<=>?[]^_`{|}~' "
- + "(most 7-bit ascii characters except 0-31, 127, ':', or '\\')",
- "--experimental_action_listener",
- "this is \\not\\ a valid label",
- "this is \\not\\ a valid label"));
- }
+ addOptions("--experimental_action_listener='this is \\not\\ a valid label'");
+ OptionsParsingException expected =
+ assertThrows(OptionsParsingException.class, () -> buildTarget("//nobuild:javalib"));
+ assertThat(expected)
+ .hasMessageThat()
+ .isEqualTo(
+ String.format(
+ "While parsing option %s='%s': invalid package name ''%s'': "
+ + "package names may contain "
+ + "A-Z, a-z, 0-9, or any of ' !\"#$%%&'()*+,-./;<=>?[]^_`{|}~' "
+ + "(most 7-bit ascii characters except 0-31, 127, ':', or '\\')",
+ "--experimental_action_listener",
+ "this is \\not\\ a valid label",
+ "this is \\not\\ a valid label"));
}
/**
diff --git a/src/test/java/com/google/devtools/build/lib/buildtool/AqueryBuildToolTest.java b/src/test/java/com/google/devtools/build/lib/buildtool/AqueryBuildToolTest.java
index b9e6518..8f1429d 100644
--- a/src/test/java/com/google/devtools/build/lib/buildtool/AqueryBuildToolTest.java
+++ b/src/test/java/com/google/devtools/build/lib/buildtool/AqueryBuildToolTest.java
@@ -75,8 +75,7 @@
addOptions("--output=text");
CommandEnvironment env = runtimeWrapper.newCommand(AqueryCommand.class);
AqueryProcessor aqueryProcessor = new AqueryProcessor(null);
- BlazeCommandResult result =
- aqueryProcessor.dumpActionGraphFromSkyframe(env, createNewRequest("aquery"));
+ BlazeCommandResult result = aqueryProcessor.dumpActionGraphFromSkyframe(env);
assertThat(result.isSuccess()).isFalse();
assertThat(result.getDetailedExitCode().getFailureDetail().getActionQuery().getCode())
diff --git a/src/test/java/com/google/devtools/build/lib/buildtool/util/BlazeRuntimeWrapper.java b/src/test/java/com/google/devtools/build/lib/buildtool/util/BlazeRuntimeWrapper.java
index d6f37e2..aa5a49e 100644
--- a/src/test/java/com/google/devtools/build/lib/buildtool/util/BlazeRuntimeWrapper.java
+++ b/src/test/java/com/google/devtools/build/lib/buildtool/util/BlazeRuntimeWrapper.java
@@ -148,49 +148,6 @@
})
.build();
runtime.initWorkspace(directories, binTools);
- optionsParser = createOptionsParser();
- }
-
- @Command(name = "build", builds = true, help = "", shortDescription = "")
- private static class DummyBuildCommand {}
-
- public OptionsParser createOptionsParser() {
- Set<Class<? extends OptionsBase>> options =
- new HashSet<>(
- ImmutableList.of(
- BuildRequestOptions.class,
- BuildEventProtocolOptions.class,
- ExecutionOptions.class,
- LocalExecutionOptions.class,
- CommonCommandOptions.class,
- ClientOptions.class,
- LoadingOptions.class,
- AnalysisOptions.class,
- KeepGoingOption.class,
- LoadingPhaseThreadsOption.class,
- PackageOptions.class,
- BuildLanguageOptions.class,
- UiOptions.class,
- SandboxOptions.class));
- options.addAll(additionalOptionsClasses);
-
- for (BlazeModule module : runtime.getBlazeModules()) {
- Iterables.addAll(options, module.getCommonCommandOptions());
- Iterables.addAll(
- options, module.getCommandOptions(DummyBuildCommand.class.getAnnotation(Command.class)));
- }
- options.addAll(runtime.getRuleClassProvider().getFragmentRegistry().getOptionsClasses());
- return OptionsParser.builder().optionsClasses(options).build();
- }
-
- private void enforceTestInvocationPolicy(OptionsParser parser) {
- InvocationPolicyEnforcer optionsPolicyEnforcer =
- new InvocationPolicyEnforcer(runtime.getModuleInvocationPolicy(), Level.FINE);
- try {
- optionsPolicyEnforcer.enforce(parser, /*command=*/ null);
- } catch (OptionsParsingException e) {
- throw new IllegalStateException(e);
- }
}
public final BlazeRuntime getRuntime() {
@@ -218,10 +175,15 @@
*/
public final CommandEnvironment newCommandWithExtensions(
Class<? extends BlazeCommand> command, List<Message> extensions) throws Exception {
+ Command commandAnnotation =
+ checkNotNull(
+ command.getAnnotation(Command.class),
+ "BlazeCommand %s missing command annotation",
+ command);
additionalOptionsClasses.addAll(
BlazeCommandUtils.getOptions(
command, runtime.getBlazeModules(), runtime.getRuleClassProvider()));
- initializeOptionsParser();
+ initializeOptionsParser(commandAnnotation);
commandCreated = true;
if (env != null) {
runtime.afterCommand(env, BlazeCommandResult.success());
@@ -236,7 +198,7 @@
runtime
.getWorkspace()
.initCommand(
- command.getAnnotation(Command.class),
+ commandAnnotation,
optionsParser,
new ArrayList<>(),
0L,
@@ -293,12 +255,47 @@
* Initializes a new options parser, parsing all the options set by {@link
* #addOptions(String...)}.
*/
- public void initializeOptionsParser() throws OptionsParsingException {
+ private void initializeOptionsParser(Command commandAnnotation) throws OptionsParsingException {
// Create the options parser and parse all the options collected so far
- optionsParser = createOptionsParser();
+ optionsParser = createOptionsParser(commandAnnotation);
optionsParser.parse(optionsToParse);
+
// Enforce the test invocation policy once the options have been added
- enforceTestInvocationPolicy(optionsParser);
+ InvocationPolicyEnforcer optionsPolicyEnforcer =
+ new InvocationPolicyEnforcer(runtime.getModuleInvocationPolicy(), Level.FINE);
+ try {
+ optionsPolicyEnforcer.enforce(optionsParser, commandAnnotation.name());
+ } catch (OptionsParsingException e) {
+ throw new IllegalStateException(e);
+ }
+ }
+
+ private OptionsParser createOptionsParser(Command commandAnnotation) {
+ Set<Class<? extends OptionsBase>> options =
+ new HashSet<>(
+ ImmutableList.of(
+ BuildRequestOptions.class,
+ BuildEventProtocolOptions.class,
+ ExecutionOptions.class,
+ LocalExecutionOptions.class,
+ CommonCommandOptions.class,
+ ClientOptions.class,
+ LoadingOptions.class,
+ AnalysisOptions.class,
+ KeepGoingOption.class,
+ LoadingPhaseThreadsOption.class,
+ PackageOptions.class,
+ BuildLanguageOptions.class,
+ UiOptions.class,
+ SandboxOptions.class));
+ options.addAll(additionalOptionsClasses);
+
+ for (BlazeModule module : runtime.getBlazeModules()) {
+ Iterables.addAll(options, module.getCommonCommandOptions());
+ Iterables.addAll(options, module.getCommandOptions(commandAnnotation));
+ }
+ options.addAll(runtime.getRuleClassProvider().getFragmentRegistry().getOptionsClasses());
+ return OptionsParser.builder().optionsClasses(options).build();
}
public void executeBuild(List<String> targets) throws Exception {
@@ -390,8 +387,7 @@
.build();
}
- BuildRequest createRequest(String commandName, List<String> targets) {
-
+ private BuildRequest createRequest(String commandName, List<String> targets) {
BuildRequest.Builder builder =
BuildRequest.builder()
.setCommandName(commandName)
diff --git a/src/test/java/com/google/devtools/build/lib/buildtool/util/BuildIntegrationTestCase.java b/src/test/java/com/google/devtools/build/lib/buildtool/util/BuildIntegrationTestCase.java
index 9bfb036..9c09203 100644
--- a/src/test/java/com/google/devtools/build/lib/buildtool/util/BuildIntegrationTestCase.java
+++ b/src/test/java/com/google/devtools/build/lib/buildtool/util/BuildIntegrationTestCase.java
@@ -85,7 +85,13 @@
import com.google.devtools.build.lib.runtime.BlazeWorkspace;
import com.google.devtools.build.lib.runtime.CommandEnvironment;
import com.google.devtools.build.lib.runtime.NoSpawnCacheModule;
+import com.google.devtools.build.lib.runtime.ServerBuilder;
import com.google.devtools.build.lib.runtime.WorkspaceBuilder;
+import com.google.devtools.build.lib.runtime.commands.BuildCommand;
+import com.google.devtools.build.lib.runtime.commands.CqueryCommand;
+import com.google.devtools.build.lib.runtime.commands.InfoCommand;
+import com.google.devtools.build.lib.runtime.commands.QueryCommand;
+import com.google.devtools.build.lib.runtime.commands.TestCommand;
import com.google.devtools.build.lib.sandbox.SandboxModule;
import com.google.devtools.build.lib.server.FailureDetails.FailureDetail;
import com.google.devtools.build.lib.server.FailureDetails.Spawn;
@@ -122,6 +128,7 @@
import com.google.devtools.build.lib.worker.WorkerModule;
import com.google.devtools.common.options.OptionsBase;
import com.google.devtools.common.options.OptionsParser;
+import com.google.devtools.common.options.OptionsParsingResult;
import com.google.errorprone.annotations.FormatMethod;
import java.io.IOException;
import java.lang.Thread.UncaughtExceptionHandler;
@@ -516,6 +523,7 @@
.setProductName(TestConstants.PRODUCT_NAME)
.setBugReporter(bugReporter)
.setStartupOptionsProvider(startupOptionsParser)
+ .addBlazeModule(new BuildIntegrationTestCommandsModule())
.addBlazeModule(connectivityModule)
.addBlazeModule(getMockBazelRepositoryModule());
getSpawnModules().forEach(builder::addBlazeModule);
@@ -581,10 +589,6 @@
runtimeWrapper.addStarlarkOption(label, value);
}
- protected OptionsParser createOptionsParser() {
- return runtimeWrapper.createOptionsParser();
- }
-
protected Action getGeneratingAction(Artifact artifact) {
ActionAnalysisMetadata action = getActionGraph().getGeneratingAction(artifact);
@@ -709,18 +713,6 @@
return runtimeWrapper.getLastResult();
}
- /**
- * Creates a BuildRequest for either blaze build or blaze analyze, using the currently-installed
- * request options.
- *
- * @param commandName blaze build or analyze command
- * @param targets the targets to be built
- */
- protected BuildRequest createNewRequest(String commandName, String... targets) throws Exception {
- runtimeWrapper.initializeOptionsParser();
- return runtimeWrapper.createRequest(commandName, Arrays.asList(targets));
- }
-
/** Utility function: parse a string as a label. */
protected static Label label(String labelString) throws LabelSyntaxException {
return Label.parseAbsolute(labelString, ImmutableMap.of());
@@ -1062,4 +1054,22 @@
exceptions.clear();
}
}
+
+ /**
+ * Performs command registration to the extent that is necessary for test execution. The list of
+ * commands isn't comprehensive and a command needn't be registered to be used. The purpose of
+ * this module is to ensure that functionality that requires commands to be explicitly registered
+ * (for example, per-command invocation policies) is sufficiently configured.
+ */
+ private static class BuildIntegrationTestCommandsModule extends BlazeModule {
+ @Override
+ public void serverInit(OptionsParsingResult startupOptions, ServerBuilder builder) {
+ builder.addCommands(
+ new BuildCommand(),
+ new QueryCommand(),
+ new CqueryCommand(),
+ new InfoCommand(),
+ new TestCommand());
+ }
+ }
}
diff --git a/src/test/java/com/google/devtools/build/lib/metrics/MetricsCollectorTest.java b/src/test/java/com/google/devtools/build/lib/metrics/MetricsCollectorTest.java
index 00db148..cc2bbab 100644
--- a/src/test/java/com/google/devtools/build/lib/metrics/MetricsCollectorTest.java
+++ b/src/test/java/com/google/devtools/build/lib/metrics/MetricsCollectorTest.java
@@ -498,7 +498,6 @@
// The options from above do not get added to the initial command environment,
// so it has to be recreated here.
- runtimeWrapper.initializeOptionsParser();
buildTarget("//foo:foo");
BuildMetrics buildMetrics = buildMetricsEventListener.event.getBuildMetrics();
assertThat(buildMetrics.getMemoryMetrics().getUsedHeapSizePostBuild()).isGreaterThan(0L);
diff --git a/src/test/java/com/google/devtools/build/lib/sandbox/SandboxedSpawnRunnerTestCase.java b/src/test/java/com/google/devtools/build/lib/sandbox/SandboxedSpawnRunnerTestCase.java
index e59e358..f4cefcd 100644
--- a/src/test/java/com/google/devtools/build/lib/sandbox/SandboxedSpawnRunnerTestCase.java
+++ b/src/test/java/com/google/devtools/build/lib/sandbox/SandboxedSpawnRunnerTestCase.java
@@ -41,7 +41,6 @@
protected CommandEnvironment getCommandEnvironmentWithExecutionStatisticsOptionEnabled(
String workspaceName) throws Exception {
runtimeWrapper.addOptions("--experimental_collect_local_sandbox_action_metrics");
- runtimeWrapper.initializeOptionsParser();
CommandEnvironment env = runtimeWrapper.newCommand();
env.setWorkspaceName(workspaceName);
env.getLocalResourceManager().setAvailableResources(LocalHostCapacity.getLocalHostCapacity());
