From 4373ecd59b8e2c95f46a340a1bb8aa7c2a6d84d4 Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Wed, 24 May 2023 12:27:35 -0700 Subject: [PATCH] Let `common` ignore unsupported options Fixes #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 #18130. PiperOrigin-RevId: 534940403 Change-Id: I2ae268ae84de3f2b07ff3d1e36bf382bce714968 --- site/en/run/bazelrc.md | 9 +- .../build/lib/runtime/BlazeOptionHandler.java | 72 ++++- .../build/lib/runtime/BlazeRuntime.java | 3 +- .../build/lib/runtime/ConfigExpander.java | 24 +- .../com/google/devtools/common/options/BUILD | 1 + .../common/options/IsolatedOptionsData.java | 89 ++++-- .../devtools/common/options/OptionsData.java | 31 +- .../common/options/OptionsParser.java | 64 ++-- .../common/options/OptionsParserImpl.java | 216 +++++++++++--- .../common/options/OptionsDataTest.java | 10 +- .../common/options/OptionsParserTest.java | 104 ++++++- src/test/py/bazel/BUILD | 4 +- src/test/py/bazel/options_test.py | 274 ++++++++++++++++++ src/test/py/bazel/starlark_options_test.py | 82 ------ 14 files changed, 787 insertions(+), 196 deletions(-) create mode 100644 src/test/py/bazel/options_test.py delete mode 100644 src/test/py/bazel/starlark_options_test.py diff --git a/site/en/run/bazelrc.md b/site/en/run/bazelrc.md index 7943da726ec114..821c11a9a84b39 100644 --- a/site/en/run/bazelrc.md +++ b/site/en/run/bazelrc.md @@ -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`.) 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..402fa1301fad71 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; @@ -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; @@ -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. @@ -78,6 +89,7 @@ public final class BlazeOptionHandler { private final Command commandAnnotation; private final InvocationPolicy invocationPolicy; private final List rcfileNotes = new ArrayList<>(); + private final ImmutableList> allOptionsClasses; BlazeOptionHandler( BlazeRuntime runtime, @@ -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()); } /** @@ -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 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()); + } } } } @@ -227,7 +278,8 @@ private void parseArgsAndConfigs(List 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); @@ -241,7 +293,10 @@ 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(), + /* fallbackData= */ null); if (commandAnnotation.builds()) { // splits project files from targets in the traditional sense @@ -372,14 +427,17 @@ void expandConfigOptions( ConfigExpander.expandConfigOptions( eventHandler, commandToRcArgs, + commandAnnotation.name(), getCommandNamesToParse(commandAnnotation), rcfileNotes::add, - optionsParser); + optionsParser, + OptionsParser.getFallbackOptionsData(allOptionsClasses)); } private static List getCommandNamesToParse(Command commandAnnotation) { List result = new ArrayList<>(); - result.add("common"); + result.add(ALWAYS_PSEUDO_COMMAND); + result.add(COMMON_PSEUDO_COMMAND); getCommandNamesToParseHelper(commandAnnotation, result); return result; } @@ -470,7 +528,9 @@ static ListMultimap 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 '" 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 45c42db49c72a3..3374ba4e1b6e65 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 @@ -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; } 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 14b20781685491..92fe5f1356c195 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 @@ -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; @@ -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 { @@ -86,9 +89,11 @@ private static boolean shouldEnablePlatformSpecificConfig( static void expandConfigOptions( EventHandler eventHandler, ListMultimap commandToRcArgs, + String currentCommand, List commandsToParse, Consumer rcFileNotesConsumer, - OptionsParser optionsParser) + OptionsParser optionsParser, + @Nullable OpaqueOptionsData fallbackData) throws OptionsParsingException { OptionValueDescription configValueDescription = @@ -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))); + } } } @@ -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 diff --git a/src/main/java/com/google/devtools/common/options/BUILD b/src/main/java/com/google/devtools/common/options/BUILD index c2f08b3ad8c7d7..b45d78d04ba434 100644 --- a/src/main/java/com/google/devtools/common/options/BUILD +++ b/src/main/java/com/google/devtools/common/options/BUILD @@ -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", 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..0ad1467f65ec45 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. + * 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(); @@ -158,7 +158,7 @@ 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. + * #from(Collection, boolean)}, and then in alphabetic order within each options class. */ public Iterable> getAllOptionDefinitions() { return nameToField.entrySet(); @@ -177,9 +177,18 @@ 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,11 +221,23 @@ 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); @@ -226,8 +247,13 @@ private static void checkAndUpdateBooleanAliases( * 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. + * + *

