Shortened the error log for unknown Starlark options.
PiperOrigin-RevId: 318526131
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 0e67901..7c1b53a 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
@@ -13,6 +13,9 @@
// limitations under the License.
package com.google.devtools.build.lib.runtime;
+import static com.google.devtools.build.lib.runtime.BlazeOptionHandler.BAD_OPTION_TAG;
+import static com.google.devtools.build.lib.runtime.BlazeOptionHandler.ERROR_SEPARATOR;
+
import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Preconditions;
import com.google.common.base.Strings;
@@ -91,16 +94,18 @@
private String shutdownReason = null;
private OutputStream logOutputStream = null;
private final LoadingCache<BlazeCommand, OpaqueOptionsData> optionsDataCache =
- CacheBuilder.newBuilder().build(
- new CacheLoader<BlazeCommand, OpaqueOptionsData>() {
- @Override
- public OpaqueOptionsData load(BlazeCommand command) {
- return OptionsParser.getOptionsData(BlazeCommandUtils.getOptions(
- command.getClass(),
- runtime.getBlazeModules(),
- runtime.getRuleClassProvider()));
- }
- });
+ CacheBuilder.newBuilder()
+ .build(
+ new CacheLoader<BlazeCommand, OpaqueOptionsData>() {
+ @Override
+ public OpaqueOptionsData load(BlazeCommand command) {
+ return OptionsParser.getOptionsData(
+ BlazeCommandUtils.getOptions(
+ command.getClass(),
+ runtime.getBlazeModules(),
+ runtime.getRuleClassProvider()));
+ }
+ });
@VisibleForTesting
BlazeCommandDispatcher(BlazeRuntime runtime, BugReporter bugReporter, BlazeCommand... commands) {
@@ -118,9 +123,7 @@
runtime.overrideCommands(ImmutableList.copyOf(commands));
}
- /**
- * Create a Blaze dispatcher that uses the specified {@code BlazeRuntime} instance.
- */
+ /** Create a Blaze dispatcher that uses the specified {@code BlazeRuntime} instance. */
@VisibleForTesting
public BlazeCommandDispatcher(BlazeRuntime runtime) {
this(runtime, runtime.getBugReporter());
@@ -184,8 +187,11 @@
switch (lockingMode) {
case WAIT:
if (!otherClientDescription.equals(currentClientDescription)) {
- outErr.printErrLn("Another command (" + currentClientDescription + ") is running. "
- + " Waiting for it to complete on the server...");
+ outErr.printErrLn(
+ "Another command ("
+ + currentClientDescription
+ + ") is running. "
+ + " Waiting for it to complete on the server...");
otherClientDescription = currentClientDescription;
}
commandLock.wait(500);
@@ -226,16 +232,17 @@
ExitCode.LOCAL_ENVIRONMENTAL_ERROR,
FailureDetails.Command.Code.PREVIOUSLY_SHUTDOWN);
}
- BlazeCommandResult result = execExclusively(
- originalCommandLine,
- invocationPolicy,
- args,
- outErr,
- firstContactTimeMillis,
- commandName,
- command,
- waitTimeInMs,
- startupOptionsTaggedWithBazelRc);
+ BlazeCommandResult result =
+ execExclusively(
+ originalCommandLine,
+ invocationPolicy,
+ args,
+ outErr,
+ firstContactTimeMillis,
+ commandName,
+ command,
+ waitTimeInMs,
+ startupOptionsTaggedWithBazelRc);
if (result.shutdown()) {
// TODO(lberki): This also handles the case where we catch an uncaught Throwable in
// execExclusively() which is not an explicit shutdown.
@@ -641,8 +648,15 @@
NoBuildEvent noBuildEvent) {
PrintingEventHandler printingEventHandler =
new PrintingEventHandler(outErr, EventKind.ALL_EVENTS);
+
+ Optional<String> badOption = retrieveBadOption(storedEventHandler.getEvents());
+
for (String note : optionHandler.getRcfileNotes()) {
- printingEventHandler.handle(Event.info(note));
+ if (badOption.isPresent()) {
+ if (note.contains(badOption.get())) {
+ printingEventHandler.handle(Event.info(note));
+ }
+ }
}
for (Event event : storedEventHandler.getEvents()) {
printingEventHandler.handle(event);
@@ -653,6 +667,15 @@
env.getEventBus().post(noBuildEvent);
}
+ private static Optional<String> retrieveBadOption(ImmutableList<Event> events) {
+ return events.stream()
+ .filter(e -> e.getTag() != null && e.getTag().equals(BAD_OPTION_TAG))
+ .map(Event::getMessage)
+ .filter(message -> message.contains(ERROR_SEPARATOR))
+ .map(message -> message.substring(0, message.indexOf(ERROR_SEPARATOR)))
+ .findFirst();
+ }
+
private OutErr bufferOut(OutErr outErr) {
OutputStream wrappedOut = new BufferedOutputStream(outErr.getOutputStream());
return OutErr.create(wrappedOut, outErr.getErrorStream());
@@ -728,19 +751,15 @@
return new UiEventHandler(outErr, eventOptions, runtime.getClock(), workspacePathFragment);
}
- /**
- * Returns the runtime instance shared by the commands that this dispatcher
- * dispatches to.
- */
+ /** Returns the runtime instance shared by the commands that this dispatcher dispatches to. */
@VisibleForTesting
public BlazeRuntime getRuntime() {
return runtime;
}
/**
- * Shuts down all the registered commands to give them a chance to cleanup or
- * close resources. Should be called by the owner of this command dispatcher
- * in all termination cases.
+ * Shuts down all the registered commands to give them a chance to cleanup or close resources.
+ * Should be called by the owner of this command dispatcher in all termination cases.
*/
public void shutdown() {
closeSilently(logOutputStream);
diff --git a/src/main/java/com/google/devtools/build/lib/runtime/BlazeOptionHandler.java b/src/main/java/com/google/devtools/build/lib/runtime/BlazeOptionHandler.java
index 7a83102..e14d704 100644
--- a/src/main/java/com/google/devtools/build/lib/runtime/BlazeOptionHandler.java
+++ b/src/main/java/com/google/devtools/build/lib/runtime/BlazeOptionHandler.java
@@ -61,6 +61,11 @@
"client_env",
"client_cwd");
+ // Marks an event to indicate a parsing error.
+ static final String BAD_OPTION_TAG = "invalidOption";
+ // Separates the invalid tag from the full error message for easier parsing.
+ static final String ERROR_SEPARATOR = " :: ";
+
private final BlazeRuntime runtime;
private final OptionsParser optionsParser;
private final BlazeWorkspace workspace;
@@ -247,10 +252,10 @@
try {
StarlarkOptionsParser.newStarlarkOptionsParser(env, optionsParser).parse(eventHandler);
} catch (OptionsParsingException e) {
- env.getReporter().handle(Event.error(e.getMessage()));
- logger.atInfo().withCause(e).log("Error parsing Starlark options");
- return createDetailedExitCode(
- "Error parsing Starlark options: " + e.getMessage(), Code.STARLARK_OPTIONS_PARSE_FAILURE);
+ String logMessage = "Error parsing Starlark options";
+ logger.atInfo().withCause(e).log(logMessage);
+ return processOptionsParsingException(
+ eventHandler, e, logMessage, Code.STARLARK_OPTIONS_PARSE_FAILURE);
}
return DetailedExitCode.success();
}
@@ -311,10 +316,10 @@
eventHandler.handle(Event.warn(warning));
}
} catch (OptionsParsingException e) {
- eventHandler.handle(Event.error(e.getMessage()));
- logger.atInfo().withCause(e).log("Error parsing options");
- return createDetailedExitCode(
- "Error parsing options" + e.getMessage(), Code.OPTIONS_PARSE_FAILURE);
+ String logMessage = "Error parsing options";
+ logger.atInfo().withCause(e).log(logMessage);
+ return processOptionsParsingException(
+ eventHandler, e, logMessage, Code.OPTIONS_PARSE_FAILURE);
}
return DetailedExitCode.success();
}
@@ -349,6 +354,25 @@
accumulator.add(commandAnnotation.name());
}
+ private static DetailedExitCode processOptionsParsingException(
+ ExtendedEventHandler eventHandler,
+ OptionsParsingException e,
+ String logMessage,
+ Code failureCode) {
+ Event error;
+ // Differentiates errors stemming from an invalid argument and errors from different parts of
+ // the codebase.
+ if (e.getInvalidArgument() != null) {
+ error =
+ Event.error(e.getInvalidArgument() + ERROR_SEPARATOR + e.getMessage())
+ .withTag(BAD_OPTION_TAG);
+ } else {
+ error = Event.error(e.getMessage());
+ }
+ eventHandler.handle(error);
+ return createDetailedExitCode(logMessage + ": " + e.getMessage(), failureCode);
+ }
+
private String getNotInRealWorkspaceError(Path doNotBuildFile) {
String message =
String.format(
diff --git a/src/main/java/com/google/devtools/build/lib/runtime/StarlarkOptionsParser.java b/src/main/java/com/google/devtools/build/lib/runtime/StarlarkOptionsParser.java
index 7d6e937..f7e5f79 100644
--- a/src/main/java/com/google/devtools/build/lib/runtime/StarlarkOptionsParser.java
+++ b/src/main/java/com/google/devtools/build/lib/runtime/StarlarkOptionsParser.java
@@ -209,11 +209,11 @@
} catch (InterruptedException | TargetParsingException e) {
Thread.currentThread().interrupt();
throw new OptionsParsingException(
- "Error loading option " + targetToBuild + ": " + e.getMessage(), e);
+ "Error loading option " + targetToBuild + ": " + e.getMessage(), targetToBuild, e);
}
Rule associatedRule = buildSetting.getAssociatedRule();
if (associatedRule == null || associatedRule.getRuleClassObject().getBuildSetting() == null) {
- throw new OptionsParsingException("Unrecognized option: " + targetToBuild);
+ throw new OptionsParsingException("Unrecognized option: " + targetToBuild, targetToBuild);
}
return buildSetting;
}
diff --git a/src/test/java/com/google/devtools/build/lib/skylark/StarlarkOptionsParsingTest.java b/src/test/java/com/google/devtools/build/lib/skylark/StarlarkOptionsParsingTest.java
index 48a58d7..e44bb2a 100644
--- a/src/test/java/com/google/devtools/build/lib/skylark/StarlarkOptionsParsingTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skylark/StarlarkOptionsParsingTest.java
@@ -82,6 +82,7 @@
() -> parseStarlarkOptions("--//fake_flag=blahblahblah"));
assertThat(e).hasMessageThat().contains("Error loading option //fake_flag");
+ assertThat(e.getInvalidArgument()).isEqualTo("//fake_flag");
}
// test --fake_flag value
@@ -96,6 +97,7 @@
() -> parseStarlarkOptions("--//fake_flag blahblahblah"));
assertThat(e).hasMessageThat().contains("Error loading option //fake_flag");
+ assertThat(e.getInvalidArgument()).isEqualTo("//fake_flag");
}
// test --fake_flag
@@ -108,6 +110,7 @@
assertThrows(OptionsParsingException.class, () -> parseStarlarkOptions("--//fake_flag"));
assertThat(e).hasMessageThat().contains("Error loading option //fake_flag");
+ assertThat(e.getInvalidArgument()).isEqualTo("//fake_flag");
}
@Test
