diff options
4 files changed, 69 insertions, 42 deletions
diff --git a/src/main/java/com/google/devtools/build/lib/util/OptionsUtils.java b/src/main/java/com/google/devtools/build/lib/util/OptionsUtils.java index bae41d869f..c94863ed86 100644 --- a/src/main/java/com/google/devtools/build/lib/util/OptionsUtils.java +++ b/src/main/java/com/google/devtools/build/lib/util/OptionsUtils.java @@ -17,8 +17,6 @@ package com.google.devtools.build.lib.util; import com.google.common.collect.ImmutableList; import com.google.devtools.build.lib.vfs.PathFragment; import com.google.devtools.common.options.Converter; -import com.google.devtools.common.options.Converters; -import com.google.devtools.common.options.OptionsParsingException; import com.google.devtools.common.options.OptionsProvider; import com.google.devtools.common.options.ParsedOptionDescription; import java.util.ArrayList; @@ -43,23 +41,7 @@ public final class OptionsUtils { if (result.length() != 0) { result.append(' '); } - String value = option.getUnconvertedValue(); - if (option.isBooleanOption()) { - boolean isEnabled = false; - try { - isEnabled = new Converters.BooleanConverter().convert(value); - } catch (OptionsParsingException e) { - throw new RuntimeException("Unexpected parsing exception", e); - } - result - .append(isEnabled ? "--" : "--no") - .append(option.getOptionDefinition().getOptionName()); - } else { - result.append("--").append(option.getOptionDefinition().getOptionName()); - if (value != null) { // Can be null for Void options. - result.append("=").append(ShellEscaper.escapeString(value)); - } - } + result.append(option.getCanonicalFormWithValueEscaper(ShellEscaper::escapeString)); } return result.toString(); } @@ -82,22 +64,7 @@ public final class OptionsUtils { if (option.isHidden()) { continue; } - String value = option.getUnconvertedValue(); - if (option.isBooleanOption()) { - boolean isEnabled = false; - try { - isEnabled = new Converters.BooleanConverter().convert(value); - } catch (OptionsParsingException e) { - throw new RuntimeException("Unexpected parsing exception", e); - } - builder.add((isEnabled ? "--" : "--no") + option.getOptionDefinition().getOptionName()); - } else { - String optionString = "--" + option.getOptionDefinition().getOptionName(); - if (value != null) { // Can be null for Void options. - optionString += "=" + value; - } - builder.add(optionString); - } + builder.add(option.getCanonicalForm()); } return builder.build(); } 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 176d51e66a..221dcf030f 100644 --- a/src/main/java/com/google/devtools/common/options/OptionsParserImpl.java +++ b/src/main/java/com/google/devtools/common/options/OptionsParserImpl.java @@ -158,12 +158,7 @@ class OptionsParserImpl { }) // Ignore expansion options. .filter(value -> !value.getOptionDefinition().isExpansionOption()) - .map( - value -> - "--" - + value.getOptionDefinition().getOptionName() - + "=" - + value.getUnconvertedValue()) + .map(ParsedOptionDescription::getDeprecatedCanonicalForm) .collect(toCollection(ArrayList::new)); } diff --git a/src/main/java/com/google/devtools/common/options/ParsedOptionDescription.java b/src/main/java/com/google/devtools/common/options/ParsedOptionDescription.java index 0910579541..1f43172fb2 100644 --- a/src/main/java/com/google/devtools/common/options/ParsedOptionDescription.java +++ b/src/main/java/com/google/devtools/common/options/ParsedOptionDescription.java @@ -15,6 +15,7 @@ package com.google.devtools.common.options; import com.google.common.collect.ImmutableList; +import java.util.function.Function; import javax.annotation.Nullable; /** @@ -49,6 +50,46 @@ public final class ParsedOptionDescription { return commandLineForm; } + public String getCanonicalForm() { + return getCanonicalFormWithValueEscaper(s -> s); + } + + public String getCanonicalFormWithValueEscaper(Function<String, String> escapingFunction) { + // For boolean flags (note that here we do not check for TriState flags, only flags with actual + // boolean values, so that we know the return type of getConvertedValue), use the --[no]flag + // form for the canonical value. + if (optionDefinition.getType().equals(boolean.class)) { + try { + return ((boolean) getConvertedValue() ? "--" : "--no") + optionDefinition.getOptionName(); + } catch (OptionsParsingException e) { + throw new RuntimeException("Unexpected parsing exception", e); + } + } else { + String optionString = "--" + optionDefinition.getOptionName(); + if (unconvertedValue != null) { // Can be null for Void options. + optionString += "=" + escapingFunction.apply(unconvertedValue); + } + return optionString; + } + } + + @Deprecated + // TODO(b/65646296) Once external dependencies are cleaned up, use getCanonicalForm() + String getDeprecatedCanonicalForm() { + String value = unconvertedValue; + // For boolean flags (note that here we do not check for TriState flags, only flags with actual + // boolean values, so that we know the return type of getConvertedValue), set them all to 1 or + // 0, instead of keeping the wide variety of values we accept in their original form. + if (optionDefinition.getType().equals(boolean.class)) { + try { + value = (boolean) getConvertedValue() ? "1" : "0"; + } catch (OptionsParsingException e) { + throw new RuntimeException("Unexpected parsing exception", e); + } + } + return String.format("--%s=%s", optionDefinition.getOptionName(), value); + } + public boolean isBooleanOption() { return optionDefinition.getType().equals(boolean.class); } diff --git a/src/test/java/com/google/devtools/common/options/OptionsParserTest.java b/src/test/java/com/google/devtools/common/options/OptionsParserTest.java index 77bcc36412..41ad1f30a5 100644 --- a/src/test/java/com/google/devtools/common/options/OptionsParserTest.java +++ b/src/test/java/com/google/devtools/common/options/OptionsParserTest.java @@ -1886,6 +1886,30 @@ public class OptionsParserTest { assertThat(canonicalize(Yesterday.class, "-h")).containsExactly("--g=1"); } + /** + * Check that all forms of boolean flags are canonicalizes to the same form. + * + * The list of accepted values is from + * {@link com.google.devtools.common.options.Converters.BooleanConverter}, and the value-less + * --[no] form is controlled by {@link OptionsParserImpl#identifyOptionAndPossibleArgument}. + */ + @Test + public void canonicalizeNormalizesBooleanFlags() throws Exception { + assertThat(canonicalize(Yesterday.class, "--g")).containsExactly("--g=1"); + assertThat(canonicalize(Yesterday.class, "--g=1")).containsExactly("--g=1"); + assertThat(canonicalize(Yesterday.class, "--g=true")).containsExactly("--g=1"); + assertThat(canonicalize(Yesterday.class, "--g=t")).containsExactly("--g=1"); + assertThat(canonicalize(Yesterday.class, "--g=yes")).containsExactly("--g=1"); + assertThat(canonicalize(Yesterday.class, "--g=y")).containsExactly("--g=1"); + + assertThat(canonicalize(Yesterday.class, "--nog")).containsExactly("--g=0"); + assertThat(canonicalize(Yesterday.class, "--g=0")).containsExactly("--g=0"); + assertThat(canonicalize(Yesterday.class, "--g=false")).containsExactly("--g=0"); + assertThat(canonicalize(Yesterday.class, "--g=f")).containsExactly("--g=0"); + assertThat(canonicalize(Yesterday.class, "--g=no")).containsExactly("--g=0"); + assertThat(canonicalize(Yesterday.class, "--g=n")).containsExactly("--g=0"); + } + public static class LongValueExample extends OptionsBase { @Option( name = "longval", @@ -2050,7 +2074,7 @@ public class OptionsParserTest { public void testWrapperCanonicalization() throws OptionsParsingException { List<String> canonicalized = canonicalize(WrapperOptionExample.class, "--wrapper=--flag1=true", "--wrapper=--flag2=87", "--wrapper=--flag3=bar"); - assertThat(canonicalized).isEqualTo(Arrays.asList("--flag1=true", "--flag2=87", "--flag3=bar")); + assertThat(canonicalized).isEqualTo(Arrays.asList("--flag1=1", "--flag2=87", "--flag3=bar")); } /** Dummy options that declares it uses only core types. */ |