If {@code allowDuplicatesParsingEquivalently} is true, then options that collide in name but + * parse equivalently (e.g. both of them accept a value or both of them do not), are allowed. */ - 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 +282,27 @@ 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); + nameToFieldBuilder, + oldNameToFieldBuilder, + booleanAliasMap, + optionName, + optionDefinition, + allowDuplicatesParsingEquivalently); } nameToFieldBuilder.put(optionName, optionDefinition); @@ -273,23 +311,36 @@ 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); + 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"); + 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..3268cbdcc4dd2b 100644 --- a/src/main/java/com/google/devtools/common/options/OptionsData.java +++ b/src/main/java/com/google/devtools/common/options/OptionsData.java @@ -31,11 +31,20 @@ final class OptionsData extends IsolatedOptionsData { /** Mapping from each option to the (unparsed) options it expands to, if any. */ private final ImmutableMap> evaluatedExpansions; + /** + * 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(); @@ -49,14 +58,25 @@ public ImmutableList getEvaluatedExpansion(OptionDefinition optionDefini return result != null ? result : EMPTY_EXPANSION; } + /** + * 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 +88,9 @@ static OptionsData from(Collection> classes) { evaluatedExpansionsBuilder.put(optionDefinition, ImmutableList.copyOf(constExpansion)); } } - return new OptionsData(isolatedData, evaluatedExpansionsBuilder.build()); + return new OptionsData( + isolatedData, + evaluatedExpansionsBuilder.buildOrThrow(), + 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 6462f1a13fec9c..0c0d2ab8dc858c 100644 --- a/src/main/java/com/google/devtools/common/options/OptionsParser.java +++ b/src/main/java/com/google/devtools/common/options/OptionsParser.java @@ -27,6 +27,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; @@ -100,8 +101,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 = @@ -117,30 +118,38 @@ public ConstructionException(String message, Throwable cause) { */ public static OpaqueOptionsData getOptionsData( List> optionsClasses) { - return getOptionsDataInternal(optionsClasses); + return getOptionsDataInternal(optionsClasses, false); + } + + 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) { + 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}. */ @@ -155,6 +164,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; } @@ -170,7 +180,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)); } /** @@ -178,7 +188,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)); } /** @@ -669,7 +679,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); } /** @@ -685,19 +695,28 @@ 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. + * @return a list of options and values that were parsed but ignored due to only resolving against + * the fallback data */ - public void parseWithSourceFunction( + @CanIgnoreReturnValue + public ImmutableList 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); + return optionsParserImplResult.ignoredArgs; } /** @@ -709,9 +728,18 @@ 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. + * @return a list of options and values that were parsed but ignored due to only resolving against + * the fallback data */ - public void parseArgsAsExpansionOfOption( - ParsedOptionDescription optionToExpand, String source, List args) + @CanIgnoreReturnValue + public ImmutableList parseArgsAsExpansionOfOption( + ParsedOptionDescription optionToExpand, + String source, + List args, + @Nullable OpaqueOptionsData fallbackData) throws OptionsParsingException { Preconditions.checkNotNull( optionToExpand, "Option for expansion not specified for arglist %s", args); @@ -721,8 +749,10 @@ public void parseArgsAsExpansionOfOption( "Priority cannot be default, which was specified for arglist %s", args); OptionsParserImplResult optionsParserImplResult = - impl.parseArgsAsExpansionOfOption(optionToExpand, o -> source, args); + impl.parseArgsAsExpansionOfOption( + optionToExpand, o -> source, args, (OptionsData) fallbackData); addResidueFromResult(optionsParserImplResult); + return optionsParserImplResult.ignoredArgs; } private void addResidueFromResult(OptionsParserImplResult result) throws OptionsParsingException { 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 4bebaaf90eb35e..2c7abc2c3c169c 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; @@ -400,15 +402,16 @@ ImmutableList getExpansionValueDescriptions( Iterator optionsIterator = options.iterator(); while (optionsIterator.hasNext()) { String unparsedFlagExpression = optionsIterator.next(); - ParsedOptionDescription parsedOption = - identifyOptionAndPossibleArgument( + identifyOptionAndPossibleArgument( unparsedFlagExpression, optionsIterator, nextOptionPriority, o -> source, implicitDependent, - expandedFrom); - builder.add(parsedOption); + expandedFrom, + /* fallbackData= */ null) + .parsedOptionDescription + .ifPresent(builder::add); nextOptionPriority = OptionPriority.nextOptionPriority(nextOptionPriority); } return builder.build(); @@ -441,10 +444,17 @@ 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); + parse( + nextPriorityPerPriorityCategory.get(priorityCat), + sourceFunction, + null, + null, + args, + fallbackData); nextPriorityPerPriorityCategory.put(priorityCat, optionsParserImplResult.nextPriority); return optionsParserImplResult; } @@ -462,10 +472,12 @@ 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<>(); + List ignoredArgs = new ArrayList<>(); Iterator argsIterator = argsPreProcessor.preProcess(args).iterator(); while (argsIterator.hasNext()) { @@ -495,29 +507,40 @@ 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( - skippedArgsDefinition, - arg, - arg, - new OptionInstanceOrigin( - priority, - sourceFunction.apply(skippedArgsDefinition), - implicitDependent, - expandedFrom), - conversionContext); + Optional.of( + ParsedOptionDescription.newParsedOptionDescription( + skippedArgsDefinition, + arg, + arg, + new OptionInstanceOrigin( + priority, + sourceFunction.apply(skippedArgsDefinition), + implicitDependent, + expandedFrom), + conversionContext)); } else { - parsedOption = + ParsedOptionDescriptionOrIgnoredArgs result = identifyOptionAndPossibleArgument( - arg, argsIterator, priority, sourceFunction, implicitDependent, expandedFrom); + arg, + argsIterator, + priority, + sourceFunction, + implicitDependent, + expandedFrom, + fallbackData); + result.ignoredArgs.ifPresent(ignoredArgs::add); + parsedOption = result.parsedOptionDescription; + } + if (parsedOption.isPresent()) { + handleNewParsedOption(parsedOption.get()); } - handleNewParsedOption(parsedOption); priority = OptionPriority.nextOptionPriority(priority); } @@ -529,23 +552,26 @@ private OptionsParserImplResult parse( } return new OptionsParserImplResult( - unparsedArgs, unparsedPostDoubleDashArgs, priority, flagAliasMappings); + unparsedArgs, unparsedPostDoubleDashArgs, ignoredArgs, priority, flagAliasMappings); } /** A class that stores residue and priority information. */ static final class OptionsParserImplResult { final List postDoubleDashResidue; final List preDoubleDashResidue; + final ImmutableList ignoredArgs; final OptionPriority nextPriority; final ImmutableMap aliases; OptionsParserImplResult( List preDashResidue, List postDashResidue, + List ignoredArgs, OptionPriority nextPriority, Map aliases) { this.preDoubleDashResidue = preDashResidue; this.postDoubleDashResidue = postDashResidue; + this.ignoredArgs = ImmutableList.copyOf(ignoredArgs); this.nextPriority = nextPriority; this.aliases = ImmutableMap.copyOf(aliases); } @@ -563,14 +589,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); } /** @@ -624,7 +652,8 @@ private void handleNewParsedOption(ParsedOptionDescription parsedOption) o -> expansionBundle.sourceOfExpansionArgs, optionDefinition.hasImplicitRequirements() ? parsedOption : null, optionDefinition.isExpansionOption() ? parsedOption : null, - expansionBundle.expansionArgs); + expansionBundle.expansionArgs, + /* fallbackData= */ null); if (!optionsParserImplResult.getResidue().isEmpty()) { // Throw an assertion here, because this indicates an error in the definition of this @@ -684,28 +713,37 @@ private ExpansionBundle setOptionValue(ParsedOptionDescription parsedOption) return expansionBundle; } - private ParsedOptionDescription identifyOptionAndPossibleArgument( + /** + * Keep the properties of {@link OptionsData} used below in sync with {@link + * #equivalentForParsing}. + * + *

