diff options
5 files changed, 219 insertions, 961 deletions
diff --git a/site/docs/user-manual.html b/site/docs/user-manual.html index bdd5cfc1a0..84c963c906 100644 --- a/site/docs/user-manual.html +++ b/site/docs/user-manual.html @@ -262,16 +262,9 @@ title: User Manual with an underscore ('_') to avoid unintentional name sharing. </p> <p> - The expansion behavior for these <code>--config=foo</code> options has - changed. The legacy behavior, still the default, is to expand these in a - fixed point expansion after all default rc options are loaded. This is - unintuitive and has caused debugging difficulties in the past. The new - behavior is to expand <code>--config=foo</code> to the options it expands to + <code>--config=foo</code> expands to the options defined in the rc files "in-place" so that the options specified for the config have the same - precedence that the <code>--config=foo</code> option had. This is more - intuitive, and can be enabled using the startup flag - <code>--expand_configs_in_place</code>, which can be included in a bazelrc - file on a <code>startup</code> line. + precedence that the <code>--config=foo</code> option had. </p> <h5>Example</h5> diff --git a/src/main/java/com/google/devtools/build/lib/runtime/BlazeCommandDispatcher.java b/src/main/java/com/google/devtools/build/lib/runtime/BlazeCommandDispatcher.java index e52b80b7df..310829f65a 100644 --- a/src/main/java/com/google/devtools/build/lib/runtime/BlazeCommandDispatcher.java +++ b/src/main/java/com/google/devtools/build/lib/runtime/BlazeCommandDispatcher.java @@ -238,18 +238,14 @@ public class BlazeCommandDispatcher { StoredEventHandler storedEventHandler = new StoredEventHandler(); BlazeOptionHandler optionHandler = - BlazeOptionHandler.getHandler( + new BlazeOptionHandler( runtime, workspace, command, commandAnnotation, // Provide the options parser so that we can cache OptionsData here. createOptionsParser(command), - invocationPolicy, - runtime - .getStartupOptionsProvider() - .getOptions(BlazeServerStartupOptions.class) - .expandConfigsInPlace); + invocationPolicy); ExitCode earlyExitCode = optionHandler.parseOptions(args, storedEventHandler); OptionsProvider options = optionHandler.getOptionsResult(); 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 107cf4518d..ad6f391ccb 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 @@ -15,7 +15,6 @@ package com.google.devtools.build.lib.runtime; import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Joiner; -import com.google.common.base.Predicates; import com.google.common.collect.ArrayListMultimap; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; @@ -38,7 +37,6 @@ import com.google.devtools.common.options.OptionsProvider; import com.google.devtools.common.options.ParsedOptionDescription; import java.io.IOException; import java.util.ArrayList; -import java.util.Collection; import java.util.HashSet; import java.util.LinkedHashSet; import java.util.List; @@ -49,9 +47,9 @@ import java.util.logging.Level; /** * Handles parsing the blaze command arguments. * - * <p>This class manages rc options, default options, and invocation policy. + * <p>This class manages rc options, configs, default options, and invocation policy. */ -public abstract class BlazeOptionHandler { +public final class BlazeOptionHandler { // Keep in sync with options added in OptionProcessor::AddRcfileArgsAndOptions() private static final ImmutableSet<String> INTERNAL_COMMAND_OPTIONS = ImmutableSet.of( @@ -63,15 +61,15 @@ public abstract class BlazeOptionHandler { "client_env", "client_cwd"); - protected final BlazeRuntime runtime; - protected final OptionsParser optionsParser; - protected final BlazeWorkspace workspace; - protected final BlazeCommand command; - protected final Command commandAnnotation; - protected final InvocationPolicy invocationPolicy; - protected final List<String> rcfileNotes = new ArrayList<>(); + private final BlazeRuntime runtime; + private final OptionsParser optionsParser; + private final BlazeWorkspace workspace; + private final BlazeCommand command; + private final Command commandAnnotation; + private final InvocationPolicy invocationPolicy; + private final List<String> rcfileNotes = new ArrayList<>(); - private BlazeOptionHandler( + BlazeOptionHandler( BlazeRuntime runtime, BlazeWorkspace workspace, BlazeCommand command, @@ -86,23 +84,6 @@ public abstract class BlazeOptionHandler { this.invocationPolicy = invocationPolicy; } - public static BlazeOptionHandler getHandler( - BlazeRuntime runtime, - BlazeWorkspace workspace, - BlazeCommand command, - Command commandAnnotation, - OptionsParser optionsParser, - InvocationPolicy invocationPolicy, - boolean expandConfigsInPlace) { - if (expandConfigsInPlace) { - return new InPlaceConfigExpansionRcHandler( - runtime, workspace, command, commandAnnotation, optionsParser, invocationPolicy); - } else { - return new FixPointConfigExpansionRcHandler( - runtime, workspace, command, commandAnnotation, optionsParser, invocationPolicy); - } - } - // Return options as OptionsProvider so the options can't be easily modified after we've // applied the invocation policy. OptionsProvider getOptionsResult() { @@ -198,14 +179,6 @@ public abstract class BlazeOptionHandler { } } - /** - * Expand the values of --config according to the definitions provided in the rc files and the - * applicable command. - */ - abstract void expandConfigOptions( - EventHandler eventHandler, ListMultimap<String, RcChunkOfArgs> commandToRcArgs) - throws OptionsParsingException; - private void parseArgsAndConfigs(List<String> args, ExtendedEventHandler eventHandler) throws OptionsParsingException { Path workspaceDirectory = workspace.getWorkspace(); @@ -314,281 +287,165 @@ public abstract class BlazeOptionHandler { } /** - * Implements the legacy way of expanding blazerc and config expansions. - * - * <p>--config options are expanded in a fixed point expansion at the end of the rc option block. - * Their expansion is defined in the rc file, and is triggered by the presence of a {@code - * --config=someConfigName} option somewhere in the user's options. The resulting option order is - * {@code <rc options> <config expansions> <command line options>}, where all of the config values - * mentioned are in the mid segment, regardless of whether they were defined in an rc file, on the - * command line, or in the expansion of other config values that were triggered earlier. If the - * same config value is provided twice (say, once on the command line and once in another config - * expansion) it will only be expanded once. - * - * <p>This behavior is relatively well-defined, but the final order of options is not intuitive. - * As a simple example, consider a user specified command line with --config=foo last. Most users - * expect the expansion of --config=foo to override earlier flags in their command line if - * necessary, but this is not the case. This is why we are phasing out this behavior. We will - * maintain the old behavior to avoid breaking users during a transition period. + * Expand the values of --config according to the definitions provided in the rc files and the + * applicable command. */ - static final class FixPointConfigExpansionRcHandler extends BlazeOptionHandler { - - private FixPointConfigExpansionRcHandler( - BlazeRuntime runtime, - BlazeWorkspace workspace, - BlazeCommand command, - Command commandAnnotation, - OptionsParser optionsParser, - InvocationPolicy invocationPolicy) { - super(runtime, workspace, command, commandAnnotation, optionsParser, invocationPolicy); + void expandConfigOptions( + EventHandler eventHandler, ListMultimap<String, RcChunkOfArgs> commandToRcArgs) + throws OptionsParsingException { + + OptionValueDescription configValueDescription = + optionsParser.getOptionValueDescription("config"); + if (configValueDescription == null || configValueDescription.getCanonicalInstances() == null) { + // No --config values were set, we can avoid this whole thing. + return; } - @Override - void expandConfigOptions( - EventHandler eventHandler, ListMultimap<String, RcChunkOfArgs> commandToRcArgs) - throws OptionsParsingException { - // Fix-point iteration until all configs are loaded. - List<String> configsLoaded = ImmutableList.of(); - Set<String> unknownConfigs = new LinkedHashSet<>(); - CommonCommandOptions commonOptions = optionsParser.getOptions(CommonCommandOptions.class); - while (!commonOptions.configs.equals(configsLoaded)) { - // Parse the configs we have not seen yet. - Set<String> missingConfigs = new LinkedHashSet<>(commonOptions.configs); - missingConfigs.removeAll(configsLoaded); - parseConfigOptionsForCommand(commandToRcArgs, missingConfigs, unknownConfigs); - // Refresh the list of config values we've processed to be the list of config values we had - // before the call to parseConfigOptionsForCommand. - configsLoaded = commonOptions.configs; - commonOptions = optionsParser.getOptions(CommonCommandOptions.class); - } - if (!unknownConfigs.isEmpty()) { - if (commonOptions.allowUndefinedConfigs) { - eventHandler.handle( - Event.warn( - "Config values are not defined in any .rc file: " - + Joiner.on(", ").join(unknownConfigs))); - } else { - throw new OptionsParsingException( - "Config values are not defined in any .rc file: " - + Joiner.on(", ").join(unknownConfigs)); - } - } + // Find the base set of configs. This does not include the config options that might be + // recursively incuded. + ImmutableList<ParsedOptionDescription> configInstances = + ImmutableList.copyOf(configValueDescription.getCanonicalInstances()); + + // Expand the configs that are mentioned in the input. Flatten these expansions before parsing + // them, to preserve order. + for (ParsedOptionDescription configInstance : configInstances) { + String configValueToExpand = (String) configInstance.getConvertedValue(); + List<String> expansion = getExpansion(eventHandler, commandToRcArgs, configValueToExpand); + optionsParser.parseArgsAsExpansionOfOption( + configInstance, String.format("expanded from --%s", configValueToExpand), expansion); } - /** - * Go through the configs given and parse their expansion if a definition was found. - * - * @param configs the configs for which to parse options. - * @param unknownConfigs a collection that the method will populate with the config values in - * {@code configs} that none of the .rc files had entries for. - */ - private void parseConfigOptionsForCommand( - ListMultimap<String, RcChunkOfArgs> commandToRcArgs, - Collection<String> configs, - Collection<String> unknownConfigs) - throws OptionsParsingException { - Set<String> knownConfigs = new HashSet<>(); - for (String commandToParse : getCommandNamesToParse(commandAnnotation)) { - for (String config : configs) { - String configDef = commandToParse + ":" + config; - for (RcChunkOfArgs rcArgs : commandToRcArgs.get(configDef)) { - // Track that we've found at least 1 definition for this config value. - knownConfigs.add(config); - rcfileNotes.add( - String.format( - "Found applicable config definition %s in file %s: %s", - configDef, rcArgs.rcFile, String.join(" ", rcArgs.args))); - optionsParser.parse(PriorityCategory.RC_FILE, rcArgs.rcFile, rcArgs.args); - } - } - } - if (configs.size() > knownConfigs.size()) { - configs - .stream() - .filter(Predicates.not(Predicates.in(knownConfigs))) - .forEachOrdered(unknownConfigs::add); + // At this point, we've expanded everything, identify duplicates, if any, to warn about + // re-application. + List<String> configs = optionsParser.getOptions(CommonCommandOptions.class).configs; + Set<String> configSet = new HashSet<>(); + LinkedHashSet<String> duplicateConfigs = new LinkedHashSet<>(); + for (String configValue : configs) { + if (!configSet.add(configValue)) { + duplicateConfigs.add(configValue); } } + if (!duplicateConfigs.isEmpty()) { + eventHandler.handle( + Event.warn( + String.format( + "The following configs were expanded more than once: %s. For repeatable flags, " + + "repeats are counted twice and may lead to unexpected behavior.", + duplicateConfigs))); + } } - /** - * Expands the rc's options in {@code <rc options> <command line>} order, with config options - * expanded wherever the --config option was mentioned. - * - * <p>This requires a bit more care than the in-place expansion. We need to be wary of potential - * cycles in the definitions of config values, and should warn when a single --config expansion - * contradicts itself. - */ - static final class InPlaceConfigExpansionRcHandler extends BlazeOptionHandler { - - private InPlaceConfigExpansionRcHandler( - BlazeRuntime runtime, - BlazeWorkspace workspace, - BlazeCommand command, - Command commandAnnotation, - OptionsParser optionsParser, - InvocationPolicy invocationPolicy) { - super(runtime, workspace, command, commandAnnotation, optionsParser, invocationPolicy); + private List<String> getExpansion( + EventHandler eventHandler, + ListMultimap<String, RcChunkOfArgs> commandToRcArgs, + String configToExpand) + throws OptionsParsingException { + LinkedHashSet<String> configAncestorSet = new LinkedHashSet<>(); + configAncestorSet.add(configToExpand); + List<String> longestChain = new ArrayList<>(); + List<String> finalExpansion = + getExpansion( + eventHandler, commandToRcArgs, configAncestorSet, configToExpand, longestChain); + + // In order to prevent warning about a long chain of 13 configs at the 10, 11, 12, and 13 + // point, we identify the longest chain for this 'high-level' --config found and only warn + // about it once. This may mean we missed a fork where each branch was independently long + // enough to warn, but the single warning should convey the message reasonably. + if (longestChain.size() >= 10) { + eventHandler.handle( + Event.warn( + String.format( + "There is a recursive chain of configs %s configs long: %s. This seems " + + "excessive, and might be hiding errors.", + longestChain.size(), longestChain))); } + return finalExpansion; + } - @Override - void expandConfigOptions( - EventHandler eventHandler, ListMultimap<String, RcChunkOfArgs> commandToRcArgs) - throws OptionsParsingException { - - OptionValueDescription configValueDescription = - optionsParser.getOptionValueDescription("config"); - if (configValueDescription == null - || configValueDescription.getCanonicalInstances() == null) { - // No --config values were set, we can avoid this whole thing. - return; - } - - // Find the base set of configs. This does not include the config options that might be - // recursively incuded. - ImmutableList<ParsedOptionDescription> configInstances = - ImmutableList.copyOf(configValueDescription.getCanonicalInstances()); - - // Expand the configs that are mentioned in the input. Flatten these expansions before parsing - // them, to preserve order. - for (ParsedOptionDescription configInstance : configInstances) { - String configValueToExpand = (String) configInstance.getConvertedValue(); - List<String> expansion = getExpansion(eventHandler, commandToRcArgs, configValueToExpand); - optionsParser.parseArgsAsExpansionOfOption( - configInstance, String.format("expanded from --%s", configValueToExpand), expansion); - } - - // At this point, we've expanded everything, identify duplicates, if any, to warn about - // re-application. - List<String> configs = optionsParser.getOptions(CommonCommandOptions.class).configs; - Set<String> configSet = new HashSet<>(); - LinkedHashSet<String> duplicateConfigs = new LinkedHashSet<>(); - for (String configValue : configs) { - if (!configSet.add(configValue)) { - duplicateConfigs.add(configValue); - } - } - if (!duplicateConfigs.isEmpty()) { - eventHandler.handle(Event.warn( + /** + * @param configAncestorSet is the chain of configs that have led to this one getting expanded. + * This should only contain the configs that expanded, recursively, to this one, and should + * not contain "siblings," as it is used to detect cycles. {@code build:foo --config=bar}, + * {@code build:bar --config=foo}, is a cycle, detected because this list will be [foo, bar] + * when we find another 'foo' to expand. However, {@code build:foo --config=bar}, {@code + * build:foo --config=bar} is not a cycle just because bar is expanded twice, and the 1st bar + * should not be in the parents list of the second bar. + * @param longestChain will be populated with the longest inheritance chain of configs. + */ + private List<String> getExpansion( + EventHandler eventHandler, + ListMultimap<String, RcChunkOfArgs> commandToRcArgs, + LinkedHashSet<String> configAncestorSet, + String configToExpand, + List<String> longestChain) + throws OptionsParsingException { + List<String> expansion = new ArrayList<>(); + boolean foundDefinition = false; + // The expansion order of rc files is first by command priority, and then in the order the + // rc files were read, respecting import statement placement. + for (String commandToParse : getCommandNamesToParse(commandAnnotation)) { + String configDef = commandToParse + ":" + configToExpand; + for (RcChunkOfArgs rcArgs : commandToRcArgs.get(configDef)) { + foundDefinition = true; + rcfileNotes.add( String.format( - "The following configs were expanded more than once: %s. For repeatable flags, " - + "repeats are counted twice and may lead to unexpected behavior.", - duplicateConfigs))); - } - } - - private List<String> getExpansion( - EventHandler eventHandler, - ListMultimap<String, RcChunkOfArgs> commandToRcArgs, - String configToExpand) - throws OptionsParsingException { - LinkedHashSet<String> configAncestorSet = new LinkedHashSet<>(); - configAncestorSet.add(configToExpand); - List<String> longestChain = new ArrayList<>(); - List<String> finalExpansion = - getExpansion( - eventHandler, commandToRcArgs, configAncestorSet, configToExpand, longestChain); - - // In order to prevent warning about a long chain of 13 configs at the 10, 11, 12, and 13 - // point, we identify the longest chain for this 'high-level' --config found and only warn - // about it once. This may mean we missed a fork where each branch was independently long - // enough to warn, but the single warning should convey the message reasonably. - if (longestChain.size() >= 10) { - eventHandler.handle( - Event.warn( - String.format( - "There is a recursive chain of configs %s configs long: %s. This seems " - + "excessive, and might be hiding errors.", - longestChain.size(), longestChain))); - } - return finalExpansion; - } - - /** - * @param configAncestorSet is the chain of configs that have led to this one getting expanded. - * This should only contain the configs that expanded, recursively, to this one, and should - * not contain "siblings," as it is used to detect cycles. {@code build:foo --config=bar}, - * {@code build:bar --config=foo}, is a cycle, detected because this list will be [foo, bar] - * when we find another 'foo' to expand. However, {@code build:foo --config=bar}, {@code - * build:foo --config=bar} is not a cycle just because bar is expanded twice, and the 1st - * bar should not be in the parents list of the second bar. - * @param longestChain will be populated with the longest inheritance chain of configs. - */ - private List<String> getExpansion( - EventHandler eventHandler, - ListMultimap<String, RcChunkOfArgs> commandToRcArgs, - LinkedHashSet<String> configAncestorSet, - String configToExpand, - List<String> longestChain) - throws OptionsParsingException { - List<String> expansion = new ArrayList<>(); - boolean foundDefinition = false; - // The expansion order of rc files is first by command priority, and then in the order the - // rc files were read, respecting import statement placement. - for (String commandToParse : getCommandNamesToParse(commandAnnotation)) { - String configDef = commandToParse + ":" + configToExpand; - for (RcChunkOfArgs rcArgs : commandToRcArgs.get(configDef)) { - foundDefinition = true; - rcfileNotes.add( - String.format( - "Found applicable config definition %s in file %s: %s", - configDef, rcArgs.rcFile, String.join(" ", rcArgs.args))); - - // For each arg in the rcARgs chunk, we first check if it is a config, and if so, expand - // it in place. We avoid cycles by tracking the parents of this config. - for (String arg : rcArgs.args) { - expansion.add(arg); - if (arg.length() >= 8 && arg.substring(0, 8).equals("--config")) { - // We have a config. For sanity, because we don't want to worry about formatting, - // we will only accept --config=value, and will not accept value on a following line. - int charOfConfigValue = arg.indexOf('='); - if (charOfConfigValue < 0) { - throw new OptionsParsingException( - String.format( - "In file %s, the definition of config %s expands to another config " - + "that either has no value or is not in the form --config=value. For " - + "recursive config definitions, please do not provide the value in a " - + "separate token, such as in the form '--config value'.", - rcArgs.rcFile, configToExpand)); - } - String newConfigValue = arg.substring(charOfConfigValue + 1); - LinkedHashSet<String> extendedConfigAncestorSet = - new LinkedHashSet<>(configAncestorSet); - if (!extendedConfigAncestorSet.add(newConfigValue)) { - throw new OptionsParsingException( - String.format( - "Config expansion has a cycle: config value %s expands to itself, " - + "see inheritance chain %s", - newConfigValue, extendedConfigAncestorSet)); - } - if (extendedConfigAncestorSet.size() > longestChain.size()) { - longestChain.clear(); - longestChain.addAll(extendedConfigAncestorSet); - } - - expansion.addAll( - getExpansion( - eventHandler, - commandToRcArgs, - extendedConfigAncestorSet, - newConfigValue, - longestChain)); + "Found applicable config definition %s in file %s: %s", + configDef, rcArgs.rcFile, String.join(" ", rcArgs.args))); + + // For each arg in the rcARgs chunk, we first check if it is a config, and if so, expand + // it in place. We avoid cycles by tracking the parents of this config. + for (String arg : rcArgs.args) { + expansion.add(arg); + if (arg.length() >= 8 && arg.substring(0, 8).equals("--config")) { + // We have a config. For sanity, because we don't want to worry about formatting, + // we will only accept --config=value, and will not accept value on a following line. + int charOfConfigValue = arg.indexOf('='); + if (charOfConfigValue < 0) { + throw new OptionsParsingException( + String.format( + "In file %s, the definition of config %s expands to another config " + + "that either has no value or is not in the form --config=value. For " + + "recursive config definitions, please do not provide the value in a " + + "separate token, such as in the form '--config value'.", + rcArgs.rcFile, configToExpand)); + } + String newConfigValue = arg.substring(charOfConfigValue + 1); + LinkedHashSet<String> extendedConfigAncestorSet = + new LinkedHashSet<>(configAncestorSet); + if (!extendedConfigAncestorSet.add(newConfigValue)) { + throw new OptionsParsingException( + String.format( + "Config expansion has a cycle: config value %s expands to itself, " + + "see inheritance chain %s", + newConfigValue, extendedConfigAncestorSet)); + } + if (extendedConfigAncestorSet.size() > longestChain.size()) { + longestChain.clear(); + longestChain.addAll(extendedConfigAncestorSet); } + + expansion.addAll( + getExpansion( + eventHandler, + commandToRcArgs, + extendedConfigAncestorSet, + newConfigValue, + longestChain)); } } } + } - if (!foundDefinition) { - String warning = "Config value " + configToExpand + " is not defined in any .rc file"; - CommonCommandOptions commonOptions = optionsParser.getOptions(CommonCommandOptions.class); - if (commonOptions.allowUndefinedConfigs) { - eventHandler.handle(Event.warn(warning)); - } else { - throw new OptionsParsingException(warning); - } + if (!foundDefinition) { + String warning = "Config value " + configToExpand + " is not defined in any .rc file"; + CommonCommandOptions commonOptions = optionsParser.getOptions(CommonCommandOptions.class); + if (commonOptions.allowUndefinedConfigs) { + eventHandler.handle(Event.warn(warning)); + } else { + throw new OptionsParsingException(warning); } - return expansion; } + return expansion; } private static List<String> getCommandNamesToParse(Command commandAnnotation) { diff --git a/src/main/java/com/google/devtools/build/lib/runtime/BlazeServerStartupOptions.java b/src/main/java/com/google/devtools/build/lib/runtime/BlazeServerStartupOptions.java index a79eb773b3..faed25a203 100644 --- a/src/main/java/com/google/devtools/build/lib/runtime/BlazeServerStartupOptions.java +++ b/src/main/java/com/google/devtools/build/lib/runtime/BlazeServerStartupOptions.java @@ -408,15 +408,16 @@ public class BlazeServerStartupOptions extends OptionsBase { ) public int connectTimeoutSecs; + @Deprecated @Option( - name = "expand_configs_in_place", - defaultValue = "true", // NOTE: only for documentation, value is always passed by the client. - documentationCategory = OptionDocumentationCategory.BAZEL_CLIENT_OPTIONS, - effectTags = {OptionEffectTag.BAZEL_INTERNAL_CONFIGURATION, OptionEffectTag.CHANGES_INPUTS}, - metadataTags = {OptionMetadataTag.EXPERIMENTAL}, - help = - "Changes the expansion of --config flags to be done in-place, as opposed to in a fixed " - + "point expansion between normal rc options and command-line specified options." - ) + name = "expand_configs_in_place", + defaultValue = "true", // NOTE: only for documentation, value is always passed by the client. + documentationCategory = OptionDocumentationCategory.BAZEL_CLIENT_OPTIONS, + effectTags = {OptionEffectTag.NO_OP}, + metadataTags = {OptionMetadataTag.DEPRECATED}, + deprecationWarning = "This option is now a no-op and will soon be deleted.", + help = + "Changed the expansion of --config flags to be done in-place, as opposed to in a fixed " + + "point expansion between normal rc options and command-line specified options.") public boolean expandConfigsInPlace; } diff --git a/src/test/java/com/google/devtools/build/lib/runtime/BlazeOptionHandlerTest.java b/src/test/java/com/google/devtools/build/lib/runtime/BlazeOptionHandlerTest.java index 33b8acb10c..e639979c0a 100644 --- a/src/test/java/com/google/devtools/build/lib/runtime/BlazeOptionHandlerTest.java +++ b/src/test/java/com/google/devtools/build/lib/runtime/BlazeOptionHandlerTest.java @@ -84,30 +84,15 @@ public class BlazeOptionHandlerTest { /* defaultSystemJavabase= */ null, productName); runtime.initWorkspace(directories, /*binTools=*/ null); - } - - private void makeFixedPointExpandingConfigOptionHandler() { - optionHandler = - BlazeOptionHandler.getHandler( - runtime, - runtime.getWorkspace(), - new C0Command(), - C0Command.class.getAnnotation(Command.class), - parser, - InvocationPolicy.getDefaultInstance(), - false); - } - private void makeInPlaceExpandingConfigOptionHandler() { optionHandler = - BlazeOptionHandler.getHandler( + new BlazeOptionHandler( runtime, runtime.getWorkspace(), new C0Command(), C0Command.class.getAnnotation(Command.class), parser, - InvocationPolicy.getDefaultInstance(), - true); + InvocationPolicy.getDefaultInstance()); } @Command( @@ -169,7 +154,8 @@ public class BlazeOptionHandlerTest { return structuredArgs; } - private void testStructureRcOptionsAndConfigs_argumentless() throws Exception { + @Test + public void testStructureRcOptionsAndConfigs_argumentless_inPlace() throws Exception { ListMultimap<String, RcChunkOfArgs> structuredRc = BlazeOptionHandler.structureRcOptionsAndConfigs( eventHandler, @@ -181,18 +167,7 @@ public class BlazeOptionHandlerTest { } @Test - public void testStructureRcOptionsAndConfigs_argumentless_fixedPoint() throws Exception { - makeFixedPointExpandingConfigOptionHandler(); - testStructureRcOptionsAndConfigs_argumentless(); - } - - @Test - public void testStructureRcOptionsAndConfigs_argumentless_inPlace() throws Exception { - makeInPlaceExpandingConfigOptionHandler(); - testStructureRcOptionsAndConfigs_argumentless(); - } - - private void testStructureRcOptionsAndConfigs_configOnly() throws Exception { + public void testStructureRcOptionsAndConfigs_configOnly_inPlace() throws Exception { BlazeOptionHandler.structureRcOptionsAndConfigs( eventHandler, Arrays.asList("rc1", "rc2"), @@ -202,18 +177,7 @@ public class BlazeOptionHandlerTest { } @Test - public void testStructureRcOptionsAndConfigs_configOnly_fixedPoint() throws Exception { - makeFixedPointExpandingConfigOptionHandler(); - testStructureRcOptionsAndConfigs_configOnly(); - } - - @Test - public void testStructureRcOptionsAndConfigs_configOnly_inPlace() throws Exception { - makeInPlaceExpandingConfigOptionHandler(); - testStructureRcOptionsAndConfigs_configOnly(); - } - - private void testStructureRcOptionsAndConfigs_invalidCommand() throws Exception { + public void testStructureRcOptionsAndConfigs_invalidCommand_inPlace() throws Exception { BlazeOptionHandler.structureRcOptionsAndConfigs( eventHandler, Arrays.asList("rc1", "rc2"), @@ -225,18 +189,7 @@ public class BlazeOptionHandlerTest { } @Test - public void testStructureRcOptionsAndConfigs_invalidCommand_fixedPoint() throws Exception { - makeFixedPointExpandingConfigOptionHandler(); - testStructureRcOptionsAndConfigs_invalidCommand(); - } - - @Test - public void testStructureRcOptionsAndConfigs_invalidCommand_inPlace() throws Exception { - makeInPlaceExpandingConfigOptionHandler(); - testStructureRcOptionsAndConfigs_invalidCommand(); - } - - private void testStructureRcOptionsAndConfigs_twoRcs() throws Exception { + public void testStructureRcOptionsAndConfigs_twoRcs_inPlace() throws Exception { ListMultimap<String, RcChunkOfArgs> structuredRc = BlazeOptionHandler.structureRcOptionsAndConfigs( eventHandler, @@ -255,18 +208,7 @@ public class BlazeOptionHandlerTest { } @Test - public void testStructureRcOptionsAndConfigs_twoRcs_fixedPoint() throws Exception { - makeFixedPointExpandingConfigOptionHandler(); - testStructureRcOptionsAndConfigs_twoRcs(); - } - - @Test - public void testStructureRcOptionsAndConfigs_twoRcs_inPlace() throws Exception { - makeInPlaceExpandingConfigOptionHandler(); - testStructureRcOptionsAndConfigs_twoRcs(); - } - - private void testStructureRcOptionsAndConfigs_importedRcs() throws Exception { + public void testStructureRcOptionsAndConfigs_importedRcs_inPlace() throws Exception { ListMultimap<String, RcChunkOfArgs> structuredRc = BlazeOptionHandler.structureRcOptionsAndConfigs( eventHandler, @@ -285,18 +227,6 @@ public class BlazeOptionHandlerTest { assertThat(eventHandler.isEmpty()).isTrue(); } - @Test - public void testStructureRcOptionsAndConfigs_importedRcs_fixedPoint() throws Exception { - makeFixedPointExpandingConfigOptionHandler(); - testStructureRcOptionsAndConfigs_importedRcs(); - } - - @Test - public void testStructureRcOptionsAndConfigs_importedRcs_inPlace() throws Exception { - makeInPlaceExpandingConfigOptionHandler(); - testStructureRcOptionsAndConfigs_importedRcs(); - } - private void testStructureRcOptionsAndConfigs_badOverrideIndex() throws Exception { ListMultimap<String, RcChunkOfArgs> structuredRc = BlazeOptionHandler.structureRcOptionsAndConfigs( @@ -321,14 +251,8 @@ public class BlazeOptionHandlerTest { } @Test - public void testStructureRcOptionsAndConfigs_badOverrideIndex_fixedPoint() throws Exception { - makeFixedPointExpandingConfigOptionHandler(); - testStructureRcOptionsAndConfigs_badOverrideIndex(); - } - - @Test public void testStructureRcOptionsAndConfigs_badOverrideIndex_inPlace() throws Exception { - makeInPlaceExpandingConfigOptionHandler(); + testStructureRcOptionsAndConfigs_badOverrideIndex(); } @@ -339,14 +263,8 @@ public class BlazeOptionHandlerTest { } @Test - public void testParseRcOptions_empty_fixedPoint() throws Exception { - makeFixedPointExpandingConfigOptionHandler(); - testParseRcOptions_empty(); - } - - @Test public void testParseRcOptions_empty_inPlace() throws Exception { - makeInPlaceExpandingConfigOptionHandler(); + testParseRcOptions_empty(); } @@ -357,14 +275,8 @@ public class BlazeOptionHandlerTest { } @Test - public void testParseRcOptions_flatRcs_residue_fixedPoint() throws Exception { - makeFixedPointExpandingConfigOptionHandler(); - testParseRcOptions_flatRcs_residue(); - } - - @Test public void testParseRcOptions_flatRcs_residue_inPlace() throws Exception { - makeInPlaceExpandingConfigOptionHandler(); + testParseRcOptions_flatRcs_residue(); } @@ -377,14 +289,8 @@ public class BlazeOptionHandlerTest { } @Test - public void testParseRcOptions_flatRcs_flags_fixedPoint() throws Exception { - makeFixedPointExpandingConfigOptionHandler(); - testParseRcOptions_flatRcs_flags(); - } - - @Test public void testParseRcOptions_flatRcs_flags_inPlace() throws Exception { - makeInPlaceExpandingConfigOptionHandler(); + testParseRcOptions_flatRcs_flags(); } @@ -395,14 +301,8 @@ public class BlazeOptionHandlerTest { } @Test - public void testParseRcOptions_importedRcs_residue_fixedPoint() throws Exception { - makeFixedPointExpandingConfigOptionHandler(); - testParseRcOptions_importedRcs_residue(); - } - - @Test public void testParseRcOptions_importedRcs_residue_inPlace() throws Exception { - makeInPlaceExpandingConfigOptionHandler(); + testParseRcOptions_importedRcs_residue(); } @@ -412,14 +312,8 @@ public class BlazeOptionHandlerTest { } @Test - public void testExpandConfigOptions_configless_fixedPoint() throws Exception { - makeFixedPointExpandingConfigOptionHandler(); - testExpandConfigOptions_configless(); - } - - @Test public void testExpandConfigOptions_configless_inPlace() throws Exception { - makeInPlaceExpandingConfigOptionHandler(); + testExpandConfigOptions_configless(); } @@ -432,14 +326,8 @@ public class BlazeOptionHandlerTest { } @Test - public void testExpandConfigOptions_withConfig_fixedPoint() throws Exception { - makeFixedPointExpandingConfigOptionHandler(); - testExpandConfigOptions_withConfig(); - } - - @Test public void testExpandConfigOptions_withConfig_inPlace() throws Exception { - makeInPlaceExpandingConfigOptionHandler(); + testExpandConfigOptions_withConfig(); } @@ -451,22 +339,8 @@ public class BlazeOptionHandlerTest { } @Test - public void testExpandConfigOptions_withConfigForUnapplicableCommand_fixedPoint() - throws Exception { - makeFixedPointExpandingConfigOptionHandler(); - try { - testExpandConfigOptions_withConfigForUnapplicableCommand(); - fail(); - } catch (OptionsParsingException e) { - assertThat(e) - .hasMessageThat() - .contains("Config values are not defined in any .rc file: other"); - } - } - - @Test public void testExpandConfigOptions_withConfigForUnapplicableCommand_inPlace() throws Exception { - makeInPlaceExpandingConfigOptionHandler(); + try { testExpandConfigOptions_withConfigForUnapplicableCommand(); fail(); @@ -482,18 +356,9 @@ public class BlazeOptionHandlerTest { } @Test - public void testExpandConfigOptions_withConfigForUnapplicableCommand_allowUndefined_fixedPoint() - throws Exception { - makeFixedPointExpandingConfigOptionHandler(); - testExpandConfigOptions_withConfigForUnapplicableCommand_allowUndefined(); - assertThat(eventHandler.getEvents()) - .contains(Event.warn("Config values are not defined in any .rc file: other")); - } - - @Test public void testExpandConfigOptions_withConfigForUnapplicableCommand_allowUndefined_inPlace() throws Exception { - makeInPlaceExpandingConfigOptionHandler(); + testExpandConfigOptions_withConfigForUnapplicableCommand_allowUndefined(); assertThat(eventHandler.getEvents()) .contains(Event.warn("Config value other is not defined in any .rc file")); @@ -507,16 +372,8 @@ public class BlazeOptionHandlerTest { } @Test - public void testAllowUndefinedConfig_fixedPoint() throws Exception { - makeFixedPointExpandingConfigOptionHandler(); - testAllowUndefinedConfig(); - assertThat(eventHandler.getEvents()) - .contains(Event.warn("Config values are not defined in any .rc file: invalid")); - } - - @Test public void testAllowUndefinedConfig_inPlace() throws Exception { - makeInPlaceExpandingConfigOptionHandler(); + testAllowUndefinedConfig(); assertThat(eventHandler.getEvents()) .contains(Event.warn("Config value invalid is not defined in any .rc file")); @@ -528,21 +385,8 @@ public class BlazeOptionHandlerTest { } @Test - public void testNoAllowUndefinedConfig_fixedPoint() { - makeFixedPointExpandingConfigOptionHandler(); - try { - testUndefinedConfig(); - fail(); - } catch (OptionsParsingException e) { - assertThat(e) - .hasMessageThat() - .contains("Config values are not defined in any .rc file: invalid"); - } - } - - @Test public void testNoAllowUndefinedConfig_inPlace() { - makeInPlaceExpandingConfigOptionHandler(); + try { testUndefinedConfig(); fail(); @@ -561,14 +405,8 @@ public class BlazeOptionHandlerTest { } @Test - public void testParseOptions_argless_fixedPoint() { - makeFixedPointExpandingConfigOptionHandler(); - testParseOptions_argless(); - } - - @Test public void testParseOptions_argless_inPlace() { - makeInPlaceExpandingConfigOptionHandler(); + testParseOptions_argless(); } @@ -580,14 +418,8 @@ public class BlazeOptionHandlerTest { } @Test - public void testParseOptions_residue_fixedPoint() { - makeFixedPointExpandingConfigOptionHandler(); - testParseOptions_residue(); - } - - @Test public void testParseOptions_residue_inPlace() { - makeInPlaceExpandingConfigOptionHandler(); + testParseOptions_residue(); } @@ -603,14 +435,8 @@ public class BlazeOptionHandlerTest { } @Test - public void testParseOptions_explicitOption_fixedPoint() { - makeFixedPointExpandingConfigOptionHandler(); - testParseOptions_explicitOption(); - } - - @Test public void testParseOptions_explicitOption_inPlace() { - makeInPlaceExpandingConfigOptionHandler(); + testParseOptions_explicitOption(); } @@ -635,14 +461,8 @@ public class BlazeOptionHandlerTest { } @Test - public void testParseOptions_rcOption_fixedPoint() { - makeFixedPointExpandingConfigOptionHandler(); - testParseOptions_rcOption(); - } - - @Test public void testParseOptions_rcOption_inPlace() { - makeInPlaceExpandingConfigOptionHandler(); + testParseOptions_rcOption(); } @@ -672,18 +492,13 @@ public class BlazeOptionHandlerTest { } @Test - public void testParseOptions_multipleRcs_fixedPoint() { - makeFixedPointExpandingConfigOptionHandler(); - testParseOptions_multipleRcs(); - } - - @Test public void testParseOptions_multipleRcs_inPlace() { - makeInPlaceExpandingConfigOptionHandler(); + testParseOptions_multipleRcs(); } - private void testParseOptions_multipleRcsWithMultipleCommands() { + @Test + public void testParseOptions_multipleRcsWithMultipleCommands_inPlace() { optionHandler.parseOptions( ImmutableList.of( "c0", @@ -717,18 +532,7 @@ public class BlazeOptionHandlerTest { } @Test - public void testParseOptions_multipleRcsWithMultipleCommands_fixedPoint() { - makeFixedPointExpandingConfigOptionHandler(); - testParseOptions_multipleRcsWithMultipleCommands(); - } - - @Test - public void testParseOptions_multipleRcsWithMultipleCommands_inPlace() { - makeInPlaceExpandingConfigOptionHandler(); - testParseOptions_multipleRcsWithMultipleCommands(); - } - - private void testParseOptions_rcOptionAndExplicit() { + public void testParseOptions_rcOptionAndExplicit_inPlace() { optionHandler.parseOptions( ImmutableList.of( "c0", @@ -748,18 +552,7 @@ public class BlazeOptionHandlerTest { } @Test - public void testParseOptions_rcOptionAndExplicit_fixedPoint() { - makeFixedPointExpandingConfigOptionHandler(); - testParseOptions_rcOptionAndExplicit(); - } - - @Test - public void testParseOptions_rcOptionAndExplicit_inPlace() { - makeInPlaceExpandingConfigOptionHandler(); - testParseOptions_rcOptionAndExplicit(); - } - - private void testParseOptions_multiCommandRcOptionAndExplicit() { + public void testParseOptions_multiCommandRcOptionAndExplicit_inPlace() { optionHandler.parseOptions( ImmutableList.of( "c0", @@ -785,18 +578,7 @@ public class BlazeOptionHandlerTest { } @Test - public void testParseOptions_multiCommandRcOptionAndExplicit_fixedPoint() { - makeFixedPointExpandingConfigOptionHandler(); - testParseOptions_multiCommandRcOptionAndExplicit(); - } - - @Test - public void testParseOptions_multiCommandRcOptionAndExplicit_inPlace() { - makeInPlaceExpandingConfigOptionHandler(); - testParseOptions_multiCommandRcOptionAndExplicit(); - } - - private void testParseOptions_multipleRcsWithMultipleCommandsPlusExplicitOption() { + public void testParseOptions_multipleRcsWithMultipleCommandsPlusExplicitOption_inPlace() { optionHandler.parseOptions( ImmutableList.of( "c0", @@ -831,18 +613,7 @@ public class BlazeOptionHandlerTest { } @Test - public void testParseOptions_multipleRcsWithMultipleCommandsPlusExplicitOption_fixedPoint() { - makeFixedPointExpandingConfigOptionHandler(); - testParseOptions_multipleRcsWithMultipleCommandsPlusExplicitOption(); - } - - @Test - public void testParseOptions_multipleRcsWithMultipleCommandsPlusExplicitOption_inPlace() { - makeInPlaceExpandingConfigOptionHandler(); - testParseOptions_multipleRcsWithMultipleCommandsPlusExplicitOption(); - } - - private void testParseOptions_explicitConfig() { + public void testParseOptions_explicitConfig_inPlace() { optionHandler.parseOptions( ImmutableList.of( "c0", @@ -860,24 +631,6 @@ public class BlazeOptionHandlerTest { + " 'c0' options: --test_multiple_string=rc", "Found applicable config definition c0:conf in file /somewhere/.blazerc: " + "--test_multiple_string=config"); - } - - @Test - public void testParseOptions_explicitConfig_fixedPoint() { - makeFixedPointExpandingConfigOptionHandler(); - testParseOptions_explicitConfig(); - - // "config" is lower priority (occurs earlier in the list) than "explicit" in the fix-point - // expansion, despite --config=conf occurring later. - TestOptions options = parser.getOptions(TestOptions.class); - assertThat(options).isNotNull(); - assertThat(options.testMultipleString).containsExactly("rc", "config", "explicit").inOrder(); - } - - @Test - public void testParseOptions_explicitConfig_inPlace() { - makeInPlaceExpandingConfigOptionHandler(); - testParseOptions_explicitConfig(); // "config" is expanded from --config=conf, which occurs last. TestOptions options = parser.getOptions(TestOptions.class); @@ -885,7 +638,8 @@ public class BlazeOptionHandlerTest { assertThat(options.testMultipleString).containsExactly("rc", "explicit", "config").inOrder(); } - private void testParseOptions_rcSpecifiedConfig() { + @Test + public void testParseOptions_rcSpecifiedConfig_inPlace() { optionHandler.parseOptions( ImmutableList.of( "c0", @@ -903,24 +657,6 @@ public class BlazeOptionHandlerTest { + " 'c0' options: --config=conf --test_multiple_string=rc", "Found applicable config definition c0:conf in file /somewhere/.blazerc: " + "--test_multiple_string=config"); - } - - @Test - public void testParseOptions_rcSpecifiedConfig_fixedPoint() { - makeFixedPointExpandingConfigOptionHandler(); - testParseOptions_rcSpecifiedConfig(); - - // "config" is higher priority (occurs later in the list) than "rc" in the fix-point - // expansion, despite --config=conf occurring before the explicit mention of "rc". - TestOptions options = parser.getOptions(TestOptions.class); - assertThat(options).isNotNull(); - assertThat(options.testMultipleString).containsExactly("rc", "config", "explicit").inOrder(); - } - - @Test - public void testParseOptions_rcSpecifiedConfig_inPlace() { - makeInPlaceExpandingConfigOptionHandler(); - testParseOptions_rcSpecifiedConfig(); // "config" is expanded from --config=conf, which occurs before the explicit mention of "rc". TestOptions options = parser.getOptions(TestOptions.class); @@ -928,7 +664,8 @@ public class BlazeOptionHandlerTest { assertThat(options.testMultipleString).containsExactly("config", "rc", "explicit").inOrder(); } - private void testParseOptions_recursiveConfig() { + @Test + public void testParseOptions_recursiveConfig_inPlace() { optionHandler.parseOptions( ImmutableList.of( "c0", @@ -953,25 +690,6 @@ public class BlazeOptionHandlerTest { + "--test_multiple_string=othercommon", "Found applicable config definition c0:other in file /somewhere/.blazerc: " + "--test_multiple_string=other"); - } - - @Test - public void testParseOptions_recursiveConfig_fixedPoint() { - makeFixedPointExpandingConfigOptionHandler(); - testParseOptions_recursiveConfig(); - - // The 2nd config, --config=other, is expanded after the config that added it. - TestOptions options = parser.getOptions(TestOptions.class); - assertThat(options).isNotNull(); - assertThat(options.testMultipleString) - .containsExactly("rc", "config1", "othercommon", "other", "explicit") - .inOrder(); - } - - @Test - public void testParseOptions_recursiveConfig_inPlace() { - makeInPlaceExpandingConfigOptionHandler(); - testParseOptions_recursiveConfig(); // The 2nd config, --config=other, is added by --config=conf after conf adds its own value. TestOptions options = parser.getOptions(TestOptions.class); @@ -981,7 +699,8 @@ public class BlazeOptionHandlerTest { .inOrder(); } - private void testParseOptions_recursiveConfigWithDifferentTokens() { + @Test + public void testParseOptions_recursiveConfigWithDifferentTokens_inPlace() { optionHandler.parseOptions( ImmutableList.of( "c0", @@ -993,33 +712,7 @@ public class BlazeOptionHandlerTest { "--rc_source=/somewhere/.blazerc", "--config=conf"), eventHandler); - } - - @Test - public void testParseOptions_recursiveConfigWithDifferentTokens_fixedPoint() { - makeFixedPointExpandingConfigOptionHandler(); - testParseOptions_recursiveConfigWithDifferentTokens(); - assertThat(eventHandler.getEvents()).isEmpty(); - assertThat(parser.getResidue()).isEmpty(); - assertThat(optionHandler.getRcfileNotes()) - .containsExactly( - "Reading rc options for 'c0' from /somewhere/.blazerc:\n" - + " 'c0' options: --test_multiple_string=rc", - "Found applicable config definition c0:conf in file /somewhere/.blazerc: " - + "--test_multiple_string=config1 --config other", - "Found applicable config definition c0:other in file /somewhere/.blazerc: " - + "--test_multiple_string=other"); - // The 2nd config, --config other, is expanded after the config that added it. - TestOptions options = parser.getOptions(TestOptions.class); - assertThat(options).isNotNull(); - assertThat(options.testMultipleString).containsExactly("rc", "config1", "other").inOrder(); - } - - @Test - public void testParseOptions_recursiveConfigWithDifferentTokens_inPlace() { - makeInPlaceExpandingConfigOptionHandler(); - testParseOptions_recursiveConfigWithDifferentTokens(); assertThat(eventHandler.getEvents()) .containsExactly( Event.error( @@ -1029,7 +722,8 @@ public class BlazeOptionHandlerTest { + "separate token, such as in the form '--config value'.")); } - private void parseComplexConfigOrderCommandLine() { + @Test + public void testParseOptions_complexConfigOrder_inPlace() { optionHandler.parseOptions( ImmutableList.of( "c0", @@ -1052,51 +746,6 @@ public class BlazeOptionHandlerTest { eventHandler); assertThat(eventHandler.getEvents()).isEmpty(); assertThat(parser.getResidue()).isEmpty(); - } - - @Test - public void testParseOptions_complexConfigOrder_fixedPoint() { - makeFixedPointExpandingConfigOptionHandler(); - parseComplexConfigOrderCommandLine(); - assertThat(optionHandler.getRcfileNotes()) - .containsExactly( - "Reading rc options for 'c0' from /somewhere/.blazerc:\n 'c0' options: " - + "--test_multiple_string=rc1 --config=foo --test_multiple_string=rc2", - "Found applicable config definition common:foo in file /somewhere/.blazerc: " - + "--test_multiple_string=foo1 --config=bar --test_multiple_string=foo2", - "Found applicable config definition common:baz in file /somewhere/.blazerc: " - + "--test_multiple_string=baz1", - "Found applicable config definition c0:foo in file /somewhere/.blazerc: " - + "--test_multiple_string=foo3 --test_multiple_string=foo4", - "Found applicable config definition c0:baz in file /somewhere/.blazerc: " - + "--test_multiple_string=baz2", - "Found applicable config definition common:bar in file /somewhere/.blazerc: " - + "--test_multiple_string=bar1", - "Found applicable config definition c0:bar in file /somewhere/.blazerc: " - + "--test_multiple_string=bar2"); - TestOptions options = parser.getOptions(TestOptions.class); - assertThat(options).isNotNull(); - assertThat(options.testMultipleString) - .containsExactly( - "rc1", - "rc2", - "foo1", - "foo2", - "baz1", - "foo3", - "foo4", - "baz2", - "bar1", - "bar2", - "explicit1", - "explicit2") - .inOrder(); - } - - @Test - public void testParseOptions_complexConfigOrder_inPlace() { - makeInPlaceExpandingConfigOptionHandler(); - parseComplexConfigOrderCommandLine(); assertThat(optionHandler.getRcfileNotes()) .containsExactly( "Reading rc options for 'c0' from /somewhere/.blazerc:\n 'c0' options: " @@ -1132,7 +781,8 @@ public class BlazeOptionHandlerTest { .inOrder(); } - private void parseConfigDoubleRecursionCommandLine() { + @Test + public void testParseOptions_repeatSubConfig_inPlace() { optionHandler.parseOptions( ImmutableList.of( "c0", @@ -1146,34 +796,6 @@ public class BlazeOptionHandlerTest { "--test_multiple_string=explicit"), eventHandler); assertThat(parser.getResidue()).isEmpty(); - } - - @Test - public void testParseOptions_repeatSubConfig_fixedPoint() { - makeFixedPointExpandingConfigOptionHandler(); - parseConfigDoubleRecursionCommandLine(); - assertThat(eventHandler.getEvents()).isEmpty(); - assertThat(optionHandler.getRcfileNotes()) - .containsExactly( - "Reading rc options for 'c0' from /somewhere/.blazerc:\n" - + " 'c0' options: --config=foo --test_multiple_string=rc", - "Found applicable config definition c0:foo in file /somewhere/.blazerc: " - + "--test_multiple_string=foo --config=bar --config=bar", - "Found applicable config definition c0:bar in file /somewhere/.blazerc: " - + "--test_multiple_string=bar"); - TestOptions options = parser.getOptions(TestOptions.class); - assertThat(options).isNotNull(); - // Bar is not repeated, it was already expanded once and the fixed point expansion - // does not attempt to expand configs a second time. - assertThat(options.testMultipleString) - .containsExactly("rc", "foo", "bar", "explicit") - .inOrder(); - } - - @Test - public void testParseOptions_repeatSubConfig_inPlace() { - makeInPlaceExpandingConfigOptionHandler(); - parseConfigDoubleRecursionCommandLine(); assertThat(eventHandler.getEvents()) .containsExactly( Event.warn( @@ -1197,7 +819,8 @@ public class BlazeOptionHandlerTest { .inOrder(); } - private void parseRepeatConfigs() { + @Test + public void testParseOptions_repeatConfig_inPlace() { optionHandler.parseOptions( ImmutableList.of( "c0", @@ -1212,31 +835,6 @@ public class BlazeOptionHandlerTest { "--config=bar"), eventHandler); assertThat(parser.getResidue()).isEmpty(); - } - - @Test - public void testParseOptions_repeatConfig_fixedPoint() { - makeFixedPointExpandingConfigOptionHandler(); - parseRepeatConfigs(); - assertThat(eventHandler.getEvents()).isEmpty(); - assertThat(optionHandler.getRcfileNotes()) - .containsExactly( - "Found applicable config definition c0:foo in file /somewhere/.blazerc: " - + "--test_multiple_string=foo --config=bar", - "Found applicable config definition c0:baz in file /somewhere/.blazerc: " - + "--test_multiple_string=baz", - "Found applicable config definition c0:bar in file /somewhere/.blazerc: " - + "--test_multiple_string=bar"); - TestOptions options = parser.getOptions(TestOptions.class); - assertThat(options).isNotNull(); - // Foo and bar are not repeated, despite repeat mentions. - assertThat(options.testMultipleString).containsExactly("foo", "baz", "bar").inOrder(); - } - - @Test - public void testParseOptions_repeatConfig_inPlace() { - makeInPlaceExpandingConfigOptionHandler(); - parseRepeatConfigs(); assertThat(eventHandler.getEvents()) .containsExactly( Event.warn( @@ -1264,7 +862,8 @@ public class BlazeOptionHandlerTest { .inOrder(); } - private void parseConfigCycleLength1CommandLine() { + @Test + public void testParseOptions_configCycleLength1_inPlace() { optionHandler.parseOptions( ImmutableList.of( "c0", @@ -1275,31 +874,6 @@ public class BlazeOptionHandlerTest { "--rc_source=/somewhere/.blazerc", "--test_multiple_string=explicit"), eventHandler); - } - - @Test - public void testParseOptions_configCycleLength1_fixedPoint() { - makeFixedPointExpandingConfigOptionHandler(); - parseConfigCycleLength1CommandLine(); - assertThat(eventHandler.getEvents()).isEmpty(); - assertThat(parser.getResidue()).isEmpty(); - assertThat(optionHandler.getRcfileNotes()) - .containsExactly( - "Reading rc options for 'c0' from /somewhere/.blazerc:\n" - + " 'c0' options: --config=foo --test_multiple_string=rc", - "Found applicable config definition c0:foo in file /somewhere/.blazerc: " - + "--test_multiple_string=foo --config=foo"); - TestOptions options = parser.getOptions(TestOptions.class); - assertThat(options).isNotNull(); - // The cycle is not expanded, since foo was already expanded once and the fixed point expansion - // does not attempt to expand configs a second time. - assertThat(options.testMultipleString).containsExactly("rc", "foo", "explicit").inOrder(); - } - - @Test - public void testParseOptions_configCycleLength1_inPlace() { - makeInPlaceExpandingConfigOptionHandler(); - parseConfigCycleLength1CommandLine(); assertThat(eventHandler.getEvents()) .contains( Event.error( @@ -1307,7 +881,8 @@ public class BlazeOptionHandlerTest { + "inheritance chain [foo]")); } - private void parseConfigCycleLength2CommandLine() { + @Test + public void testParseOptions_configCycleLength2_inPlace() { optionHandler.parseOptions( ImmutableList.of( "c0", @@ -1320,35 +895,6 @@ public class BlazeOptionHandlerTest { "--rc_source=/somewhere/.blazerc", "--test_multiple_string=explicit"), eventHandler); - } - - @Test - public void testParseOptions_configCycleLength2_fixedPoint() { - makeFixedPointExpandingConfigOptionHandler(); - parseConfigCycleLength2CommandLine(); - assertThat(eventHandler.getEvents()).isEmpty(); - assertThat(parser.getResidue()).isEmpty(); - assertThat(optionHandler.getRcfileNotes()) - .containsExactly( - "Reading rc options for 'c0' from /somewhere/.blazerc:\n" - + " 'c0' options: --config=foo --test_multiple_string=rc", - "Found applicable config definition c0:foo in file /somewhere/.blazerc: " - + "--test_multiple_string=foo --config=bar", - "Found applicable config definition c0:bar in file /somewhere/.blazerc: " - + "--test_multiple_string=bar --config=foo"); - TestOptions options = parser.getOptions(TestOptions.class); - assertThat(options).isNotNull(); - // The cycle is not expanded, since foo was already expanded once and the fixed point expansion - // does not attempt to expand configs a second time. - assertThat(options.testMultipleString) - .containsExactly("rc", "foo", "bar", "explicit") - .inOrder(); - } - - @Test - public void testParseOptions_configCycleLength2_inPlace() { - makeInPlaceExpandingConfigOptionHandler(); - parseConfigCycleLength2CommandLine(); assertThat(eventHandler.getEvents()) .contains( Event.error( @@ -1356,7 +902,8 @@ public class BlazeOptionHandlerTest { + "inheritance chain [foo, bar]")); } - private void recursivelyIncludedRepeatConfigCommandLine() { + @Test + public void testParseOptions_recursiveConfigWasAlreadyPresent_inPlace() { optionHandler.parseOptions( ImmutableList.of( "c0", @@ -1371,38 +918,6 @@ public class BlazeOptionHandlerTest { "--test_multiple_string=explicit"), eventHandler); assertThat(parser.getResidue()).isEmpty(); - } - - @Test - public void testParseOptions_recursiveConfigWasAlreadyPresent_fixedPoint() { - makeFixedPointExpandingConfigOptionHandler(); - recursivelyIncludedRepeatConfigCommandLine(); - assertThat(eventHandler.getEvents()).isEmpty(); - - // The 2nd config, --config=other, is expanded at the same time as --config=conf, since they are - // both initially present. The "common" definition is therefore first. other is not reexpanded - // when it is added by --config=conf, since it was already included. - assertThat(optionHandler.getRcfileNotes()) - .containsExactly( - "Reading rc options for 'c0' from /somewhere/.blazerc:\n" - + " 'c0' options: --config=other --config=conf --test_multiple_string=rc", - "Found applicable config definition common:other in file /somewhere/.blazerc: " - + "--test_multiple_string=othercommon", - "Found applicable config definition c0:conf in file /somewhere/.blazerc: " - + "--test_multiple_string=config1 --config=other", - "Found applicable config definition c0:other in file /somewhere/.blazerc: " - + "--test_multiple_string=other"); - TestOptions options = parser.getOptions(TestOptions.class); - assertThat(options).isNotNull(); - assertThat(options.testMultipleString) - .containsExactly("rc", "othercommon", "other", "config1", "explicit") - .inOrder(); - } - - @Test - public void testParseOptions_recursiveConfigWasAlreadyPresent_inPlace() { - makeInPlaceExpandingConfigOptionHandler(); - recursivelyIncludedRepeatConfigCommandLine(); assertThat(eventHandler.getEvents()) .containsExactly( Event.warn( @@ -1459,7 +974,8 @@ public class BlazeOptionHandlerTest { "--default_override=0:c0:lambda=--config=mu", "--default_override=0:c0:mu=--test_multiple_string=mu"); - private void testParseOptions_longChainOfConfigs_12long() { + @Test + public void testParseOptions_longChain_inPlace() { ImmutableList<String> args = ImmutableList.<String>builder() .add("c0") @@ -1503,19 +1019,6 @@ public class BlazeOptionHandlerTest { "alpha", "beta", "gamma", "delta", "epsilon", "zeta", "eta", "theta", "iota", "kappa", "lambda", "mu") .inOrder(); - } - - @Test - public void testParseOptions_longChain_FixedPoint() { - makeFixedPointExpandingConfigOptionHandler(); - testParseOptions_longChainOfConfigs_12long(); - assertThat(eventHandler.getEvents()).isEmpty(); - } - - @Test - public void testParseOptions_longChain_inPlace() { - makeInPlaceExpandingConfigOptionHandler(); - testParseOptions_longChainOfConfigs_12long(); // Expect only one warning, we don't want multiple warnings for the same chain. assertThat(eventHandler.getEvents()) .containsExactly( @@ -1540,24 +1043,8 @@ public class BlazeOptionHandlerTest { } @Test - public void testParseOptions_2LongChains_FixedPoint() { - makeFixedPointExpandingConfigOptionHandler(); - testParseOptions_twoLongChains(); - // In fixed point, the repetition --config=gamma does not led to a new expansion, but it does - // mean that gamma gets expanded in the first round, so the ordering is weird. - TestOptions options = parser.getOptions(TestOptions.class); - assertThat(options).isNotNull(); - assertThat(options.testMultipleString) - .containsExactly( - "alpha", "gamma", "beta", "delta", "epsilon", "zeta", "eta", "theta", "iota", "kappa", - "lambda", "mu") - .inOrder(); - assertThat(eventHandler.getEvents()).isEmpty(); - } - - @Test public void testParseOptions_2LongChains_inPlace() { - makeInPlaceExpandingConfigOptionHandler(); + testParseOptions_twoLongChains(); // Expect the second --config=gamma to have started a second chain, and get warnings about both. TestOptions options = parser.getOptions(TestOptions.class); @@ -1584,7 +1071,8 @@ public class BlazeOptionHandlerTest { + "counted twice and may lead to unexpected behavior.")); } - private void testWarningFlag() { + @Test + public void testWarningFlag_inPlace() { optionHandler.parseOptions( ImmutableList.of( "c0", @@ -1599,18 +1087,7 @@ public class BlazeOptionHandlerTest { } @Test - public void testWarningFlag_fixedPoint() { - makeFixedPointExpandingConfigOptionHandler(); - testWarningFlag(); - } - - @Test - public void testWarningFlag_inPlace() { - makeInPlaceExpandingConfigOptionHandler(); - testWarningFlag(); - } - - private void testWarningFlag_byConfig_notTriggered() { + public void testWarningFlag_byConfig_notTriggered_inPlace() { optionHandler.parseOptions( ImmutableList.of( "c0", @@ -1624,18 +1101,7 @@ public class BlazeOptionHandlerTest { } @Test - public void testWarningFlag_byConfig_notTriggered_fixedPoint() { - makeFixedPointExpandingConfigOptionHandler(); - testWarningFlag_byConfig_notTriggered(); - } - - @Test - public void testWarningFlag_byConfig_notTriggered_inPlace() { - makeInPlaceExpandingConfigOptionHandler(); - testWarningFlag_byConfig_notTriggered(); - } - - private void testWarningFlag_byConfig_triggered() { + public void testWarningFlag_byConfig_triggered_inPlace() { optionHandler.parseOptions( ImmutableList.of( "c0", @@ -1654,41 +1120,7 @@ public class BlazeOptionHandlerTest { } @Test - public void testWarningFlag_byConfig_triggered_fixedPoint() { - makeFixedPointExpandingConfigOptionHandler(); - testWarningFlag_byConfig_triggered(); - } - - @Test - public void testWarningFlag_byConfig_triggered_inPlace() { - makeInPlaceExpandingConfigOptionHandler(); - testWarningFlag_byConfig_triggered(); - } - - @Test - public void testConfigAfterExplicit_fixedPoint() { - makeFixedPointExpandingConfigOptionHandler(); - optionHandler.parseOptions( - ImmutableList.of( - "c0", - "--test_string=explicitValue", - "--config=conf", - "--default_override=0:c0:conf=--test_string=fromConf", - "--rc_source=/somewhere/.blazerc"), - eventHandler); - TestOptions parseResult = parser.getOptions(TestOptions.class); - assertThat(eventHandler.getEvents()).isEmpty(); - // The fact that --config=conf comes after the explicit value does not matter - assertThat(parseResult.testString).isEqualTo("explicitValue"); - assertThat(optionHandler.getRcfileNotes()) - .containsExactly( - "Found applicable config definition c0:conf in file /somewhere/.blazerc: " - + "--test_string=fromConf"); - } - - @Test public void testConfigAfterExplicit_inPlace() { - makeInPlaceExpandingConfigOptionHandler(); optionHandler.parseOptions( ImmutableList.of( "c0", @@ -1714,28 +1146,7 @@ public class BlazeOptionHandlerTest { } @Test - public void testExplicitOverridesConfig_fixedPoint() { - makeFixedPointExpandingConfigOptionHandler(); - optionHandler.parseOptions( - ImmutableList.of( - "c0", - "--config=conf", - "--test_string=explicitValue", - "--default_override=0:c0:conf=--test_string=fromConf", - "--rc_source=/somewhere/.blazerc"), - eventHandler); - TestOptions parseResult = parser.getOptions(TestOptions.class); - assertThat(eventHandler.getEvents()).isEmpty(); - assertThat(parseResult.testString).isEqualTo("explicitValue"); - assertThat(optionHandler.getRcfileNotes()) - .containsExactly( - "Found applicable config definition c0:conf in file /somewhere/.blazerc: " - + "--test_string=fromConf"); - } - - @Test public void testExplicitOverridesConfig_inPlace() { - makeInPlaceExpandingConfigOptionHandler(); optionHandler.parseOptions( ImmutableList.of( "c0", |