Skip to content

Commit

Permalink
Support all-supported pseudo command in rc files
Browse files Browse the repository at this point in the history 
Options specified on the `all-supported` command in rc files are applied
to all commands that support them. If an option isn't supported by the
current command but by some other Bazel command, it is ignored. If it
isn't supported by any Bazel command, the usual error message is shown.
  • Loading branch information
@fmeum
fmeum committed Apr 18, 2023
1 parent 91584dd commit 065d09d
Show file tree
Hide file tree
Showing 10 changed files with 270 additions and 116 deletions.
33 changes: 28 additions & 5 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 @@ -33,8 +35,10 @@
import com.google.devtools.build.lib.vfs.Path;
import com.google.devtools.common.options.Converters;
import com.google.devtools.common.options.InvocationPolicyEnforcer;
import com.google.devtools.common.options.OpaqueOptionsData;
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 +70,8 @@ public final class BlazeOptionHandler {
"client_env",
"client_cwd");

private static final String ALL_SUPPORTED_PSEUDO_COMMAND = "all-supported";

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

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

/**
Expand Down Expand Up @@ -191,7 +203,14 @@ 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(ALL_SUPPORTED_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.
optionsParser.parseWithSourceFunction(PriorityCategory.RC_FILE, o -> rcArgs.getRcFile(),
rcArgs.getArgs(), OptionsParser.getFallbackOptionsData(allOptionsClasses));
} else {
optionsParser.parse(PriorityCategory.RC_FILE, rcArgs.getRcFile(), rcArgs.getArgs());
}
}
}
}
Expand Down Expand Up @@ -227,7 +246,8 @@ private void parseArgsAndConfigs(List<String> args, ExtendedEventHandler eventHa
optionsParser.parseWithSourceFunction(
PriorityCategory.COMMAND_LINE,
commandOptionSourceFunction,
defaultOverridesAndRcSources.build());
defaultOverridesAndRcSources.build(),
null);

// Command-specific options from .blazerc passed in via --default_override and --rc_source.
ClientOptions rcFileOptions = optionsParser.getOptions(ClientOptions.class);
Expand All @@ -241,7 +261,7 @@ 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(), null);

if (commandAnnotation.builds()) {
// splits project files from targets in the traditional sense
Expand Down Expand Up @@ -374,12 +394,14 @@ void expandConfigOptions(
commandToRcArgs,
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(ALL_SUPPORTED_PSEUDO_COMMAND);
getCommandNamesToParseHelper(commandAnnotation, result);
return result;
}
Expand Down Expand Up @@ -470,7 +492,8 @@ 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("common") && !command.equals(
ALL_SUPPORTED_PSEUDO_COMMAND)) {
eventHandler.handle(
Event.warn(
"while reading option defaults file '"
Expand Down
2 changes: 1 addition & 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 @@ -1113,7 +1113,7 @@ 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, null);
return parser;
}

Expand Down
10 changes: 7 additions & 3 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 @@ -19,6 +19,7 @@
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 Down Expand Up @@ -88,7 +89,8 @@ static void expandConfigOptions(
ListMultimap<String, RcChunkOfArgs> commandToRcArgs,
List<String> commandsToParse,
Consumer<String> rcFileNotesConsumer,
OptionsParser optionsParser)
OptionsParser optionsParser,
OpaqueOptionsData fallbackData)
throws OptionsParsingException {

OptionValueDescription configValueDescription =
Expand All @@ -113,7 +115,8 @@ static void expandConfigOptions(
optionsParser.parseArgsAsExpansionOfOption(
configInstance,
String.format("expanded from --config=%s", configValueToExpand),
expansion);
expansion,
fallbackData);
}
}

Expand All @@ -131,7 +134,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 @@ -49,6 +49,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
74 changes: 46 additions & 28 deletions src/main/java/com/google/devtools/common/options/IsolatedOptionsData.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -83,22 +83,22 @@ public static ImmutableList<OptionDefinition> getAllOptionDefinitionsForClass(

/**
* Mapping from each options class to its no-arg constructor. Entries appear in the same order
* that they were passed to {@link #from(Collection)}.
* that they were passed to {@link #from(Collection, boolean)}.
*/
private final ImmutableMap<Class<? extends OptionsBase>, Constructor<?>> optionsClasses;

