Skip to content

Commit

Permalink
Let common ignore unsupported options
Browse files Browse the repository at this point in the history 
Fixes bazelbuild#3054

RELNOTES: Options specified on the pseudo-command `common` in `.rc` files are now ignored by commands that do not support them as long as they are valid options for *any* Bazel command. Previously, commands that did not support all options given for `common` would fail to run. These previous semantics of `common` are now available via the new `always` pseudo-command.

Closes bazelbuild#18130.

PiperOrigin-RevId: 534940403
Change-Id: I2ae268ae84de3f2b07ff3d1e36bf382bce714968
  • Loading branch information
@fmeum @iancha1992
fmeum authored and iancha1992 committed May 25, 2023
1 parent 15fc292 commit 45a637c
Show file tree
Hide file tree
Showing 14 changed files with 816 additions and 196 deletions.
9 changes: 8 additions & 1 deletion site/en/run/bazelrc.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,14 @@ line specifies when these defaults are applied:
- `startup`: startup options, which go before the command, and are described
in `bazel help startup_options`.
- `common`: options that apply to all Bazel commands.
- `common`: options that should be applied to all Bazel commands that support
them. If a command does not support an option specified in this way, the
option is ignored so long as it is valid for *some* other Bazel command.
Note that this only applies to option names: If the current command accepts
an option with the specified name, but doesn't support the specified value,
it will fail.
- `always`: options that apply to all Bazel commands. If a command does not
support an option specified in this way, it will fail.
- _`command`_: Bazel command, such as `build` or `query` to which the options
apply. These options also apply to all commands that inherit from the
specified command. (For example, `test` inherits from `build`.)
Expand Down
72 changes: 66 additions & 6 deletions src/main/java/com/google/devtools/build/lib/runtime/BlazeOptionHandler.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@
// limitations under the License.
package com.google.devtools.build.lib.runtime;

import static com.google.common.collect.ImmutableList.toImmutableList;

import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Joiner;
import com.google.common.collect.ArrayListMultimap;
Expand All @@ -35,6 +37,7 @@
import com.google.devtools.common.options.InvocationPolicyEnforcer;
import com.google.devtools.common.options.OptionDefinition;
import com.google.devtools.common.options.OptionPriority.PriorityCategory;
import com.google.devtools.common.options.OptionsBase;
import com.google.devtools.common.options.OptionsParser;
import com.google.devtools.common.options.OptionsParsingException;
import com.google.devtools.common.options.OptionsParsingResult;
Expand Down Expand Up @@ -66,6 +69,14 @@ public final class BlazeOptionHandler {
"client_env",
"client_cwd");

// All options set on this pseudo command are inherited by all commands, with unrecognized options
// resulting in an error.
private static final String ALWAYS_PSEUDO_COMMAND = "always";

// All options set on this pseudo command are inherited by all commands, with unrecognized options
// being ignored as long as they are recognized by at least one (other) command.
private static final String COMMON_PSEUDO_COMMAND = "common";

