From 065d09d42bd71b79b760e94210eb96ea9c791d3e Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Sun, 16 Apr 2023 09:13:33 +0200 Subject: [PATCH] Support `all-supported` pseudo command in rc files 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. --- .../build/lib/runtime/BlazeOptionHandler.java | 33 +++- .../build/lib/runtime/BlazeRuntime.java | 2 +- .../build/lib/runtime/ConfigExpander.java | 10 +- .../com/google/devtools/common/options/BUILD | 1 + .../common/options/IsolatedOptionsData.java | 74 +++++---- .../devtools/common/options/OptionsData.java | 43 +++-- .../common/options/OptionsParser.java | 55 +++++-- .../common/options/OptionsParserImpl.java | 149 +++++++++++++----- .../common/options/OptionsDataTest.java | 10 +- .../common/options/OptionsParserTest.java | 9 +- 10 files changed, 270 insertions(+), 116 deletions(-) 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 35ef818191765e..00b91b41b89ed9 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 @@ -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; @@ -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; @@ -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. @@ -78,6 +84,7 @@ public final class BlazeOptionHandler { private final Command commandAnnotation; private final InvocationPolicy invocationPolicy; private final List rcfileNotes = new ArrayList<>(); + private final List> allOptionsClasses; BlazeOptionHandler( BlazeRuntime runtime, @@ -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()); } /** @@ -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()); + } } } } @@ -227,7 +246,8 @@ private void parseArgsAndConfigs(List 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); @@ -241,7 +261,7 @@ private void parseArgsAndConfigs(List 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 @@ -374,12 +394,14 @@ void expandConfigOptions( commandToRcArgs, getCommandNamesToParse(commandAnnotation), rcfileNotes::add, - optionsParser); + optionsParser, + OptionsParser.getFallbackOptionsData(allOptionsClasses)); } private static List getCommandNamesToParse(Command commandAnnotation) { List result = new ArrayList<>(); result.add("common"); + result.add(ALL_SUPPORTED_PSEUDO_COMMAND); getCommandNamesToParseHelper(commandAnnotation, result); return result; } @@ -470,7 +492,8 @@ static ListMultimap 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 '" diff --git a/src/main/java/com/google/devtools/build/lib/runtime/BlazeRuntime.java b/src/main/java/com/google/devtools/build/lib/runtime/BlazeRuntime.java index d79a509bf990b7..076b5e10d66790 100644 --- a/src/main/java/com/google/devtools/build/lib/runtime/BlazeRuntime.java +++ b/src/main/java/com/google/devtools/build/lib/runtime/BlazeRuntime.java @@ -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; } diff --git a/src/main/java/com/google/devtools/build/lib/runtime/ConfigExpander.java b/src/main/java/com/google/devtools/build/lib/runtime/ConfigExpander.java index 998908a5102698..508319bad9f6d4 100644 --- a/src/main/java/com/google/devtools/build/lib/runtime/ConfigExpander.java +++ b/src/main/java/com/google/devtools/build/lib/runtime/ConfigExpander.java @@ -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; @@ -88,7 +89,8 @@ static void expandConfigOptions( ListMultimap commandToRcArgs, List commandsToParse, Consumer rcFileNotesConsumer, - OptionsParser optionsParser) + OptionsParser optionsParser, + OpaqueOptionsData fallbackData) throws OptionsParsingException { OptionValueDescription configValueDescription = @@ -113,7 +115,8 @@ static void expandConfigOptions( optionsParser.parseArgsAsExpansionOfOption( configInstance, String.format("expanded from --config=%s", configValueToExpand), - expansion); + expansion, + fallbackData); } } @@ -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 diff --git a/src/main/java/com/google/devtools/common/options/BUILD b/src/main/java/com/google/devtools/common/options/BUILD index 50d66920e93e15..b5120de838dc0c 100644 --- a/src/main/java/com/google/devtools/common/options/BUILD +++ b/src/main/java/com/google/devtools/common/options/BUILD @@ -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", diff --git a/src/main/java/com/google/devtools/common/options/IsolatedOptionsData.java b/src/main/java/com/google/devtools/common/options/IsolatedOptionsData.java index 1c7c8ecd172e8a..467477a9b7c3d3 100644 --- a/src/main/java/com/google/devtools/common/options/IsolatedOptionsData.java +++ b/src/main/java/com/google/devtools/common/options/IsolatedOptionsData.java @@ -83,22 +83,22 @@ public static ImmutableList 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, 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 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 oldNameToField; @@ -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> getOptionsClasses() { return optionsClasses.keySet(); @@ -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> getAllOptionDefinitions() { return nameToField.entrySet(); @@ -177,9 +177,15 @@ public boolean getUsesOnlyCoreTypes(Class optionsClass) { * both single-character abbreviations and full names. */ private static void checkForCollisions( - Map aFieldMap, A optionName, String description) + Map 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); } @@ -212,22 +218,27 @@ private static void checkAndUpdateBooleanAliases( Map nameToFieldMap, Map oldNameToFieldMap, Map 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> classes) { + static IsolatedOptionsData from(Collection> classes, + boolean allowDuplicatesParsingEquivalently) { // Mind which fields have to preserve order. Map, Constructor> constructorBuilder = new LinkedHashMap<>(); Map nameToFieldBuilder = new LinkedHashMap<>(); @@ -256,15 +267,18 @@ static IsolatedOptionsData from(Collection> 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); @@ -273,23 +287,27 @@ static IsolatedOptionsData from(Collection> 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) { diff --git a/src/main/java/com/google/devtools/common/options/OptionsData.java b/src/main/java/com/google/devtools/common/options/OptionsData.java index 2a3e485b7bf5dd..2cbf251b0a20c0 100644 --- a/src/main/java/com/google/devtools/common/options/OptionsData.java +++ b/src/main/java/com/google/devtools/common/options/OptionsData.java @@ -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> 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> evaluatedExpansions) { + IsolatedOptionsData base, Map> evaluatedExpansions, + boolean allowDuplicatesParsingEquivalently) { super(base); this.evaluatedExpansions = ImmutableMap.copyOf(evaluatedExpansions); + this.allowDuplicatesParsingEquivalently = allowDuplicatesParsingEquivalently; } private static final ImmutableList EMPTY_EXPANSION = ImmutableList.of(); @@ -50,13 +62,23 @@ public ImmutableList 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> classes) { - IsolatedOptionsData isolatedData = IsolatedOptionsData.from(classes); + static OptionsData from(Collection> classes, + boolean allowDuplicatesParsingEquivalently) { + IsolatedOptionsData isolatedData = IsolatedOptionsData.from(classes, + allowDuplicatesParsingEquivalently); // All that's left is to compute expansions. ImmutableMap.Builder> evaluatedExpansionsBuilder = @@ -68,6 +90,7 @@ static OptionsData from(Collection> classes) { evaluatedExpansionsBuilder.put(optionDefinition, ImmutableList.copyOf(constExpansion)); } } - return new OptionsData(isolatedData, evaluatedExpansionsBuilder.build()); + return new OptionsData(isolatedData, evaluatedExpansionsBuilder.build(), + allowDuplicatesParsingEquivalently); } } diff --git a/src/main/java/com/google/devtools/common/options/OptionsParser.java b/src/main/java/com/google/devtools/common/options/OptionsParser.java index fdea7cc5d4bd08..a3db0b2abe380e 100644 --- a/src/main/java/com/google/devtools/common/options/OptionsParser.java +++ b/src/main/java/com/google/devtools/common/options/OptionsParser.java @@ -30,6 +30,7 @@ import com.google.common.collect.ListMultimap; import com.google.common.collect.MoreCollectors; import com.google.common.escape.Escaper; +import com.google.devtools.build.lib.util.Pair; import com.google.devtools.common.options.OptionsParserImpl.OptionsParserImplResult; import com.google.errorprone.annotations.CanIgnoreReturnValue; import java.util.ArrayList; @@ -103,8 +104,8 @@ public ConstructionException(String message, Throwable cause) { * cache is very unlikely to grow to a significant amount of memory, because there's only a fixed * set of options classes on the classpath. */ - private static final Map>, OptionsData> optionsData = - new HashMap<>(); + private static final Map>, Boolean>, OptionsData> + optionsData = new HashMap<>(); /** Skipped prefixes for starlark options. */ public static final ImmutableList STARLARK_SKIPPED_PREFIXES = @@ -120,30 +121,40 @@ public ConstructionException(String message, Throwable cause) { */ public static OpaqueOptionsData getOptionsData( List> optionsClasses) { - return getOptionsDataInternal(optionsClasses); + return getOptionsDataInternal(optionsClasses, false); } - /** Returns the {@link OptionsData} associated with the given list of options classes. */ - static synchronized OptionsData getOptionsDataInternal( + public static OpaqueOptionsData getFallbackOptionsData( List> optionsClasses) { + return getOptionsDataInternal(optionsClasses, true); + } + + /** + * Returns the {@link OptionsData} associated with the given list of options classes. + */ + static synchronized OptionsData getOptionsDataInternal( + List> optionsClasses, + boolean allowDuplicatesParsingEquivalently) { ImmutableList> immutableOptionsClasses = ImmutableList.copyOf(optionsClasses); - OptionsData result = optionsData.get(immutableOptionsClasses); + Pair>, Boolean> cacheKey = Pair.of( + immutableOptionsClasses, allowDuplicatesParsingEquivalently); + OptionsData result = optionsData.get(cacheKey); if (result == null) { try { - result = OptionsData.from(immutableOptionsClasses); + result = OptionsData.from(immutableOptionsClasses, allowDuplicatesParsingEquivalently); } catch (Exception e) { Throwables.throwIfInstanceOf(e, ConstructionException.class); throw new ConstructionException(e.getMessage(), e); } - optionsData.put(immutableOptionsClasses, result); + optionsData.put(cacheKey, result); } return result; } /** Returns the {@link OptionsData} associated with the given options class. */ static OptionsData getOptionsDataInternal(Class optionsClass) { - return getOptionsDataInternal(ImmutableList.of(optionsClass)); + return getOptionsDataInternal(ImmutableList.of(optionsClass), false); } /** A helper class to create new instances of {@link OptionsParser}. */ @@ -158,6 +169,7 @@ private Builder(OptionsParserImpl.Builder implBuilder) { /** Directly sets the {@link OptionsData} used by this parser. */ @CanIgnoreReturnValue public Builder optionsData(OptionsData optionsData) { + Preconditions.checkArgument(!optionsData.createdWithAllowDuplicatesParsingEquivalently()); this.implBuilder.optionsData(optionsData); return this; } @@ -173,7 +185,7 @@ public Builder optionsData(OpaqueOptionsData optionsData) { @SafeVarargs public final Builder optionsClasses(Class... optionsClasses) { return this.optionsData( - (OpaqueOptionsData) getOptionsDataInternal(ImmutableList.copyOf(optionsClasses))); + (OpaqueOptionsData) getOptionsDataInternal(ImmutableList.copyOf(optionsClasses), false)); } /** @@ -181,7 +193,7 @@ public final Builder optionsClasses(Class... optionsClass */ public Builder optionsClasses(Iterable> optionsClasses) { return this.optionsData( - (OpaqueOptionsData) getOptionsDataInternal(ImmutableList.copyOf(optionsClasses))); + (OpaqueOptionsData) getOptionsDataInternal(ImmutableList.copyOf(optionsClasses), false)); } /** @@ -699,7 +711,7 @@ public void parse(List args) throws OptionsParsingException { */ public void parse(OptionPriority.PriorityCategory priority, String source, List args) throws OptionsParsingException { - parseWithSourceFunction(priority, o -> source, args); + parseWithSourceFunction(priority, o -> source, args, null); } /** @@ -715,17 +727,22 @@ public void parse(OptionPriority.PriorityCategory priority, String source, List< * each option will be given an index to track its position. If parse() has already been * called at this priority, the indexing will continue where it left off, to keep ordering. * @param sourceFunction a function that maps option names to the source of the option. + * @param fallbackData if provided, the full collection of options that should be parsed and + * ignored without raising an error if they are not recognized by the options classes + * registered with this parser. * @param args the arg list to parse. Each element might be an option, a value linked to an * option, or residue. */ public void parseWithSourceFunction( OptionPriority.PriorityCategory priority, Function sourceFunction, - List args) + List args, + @Nullable OpaqueOptionsData fallbackData) throws OptionsParsingException { Preconditions.checkNotNull(priority); Preconditions.checkArgument(priority != OptionPriority.PriorityCategory.DEFAULT); - OptionsParserImplResult optionsParserImplResult = impl.parse(priority, sourceFunction, args); + OptionsParserImplResult optionsParserImplResult = impl.parse(priority, sourceFunction, args, + (OptionsData) fallbackData); addResidueFromResult(optionsParserImplResult); aliases.putAll(optionsParserImplResult.aliases); } @@ -739,9 +756,13 @@ public void parseWithSourceFunction( * @param source a description of where the expansion arguments came from. * @param args the arguments to parse as the expansion. Order matters, as the value of a flag may * be in the following argument. + * @param fallbackData if provided, the full collection of options that should be parsed and + * ignored without raising an error if they are not recognized by the options classes + * registered with this parser. */ public void parseArgsAsExpansionOfOption( - ParsedOptionDescription optionToExpand, String source, List args) + ParsedOptionDescription optionToExpand, String source, List args, + @Nullable OpaqueOptionsData fallbackData) throws OptionsParsingException { Preconditions.checkNotNull( optionToExpand, "Option for expansion not specified for arglist %s", args); @@ -750,8 +771,8 @@ public void parseArgsAsExpansionOfOption( != OptionPriority.PriorityCategory.DEFAULT, "Priority cannot be default, which was specified for arglist %s", args); - OptionsParserImplResult optionsParserImplResult = - impl.parseArgsAsExpansionOfOption(optionToExpand, o -> source, args); + OptionsParserImplResult optionsParserImplResult = impl.parseArgsAsExpansionOfOption( + optionToExpand, o -> source, args, (OptionsData) fallbackData); addResidueFromResult(optionsParserImplResult); } diff --git a/src/main/java/com/google/devtools/common/options/OptionsParserImpl.java b/src/main/java/com/google/devtools/common/options/OptionsParserImpl.java index d6a7bee4693db7..cae3259c6729e3 100644 --- a/src/main/java/com/google/devtools/common/options/OptionsParserImpl.java +++ b/src/main/java/com/google/devtools/common/options/OptionsParserImpl.java @@ -39,7 +39,9 @@ import java.util.List; import java.util.Map; import java.util.Objects; +import java.util.Optional; import java.util.Set; +import java.util.function.BiFunction; import java.util.function.Function; import java.util.stream.Collectors; import javax.annotation.Nullable; @@ -406,15 +408,15 @@ ImmutableList getExpansionValueDescriptions( Iterator optionsIterator = options.iterator(); while (optionsIterator.hasNext()) { String unparsedFlagExpression = optionsIterator.next(); - ParsedOptionDescription parsedOption = - identifyOptionAndPossibleArgument( - unparsedFlagExpression, - optionsIterator, - nextOptionPriority, - o -> source, - implicitDependent, - expandedFrom); - builder.add(parsedOption); + identifyOptionAndPossibleArgument( + unparsedFlagExpression, + optionsIterator, + nextOptionPriority, + o -> source, + implicitDependent, + expandedFrom, + /* fallbackData */ null) + .ifPresent(builder::add); nextOptionPriority = OptionPriority.nextOptionPriority(nextOptionPriority); } return builder.build(); @@ -447,10 +449,12 @@ List getSkippedArgs() { OptionsParserImplResult parse( PriorityCategory priorityCat, Function sourceFunction, - List args) + List args, + OptionsData fallbackData) throws OptionsParsingException { - OptionsParserImplResult optionsParserImplResult = - parse(nextPriorityPerPriorityCategory.get(priorityCat), sourceFunction, null, null, args); + OptionsParserImplResult optionsParserImplResult = parse( + nextPriorityPerPriorityCategory.get(priorityCat), sourceFunction, null, null, args, + fallbackData); nextPriorityPerPriorityCategory.put(priorityCat, optionsParserImplResult.nextPriority); return optionsParserImplResult; } @@ -468,7 +472,8 @@ private OptionsParserImplResult parse( Function sourceFunction, ParsedOptionDescription implicitDependent, ParsedOptionDescription expandedFrom, - List args) + List args, + OptionsData fallbackData) throws OptionsParsingException { List unparsedArgs = new ArrayList<>(); List unparsedPostDoubleDashArgs = new ArrayList<>(); @@ -501,14 +506,14 @@ private OptionsParserImplResult parse( break; } - ParsedOptionDescription parsedOption; + Optional parsedOption; if (containsSkippedPrefix(arg)) { // Parse the skipped arg into a synthetic allowMultiple option to preserve its order // relative to skipped args coming from expansions. Simply adding it to the residue would // end up placing expanded skipped args after all explicitly given skipped args, which isn't // correct. parsedOption = - ParsedOptionDescription.newParsedOptionDescription( + Optional.of(ParsedOptionDescription.newParsedOptionDescription( skippedArgsDefinition, arg, arg, @@ -517,13 +522,14 @@ private OptionsParserImplResult parse( sourceFunction.apply(skippedArgsDefinition), implicitDependent, expandedFrom), - conversionContext); + conversionContext)); } else { - parsedOption = - identifyOptionAndPossibleArgument( - arg, argsIterator, priority, sourceFunction, implicitDependent, expandedFrom); + parsedOption = identifyOptionAndPossibleArgument(arg, argsIterator, priority, + sourceFunction, implicitDependent, expandedFrom, fallbackData); + } + if (parsedOption.isPresent()) { + handleNewParsedOption(parsedOption.get()); } - handleNewParsedOption(parsedOption); priority = OptionPriority.nextOptionPriority(priority); } @@ -569,14 +575,16 @@ public List getResidue() { OptionsParserImplResult parseArgsAsExpansionOfOption( ParsedOptionDescription optionToExpand, Function sourceFunction, - List args) + List args, + OptionsData fallbackData) throws OptionsParsingException { return parse( OptionPriority.getChildPriority(optionToExpand.getPriority()), sourceFunction, null, optionToExpand, - args); + args, + fallbackData); } /** @@ -630,7 +638,8 @@ private void handleNewParsedOption(ParsedOptionDescription parsedOption) o -> expansionBundle.sourceOfExpansionArgs, optionDefinition.hasImplicitRequirements() ? parsedOption : null, optionDefinition.isExpansionOption() ? parsedOption : null, - expansionBundle.expansionArgs); + expansionBundle.expansionArgs, + null); if (!optionsParserImplResult.getResidue().isEmpty()) { // Throw an assertion here, because this indicates an error in the definition of this @@ -693,28 +702,34 @@ private ExpansionBundle setOptionValue(ParsedOptionDescription parsedOption) return expansionBundle; } - private ParsedOptionDescription identifyOptionAndPossibleArgument( + /** + * Keep the properties of OptionsData used below in sync with {@link #equivalentForParsing}. + */ + private Optional identifyOptionAndPossibleArgument( String arg, Iterator nextArgs, OptionPriority priority, Function sourceFunction, ParsedOptionDescription implicitDependent, - ParsedOptionDescription expandedFrom) + ParsedOptionDescription expandedFrom, + @Nullable OptionsData fallbackData) throws OptionsParsingException { // Store the way this option was parsed on the command line. StringBuilder commandLineForm = new StringBuilder(); commandLineForm.append(arg); String unconvertedValue = null; - OptionDefinition optionDefinition; + OptionLookupResult lookupResult; boolean booleanValue = true; if (arg.length() == 2) { // -l (may be nullary or unary) - optionDefinition = optionsData.getFieldForAbbrev(arg.charAt(1)); + lookupResult = getWithFallback(OptionsData::getFieldForAbbrev, arg.charAt(1), + fallbackData); booleanValue = true; } else if (arg.length() == 3 && arg.charAt(2) == '-') { // -l- (boolean) - optionDefinition = optionsData.getFieldForAbbrev(arg.charAt(1)); + lookupResult = getWithFallback(OptionsData::getFieldForAbbrev, arg.charAt(1), + fallbackData); booleanValue = false; } else if (arg.startsWith("--")) { // --long_option @@ -727,16 +742,17 @@ private ParsedOptionDescription identifyOptionAndPossibleArgument( throw new OptionsParsingException("Invalid options syntax: " + arg, arg); } unconvertedValue = equalsAt == -1 ? null : arg.substring(equalsAt + 1); - optionDefinition = optionsData.getOptionDefinitionFromName(name); + lookupResult = getWithFallback(OptionsData::getOptionDefinitionFromName, name, fallbackData); // Look for a "no"-prefixed option name: "no". - if (optionDefinition == null && name.startsWith("no")) { + if (lookupResult == null && name.startsWith("no")) { name = name.substring(2); - optionDefinition = optionsData.getOptionDefinitionFromName(name); + lookupResult = getWithFallback(OptionsData::getOptionDefinitionFromName, name, + fallbackData); booleanValue = false; - if (optionDefinition != null) { + if (lookupResult != null) { // TODO(bazel-team): Add tests for these cases. - if (!optionDefinition.usesBooleanValueSyntax()) { + if (!lookupResult.definition.usesBooleanValueSyntax()) { throw new OptionsParsingException( "Illegal use of 'no' prefix on non-boolean option: " + arg, arg); } @@ -751,16 +767,16 @@ private ParsedOptionDescription identifyOptionAndPossibleArgument( throw new OptionsParsingException("Invalid options syntax: " + arg, arg); } - if (optionDefinition == null || shouldIgnoreOption(optionDefinition)) { + if (lookupResult == null || shouldIgnoreOption(lookupResult.definition)) { // Do not recognize internal options, which are treated as if they did not exist. throw new OptionsParsingException("Unrecognized option: " + arg, arg); } if (unconvertedValue == null) { // Special-case boolean to supply value based on presence of "no" prefix. - if (optionDefinition.usesBooleanValueSyntax()) { + if (lookupResult.definition.usesBooleanValueSyntax()) { unconvertedValue = booleanValue ? "1" : "0"; - } else if (optionDefinition.getType().equals(Void.class)) { + } else if (lookupResult.definition.getType().equals(Void.class)) { // This is expected, Void type options have no args. } else if (nextArgs.hasNext()) { // "--flag value" form @@ -771,22 +787,69 @@ private ParsedOptionDescription identifyOptionAndPossibleArgument( } } - return ParsedOptionDescription.newParsedOptionDescription( - optionDefinition, + if (lookupResult.fromFallback) { + // The option was not found on the current command, but is a valid option for some other + // command. Ignore it. + return Optional.empty(); + } + + return Optional.of(ParsedOptionDescription.newParsedOptionDescription( + lookupResult.definition, commandLineForm.toString(), unconvertedValue, - new OptionInstanceOrigin( - priority, sourceFunction.apply(optionDefinition), implicitDependent, expandedFrom), - conversionContext); + new OptionInstanceOrigin(priority, sourceFunction.apply(lookupResult.definition), + implicitDependent, expandedFrom), + conversionContext)); + } + + /** + * Keep in sync with the properties of OptionsData used in + * {@link #identifyOptionAndPossibleArgument} + */ + public static boolean equivalentForParsing(OptionDefinition definition, + OptionDefinition otherDefinition) { + if (definition.equals(otherDefinition)) { + return true; + } + return (definition.usesBooleanValueSyntax() == otherDefinition.usesBooleanValueSyntax()) + && (definition.getType().equals(Void.class) == otherDefinition.getType().equals(Void.class)) + && (ImmutableList.copyOf(definition.getOptionMetadataTags()) + .contains(OptionMetadataTag.INTERNAL) == ImmutableList.copyOf( + otherDefinition.getOptionMetadataTags()).contains(OptionMetadataTag.INTERNAL)); + } + + private static final class OptionLookupResult { + + final OptionDefinition definition; + final boolean fromFallback; + + private OptionLookupResult(OptionDefinition definition, boolean fromFallback) { + this.definition = definition; + this.fromFallback = fromFallback; + } + } + + private OptionLookupResult getWithFallback( + BiFunction getter, T param, OptionsData fallbackData) { + OptionDefinition optionDefinition; + if ((optionDefinition = getter.apply(optionsData, param)) != null) { + return new OptionLookupResult(optionDefinition, false); + } + if (fallbackData != null && (optionDefinition = getter.apply(fallbackData, param)) != null) { + return new OptionLookupResult(optionDefinition, true); + } + return null; } private boolean shouldIgnoreOption(OptionDefinition optionDefinition) { return ignoreInternalOptions && ImmutableList.copyOf(optionDefinition.getOptionMetadataTags()) - .contains(OptionMetadataTag.INTERNAL); + .contains(OptionMetadataTag.INTERNAL); } - /** Gets the result of parsing the options. */ + /** + * Gets the result of parsing the options. + */ O getParsedOptions(Class optionsClass) { // Create the instance: O optionsInstance; diff --git a/src/test/java/com/google/devtools/common/options/OptionsDataTest.java b/src/test/java/com/google/devtools/common/options/OptionsDataTest.java index ed922c0e6e87c1..776263e7a61019 100644 --- a/src/test/java/com/google/devtools/common/options/OptionsDataTest.java +++ b/src/test/java/com/google/devtools/common/options/OptionsDataTest.java @@ -33,15 +33,14 @@ public class OptionsDataTest { private static IsolatedOptionsData construct(Class optionsClass) throws OptionsParser.ConstructionException { - return IsolatedOptionsData.from(ImmutableList.>of(optionsClass)); + return IsolatedOptionsData.from(ImmutableList.of(optionsClass), false); } private static IsolatedOptionsData construct( Class optionsClass1, Class optionsClass2) throws OptionsParser.ConstructionException { - return IsolatedOptionsData.from( - ImmutableList.>of(optionsClass1, optionsClass2)); + return IsolatedOptionsData.from(ImmutableList.of(optionsClass1, optionsClass2), false); } private static IsolatedOptionsData construct( @@ -49,9 +48,8 @@ private static IsolatedOptionsData construct( Class optionsClass2, Class optionsClass3) throws OptionsParser.ConstructionException { - return IsolatedOptionsData.from( - ImmutableList.>of( - optionsClass1, optionsClass2, optionsClass3)); + return IsolatedOptionsData.from(ImmutableList.of(optionsClass1, optionsClass2, optionsClass3), + false); } /** Dummy options class. */ diff --git a/src/test/java/com/google/devtools/common/options/OptionsParserTest.java b/src/test/java/com/google/devtools/common/options/OptionsParserTest.java index b6b63d9a6ece15..5927b1faf5be67 100644 --- a/src/test/java/com/google/devtools/common/options/OptionsParserTest.java +++ b/src/test/java/com/google/devtools/common/options/OptionsParserTest.java @@ -275,7 +275,8 @@ public void parseWithSourceFunctionThrowsExceptionIfResidueIsNotAllowed() { parser.parseWithSourceFunction( PriorityCategory.COMMAND_LINE, sourceFunction, - ImmutableList.of("residue", "not", "allowed", "in", "parseWithSource"))); + ImmutableList.of("residue", "not", "allowed", "in", "parseWithSource"), + null)); assertThat(e) .hasMessageThat() .isEqualTo("Unrecognized arguments: residue not allowed in parseWithSource"); @@ -295,7 +296,8 @@ public void parseWithSourceFunctionDoesntThrowExceptionIfResidueIsAllowed() thro parser.parseWithSourceFunction( PriorityCategory.COMMAND_LINE, sourceFunction, - ImmutableList.of("residue", "is", "allowed", "in", "parseWithSource")); + ImmutableList.of("residue", "is", "allowed", "in", "parseWithSource"), + null); assertThat(parser.getResidue()) .containsExactly("residue", "is", "allowed", "in", "parseWithSource"); } @@ -320,7 +322,8 @@ public void parseArgsAsExpansionOfOptionThrowsExceptionIfResidueIsNotAllowed() t parser.parseArgsAsExpansionOfOption( optionToExpand, "source", - ImmutableList.of("--underlying=direct_value", "residue", "in", "expansion"))); + ImmutableList.of("--underlying=direct_value", "residue", "in", "expansion"), + null)); assertThat(parser.getResidue()).isNotEmpty(); assertThat(e).hasMessageThat().isEqualTo("Unrecognized arguments: residue in expansion"); }