/**
* Mapping from option name to {@code OptionDefinition}. Entries appear ordered first by their
* options class (the order in which they were passed to {@link #from(Collection)}, and then in
* alphabetic order within each options class.
* options class (the order in which they were passed to {@link #from(Collection, boolean)}, and
* then in alphabetic order within each options class.
*/
private final ImmutableMap<String, OptionDefinition> nameToField;

/**
* For options that have an "OldName", this is a mapping from old name to its corresponding {@code
* OptionDefinition}. Entries appear ordered first by their options class (the order in which they
* were passed to {@link #from(Collection)}, and then in alphabetic order within each options
* class.
* For options that have an "OldName", this is a mapping from old name to its corresponding
* {@code OptionDefinition}. Entries appear ordered first by their options class (the order in
* which they were passed to {@link #from(Collection, boolean)}, and then in alphabetic order
* within each options class.
*/
private final ImmutableMap<String, OptionDefinition> oldNameToField;

Expand Down Expand Up @@ -136,7 +136,7 @@ protected IsolatedOptionsData(IsolatedOptionsData other) {

/**
* Returns all options classes indexed by this options data object, in the order they were passed
* to {@link #from(Collection)}.
* to {@link #from(Collection, boolean)}.
*/
public Collection<Class<? extends OptionsBase>> getOptionsClasses() {
return optionsClasses.keySet();
Expand All @@ -157,8 +157,8 @@ public OptionDefinition getOptionDefinitionFromName(String name) {

/**
* Returns all {@link OptionDefinition} objects loaded, mapped by their canonical names. Entries
* appear ordered first by their options class (the order in which they were passed to {@link
* #from(Collection)}, and then in alphabetic order within each options class.
* appear ordered first by their options class (the order in which they were passed to
* {@link #from(Collection, boolean)}, and then in alphabetic order within each options class.
*/
public Iterable<Map.Entry<String, OptionDefinition>> getAllOptionDefinitions() {
return nameToField.entrySet();
Expand All @@ -177,9 +177,15 @@ public boolean getUsesOnlyCoreTypes(Class<? extends OptionsBase> optionsClass) {
* both single-character abbreviations and full names.
*/
private static <A> void checkForCollisions(
Map<A, OptionDefinition> aFieldMap, A optionName, String description)
Map<A, OptionDefinition> aFieldMap, A optionName, OptionDefinition definition,
String description, boolean allowDuplicatesParsingEquivalently)
throws DuplicateOptionDeclarationException {
if (aFieldMap.containsKey(optionName)) {
OptionDefinition otherDefinition = aFieldMap.get(optionName);
if (allowDuplicatesParsingEquivalently && OptionsParserImpl.equivalentForParsing(
otherDefinition, definition)) {
return;
}
throw new DuplicateOptionDeclarationException(
"Duplicate option name, due to " + description + ": --" + optionName);
}
Expand Down Expand Up @@ -212,22 +218,27 @@ private static void checkAndUpdateBooleanAliases(
Map<String, OptionDefinition> nameToFieldMap,
Map<String, OptionDefinition> oldNameToFieldMap,
Map<String, String> booleanAliasMap,
String optionName)
String optionName,
OptionDefinition optionDefinition,
boolean allowDuplicatesParsingEquivalently)
throws DuplicateOptionDeclarationException {
// Check that the negating alias does not conflict with existing flags.
checkForCollisions(nameToFieldMap, "no" + optionName, "boolean option alias");
checkForCollisions(oldNameToFieldMap, "no" + optionName, "boolean option alias");
checkForCollisions(nameToFieldMap, "no" + optionName, optionDefinition, "boolean option alias",
allowDuplicatesParsingEquivalently);
checkForCollisions(oldNameToFieldMap, "no" + optionName, optionDefinition,
"boolean option alias", allowDuplicatesParsingEquivalently);

// Record that the boolean option takes up additional namespace for its negating alias.
booleanAliasMap.put("no" + optionName, optionName);
}

/**
* Constructs an {@link IsolatedOptionsData} object for a parser that knows about the given {@link
* OptionsBase} classes. No inter-option analysis is done. Performs basic validity checks on each
* option in isolation.
* Constructs an {@link IsolatedOptionsData} object for a parser that knows about the given
* {@link OptionsBase} classes. No inter-option analysis is done. Performs basic validity checks
* on each option in isolation.
*/
static IsolatedOptionsData from(Collection<Class<? extends OptionsBase>> classes) {
static IsolatedOptionsData from(Collection<Class<? extends OptionsBase>> classes,
boolean allowDuplicatesParsingEquivalently) {
// Mind which fields have to preserve order.
Map<Class<? extends OptionsBase>, Constructor<?>> constructorBuilder = new LinkedHashMap<>();
Map<String, OptionDefinition> nameToFieldBuilder = new LinkedHashMap<>();
Expand Down Expand Up @@ -256,15 +267,18 @@ static IsolatedOptionsData from(Collection<Class<? extends OptionsBase>> classes
for (OptionDefinition optionDefinition : optionDefinitions) {
try {
String optionName = optionDefinition.getOptionName();
checkForCollisions(nameToFieldBuilder, optionName, "option name collision");
checkForCollisions(nameToFieldBuilder, optionName, optionDefinition,
"option name collision", allowDuplicatesParsingEquivalently);
checkForCollisions(
oldNameToFieldBuilder,
optionName,
"option name collision with another option's old name");
optionDefinition,
"option name collision with another option's old name",
allowDuplicatesParsingEquivalently);
checkForBooleanAliasCollisions(booleanAliasMap, optionName, "option");
if (optionDefinition.usesBooleanValueSyntax()) {
checkAndUpdateBooleanAliases(
nameToFieldBuilder, oldNameToFieldBuilder, booleanAliasMap, optionName);
checkAndUpdateBooleanAliases(nameToFieldBuilder, oldNameToFieldBuilder, booleanAliasMap,
optionName, optionDefinition, allowDuplicatesParsingEquivalently);
}
nameToFieldBuilder.put(optionName, optionDefinition);

Expand All @@ -273,23 +287,27 @@ static IsolatedOptionsData from(Collection<Class<? extends OptionsBase>> classes
checkForCollisions(
nameToFieldBuilder,
oldName,
"old option name collision with another option's canonical name");
optionDefinition,
"old option name collision with another option's canonical name",
allowDuplicatesParsingEquivalently);
checkForCollisions(
oldNameToFieldBuilder,
oldName,
"old option name collision with another old option name");
optionDefinition,
"old option name collision with another old option name",
allowDuplicatesParsingEquivalently);
checkForBooleanAliasCollisions(booleanAliasMap, oldName, "old option name");
// If boolean, repeat the alias dance for the old name.
if (optionDefinition.usesBooleanValueSyntax()) {
checkAndUpdateBooleanAliases(
nameToFieldBuilder, oldNameToFieldBuilder, booleanAliasMap, oldName);
checkAndUpdateBooleanAliases(nameToFieldBuilder, oldNameToFieldBuilder,
booleanAliasMap, oldName, optionDefinition, allowDuplicatesParsingEquivalently);
}
// Now that we've checked for conflicts, confidently store the old name.
oldNameToFieldBuilder.put(oldName, optionDefinition);
}
if (optionDefinition.getAbbreviation() != '\0') {
checkForCollisions(
abbrevToFieldBuilder, optionDefinition.getAbbreviation(), "option abbreviation");
checkForCollisions(abbrevToFieldBuilder, optionDefinition.getAbbreviation(),
optionDefinition, "option abbreviation", allowDuplicatesParsingEquivalently);
abbrevToFieldBuilder.put(optionDefinition.getAbbreviation(), optionDefinition);
}
} catch (DuplicateOptionDeclarationException e) {
Expand Down
43 changes: 33 additions & 10 deletions src/main/java/com/google/devtools/common/options/OptionsData.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,14 +28,26 @@
@Immutable
final class OptionsData extends IsolatedOptionsData {

/** Mapping from each option to the (unparsed) options it expands to, if any. */
/**
* Mapping from each option to the (unparsed) options it expands to, if any.
*/
private final ImmutableMap<OptionDefinition, ImmutableList<String>> evaluatedExpansions;

/** Construct {@link OptionsData} by extending an {@link IsolatedOptionsData} with new info. */
/**
* Whether this options data has been created with duplicate options definitions allowed as long
* as those options are parsed (but not necessarily evaluated) equivalently.
*/
private final boolean allowDuplicatesParsingEquivalently;

/**
* Construct {@link OptionsData} by extending an {@link IsolatedOptionsData} with new info.
*/
private OptionsData(
IsolatedOptionsData base, Map<OptionDefinition, ImmutableList<String>> evaluatedExpansions) {
IsolatedOptionsData base, Map<OptionDefinition, ImmutableList<String>> evaluatedExpansions,
boolean allowDuplicatesParsingEquivalently) {
super(base);
this.evaluatedExpansions = ImmutableMap.copyOf(evaluatedExpansions);
this.allowDuplicatesParsingEquivalently = allowDuplicatesParsingEquivalently;
}

private static final ImmutableList<String> EMPTY_EXPANSION = ImmutableList.<String>of();
Expand All @@ -50,13 +62,23 @@ public ImmutableList<String> getEvaluatedExpansion(OptionDefinition optionDefini
}

/**
* Constructs an {@link OptionsData} object for a parser that knows about the given {@link
* OptionsBase} classes. In addition to the work done to construct the {@link
* IsolatedOptionsData}, this also computes expansion information. If an option has an expansion,
* try to precalculate its here.
* Returns whether this options data has been created with duplicate options definitions allowed
* as long as those options are parsed (but not necessarily evaluated) equivalently.
*/
public boolean createdWithAllowDuplicatesParsingEquivalently() {
return allowDuplicatesParsingEquivalently;
}

/**
* Constructs an {@link OptionsData} object for a parser that knows about the given
* {@link OptionsBase} classes. In addition to the work done to construct the
* {@link IsolatedOptionsData}, this also computes expansion information. If an option has an
* expansion, try to precalculate its here.
*/
static OptionsData from(Collection<Class<? extends OptionsBase>> classes) {
IsolatedOptionsData isolatedData = IsolatedOptionsData.from(classes);
static OptionsData from(Collection<Class<? extends OptionsBase>> classes,
boolean allowDuplicatesParsingEquivalently) {
IsolatedOptionsData isolatedData = IsolatedOptionsData.from(classes,
allowDuplicatesParsingEquivalently);

// All that's left is to compute expansions.
ImmutableMap.Builder<OptionDefinition, ImmutableList<String>> evaluatedExpansionsBuilder =
Expand All @@ -68,6 +90,7 @@ static OptionsData from(Collection<Class<? extends OptionsBase>> classes) {
evaluatedExpansionsBuilder.put(optionDefinition, ImmutableList.copyOf(constExpansion));
}
}
return new OptionsData(isolatedData, evaluatedExpansionsBuilder.build());
return new OptionsData(isolatedData, evaluatedExpansionsBuilder.build(),
allowDuplicatesParsingEquivalently);
}
}
Loading

0 comments on commit 065d09d

Please sign in to comment.