If an option is not found in the current {@link OptionsData}, but is found in the specified + * fallback data, a {@link ParsedOptionDescriptionOrIgnoredArgs} with no {@link + * ParsedOptionDescription}, but the ignored arguments is returned. + */ + private ParsedOptionDescriptionOrIgnoredArgs 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 @@ -718,16 +756,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); } @@ -742,16 +781,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 @@ -762,13 +801,100 @@ private ParsedOptionDescription identifyOptionAndPossibleArgument( } } - return ParsedOptionDescription.newParsedOptionDescription( - optionDefinition, - commandLineForm.toString(), - unconvertedValue, - new OptionInstanceOrigin( - priority, sourceFunction.apply(optionDefinition), implicitDependent, expandedFrom), - conversionContext); + if (lookupResult.fromFallback) { + // The option was not found on the current command, but is a valid option for some other + // command. Ignore it. + return new ParsedOptionDescriptionOrIgnoredArgs( + Optional.empty(), Optional.of(commandLineForm.toString())); + } + + return new ParsedOptionDescriptionOrIgnoredArgs( + Optional.of( + ParsedOptionDescription.newParsedOptionDescription( + lookupResult.definition, + commandLineForm.toString(), + unconvertedValue, + new OptionInstanceOrigin( + priority, + sourceFunction.apply(lookupResult.definition), + implicitDependent, + expandedFrom), + conversionContext)), + Optional.empty()); + } + + /** + * Two option definitions are considered equivalent for parsing if they result in the same control + * flow through {@link #identifyOptionAndPossibleArgument}. This is crucial to ensure that the + * beginning of the next option can be determined unambiguously when parsing with fallback data. + * + *

Examples: + * + *

    + *
  • Both {@code query} and {@code cquery} have a {@code --output} option, but the options + * accept different sets of values (e.g. {@code cquery} has {@code --output=files}, but + * {@code query} doesn't. However, since both options accept a string value, they parse + * equivalently as far as {@link #identifyOptionAndPossibleArgument} is concerned - + * potential failures due to unsupported values occur after parsing, during value + * conversion. There is no ambiguity in how many command-line arguments are consumed + * depending on which option definition is used. + *
  • If the hypothetical {@code foo} command also had a {@code --output} option, but it were + * boolean-valued, then the two option definitions would not be equivalent for + * parsing: The command line {@code --output --copt=foo} would parse as {@code {"output": + * "--copt=foo"}} for the {@code cquery} command, but as {@code {"output": true, "copt": + * "foo"}} for the {@code foo} command, thus resulting in parsing ambiguities between the + * two commands. + *
+ */ + 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)); + } + + // TODO: Replace with a sealed interface unwrapped via pattern matching when available. + private static final class ParsedOptionDescriptionOrIgnoredArgs { + + final Optional parsedOptionDescription; + final Optional ignoredArgs; + + private ParsedOptionDescriptionOrIgnoredArgs( + Optional parsedOptionDescription, Optional ignoredArgs) { + Preconditions.checkArgument(parsedOptionDescription.isPresent() != ignoredArgs.isPresent()); + this.parsedOptionDescription = parsedOptionDescription; + this.ignoredArgs = ignoredArgs; + } + } + + private static final class OptionLookupResult { + + final OptionDefinition definition; + final boolean fromFallback; + + private OptionLookupResult(OptionDefinition definition, boolean fromFallback) { + this.definition = definition; + this.fromFallback = fromFallback; + } + } + + @Nullable + 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) { 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..bce50e8c2712ff 100644 --- a/src/test/java/com/google/devtools/common/options/OptionsDataTest.java +++ b/src/test/java/com/google/devtools/common/options/OptionsDataTest.java @@ -33,7 +33,8 @@ public class OptionsDataTest { private static IsolatedOptionsData construct(Class optionsClass) throws OptionsParser.ConstructionException { - return IsolatedOptionsData.from(ImmutableList.>of(optionsClass)); + return IsolatedOptionsData.from( + ImmutableList.of(optionsClass), /* allowDuplicatesParsingEquivalently= */ false); } private static IsolatedOptionsData construct( @@ -41,7 +42,8 @@ private static IsolatedOptionsData construct( Class optionsClass2) throws OptionsParser.ConstructionException { return IsolatedOptionsData.from( - ImmutableList.>of(optionsClass1, optionsClass2)); + ImmutableList.of(optionsClass1, optionsClass2), + /* allowDuplicatesParsingEquivalently= */ false); } private static IsolatedOptionsData construct( @@ -50,8 +52,8 @@ private static IsolatedOptionsData construct( Class optionsClass3) throws OptionsParser.ConstructionException { return IsolatedOptionsData.from( - ImmutableList.>of( - optionsClass1, optionsClass2, optionsClass3)); + ImmutableList.of(optionsClass1, optionsClass2, optionsClass3), + /* allowDuplicatesParsingEquivalently= */ 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..35eef8d372ce3d 100644 --- a/src/test/java/com/google/devtools/common/options/OptionsParserTest.java +++ b/src/test/java/com/google/devtools/common/options/OptionsParserTest.java @@ -23,6 +23,7 @@ import com.google.common.collect.ImmutableMap; import com.google.devtools.common.options.Converters.CommaSeparatedOptionListConverter; import com.google.devtools.common.options.OptionPriority.PriorityCategory; +import com.google.devtools.common.options.OptionsParser.ConstructionException; import java.util.ArrayList; import java.util.Collections; import java.util.HashMap; @@ -209,20 +210,67 @@ public static class ExampleInternalOptions extends OptionsBase { public boolean privateBoolean; @Option( - name = "internal_string", - metadataTags = {OptionMetadataTag.INTERNAL}, - documentationCategory = OptionDocumentationCategory.UNDOCUMENTED, - effectTags = {OptionEffectTag.NO_OP}, - defaultValue = "super secret" - ) + name = "internal_string", + metadataTags = {OptionMetadataTag.INTERNAL}, + documentationCategory = OptionDocumentationCategory.UNDOCUMENTED, + effectTags = {OptionEffectTag.NO_OP}, + defaultValue = "super secret") public String privateString; } + public static class ExampleEquivalentWithFoo extends OptionsBase { + + @Option( + name = "foo", + category = "one", + documentationCategory = OptionDocumentationCategory.UNCATEGORIZED, + effectTags = {OptionEffectTag.NO_OP}, + defaultValue = "differentDefault") + public String foo; + + @Option( + name = "bar", + category = "one", + documentationCategory = OptionDocumentationCategory.UNCATEGORIZED, + effectTags = {OptionEffectTag.NO_OP}, + defaultValue = "differentDefault") + public String bar; + + @Option( + name = "ignored_with_value", + category = "one", + documentationCategory = OptionDocumentationCategory.UNCATEGORIZED, + effectTags = {OptionEffectTag.NO_OP}, + defaultValue = "differentDefault") + public String ignoredWithValue; + + @Option( + name = "ignored_without_value", + category = "one", + documentationCategory = OptionDocumentationCategory.UNCATEGORIZED, + effectTags = {OptionEffectTag.NO_OP}, + defaultValue = "false") + public boolean ignoredWithoutValue; + } + + public static class ExampleIncompatibleWithFoo extends OptionsBase { + + @Option( + name = "foo", + category = "one", + documentationCategory = OptionDocumentationCategory.UNCATEGORIZED, + effectTags = {OptionEffectTag.NO_OP}, + defaultValue = "true") + public boolean foo; + } + public static class StringConverter extends Converter.Contextless { + @Override public String convert(String input) { return input; } + @Override public String getTypeDescription() { return "a string"; @@ -275,7 +323,8 @@ public void parseWithSourceFunctionThrowsExceptionIfResidueIsNotAllowed() { parser.parseWithSourceFunction( PriorityCategory.COMMAND_LINE, sourceFunction, - ImmutableList.of("residue", "not", "allowed", "in", "parseWithSource"))); + ImmutableList.of("residue", "not", "allowed", "in", "parseWithSource"), + /* fallbackData= */ null)); assertThat(e) .hasMessageThat() .isEqualTo("Unrecognized arguments: residue not allowed in parseWithSource"); @@ -295,7 +344,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"), + /* fallbackData= */ null); assertThat(parser.getResidue()) .containsExactly("residue", "is", "allowed", "in", "parseWithSource"); } @@ -320,7 +370,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"), + /* fallbackData= */ null)); assertThat(parser.getResidue()).isNotEmpty(); assertThat(e).hasMessageThat().isEqualTo("Unrecognized arguments: residue in expansion"); } @@ -494,7 +545,7 @@ public void parserCanBeCalledRepeatedly() throws OptionsParsingException { } @Test - public void multipleOccuringOption() throws OptionsParsingException { + public void multipleOccurringOption() throws OptionsParsingException { OptionsParser parser = OptionsParser.builder().optionsClasses(ExampleFoo.class).build(); parser.parse("--bing", "abcdef", "--foo", "foo1", "--bing", "123456" ); assertThat(parser.getOptions(ExampleFoo.class).bing).containsExactly("abcdef", "123456"); @@ -1846,7 +1897,7 @@ public void canonicalizeImplicitDepsNotListed() throws Exception { } @Test - public void canonicalizeSkipsDuplicateAndStillOmmitsImplicitDeps() throws Exception { + public void canonicalizeSkipsDuplicateAndStillOmitsImplicitDeps() throws Exception { assertThat(canonicalize(Yesterday.class, "--e=x", "--e=y")).containsExactly("--e=y"); } @@ -2398,6 +2449,37 @@ public void negativeExternalTargetPatternsInOptions_failsDistinctively() { .contains("Flags corresponding to Starlark-defined build settings always start with '--'"); } + @Test + public void fallbackOptions_optionsParsingEquivalently() throws OptionsParsingException { + OpaqueOptionsData fallbackData = + OptionsParser.getFallbackOptionsData( + ImmutableList.of(ExampleFoo.class, ExampleEquivalentWithFoo.class)); + OptionsParser parser = OptionsParser.builder().optionsClasses(ExampleFoo.class).build(); + parser.parseWithSourceFunction( + PriorityCategory.RC_FILE, + o -> ".bazelrc", + ImmutableList.of( + "--ignored_with_value", "--foo", "--foo=bar", "--ignored_without_value", "--bar", "1"), + fallbackData); + + assertThat(parser.getOptions(ExampleFoo.class)).isNotNull(); + assertThat(parser.getOptions(ExampleFoo.class).foo).isEqualTo("bar"); + assertThat(parser.getOptions(ExampleFoo.class).bar).isEqualTo(1); + + assertThat(parser.getOptions(ExampleEquivalentWithFoo.class)).isNull(); + } + + @Test + public void fallbackOptions_optionsParsingDifferently() { + Exception e = + assertThrows( + ConstructionException.class, + () -> + OptionsParser.getFallbackOptionsData( + ImmutableList.of(ExampleFoo.class, ExampleIncompatibleWithFoo.class))); + assertThat(e).hasCauseThat().isInstanceOf(DuplicateOptionDeclarationException.class); + } + private static OptionInstanceOrigin createInvocationPolicyOrigin() { return createInvocationPolicyOrigin(/*implicitDependent=*/ null, /*expandedFrom=*/ null); } diff --git a/src/test/py/bazel/BUILD b/src/test/py/bazel/BUILD index 4fa55fe5dd2a5d..2dce0908e1ee77 100644 --- a/src/test/py/bazel/BUILD +++ b/src/test/py/bazel/BUILD @@ -335,8 +335,8 @@ py_test( ) py_test( - name = "starlark_options_test", - srcs = ["starlark_options_test.py"], + name = "options_test", + srcs = ["options_test.py"], deps = [":test_base"], ) diff --git a/src/test/py/bazel/options_test.py b/src/test/py/bazel/options_test.py new file mode 100644 index 00000000000000..932931ec39c4ca --- /dev/null +++ b/src/test/py/bazel/options_test.py @@ -0,0 +1,274 @@ +# Copyright 2022 The Bazel Authors. All rights reserved. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +import unittest + +from src.test.py.bazel import test_base + + +class OptionsTest(test_base.TestBase): + + def testCanOverrideStarlarkFlagInBazelrcConfigStanza(self): + self.ScratchFile("WORKSPACE.bazel") + self.ScratchFile( + "bazelrc", + [ + "build:red --//f:color=red", + ], + ) + self.ScratchFile( + "f/BUILD.bazel", + [ + 'load(":f.bzl", "color", "r")', + "color(", + ' name = "color",', + ' build_setting_default = "white",', + ")", + 'r(name = "r")', + ], + ) + self.ScratchFile( + "f/f.bzl", + [ + 'ColorValue = provider("color")', + "def _color_impl(ctx):", + " return [ColorValue(color = ctx.build_setting_value)]", + "color = rule(", + " implementation = _color_impl,", + "build_setting = config.string(flag = True),", + ")", + "def _r_impl(ctx):", + " print(ctx.attr._color[ColorValue].color)", + " return [DefaultInfo()]", + "r = rule(", + " implementation = _r_impl,", + ' attrs = {"_color": attr.label(default = "//f:color")},', + ")", + ], + ) + + _, _, stderr = self.RunBazel([ + "--bazelrc=bazelrc", + "build", + "--nobuild", + "//f:r", + "--config=red", + "--//f:color=green", + ]) + self.assertTrue( + any("/f/f.bzl:9:10: green" in line for line in stderr), + "\n".join(stderr), + ) + + _, _, stderr = self.RunBazel([ + "--bazelrc=bazelrc", + "build", + "--nobuild", + "//f:r", + "--//f:color=green", + "--config=red", + ]) + self.assertTrue( + any("/f/f.bzl:9:10: red" in line for line in stderr), + "\n".join(stderr), + ) + + def testCommonPseudoCommand(self): + self.ScratchFile("WORKSPACE.bazel") + self.ScratchFile( + ".bazelrc", + [ + "common --copt=-Dfoo", + "common --copt -Dbar", + "common:my-config --copt=-Dbaz", + "common:my-config --copt -Dquz", + ], + ) + self.ScratchFile( + "pkg/BUILD.bazel", + [ + "cc_binary(name='main',srcs=['main.cc'])", + ], + ) + self.ScratchFile( + "pkg/main.cc", + [ + "#include ", + "int main() {", + "#ifdef foo", + ' printf("foo\\n");', + "#endif", + "#ifdef bar", + ' printf("bar\\n");', + "#endif", + "#ifdef baz", + ' printf("baz\\n");', + "#endif", + "#ifdef quz", + ' printf("quz\\n");', + "#endif", + " return 0;", + "}", + ], + ) + + # Check that run honors the common flags. + _, stdout, stderr = self.RunBazel([ + "run", + "--announce_rc", + "//pkg:main", + ]) + self.assertEqual( + ["foo", "bar"], + stdout, + ) + self.assertNotRegex( + "\n".join(stderr), + "Ignored as unsupported", + ) + + _, stdout, stderr = self.RunBazel([ + "run", + "--announce_rc", + "--config=my-config", + "//pkg:main", + ]) + self.assertEqual( + ["foo", "bar", "baz", "quz"], + stdout, + ) + self.assertNotRegex( + "\n".join(stderr), + "Ignored as unsupported", + ) + + # Check that query ignores the unsupported common flags. + _, stdout, stderr = self.RunBazel([ + "query", + "--announce_rc", + "//pkg:main", + ]) + self.assertRegex( + "\n".join(stderr), + "Ignored as unsupported by 'query': --copt=-Dfoo --copt -Dbar", + ) + self.assertNotRegex( + "\n".join(stderr), + "Ignored as unsupported by 'query': --copt=-Dbaz --copt -Dquz", + ) + + _, stdout, stderr = self.RunBazel([ + "query", + "--announce_rc", + "--config=my-config", + "//pkg:main", + ]) + self.assertRegex( + "\n".join(stderr), + "Ignored as unsupported by 'query': --copt=-Dfoo --copt -Dbar", + ) + self.assertRegex( + "\n".join(stderr), + "Ignored as unsupported by 'query': --copt=-Dbaz --copt -Dquz", + ) + + def testCommonPseudoCommand_singleLineParsesUnambiguously(self): + self.ScratchFile("WORKSPACE.bazel") + self.ScratchFile( + ".bazelrc", + [ + # First and third option are ignored by build, but valid options for + # cquery. The first one expects no value, the third one does. + "common --implicit_deps --copt=-Dfoo --output files --copt=-Dbar", + ], + ) + self.ScratchFile( + "pkg/BUILD.bazel", + [ + "cc_binary(name='main',srcs=['main.cc'])", + ], + ) + self.ScratchFile( + "pkg/main.cc", + [ + "#include ", + "int main() {", + "#ifdef foo", + ' printf("foo\\n");', + "#endif", + "#ifdef bar", + ' printf("bar\\n");', + "#endif", + " return 0;", + "}", + ], + ) + + # Check that run honors the common flags. + _, stdout, _ = self.RunBazel([ + "run", + "//pkg:main", + ]) + self.assertEqual( + ["foo", "bar"], + stdout, + ) + + def testCommonPseudoCommand_unsupportedOptionValue(self): + self.ScratchFile("WORKSPACE.bazel") + self.ScratchFile( + ".bazelrc", + [ + "common --output=starlark", + ], + ) + self.ScratchFile( + "pkg/BUILD.bazel", + [ + "cc_binary(name='main',srcs=['main.cc'])", + ], + ) + + # Check that cquery honors the common flag. + _, stdout, _ = self.RunBazel([ + "cquery", + "--starlark:expr=target.label.name", + "//pkg:main", + ]) + self.assertEqual( + ["main"], + stdout, + ) + + # Check that query fails as it supports the --output flag, but not its + # value. + exit_code, stdout, stderr = self.RunBazel( + [ + "query", + "//pkg:main", + ], + allow_failure=True, + ) + self.AssertExitCode(exit_code, 2, stderr) + self.assertTrue( + any( + "ERROR: Invalid output format 'starlark'." in line + for line in stderr + ), + stderr, + ) + + +if __name__ == "__main__": + unittest.main() diff --git a/src/test/py/bazel/starlark_options_test.py b/src/test/py/bazel/starlark_options_test.py deleted file mode 100644 index f6433b5cc577f9..00000000000000 --- a/src/test/py/bazel/starlark_options_test.py +++ /dev/null @@ -1,82 +0,0 @@ -# Copyright 2022 The Bazel Authors. All rights reserved. -# -# Licensed under the Apache License, Version 2.0 (the "License"); -# you may not use this file except in compliance with the License. -# You may obtain a copy of the License at -# -# http://www.apache.org/licenses/LICENSE-2.0 -# -# Unless required by applicable law or agreed to in writing, software -# distributed under the License is distributed on an "AS IS" BASIS, -# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -# See the License for the specific language governing permissions and -# limitations under the License. - -import unittest - -from src.test.py.bazel import test_base - - -class StarlarkOptionsTest(test_base.TestBase): - - def testCanOverrideStarlarkFlagInBazelrcConfigStanza(self): - self.ScratchFile("WORKSPACE.bazel") - self.ScratchFile("bazelrc", [ - "build:red --//f:color=red", - ]) - self.ScratchFile("f/BUILD.bazel", [ - 'load(":f.bzl", "color", "r")', - "color(", - ' name = "color",', - ' build_setting_default = "white",', - ")", - 'r(name = "r")', - ]) - self.ScratchFile("f/f.bzl", [ - 'ColorValue = provider("color")', - "def _color_impl(ctx):", - " return [ColorValue(color = ctx.build_setting_value)]", - "color = rule(", - " implementation = _color_impl,", - "build_setting = config.string(flag = True),", - ")", - "def _r_impl(ctx):", - " print(ctx.attr._color[ColorValue].color)", - " return [DefaultInfo()]", - "r = rule(", - " implementation = _r_impl,", - ' attrs = {"_color": attr.label(default = "//f:color")},', - ")", - ]) - - exit_code, _, stderr = self.RunBazel([ - "--bazelrc=bazelrc", - "build", - "--nobuild", - "//f:r", - "--config=red", - "--//f:color=green", - ]) - self.AssertExitCode(exit_code, 0, stderr) - self.assertTrue( - any("/f/f.bzl:9:10: green" in line for line in stderr), - "\n".join(stderr), - ) - - exit_code, _, stderr = self.RunBazel([ - "--bazelrc=bazelrc", - "build", - "--nobuild", - "//f:r", - "--//f:color=green", - "--config=red", - ]) - self.AssertExitCode(exit_code, 0, stderr) - self.assertTrue( - any("/f/f.bzl:9:10: red" in line for line in stderr), - "\n".join(stderr), - ) - - -if __name__ == "__main__": - unittest.main()