From a79697548a5506304006d476854f1d2688cc04f1 Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Mon, 29 Jul 2024 11:49:37 -0700 Subject: [PATCH] Fail on inapplicable flags on non-common commands expanded from configs This regressed in https://github.com/bazelbuild/bazel/commit/44d395338ab9a27596b0796c14076641f4cb2093. Fixes #22980 Closes #23105. PiperOrigin-RevId: 657276908 Change-Id: If2e88455a344082bfbec405c30c148c0d044adb6 --- .../build/lib/runtime/BlazeOptionHandler.java | 2 +- .../build/lib/runtime/ConfigExpander.java | 57 ++++++++++++------- .../common/options/ArgsPreProcessor.java | 3 +- .../common/options/OptionsParser.java | 43 +++++++++----- .../common/options/OptionsParserImpl.java | 37 ++++++------ .../options/ParamsFilePreProcessor.java | 18 ++++-- .../common/options/OptionsParserTest.java | 6 +- .../options/ParamsFilePreProcessorTest.java | 21 +++++-- ...ShellQuotedParamsFilePreProcessorTest.java | 31 ++++++---- .../UnquotedParamsFilePreProcessorTest.java | 11 +++- src/test/py/bazel/options_test.py | 27 +++++++++ 11 files changed, 172 insertions(+), 84 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/runtime/BlazeOptionHandler.java b/src/main/java/com/google/devtools/build/lib/runtime/BlazeOptionHandler.java index 9c862cb7019863..cb8e2556bdb358 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 @@ -85,7 +85,7 @@ public final class BlazeOptionHandler { // All options set on this pseudo command are inherited by all commands, with unrecognized options // being ignored as long as they are recognized by at least one (other) command. - private static final String COMMON_PSEUDO_COMMAND = "common"; + static final String COMMON_PSEUDO_COMMAND = "common"; private static final String BUILD_COMMAND = "build"; private static final ImmutableSet BUILD_COMMAND_ANCESTORS = 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 92fe5f1356c195..cfbb9cb24801fe 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 @@ -23,6 +23,7 @@ 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.OptionsParser.ArgAndFallbackData; import com.google.devtools.common.options.OptionsParsingException; import com.google.devtools.common.options.ParsedOptionDescription; import java.util.ArrayList; @@ -108,19 +109,19 @@ static void expandConfigOptions( // them, to preserve order. for (ParsedOptionDescription configInstance : configInstances) { String configValueToExpand = (String) configInstance.getConvertedValue(); - List expansion = + var expansion = getExpansion( eventHandler, commandToRcArgs, commandsToParse, configValueToExpand, - rcFileNotesConsumer); + rcFileNotesConsumer, + fallbackData); var ignoredArgs = optionsParser.parseArgsAsExpansionOfOption( configInstance, String.format("expanded from --config=%s", configValueToExpand), - expansion, - fallbackData); + expansion); if (!ignoredArgs.isEmpty()) { rcFileNotesConsumer.accept( String.format( @@ -134,18 +135,25 @@ static void expandConfigOptions( optionsParser.getOptionValueDescription("enable_platform_specific_config"); if (shouldEnablePlatformSpecificConfig( enablePlatformSpecificConfigDescription, commandToRcArgs, commandsToParse)) { - List expansion = + var expansion = getExpansion( eventHandler, commandToRcArgs, commandsToParse, getPlatformName(), - rcFileNotesConsumer); - optionsParser.parseArgsAsExpansionOfOption( - Iterables.getOnlyElement(enablePlatformSpecificConfigDescription.getCanonicalInstances()), - String.format("enabled by --enable_platform_specific_config"), - expansion, - fallbackData); + rcFileNotesConsumer, + fallbackData); + ParsedOptionDescription optionToExpand = + Iterables.getOnlyElement(enablePlatformSpecificConfigDescription.getCanonicalInstances()); + var ignoredArgs = + optionsParser.parseArgsAsExpansionOfOption( + optionToExpand, "enabled by --enable_platform_specific_config", expansion); + if (!ignoredArgs.isEmpty()) { + rcFileNotesConsumer.accept( + String.format( + "Ignored as unsupported by '%s': %s", + currentCommand, Joiner.on(' ').join(ignoredArgs))); + } } // At this point, we've expanded everything, identify duplicates, if any, to warn about @@ -168,24 +176,26 @@ static void expandConfigOptions( } } - private static List getExpansion( + private static List getExpansion( EventHandler eventHandler, ListMultimap commandToRcArgs, List commandsToParse, String configToExpand, - Consumer rcFileNotesConsumer) + Consumer rcFileNotesConsumer, + @Nullable OpaqueOptionsData fallbackData) throws OptionsParsingException { LinkedHashSet configAncestorSet = new LinkedHashSet<>(); configAncestorSet.add(configToExpand); List longestChain = new ArrayList<>(); - List finalExpansion = + List finalExpansion = getExpansion( commandToRcArgs, commandsToParse, configAncestorSet, configToExpand, longestChain, - rcFileNotesConsumer); + rcFileNotesConsumer, + fallbackData); // 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 @@ -212,15 +222,16 @@ private static List getExpansion( * should not be in the parents list of the second bar. * @param longestChain will be populated with the longest inheritance chain of configs. */ - private static List getExpansion( + private static List getExpansion( ListMultimap commandToRcArgs, List commandsToParse, LinkedHashSet configAncestorSet, String configToExpand, List longestChain, - Consumer rcFileNotesConsumer) + Consumer rcFileNotesConsumer, + @Nullable OpaqueOptionsData fallbackData) throws OptionsParsingException { - List expansion = new ArrayList<>(); + List 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. @@ -236,7 +247,12 @@ private static List getExpansion( // 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.getArgs()) { - expansion.add(arg); + expansion.add( + new ArgAndFallbackData( + arg, + commandToParse.equals(BlazeOptionHandler.COMMON_PSEUDO_COMMAND) + ? fallbackData + : null)); if (arg.length() >= 8 && arg.substring(0, 8).equals("--config")) { // We have a config. Because we don't want to worry about formatting, // we will only accept --config=value, and will not accept value on a following line. @@ -272,7 +288,8 @@ private static List getExpansion( extendedConfigAncestorSet, newConfigValue, longestChain, - rcFileNotesConsumer)); + rcFileNotesConsumer, + fallbackData)); } } } diff --git a/src/main/java/com/google/devtools/common/options/ArgsPreProcessor.java b/src/main/java/com/google/devtools/common/options/ArgsPreProcessor.java index 00e8033debd029..1208d0f88e33cc 100644 --- a/src/main/java/com/google/devtools/common/options/ArgsPreProcessor.java +++ b/src/main/java/com/google/devtools/common/options/ArgsPreProcessor.java @@ -13,10 +13,11 @@ // limitations under the License. package com.google.devtools.common.options; +import com.google.devtools.common.options.OptionsParser.ArgAndFallbackData; import java.util.List; /** Defines a preprocessing service for the "args" string list that is executed before parsing. */ @FunctionalInterface interface ArgsPreProcessor { - List preProcess(List args) throws OptionsParsingException; + List preProcess(List args) throws OptionsParsingException; } 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 47a650b894115c..e38ff1d60d36b9 100644 --- a/src/main/java/com/google/devtools/common/options/OptionsParser.java +++ b/src/main/java/com/google/devtools/common/options/OptionsParser.java @@ -28,6 +28,7 @@ import com.google.common.collect.ImmutableSortedMap; import com.google.common.collect.Iterables; import com.google.common.collect.ListMultimap; +import com.google.common.collect.Lists; import com.google.common.collect.MoreCollectors; import com.google.common.escape.Escaper; import com.google.devtools.build.lib.util.Pair; @@ -709,7 +710,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, null); + parseWithSourceFunction(priority, o -> source, args, /* fallbackData= */ null); } /** @@ -725,9 +726,6 @@ public void parse(OptionPriority.PriorityCategory priority, String source, List< * each option will be given an index to track its position. If parse() has already been * called at this priority, the indexing will continue where it left off, to keep ordering. * @param sourceFunction a function that maps option names to the source of the option. - * @param fallbackData if provided, the full collection of options that should be parsed and - * ignored without raising an error if they are not recognized by the options classes - * registered with this parser. * @param args the arg list to parse. Each element might be an option, a value linked to an * option, or residue. * @return a list of options and values that were parsed but ignored due to only resolving against @@ -743,7 +741,8 @@ public ImmutableList parseWithSourceFunction( Preconditions.checkNotNull(priority); Preconditions.checkArgument(priority != OptionPriority.PriorityCategory.DEFAULT); OptionsParserImplResult optionsParserImplResult = - impl.parse(priority, sourceFunction, args, (OptionsData) fallbackData); + impl.parse( + priority, sourceFunction, ArgAndFallbackData.wrapWithFallbackData(args, fallbackData)); addResidueFromResult(optionsParserImplResult); aliases.putAll(optionsParserImplResult.aliases); return optionsParserImplResult.ignoredArgs; @@ -757,19 +756,15 @@ public ImmutableList parseWithSourceFunction( * will have the same priority as this option. * @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. + * be in the following argument. Each arg is optionally annotated with the full collection of + * options that should be parsed and ignored without raising an error if they are not + * recognized by the options classes registered with this parser. * @return a list of options and values that were parsed but ignored due to only resolving against * the fallback data */ @CanIgnoreReturnValue public ImmutableList parseArgsAsExpansionOfOption( - ParsedOptionDescription optionToExpand, - String source, - List args, - @Nullable OpaqueOptionsData fallbackData) + ParsedOptionDescription optionToExpand, String source, List args) throws OptionsParsingException { Preconditions.checkNotNull( optionToExpand, "Option for expansion not specified for arglist %s", args); @@ -779,8 +774,7 @@ public ImmutableList parseArgsAsExpansionOfOption( "Priority cannot be default, which was specified for arglist %s", args); OptionsParserImplResult optionsParserImplResult = - impl.parseArgsAsExpansionOfOption( - optionToExpand, o -> source, args, (OptionsData) fallbackData); + impl.parseArgsAsExpansionOfOption(optionToExpand, o -> source, args); addResidueFromResult(optionsParserImplResult); return optionsParserImplResult.ignoredArgs; } @@ -935,4 +929,23 @@ public static boolean getUsesOnlyCoreTypes(Class optionsC OptionsData data = OptionsParser.getOptionsDataInternal(optionsClass); return data.getUsesOnlyCoreTypes(optionsClass); } + + /** + * A container for an arg and associated options that should be silently ignored when parsed but + * not recognized by the current command. + */ + public static final class ArgAndFallbackData { + public final String arg; + @Nullable final OptionsData fallbackData; + + public ArgAndFallbackData(String arg, @Nullable OpaqueOptionsData fallbackData) { + this.arg = Preconditions.checkNotNull(arg); + this.fallbackData = (OptionsData) fallbackData; + } + + public static List wrapWithFallbackData( + List args, @Nullable OpaqueOptionsData fallbackData) { + return Lists.transform(args, arg -> new ArgAndFallbackData(arg, fallbackData)); + } + } } 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 fb3fe516d54f1b..4fc6af437e98a8 100644 --- a/src/main/java/com/google/devtools/common/options/OptionsParserImpl.java +++ b/src/main/java/com/google/devtools/common/options/OptionsParserImpl.java @@ -26,6 +26,7 @@ import com.google.common.collect.Iterators; import com.google.devtools.common.options.OptionPriority.PriorityCategory; import com.google.devtools.common.options.OptionValueDescription.ExpansionBundle; +import com.google.devtools.common.options.OptionsParser.ArgAndFallbackData; import com.google.devtools.common.options.OptionsParser.OptionDescription; import com.google.errorprone.annotations.CanIgnoreReturnValue; import com.google.errorprone.annotations.Keep; @@ -452,17 +453,10 @@ List getSkippedArgs() { OptionsParserImplResult parse( PriorityCategory priorityCat, Function sourceFunction, - List args, - OptionsData fallbackData) + List args) throws OptionsParsingException { OptionsParserImplResult optionsParserImplResult = - parse( - nextPriorityPerPriorityCategory.get(priorityCat), - sourceFunction, - null, - null, - args, - fallbackData); + parse(nextPriorityPerPriorityCategory.get(priorityCat), sourceFunction, null, null, args); nextPriorityPerPriorityCategory.put(priorityCat, optionsParserImplResult.nextPriority); return optionsParserImplResult; } @@ -480,16 +474,19 @@ private OptionsParserImplResult parse( Function sourceFunction, ParsedOptionDescription implicitDependent, ParsedOptionDescription expandedFrom, - List args, - OptionsData fallbackData) + List args) throws OptionsParsingException { List unparsedArgs = new ArrayList<>(); List unparsedPostDoubleDashArgs = new ArrayList<>(); List ignoredArgs = new ArrayList<>(); - Iterator argsIterator = argsPreProcessor.preProcess(args).iterator(); - while (argsIterator.hasNext()) { - String arg = argsIterator.next(); + Iterator argsAndFallbackDataIterator = + argsPreProcessor.preProcess(args).iterator(); + Iterator argsIterator = Iterators.transform(argsAndFallbackDataIterator, a -> a.arg); + while (argsAndFallbackDataIterator.hasNext()) { + ArgAndFallbackData argAndFallbackData = argsAndFallbackDataIterator.next(); + String arg = argAndFallbackData.arg; + @Nullable OptionsData fallbackData = argAndFallbackData.fallbackData; if (!arg.startsWith("-")) { unparsedArgs.add(arg); @@ -597,16 +594,14 @@ public List getResidue() { OptionsParserImplResult parseArgsAsExpansionOfOption( ParsedOptionDescription optionToExpand, Function sourceFunction, - List args, - OptionsData fallbackData) + List args) throws OptionsParsingException { return parse( OptionPriority.getChildPriority(optionToExpand.getPriority()), sourceFunction, null, optionToExpand, - args, - fallbackData); + args); } /** @@ -647,7 +642,8 @@ void setOptionValueAtSpecificPriorityWithoutExpansion( } /** Takes care of tracking the parsed option's value in relation to other options. */ - private void handleNewParsedOption(ParsedOptionDescription parsedOption, OptionsData fallbackData) + private void handleNewParsedOption( + ParsedOptionDescription parsedOption, @Nullable OptionsData fallbackData) throws OptionsParsingException { OptionDefinition optionDefinition = parsedOption.getOptionDefinition(); ExpansionBundle expansionBundle = setOptionValue(parsedOption); @@ -660,8 +656,7 @@ private void handleNewParsedOption(ParsedOptionDescription parsedOption, Options o -> expansionBundle.sourceOfExpansionArgs, optionDefinition.hasImplicitRequirements() ? parsedOption : null, optionDefinition.isExpansionOption() ? parsedOption : null, - expansionBundle.expansionArgs, - fallbackData); + ArgAndFallbackData.wrapWithFallbackData(expansionBundle.expansionArgs, fallbackData)); if (!optionsParserImplResult.getResidue().isEmpty()) { // Throw an assertion here, because this indicates an error in the definition of this diff --git a/src/main/java/com/google/devtools/common/options/ParamsFilePreProcessor.java b/src/main/java/com/google/devtools/common/options/ParamsFilePreProcessor.java index 87f87f6118e634..c381d18ff652b4 100644 --- a/src/main/java/com/google/devtools/common/options/ParamsFilePreProcessor.java +++ b/src/main/java/com/google/devtools/common/options/ParamsFilePreProcessor.java @@ -13,6 +13,8 @@ // limitations under the License. package com.google.devtools.common.options; +import com.google.common.collect.Lists; +import com.google.devtools.common.options.OptionsParser.ArgAndFallbackData; import java.io.IOException; import java.nio.file.FileSystem; import java.nio.file.Path; @@ -49,18 +51,22 @@ public abstract class ParamsFilePreProcessor implements ArgsPreProcessor { * @throws OptionsParsingException if the path does not exist. */ @Override - public List preProcess(List args) throws OptionsParsingException { - if (!args.isEmpty() && args.get(0).startsWith("@")) { + public List preProcess(List args) + throws OptionsParsingException { + if (!args.isEmpty() && args.get(0).arg.startsWith("@")) { if (args.size() > 1) { throw new OptionsParsingException( - String.format(TOO_MANY_ARGS_ERROR_MESSAGE_FORMAT, args), args.get(0)); + String.format( + TOO_MANY_ARGS_ERROR_MESSAGE_FORMAT, + Lists.transform(args, argAndFallbackData -> argAndFallbackData.arg)), + args.get(0).arg); } - Path path = fs.getPath(args.get(0).substring(1)); + Path path = fs.getPath(args.get(0).arg.substring(1)); try { - return parse(path); + return ArgAndFallbackData.wrapWithFallbackData(parse(path), args.get(0).fallbackData); } catch (RuntimeException | IOException e) { throw new OptionsParsingException( - String.format(ERROR_MESSAGE_FORMAT, path, e.getMessage()), args.get(0), e); + String.format(ERROR_MESSAGE_FORMAT, path, e.getMessage()), args.get(0).arg, e); } } return args; 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 64d78bb1b18dfe..f44e9c1679f4bd 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.ArgAndFallbackData; import com.google.devtools.common.options.OptionsParser.ConstructionException; import java.util.ArrayList; import java.util.Collections; @@ -370,8 +371,9 @@ public void parseArgsAsExpansionOfOptionThrowsExceptionIfResidueIsNotAllowed() t parser.parseArgsAsExpansionOfOption( optionToExpand, "source", - ImmutableList.of("--underlying=direct_value", "residue", "in", "expansion"), - /* fallbackData= */ null)); + ArgAndFallbackData.wrapWithFallbackData( + ImmutableList.of("--underlying=direct_value", "residue", "in", "expansion"), + /* fallbackData= */ null))); assertThat(parser.getResidue()).isNotEmpty(); assertThat(e).hasMessageThat().isEqualTo("Unrecognized arguments: residue in expansion"); } diff --git a/src/test/java/com/google/devtools/common/options/ParamsFilePreProcessorTest.java b/src/test/java/com/google/devtools/common/options/ParamsFilePreProcessorTest.java index 700c34f7098c3c..18d58607fa7a2c 100644 --- a/src/test/java/com/google/devtools/common/options/ParamsFilePreProcessorTest.java +++ b/src/test/java/com/google/devtools/common/options/ParamsFilePreProcessorTest.java @@ -17,7 +17,9 @@ import static org.junit.Assert.assertThrows; import com.google.common.collect.ImmutableList; +import com.google.common.collect.Lists; import com.google.common.jimfs.Jimfs; +import com.google.devtools.common.options.OptionsParser.ArgAndFallbackData; import java.io.IOException; import java.nio.file.FileSystem; import java.nio.file.Path; @@ -58,21 +60,21 @@ public void setup() { @Test public void testNoArgs() throws OptionsParsingException { - List args = paramsFilePreProcessor.preProcess(ImmutableList.of()); + List args = preProcess(paramsFilePreProcessor, ImmutableList.of()); assertThat(args).isEmpty(); } @Test public void testNoParamsFile() throws OptionsParsingException { List rawArgs = ImmutableList.of("--foo", "foo val"); - List args = paramsFilePreProcessor.preProcess(rawArgs); + List args = preProcess(paramsFilePreProcessor, rawArgs); assertThat(args).containsExactlyElementsIn(rawArgs).inOrder(); } @Test public void testParamsFileNotFirst() throws OptionsParsingException { List rawArgs = ImmutableList.of("--foo", "foo val", "@paramsFile"); - List args = paramsFilePreProcessor.preProcess(rawArgs); + List args = preProcess(paramsFilePreProcessor, rawArgs); assertThat(args).containsExactlyElementsIn(rawArgs).inOrder(); } @@ -81,7 +83,7 @@ public void testTooManyArgs() throws OptionsParsingException { List rawArgs = ImmutableList.of("@paramsFile", "--foo", "foo val"); OptionsParsingException expected = assertThrows( - OptionsParsingException.class, () -> paramsFilePreProcessor.preProcess(rawArgs)); + OptionsParsingException.class, () -> preProcess(paramsFilePreProcessor, rawArgs)); assertThat(expected) .hasMessageThat() .isEqualTo( @@ -101,7 +103,7 @@ protected List parse(Path paramsFile) throws IOException, OptionsParsing OptionsParsingException expected = assertThrows( OptionsParsingException.class, - () -> exceptionParser.preProcess(ImmutableList.of("@" + paramsFileName))); + () -> preProcess(exceptionParser, ImmutableList.of("@" + paramsFileName))); assertThat(expected) .hasMessageThat() .isEqualTo( @@ -113,7 +115,14 @@ protected List parse(Path paramsFile) throws IOException, OptionsParsing @Test public void testParamsFile() throws OptionsParsingException { - List args = paramsFilePreProcessor.preProcess(ImmutableList.of("@paramsFile")); + List args = preProcess(paramsFilePreProcessor, ImmutableList.of("@paramsFile")); assertThat(args).containsExactlyElementsIn(PARAM_FILE_ARGS).inOrder(); } + + private static List preProcess(ParamsFilePreProcessor preProcessor, List args) + throws OptionsParsingException { + return Lists.transform( + preProcessor.preProcess(ArgAndFallbackData.wrapWithFallbackData(args, null)), + argAndFallbackData -> argAndFallbackData.arg); + } } diff --git a/src/test/java/com/google/devtools/common/options/ShellQuotedParamsFilePreProcessorTest.java b/src/test/java/com/google/devtools/common/options/ShellQuotedParamsFilePreProcessorTest.java index d9aed4892a96a9..515ff0ddc18e59 100644 --- a/src/test/java/com/google/devtools/common/options/ShellQuotedParamsFilePreProcessorTest.java +++ b/src/test/java/com/google/devtools/common/options/ShellQuotedParamsFilePreProcessorTest.java @@ -17,7 +17,9 @@ import static org.junit.Assert.assertThrows; import com.google.common.collect.ImmutableList; +import com.google.common.collect.Lists; import com.google.common.jimfs.Jimfs; +import com.google.devtools.common.options.OptionsParser.ArgAndFallbackData; import java.io.IOException; import java.nio.charset.StandardCharsets; import java.nio.file.FileSystem; @@ -54,7 +56,7 @@ public void testUnquotedWhitespaceSeparated() throws IOException, OptionsParsing ImmutableList.of("arg1 arg2 arg3\targ4\\ arg5"), StandardCharsets.UTF_8, StandardOpenOption.CREATE); - List args = paramsFilePreProcessor.preProcess(ImmutableList.of("@" + paramsFile)); + List args = preProcess(paramsFilePreProcessor, ImmutableList.of("@" + paramsFile)); assertThat(args).containsExactly("arg1", "arg2", "", "arg3", "arg4\\", "arg5").inOrder(); } @@ -65,7 +67,7 @@ public void testUnquotedNewlineSeparated() throws IOException, OptionsParsingExc ImmutableList.of("arg1\narg2\rarg3\r\narg4\n\rarg5"), StandardCharsets.UTF_8, StandardOpenOption.CREATE); - List args = paramsFilePreProcessor.preProcess(ImmutableList.of("@" + paramsFile)); + List args = preProcess(paramsFilePreProcessor, ImmutableList.of("@" + paramsFile)); assertThat(args).containsExactly("arg1", "arg2", "arg3", "arg4", "", "arg5").inOrder(); } @@ -76,7 +78,7 @@ public void testQuotedWhitespaceSeparated() throws IOException, OptionsParsingEx ImmutableList.of("'arg1' 'arg2' '' 'arg3'\t'arg4'\\ 'arg5'"), StandardCharsets.UTF_8, StandardOpenOption.CREATE); - List args = paramsFilePreProcessor.preProcess(ImmutableList.of("@" + paramsFile)); + List args = preProcess(paramsFilePreProcessor, ImmutableList.of("@" + paramsFile)); assertThat(args).containsExactly("arg1", "arg2", "", "arg3", "arg4\\", "arg5").inOrder(); } @@ -87,7 +89,7 @@ public void testQuotedNewlineSeparated() throws IOException, OptionsParsingExcep ImmutableList.of("'arg1'\n'arg2'\r'arg3'\r\n'arg4'\n''\r'arg5'"), StandardCharsets.UTF_8, StandardOpenOption.CREATE); - List args = paramsFilePreProcessor.preProcess(ImmutableList.of("@" + paramsFile)); + List args = preProcess(paramsFilePreProcessor, ImmutableList.of("@" + paramsFile)); assertThat(args).containsExactly("arg1", "arg2", "arg3", "arg4", "", "arg5").inOrder(); } @@ -98,7 +100,7 @@ public void testQuotedContainingWhitespace() throws IOException, OptionsParsingE ImmutableList.of("'arg1 arg2 arg3\targ4'"), StandardCharsets.UTF_8, StandardOpenOption.CREATE); - List args = paramsFilePreProcessor.preProcess(ImmutableList.of("@" + paramsFile)); + List args = preProcess(paramsFilePreProcessor, ImmutableList.of("@" + paramsFile)); assertThat(args).containsExactly("arg1 arg2 arg3\targ4").inOrder(); } @@ -109,7 +111,7 @@ public void testQuotedContainingNewline() throws IOException, OptionsParsingExce ImmutableList.of("'arg1\narg2\rarg3\r\narg4\n\rarg5'"), StandardCharsets.UTF_8, StandardOpenOption.CREATE); - List args = paramsFilePreProcessor.preProcess(ImmutableList.of("@" + paramsFile)); + List args = preProcess(paramsFilePreProcessor, ImmutableList.of("@" + paramsFile)); assertThat(args).containsExactly("arg1\narg2\rarg3\r\narg4\n\rarg5").inOrder(); } @@ -120,7 +122,7 @@ public void testQuotedContainingQuotes() throws IOException, OptionsParsingExcep ImmutableList.of("'ar'\\''g1'", "'a'\\''rg'\\''2'"), StandardCharsets.UTF_8, StandardOpenOption.CREATE); - List args = paramsFilePreProcessor.preProcess(ImmutableList.of("@" + paramsFile)); + List args = preProcess(paramsFilePreProcessor, ImmutableList.of("@" + paramsFile)); assertThat(args).containsExactly("ar'g1", "a'rg'2").inOrder(); } @@ -131,7 +133,7 @@ public void testPartialQuoting() throws IOException, OptionsParsingException { ImmutableList.of("ar'g1 ar'g2 arg3"), StandardCharsets.UTF_8, StandardOpenOption.CREATE); - List args = paramsFilePreProcessor.preProcess(ImmutableList.of("@" + paramsFile)); + List args = preProcess(paramsFilePreProcessor, ImmutableList.of("@" + paramsFile)); assertThat(args).containsExactly("arg1 arg2", "arg3").inOrder(); } @@ -145,7 +147,7 @@ public void testUnfinishedQuote() throws IOException, OptionsParsingException { OptionsParsingException expected = assertThrows( OptionsParsingException.class, - () -> paramsFilePreProcessor.preProcess(ImmutableList.of("@" + paramsFile))); + () -> preProcess(paramsFilePreProcessor, ImmutableList.of("@" + paramsFile))); assertThat(expected) .hasMessageThat() .isEqualTo( @@ -162,7 +164,14 @@ public void testUnquotedEscapedQuote() throws IOException, OptionsParsingExcepti ImmutableList.of("ar\\'g1'"), StandardCharsets.UTF_8, StandardOpenOption.CREATE); - List args = paramsFilePreProcessor.preProcess(ImmutableList.of("@" + paramsFile)); + List args = preProcess(paramsFilePreProcessor, ImmutableList.of("@" + paramsFile)); assertThat(args).containsExactly("ar\\g1").inOrder(); } -} \ No newline at end of file + + private static List preProcess(ParamsFilePreProcessor preProcessor, List args) + throws OptionsParsingException { + return Lists.transform( + preProcessor.preProcess(ArgAndFallbackData.wrapWithFallbackData(args, null)), + argAndFallbackData -> argAndFallbackData.arg); + } +} diff --git a/src/test/java/com/google/devtools/common/options/UnquotedParamsFilePreProcessorTest.java b/src/test/java/com/google/devtools/common/options/UnquotedParamsFilePreProcessorTest.java index d07c0feb30029c..c3c97bbd1c65e7 100644 --- a/src/test/java/com/google/devtools/common/options/UnquotedParamsFilePreProcessorTest.java +++ b/src/test/java/com/google/devtools/common/options/UnquotedParamsFilePreProcessorTest.java @@ -16,7 +16,9 @@ import static com.google.common.truth.Truth.assertThat; import com.google.common.collect.ImmutableList; +import com.google.common.collect.Lists; import com.google.common.jimfs.Jimfs; +import com.google.devtools.common.options.OptionsParser.ArgAndFallbackData; import java.io.IOException; import java.nio.charset.StandardCharsets; import java.nio.file.FileSystem; @@ -51,9 +53,16 @@ public void testNewlines() throws IOException, OptionsParsingException { ImmutableList.of("arg1\narg2\rarg3\r\narg4 arg5\targ6\n\rarg7\\ arg8"), StandardCharsets.UTF_8, StandardOpenOption.CREATE); - List args = paramsFilePreProcessor.preProcess(ImmutableList.of("@" + paramsFile)); + List args = preProcess(paramsFilePreProcessor, ImmutableList.of("@" + paramsFile)); assertThat(args) .containsExactly("arg1", "arg2", "arg3", "arg4 arg5\targ6", "", "arg7\\ arg8") .inOrder(); } + + private static List preProcess(ParamsFilePreProcessor preProcessor, List args) + throws OptionsParsingException { + return Lists.transform( + preProcessor.preProcess(ArgAndFallbackData.wrapWithFallbackData(args, null)), + argAndFallbackData -> argAndFallbackData.arg); + } } diff --git a/src/test/py/bazel/options_test.py b/src/test/py/bazel/options_test.py index 074e5de65b3bc0..cb512f2bbc9450 100644 --- a/src/test/py/bazel/options_test.py +++ b/src/test/py/bazel/options_test.py @@ -282,6 +282,33 @@ def testCommonPseudoCommand_allowResidueFalseCommandIgnoresStarlarkOptions( # Check that version doesn't fail. self.RunBazel(["version"]) + def testConfigExpansion_failsOnUnsupportedFlag(self): + self.ScratchFile("MODULE.bazel") + self.ScratchFile("BUILD.bazel") + self.ScratchFile( + ".bazelrc", + [ + "build:abc --copt", + "build:abc -DFOO", + "common:abc --noverbose_test_summary", + "common:abc --verbose_test_summary", + "build:abc --copt", + "build:abc -DFOO", + "build:abc --verbose_test_summary", + "common:def --config=abc", + ], + ) + + exit_code, _, stderr = self.RunBazel( + ["build", "--announce_rc", "--config=def", "//..."], allow_failure=True + ) + self.AssertExitCode(exit_code, 2, stderr) + self.assertIn( + "ERROR: --verbose_test_summary :: Unrecognized option:" + " --verbose_test_summary", + stderr, + ) + if __name__ == "__main__": absltest.main()