Skip to content

Commit

Permalink
Add a conversionContext parameter to option converters
Browse files Browse the repository at this point in the history
We need to pass flag values through the repo mapping of the main repo, which means that at flag parsing time, we need access to the main repo mapping. To that end, we add a nullable untyped `conversionContext` parameter to the `Converter#convert` method, which is unused in this CL but will be used in a follow-up.

Note that we can't directly add a `RepositoryMapping` parameter because the c.g.devtools.common.options package is a transitive dependency of c.g.devtools.build.lib.cmdline (which RepositoryMapping lives in). So this `conversionContext` will unfortunately need to be an Object.

Reviewers: Please focus on reviewing changes in the c.g.devtools.common.options package. All the other changes in this CL are simply adding a `conversionContext` parameter to implementors of `Converter`, or passing this parameter to delegates, or superclasses.

Work towards bazelbuild#14852

PiperOrigin-RevId: 459278433
Change-Id: I98b3842305c34d2d0c33e7411c1024897fb0170a
  • Loading branch information
Wyverald authored and aranguyen committed Jul 20, 2022
1 parent 4fdb7ef commit ae12b31
Show file tree
Hide file tree
Showing 69 changed files with 532 additions and 325 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -228,7 +228,7 @@ static Options parseCommandLineOptions(String[] args) throws IOException {
}

