From 85daf39b53d78c2635dfa0a429f6797fc3315574 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. --- site/en/run/bazelrc.md | 8 +- .../build/lib/runtime/BlazeOptionHandler.java | 42 ++++- .../build/lib/runtime/BlazeRuntime.java | 2 +- .../build/lib/runtime/ConfigExpander.java | 10 +- .../com/google/devtools/common/options/BUILD | 1 + .../common/options/IsolatedOptionsData.java | 77 +++++--- .../devtools/common/options/OptionsData.java | 43 ++++- .../common/options/OptionsParser.java | 55 ++++-- .../common/options/OptionsParserImpl.java | 153 ++++++++++----- .../common/options/OptionsDataTest.java | 10 +- .../common/options/OptionsParserTest.java | 99 +++++++++- src/test/py/bazel/BUILD | 4 +- src/test/py/bazel/options_test.py | 174 ++++++++++++++++++ src/test/py/bazel/starlark_options_test.py | 80 -------- 14 files changed, 554 insertions(+), 204 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 0e0518aae15e63..4c247ec10d8175 100644 --- a/site/en/run/bazelrc.md +++ b/site/en/run/bazelrc.md @@ -105,7 +105,13 @@ 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 apply to all Bazel commands. If a command does not + support an option specified in this way, it will fail. +- `all-supported`: 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 will be ignored. 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. - _`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..113569aa4db35a 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,12 @@ 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 +204,22 @@ 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. + // + // 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 "all-supported" + // pseudo command can be parsed unambiguously. + 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 +255,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 +270,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 +403,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 +501,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..cf08cf274639b9 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,30 @@ 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. + * + *

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 +270,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 +290,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..5a08f7ed1ff2d0 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,38 @@ 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, an empty {@link Optional} is returned rather than throwing an exception. + */ + 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 +746,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 +771,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 +791,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..f23afb2ea4c115 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,74 @@ 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 = "not_foo", + category = "one", + documentationCategory = OptionDocumentationCategory.UNCATEGORIZED, + effectTags = {OptionEffectTag.NO_OP}, + defaultValue = "differentDefault" + ) + public String notFoo; + + @Option( + name = "not_bar", + category = "one", + documentationCategory = OptionDocumentationCategory.UNCATEGORIZED, + effectTags = {OptionEffectTag.NO_OP}, + defaultValue = "differentDefault" + ) + public String notBar; + } + + 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 +330,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 +351,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 +377,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"); } @@ -2398,6 +2456,31 @@ 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("--foo=bar", "--not_foo=baz", "--bar", "1", "--not_bar", "baz"), + 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 9ab83f5e719010..4b7043e8fea777 100644 --- a/src/test/py/bazel/BUILD +++ b/src/test/py/bazel/BUILD @@ -329,8 +329,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..004d1d790a2411 --- /dev/null +++ b/src/test/py/bazel/options_test.py @@ -0,0 +1,174 @@ +# 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 testAllSupportedPseudoCommand(self): + self.ScratchFile("WORKSPACE.bazel") + self.ScratchFile(".bazelrc", [ + "all-supported --copt=-Dfoo", + "all-supported --copt -Dbar", + "all-supported:my-config --copt=-Dbaz", + "all-supported: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 all-supported flags. + _, stdout, _ = self.RunBazel([ + "run", + "//pkg:main", + ]) + self.assertEquals( + ["foo", "bar"], + stdout, + ) + + _, stdout, _ = self.RunBazel([ + "run", + "--config=my-config", + "//pkg:main", + ]) + self.assertEquals( + ["foo", "bar", "baz", "quz"], + stdout, + ) + + # Check that query ignores the unsupported all-supported flags. + _, stdout, _ = self.RunBazel([ + "query", + "//pkg:main", + ]) + + _, stdout, _ = self.RunBazel([ + "query", + "--config=my-config", + "//pkg:main", + ]) + + def testAllSupportedPseudoCommand_unsupportedOptionValue(self): + self.ScratchFile("WORKSPACE.bazel") + self.ScratchFile(".bazelrc", [ + "all-supported --output=starlark", + ]) + self.ScratchFile("pkg/BUILD.bazel", [ + "cc_binary(name='main',srcs=['main.cc'])", + ]) + + # Check that cquery honors the all-supported flag. + _, stdout, _ = self.RunBazel([ + "cquery", + "--starlark:expr=target.label.name", + "//pkg:main", + ]) + self.assertEquals( + ["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 4a22c16d3a2248..00000000000000 --- a/src/test/py/bazel/starlark_options_test.py +++ /dev/null @@ -1,80 +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")},', - ")", - ]) - - _, _, 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), - ) - - -if __name__ == "__main__": - unittest.main()