// 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.
Expand All @@ -78,6 +89,7 @@ public final class BlazeOptionHandler {
private final Command commandAnnotation;
private final InvocationPolicy invocationPolicy;
private final List<String> rcfileNotes = new ArrayList<>();
private final ImmutableList<Class<? extends OptionsBase>> allOptionsClasses;

BlazeOptionHandler(
BlazeRuntime runtime,
Expand All @@ -92,6 +104,16 @@ public final class BlazeOptionHandler {
this.commandAnnotation = commandAnnotation;
this.optionsParser = optionsParser;
this.invocationPolicy = invocationPolicy;
this.allOptionsClasses =
runtime.getCommandMap().values().stream()
.map(BlazeCommand::getClass)
.flatMap(
cmd ->
BlazeCommandUtils.getOptions(
cmd, runtime.getBlazeModules(), runtime.getRuleClassProvider())
.stream())
.distinct()
.collect(toImmutableList());
}

/**
Expand Down Expand Up @@ -191,7 +213,36 @@ void parseRcOptions(
"%s:\n %s'%s' options: %s",
source, inherited, commandToParse, Joiner.on(' ').join(rcArgs.getArgs())));
}
optionsParser.parse(PriorityCategory.RC_FILE, rcArgs.getRcFile(), rcArgs.getArgs());
if (commandToParse.equals(COMMON_PSEUDO_COMMAND)) {
// Pass in options data for all commands supported by the runtime so that options that
// apply to some but not the current command can be ignored.
//
// Important note: The consistency checks performed by
// OptionsParser#getFallbackOptionsData ensure that there aren't any two options across
// all commands that have the same name but parse differently (e.g. because one accepts
// a value and the other doesn't). This means that the options available on a command
// limit the options available on other commands even without command inheritance. This
// restriction is necessary to ensure that the options specified on the "common"
// pseudo command can be parsed unambiguously.
ImmutableList<String> ignoredArgs =
optionsParser.parseWithSourceFunction(
PriorityCategory.RC_FILE,
o -> rcArgs.getRcFile(),
rcArgs.getArgs(),
OptionsParser.getFallbackOptionsData(allOptionsClasses));
if (!ignoredArgs.isEmpty()) {
// Append richer information to the note.
int index = rcfileNotes.size() - 1;
String note = rcfileNotes.get(index);
note +=
String.format(
"\n Ignored as unsupported by '%s': %s",
commandAnnotation.name(), Joiner.on(' ').join(ignoredArgs));
rcfileNotes.set(index, note);
}
} else {
optionsParser.parse(PriorityCategory.RC_FILE, rcArgs.getRcFile(), rcArgs.getArgs());
}
}
}
}
Expand Down Expand Up @@ -227,7 +278,8 @@ private void parseArgsAndConfigs(List<String> args, ExtendedEventHandler eventHa
optionsParser.parseWithSourceFunction(
PriorityCategory.COMMAND_LINE,
commandOptionSourceFunction,
defaultOverridesAndRcSources.build());
defaultOverridesAndRcSources.build(),
/* fallbackData= */ null);

// Command-specific options from .blazerc passed in via --default_override and --rc_source.
ClientOptions rcFileOptions = optionsParser.getOptions(ClientOptions.class);
Expand All @@ -241,7 +293,10 @@ private void parseArgsAndConfigs(List<String> args, ExtendedEventHandler eventHa

// Parses the remaining command-line options.
optionsParser.parseWithSourceFunction(
PriorityCategory.COMMAND_LINE, commandOptionSourceFunction, remainingCmdLine.build());
PriorityCategory.COMMAND_LINE,
commandOptionSourceFunction,
remainingCmdLine.build(),
/* fallbackData= */ null);

if (commandAnnotation.builds()) {
// splits project files from targets in the traditional sense
Expand Down Expand Up @@ -372,14 +427,17 @@ void expandConfigOptions(
ConfigExpander.expandConfigOptions(
eventHandler,
commandToRcArgs,
commandAnnotation.name(),
getCommandNamesToParse(commandAnnotation),
rcfileNotes::add,
optionsParser);
optionsParser,
OptionsParser.getFallbackOptionsData(allOptionsClasses));
}

private static List<String> getCommandNamesToParse(Command commandAnnotation) {
List<String> result = new ArrayList<>();
result.add("common");
result.add(ALWAYS_PSEUDO_COMMAND);
result.add(COMMON_PSEUDO_COMMAND);
getCommandNamesToParseHelper(commandAnnotation, result);
return result;
}
Expand Down Expand Up @@ -470,7 +528,9 @@ static ListMultimap<String, RcChunkOfArgs> structureRcOptionsAndConfigs(
if (index > 0) {
command = command.substring(0, index);
}
if (!validCommands.contains(command) && !command.equals("common")) {
if (!validCommands.contains(command)
&& !command.equals(ALWAYS_PSEUDO_COMMAND)
&& !command.equals(COMMON_PSEUDO_COMMAND)) {
eventHandler.handle(
Event.warn(
"while reading option defaults file '"
Expand Down
3 changes: 2 additions & 1 deletion src/main/java/com/google/devtools/build/lib/runtime/BlazeRuntime.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1110,7 +1110,8 @@ private static OptionsParsingResult parseStartupOptions(

// Then parse the command line again, this time with the correct option sources
parser = OptionsParser.builder().optionsClasses(optionClasses).allowResidue(false).build();
parser.parseWithSourceFunction(PriorityCategory.COMMAND_LINE, sourceFunction, args);
parser.parseWithSourceFunction(
PriorityCategory.COMMAND_LINE, sourceFunction, args, /* fallbackData= */ null);
return parser;
}

Expand Down
24 changes: 20 additions & 4 deletions src/main/java/com/google/devtools/build/lib/runtime/ConfigExpander.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,12 +13,14 @@
// limitations under the License.
package com.google.devtools.build.lib.runtime;

import com.google.common.base.Joiner;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.Iterables;
import com.google.common.collect.ListMultimap;
import com.google.devtools.build.lib.events.Event;
import com.google.devtools.build.lib.events.EventHandler;
import com.google.devtools.build.lib.util.OS;
import com.google.devtools.common.options.OpaqueOptionsData;
import com.google.devtools.common.options.OptionValueDescription;
import com.google.devtools.common.options.OptionsParser;
import com.google.devtools.common.options.OptionsParsingException;
Expand All @@ -29,6 +31,7 @@
import java.util.List;
import java.util.Set;
import java.util.function.Consumer;
import javax.annotation.Nullable;

/** Encapsulates logic for performing --config option expansion. */
final class ConfigExpander {
Expand Down Expand Up @@ -86,9 +89,11 @@ private static boolean shouldEnablePlatformSpecificConfig(
static void expandConfigOptions(
EventHandler eventHandler,
ListMultimap<String, RcChunkOfArgs> commandToRcArgs,
String currentCommand,
List<String> commandsToParse,
Consumer<String> rcFileNotesConsumer,
OptionsParser optionsParser)
OptionsParser optionsParser,
@Nullable OpaqueOptionsData fallbackData)
throws OptionsParsingException {

OptionValueDescription configValueDescription =
Expand All @@ -110,8 +115,18 @@ static void expandConfigOptions(
commandsToParse,
configValueToExpand,
rcFileNotesConsumer);
optionsParser.parseArgsAsExpansionOfOption(
configInstance, String.format("expanded from --%s", configValueToExpand), expansion);
var ignoredArgs =
optionsParser.parseArgsAsExpansionOfOption(
configInstance,
String.format("expanded from --config=%s", configValueToExpand),
expansion,
fallbackData);
if (!ignoredArgs.isEmpty()) {
rcFileNotesConsumer.accept(
String.format(
"Ignored as unsupported by '%s': %s",
currentCommand, Joiner.on(' ').join(ignoredArgs)));
}
}
}

Expand All @@ -129,7 +144,8 @@ static void expandConfigOptions(
optionsParser.parseArgsAsExpansionOfOption(
Iterables.getOnlyElement(enablePlatformSpecificConfigDescription.getCanonicalInstances()),
String.format("enabled by --enable_platform_specific_config"),
expansion);
expansion,
fallbackData);
}

// At this point, we've expanded everything, identify duplicates, if any, to warn about
Expand Down
1 change: 1 addition & 0 deletions src/main/java/com/google/devtools/common/options/BUILD
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ java_library(
],
),
deps = [
"//src/main/java/com/google/devtools/build/lib/util:pair",
"//third_party:auto_value",
"//third_party:caffeine",
"//third_party:guava",
Expand Down
Loading

0 comments on commit 45a637c

Please sign in to comment.