/** Validating converter for Paths. A Path is considered valid if it resolves to a file. */
public static class PathConverter implements Converter<Path> {
public static class PathConverter extends Converter.Contextless<Path> {

private final boolean mustExist;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -167,7 +167,8 @@ public int hashCode() {
+ getLocalTestCount() * p * p;
}

public static class ResourceSetConverter implements Converter<ResourceSet> {
/** Converter for {@link ResourceSet}. */
public static class ResourceSetConverter extends Converter.Contextless<ResourceSet> {
private static final Splitter SPLITTER = Splitter.on(',');

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -245,7 +245,7 @@ public static DetailedExitCode createDetailedExitCode(String message, Code detai
}

/** Converter for {@code --embed_label} which rejects strings that span multiple lines. */
public static final class OneLineStringConverter implements Converter<String> {
public static final class OneLineStringConverter extends Converter.Contextless<String> {

@Override
public String convert(String input) throws OptionsParsingException {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@
* <p>If the compilation happens remotely then the cpu of the remote machine might be different from
* the auto-detected one and the --cpu and --host_cpu options must be set explicitly.
*/
public class AutoCpuConverter implements Converter<String> {
public class AutoCpuConverter extends Converter.Contextless<String> {
@Override
public String convert(String input) throws OptionsParsingException {
if (input.isEmpty()) {
Expand Down Expand Up @@ -83,7 +83,10 @@ public String convert(String input) throws OptionsParsingException {
return input;
}

/** Reverses the conversion performed by {@link #convert} to return the matching OS, CPU pair. */
/**
* Reverses the conversion performed by {@link Converter#convert} to return the matching OS, CPU
* pair.
*/
public static Pair<CPU, OS> reverse(String input) {
if (input == null || input.length() == 0 || "unknown".equals(input)) {
// Use the auto-detected values.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ public class CoreOptionConverters {
.buildOrThrow();

/** A converter from strings to Starlark int values. */
private static class StarlarkIntConverter implements Converter<StarlarkInt> {
private static class StarlarkIntConverter extends Converter.Contextless<StarlarkInt> {
@Override
public StarlarkInt convert(String input) throws OptionsParsingException {
// Note that Starlark rule attribute values are currently restricted
Expand All @@ -85,8 +85,8 @@ public String getTypeDescription() {
/** A converter from strings to Labels. */
public static class LabelConverter implements Converter<Label> {
@Override
public Label convert(String input) throws OptionsParsingException {
return convertOptionsLabel(input);
public Label convert(String input, Object conversionContext) throws OptionsParsingException {
return convertOptionsLabel(input, conversionContext);
}

@Override
Expand All @@ -98,10 +98,11 @@ public String getTypeDescription() {
/** A converter from comma-separated strings to Label lists. */
public static class LabelListConverter implements Converter<List<Label>> {
@Override
public List<Label> convert(String input) throws OptionsParsingException {
public List<Label> convert(String input, Object conversionContext)
throws OptionsParsingException {
ImmutableList.Builder<Label> result = ImmutableList.builder();
for (String label : Splitter.on(",").omitEmptyStrings().split(input)) {
result.add(convertOptionsLabel(label));
result.add(convertOptionsLabel(label, conversionContext));
}
return result.build();
}
Expand All @@ -119,8 +120,8 @@ public String getTypeDescription() {
public static class EmptyToNullLabelConverter implements Converter<Label> {
@Override
@Nullable
public Label convert(String input) throws OptionsParsingException {
return input.isEmpty() ? null : convertOptionsLabel(input);
public Label convert(String input, Object conversionContext) throws OptionsParsingException {
return input.isEmpty() ? null : convertOptionsLabel(input, conversionContext);
}

@Override
Expand All @@ -139,8 +140,8 @@ protected DefaultLabelConverter(String defaultValue) {
}

@Override
public Label convert(String input) throws OptionsParsingException {
return input.isEmpty() ? defaultValue : convertOptionsLabel(input);
public Label convert(String input, Object conversionContext) throws OptionsParsingException {
return input.isEmpty() ? defaultValue : convertOptionsLabel(input, conversionContext);
}

@Override
Expand All @@ -152,7 +153,8 @@ public String getTypeDescription() {
/** Flag converter for a map of unique keys with optional labels as values. */
public static class LabelMapConverter implements Converter<Map<String, Label>> {
@Override
public Map<String, Label> convert(String input) throws OptionsParsingException {
public Map<String, Label> convert(String input, Object conversionContext)
throws OptionsParsingException {
// Use LinkedHashMap so we can report duplicate keys more easily while preserving order
Map<String, Label> result = new LinkedHashMap<>();
for (String entry : Splitter.on(",").omitEmptyStrings().trimResults().split(input)) {
Expand All @@ -165,7 +167,7 @@ public Map<String, Label> convert(String input) throws OptionsParsingException {
} else {
key = entry.substring(0, sepIndex);
String value = entry.substring(sepIndex + 1);
label = value.isEmpty() ? null : convertOptionsLabel(value);
label = value.isEmpty() ? null : convertOptionsLabel(value, conversionContext);
}
if (result.containsKey(key)) {
throw new OptionsParsingException("Key '" + key + "' appears twice");
Expand Down Expand Up @@ -202,7 +204,8 @@ public StrictDepsConverter() {
}
}

private static final Label convertOptionsLabel(String input) throws OptionsParsingException {
private static Label convertOptionsLabel(String input, @Nullable Object conversionContext)
throws OptionsParsingException {
try {
// Check if the input starts with '/'. We don't check for "//" so that
// we get a better error message if the user accidentally tries to use
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -254,7 +254,7 @@ public class CoreOptions extends FragmentOptions implements Cloneable {
public boolean enableAspectHints;

/** Regardless of input, converts to an empty list. For use with affectedByStarlarkTransition */
public static class EmptyListConverter implements Converter<List<String>> {
public static class EmptyListConverter extends Converter.Contextless<List<String>> {
@Override
public List<String> convert(String input) throws OptionsParsingException {
return ImmutableList.of();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ Pattern pattern() {

/** Constructs an instance of ExecutionInfoModifier by parsing an option string. */
public static class Converter
implements com.google.devtools.common.options.Converter<ExecutionInfoModifier> {
extends com.google.devtools.common.options.Converter.Contextless<ExecutionInfoModifier> {
@Override
public ExecutionInfoModifier convert(String input) throws OptionsParsingException {
if (Strings.isNullOrEmpty(input)) {
Expand All @@ -77,7 +77,7 @@ public ExecutionInfoModifier convert(String input) throws OptionsParsingExceptio
// Convert to get a useful exception if it's not a valid pattern, but use the regex
// (see comment in Expression)
new RegexPatternConverter()
.convert(specMatcher.group("pattern"))
.convert(specMatcher.group("pattern"), /*conversionContext=*/ null)
.regexPattern()
.pattern(),
specMatcher.group("sign").equals("-"),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,15 +37,13 @@ public final class PerLabelOptions {
private final List<String> optionsList;

/**
* Converts a String to a {@link PerLabelOptions} object. The syntax of the
* string is {@code regex_filter@option_1,option_2,...,option_n}. Where
* regex_filter stands for the String representation of a {@link RegexFilter},
* and {@code option_1} to {@code option_n} stand for arbitrary command line
* options. If an option contains a comma it has to be quoted with a
* backslash. Options can contain @. Only the first @ is used to split the
* string.
* Converts a String to a {@link PerLabelOptions} object. The syntax of the string is {@code
* regex_filter@option_1,option_2,...,option_n}. Where regex_filter stands for the String
* representation of a {@link RegexFilter}, and {@code option_1} to {@code option_n} stand for
* arbitrary command line options. If an option contains a comma it has to be quoted with a
* backslash. Options can contain @. Only the first @ is used to split the string.
*/
public static class PerLabelOptionsConverter implements Converter<PerLabelOptions> {
public static class PerLabelOptionsConverter extends Converter.Contextless<PerLabelOptions> {

@Override
public PerLabelOptions convert(String input) throws OptionsParsingException {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,12 +26,11 @@
import java.util.Objects;
import javax.annotation.Nullable;

/**
* --run_under options converter.
*/
/** --run_under options converter. */
public class RunUnderConverter implements Converter<RunUnder> {
@Override
public RunUnder convert(final String input) throws OptionsParsingException {
public RunUnder convert(final String input, Object conversionContext)
throws OptionsParsingException {
final List<String> runUnderList = new ArrayList<>();
try {
ShellUtils.tokenize(runUnderList, input);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -308,12 +308,13 @@ private static BuildOptions applyTransition(
// of returning either a scalar or list.
List<?> optionValueAsList = (List<?>) optionValue;
if (optionValueAsList.isEmpty()) {
convertedValue = def.getDefaultValue();
convertedValue = ImmutableList.of();
} else {
convertedValue =
def.getConverter()
.convert(
optionValueAsList.stream().map(Object::toString).collect(joining(",")));
optionValueAsList.stream().map(Object::toString).collect(joining(",")),
/*conversionContext=*/ null);
}
} else if (def.getType() == List.class && optionValue == null) {
throw ValidationException.format(
Expand All @@ -325,7 +326,8 @@ private static BuildOptions applyTransition(
} else if (def.getType().equals(boolean.class) && optionValue instanceof Boolean) {
convertedValue = optionValue;
} else if (optionValue instanceof String) {
convertedValue = def.getConverter().convert((String) optionValue);
convertedValue =
def.getConverter().convert((String) optionValue, /*conversionContext=*/ null);
} else {
throw ValidationException.format("Invalid value type for option '%s'", optionName);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -218,7 +218,7 @@ static TargetModule create(String name, Version version) {
}

/** Converts a module target argument string to a properly typed {@link TargetModule} */
static class TargetModuleConverter implements Converter<TargetModule> {
static class TargetModuleConverter extends Converter.Contextless<TargetModule> {

@Override
public TargetModule convert(String input) throws OptionsParsingException {
Expand Down Expand Up @@ -260,17 +260,19 @@ public String getTypeDescription() {
}

/** Converts a comma-separated module list argument (i.e. A@1.0,B@2.0) */
public static class TargetModuleListConverter implements Converter<ImmutableList<TargetModule>> {
public static class TargetModuleListConverter
extends Converter.Contextless<ImmutableList<TargetModule>> {

@Override
public ImmutableList<TargetModule> convert(String input) throws OptionsParsingException {
CommaSeparatedNonEmptyOptionListConverter listConverter =
new CommaSeparatedNonEmptyOptionListConverter();
TargetModuleConverter targetModuleConverter = new TargetModuleConverter();
ImmutableList<String> targetStrings = listConverter.convert(input);
ImmutableList<String> targetStrings =
listConverter.convert(input, /*conversionContext=*/ null);
ImmutableList.Builder<TargetModule> targetModules = new ImmutableList.Builder<>();
for (String targetInput : targetStrings) {
targetModules.add(targetModuleConverter.convert(targetInput));
targetModules.add(targetModuleConverter.convert(targetInput, /*conversionContext=*/ null));
}
return targetModules.build();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -257,7 +257,8 @@ public Converter() {
/**
* Converts from an equals-separated pair of strings into RepositoryName->PathFragment mapping.
*/
public static class RepositoryOverrideConverter implements Converter<RepositoryOverride> {
public static class RepositoryOverrideConverter
extends Converter.Contextless<RepositoryOverride> {

@Override
public RepositoryOverride convert(String input) throws OptionsParsingException {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,16 +22,16 @@
import java.util.Set;

/**
* Converter that translates a string of the form "value1,value2,-value3,value4"
* into a corresponding set of allowed Enum values.
* Converter that translates a string of the form "value1,value2,-value3,value4" into a
* corresponding set of allowed Enum values.
*
* <p>Values preceded by '-' are excluded from this set. So "value1,-value2,value3"
* translates to the set [EnumType.value1, EnumType.value3].
* <p>Values preceded by '-' are excluded from this set. So "value1,-value2,value3" translates to
* the set [EnumType.value1, EnumType.value3].
*
* <p>If *all* values are exclusions (e.g. "-value1,-value2,-value3"), the returned
* set contains all values for the Enum type *except* those specified.
* <p>If *all* values are exclusions (e.g. "-value1,-value2,-value3"), the returned set contains all
* values for the Enum type *except* those specified.
*/
class EnumFilterConverter<E extends Enum<E>> implements Converter<Set<E>> {
class EnumFilterConverter<E extends Enum<E>> extends Converter.Contextless<Set<E>> {

private final Set<String> allowedValues = new LinkedHashSet<>();
private final Class<E> typeClass;
Expand All @@ -54,7 +54,7 @@ class EnumFilterConverter<E extends Enum<E>> implements Converter<Set<E>> {
/**
* Returns the set of allowed values for the option.
*
* Implements {@link #convert(String)}.
* <p>Implements {@link Converter#convert(String, Object)}.
*/
@Override
public Set<E> convert(String input) throws OptionsParsingException {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -121,9 +121,9 @@ public TestSizeFilterConverter() {
/**
* {@inheritDoc}
*
* <p>This override is necessary to prevent OptionsData
* from throwing a "must be assignable from the converter return type" exception.
* OptionsData doesn't recognize the generic type and actual type are the same.
* <p>This override is necessary to prevent OptionsData from throwing a "must be assignable from
* the converter return type" exception. OptionsData doesn't recognize the generic type and
* actual type are the same.
*/
@Override
public final Set<TestSize> convert(String input) throws OptionsParsingException {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -190,10 +190,9 @@ public static TestTimeout getSuggestedTestTimeout(int timeInSeconds) {
return SUGGESTED_TIMEOUT.get(timeInSeconds);
}

/**
* Converter for the --test_timeout option.
*/
public static class TestTimeoutConverter implements Converter<Map<TestTimeout, Duration>> {
/** Converter for the --test_timeout option. */
public static class TestTimeoutConverter
extends Converter.Contextless<Map<TestTimeout, Duration>> {
public TestTimeoutConverter() {}

@Override
Expand Down Expand Up @@ -250,9 +249,9 @@ public TestTimeoutFilterConverter() {
/**
* {@inheritDoc}
*
* <p>This override is necessary to prevent OptionsData
* from throwing a "must be assignable from the converter return type" exception.
* OptionsData doesn't recognize the generic type and actual type are the same.
* <p>This override is necessary to prevent OptionsData from throwing a "must be assignable from
* the converter return type" exception. OptionsData doesn't recognize the generic type and
* actual type are the same.
*/
@Override
public final Set<TestTimeout> convert(String input) throws OptionsParsingException {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,10 +38,8 @@
/** Options for configuring Packages -- loading and default behaviors. */
public class PackageOptions extends OptionsBase {

/**
* Converter for the {@code --default_visibility} option.
*/
public static class DefaultVisibilityConverter implements Converter<RuleVisibility> {
/** Converter for the {@code --default_visibility} option. */
public static class DefaultVisibilityConverter extends Converter.Contextless<RuleVisibility> {
@Override
public RuleVisibility convert(String input) throws OptionsParsingException {
if (input.equals("public")) {
Expand Down Expand Up @@ -194,11 +192,9 @@ public ParallelismConverter() throws OptionsParsingException {
+ "previous run's cache.")
public boolean checkOutputFiles;

/**
* A converter from strings containing comma-separated names of packages to lists of strings.
*/
/** A converter from strings containing comma-separated names of packages to lists of strings. */
public static class CommaSeparatedPackageNameListConverter
implements Converter<List<PackageIdentifier>> {
extends Converter.Contextless<List<PackageIdentifier>> {

private static final Splitter COMMA_SPLITTER = Splitter.on(',');

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,8 @@ private MemoryProfileStableHeapParameters(int numTimesToDoGc, Duration timeToSle

/** Converter for {@code MemoryProfileStableHeapParameters} option. */
public static class Converter
implements com.google.devtools.common.options.Converter<MemoryProfileStableHeapParameters> {
extends com.google.devtools.common.options.Converter.Contextless<
MemoryProfileStableHeapParameters> {
private static final Splitter SPLITTER = Splitter.on(',');

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -218,15 +218,15 @@ public final class RemoteOptions extends OptionsBase {
public String remoteBytestreamUriPrefix;

/** Returns the specified duration. Assumes seconds if unitless. */
public static class RemoteTimeoutConverter implements Converter<Duration> {
public static class RemoteTimeoutConverter extends Converter.Contextless<Duration> {
private static final Pattern UNITLESS_REGEX = Pattern.compile("^[0-9]+$");

@Override
public Duration convert(String input) throws OptionsParsingException {
if (UNITLESS_REGEX.matcher(input).matches()) {
input += "s";
}
return new Converters.DurationConverter().convert(input);
return new Converters.DurationConverter().convert(input, /*conversionContext=*/ null);
}

@Override
Expand Down
Loading

0 comments on commit ae12b31

Please sign in to comment.