Skip to content

Commit

Permalink
Refactor build options merging to better handle all options.
Browse files Browse the repository at this point in the history 
Also properly handles implicit options.

Work towards platform-based flags: #19409.

PiperOrigin-RevId: 639053307
Change-Id: Iceb376d81fdc874358ad0d5b88923803b229d784
  • Loading branch information
@katre @copybara-github
katre authored and copybara-github committed May 31, 2024
1 parent f527947 commit 219f118
Show file tree
Hide file tree
Showing 5 changed files with 110 additions and 48 deletions.
88 changes: 44 additions & 44 deletions src/main/java/com/google/devtools/build/lib/analysis/config/BuildOptions.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
import com.google.devtools.build.lib.skyframe.serialization.autocodec.SerializationConstant;
import com.google.devtools.build.lib.util.Fingerprint;
import com.google.devtools.common.options.OptionDefinition;
import com.google.devtools.common.options.OptionValueDescription;
import com.google.devtools.common.options.OptionsBase;
import com.google.devtools.common.options.OptionsParser;
import com.google.devtools.common.options.OptionsParsingException;
Expand Down Expand Up @@ -275,54 +276,43 @@ private BuildOptions(
* @return the new options after applying the parsing result to the original options
*/
public BuildOptions applyParsingResult(OptionsParsingResult parsingResult) {
Map<Class<? extends FragmentOptions>, FragmentOptions> modifiedFragments =
toModifiedFragments(parsingResult);

BuildOptions.Builder builder = builder();
for (Map.Entry<Class<? extends FragmentOptions>, FragmentOptions> classAndFragment :
fragmentOptionsMap.entrySet()) {
Class<? extends FragmentOptions> fragmentClass = classAndFragment.getKey();
if (modifiedFragments.containsKey(fragmentClass)) {
builder.addFragmentOptions(modifiedFragments.get(fragmentClass));
} else {
builder.addFragmentOptions(classAndFragment.getValue());
}
}

Map<Label, Object> starlarkOptions = new HashMap<>(starlarkOptionsMap);
Map<Label, Object> parsedStarlarkOptions =
labelizeStarlarkOptions(parsingResult.getStarlarkOptions());
for (Map.Entry<Label, Object> starlarkOption : parsedStarlarkOptions.entrySet()) {
starlarkOptions.put(starlarkOption.getKey(), starlarkOption.getValue());
}
builder.addStarlarkOptions(starlarkOptions);
return builder.build();
}

private Map<Class<? extends FragmentOptions>, FragmentOptions> toModifiedFragments(
OptionsParsingResult parsingResult) {
Map<Class<? extends FragmentOptions>, FragmentOptions> replacedOptions = new HashMap<>();
for (ParsedOptionDescription parsedOption : parsingResult.asListOfExplicitOptions()) {
OptionDefinition optionDefinition = parsedOption.getOptionDefinition();
BuildOptions.Builder builder = this.toBuilder();

// Handle native options.
for (OptionValueDescription optionValue : parsingResult.allOptionValues()) {
OptionDefinition optionDefinition = optionValue.getOptionDefinition();
// All options obtained from an options parser are guaranteed to have been defined in an
// FragmentOptions class.
Class<? extends FragmentOptions> fragmentOptionClass =
optionDefinition.getDeclaringClass(FragmentOptions.class);

FragmentOptions originalFragment = fragmentOptionsMap.get(fragmentOptionClass);
if (originalFragment == null) {
FragmentOptions fragment = builder.getFragmentOptions(fragmentOptionClass);
if (fragment == null) {
// Preserve trimming by ignoring fragments not present in the original options.
continue;
}
FragmentOptions newOptions =
replacedOptions.computeIfAbsent(fragmentOptionClass, unused -> originalFragment.clone());
Object value =
parsingResult.getOptionValueDescription(optionDefinition.getOptionName()).getValue();
optionDefinition.setValue(newOptions, value);
updateOptionValue(fragment, optionDefinition, optionValue);
}

// Also copy Starlark options
Map<Label, Object> parsedStarlarkOptions =
labelizeStarlarkOptions(parsingResult.getStarlarkOptions());
for (Map.Entry<Label, Object> starlarkOption : parsedStarlarkOptions.entrySet()) {
builder.addStarlarkOption(starlarkOption.getKey(), starlarkOption.getValue());
}

return replacedOptions;
return builder.build();
}

private static void updateOptionValue(
FragmentOptions fragment,
OptionDefinition optionDefinition,
OptionValueDescription optionValue) {
// TODO: https://github.com/bazelbuild/bazel/issues/22453 - This will completely overwrite
// accumulating flags, which is almost certainly not what users want. Instead this should
// intelligently merge options.
Object value = optionValue.getValue();
optionDefinition.setValue(fragment, value);
}

/**
Expand Down Expand Up @@ -417,6 +407,23 @@ public <T extends FragmentOptions> Builder addFragmentOptions(T options) {
return this;
}

/**
* Returns the {@link FragmentOptions} for the given class, or {@code null} if that fragment is
* not present.
*/
@Nullable
@SuppressWarnings("unchecked")
public <T extends FragmentOptions> T getFragmentOptions(Class<T> key) {
return (T) fragmentOptions.get(key);
}

/** Removes the value for the {@link FragmentOptions} with the given FragmentOptions class. */
@CanIgnoreReturnValue
public Builder removeFragmentOptions(Class<? extends FragmentOptions> key) {
fragmentOptions.remove(key);
return this;
}

/**
* Adds multiple Starlark options to the builder. Overrides previous instances of the same key.
*/
Expand All @@ -433,13 +440,6 @@ public Builder addStarlarkOption(Label key, Object value) {
return this;
}

/** Removes the value for the {@link FragmentOptions} with the given FragmentOptions class. */
@CanIgnoreReturnValue
public Builder removeFragmentOptions(Class<? extends FragmentOptions> key) {
fragmentOptions.remove(key);
return this;
}

/** Removes the value for the Starlark option with the given key. */
@CanIgnoreReturnValue
public Builder removeStarlarkOption(Label key) {
Expand Down
5 changes: 5 additions & 0 deletions src/main/java/com/google/devtools/common/options/OptionsParser.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -852,6 +852,11 @@ public List<OptionValueDescription> asListOfOptionValues() {
return impl.asListOfEffectiveOptions();
}

@Override
public List<OptionValueDescription> allOptionValues() {
return impl.allOptionValues();
}

@Override
public List<String> canonicalize() {
return impl.asCanonicalizedList();
Expand Down
13 changes: 11 additions & 2 deletions src/main/java/com/google/devtools/common/options/OptionsParserImpl.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@

package com.google.devtools.common.options;

import static com.google.common.collect.ImmutableList.toImmutableList;
import static com.google.devtools.common.options.OptionPriority.PriorityCategory.INVOCATION_POLICY;
import static java.util.Comparator.comparing;
import static java.util.stream.Collectors.toCollection;
Expand Down Expand Up @@ -282,7 +283,7 @@ List<ParsedOptionDescription> getSkippedOptions() {
List<String> asCanonicalizedList() {
return asCanonicalizedListOfParsedOptions().stream()
.map(ParsedOptionDescription::getDeprecatedCanonicalForm)
.collect(ImmutableList.toImmutableList());
.collect(toImmutableList());
}

/** Implements {@link OptionsParser#canonicalize}. */
Expand All @@ -293,7 +294,7 @@ List<ParsedOptionDescription> asCanonicalizedListOfParsedOptions() {
.flatMap(Collection::stream)
// Return the effective (canonical) options in the order they were applied.
.sorted(comparing(ParsedOptionDescription::getPriority))
.collect(ImmutableList.toImmutableList());
.collect(toImmutableList());
}

/** Implements {@link OptionsParser#asListOfOptionValues()}. */
Expand All @@ -312,6 +313,14 @@ List<OptionValueDescription> asListOfEffectiveOptions() {
return result;
}

List<OptionValueDescription> allOptionValues() {
return optionsData.getAllOptionDefinitions().stream()
.map(Map.Entry::getValue)
.map(optionValues::get)
.filter(optionValue -> optionValue != null)
.collect(toImmutableList());
}

private void maybeAddDeprecationWarning(
OptionDefinition optionDefinition, PriorityCategory priority) {
// Don't add a warning for deprecated flag set by the invocation policy.
Expand Down
4 changes: 4 additions & 0 deletions src/main/java/com/google/devtools/common/options/OptionsParsingResult.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,10 @@ public interface OptionsParsingResult extends OptionsProvider {
// TODO(b/150222792): make this aware of Starlark options
List<OptionValueDescription> asListOfOptionValues();

/** Returns all non-default option values, including implicit options. */
// TODO(b/150222792): make this aware of Starlark options
List<OptionValueDescription> allOptionValues();

/**
* Canonicalizes the list of options that this OptionsParser has parsed.
*
Expand Down
48 changes: 46 additions & 2 deletions src/test/java/com/google/devtools/build/lib/analysis/config/BuildOptionsTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,21 @@ public DummyTestOptions() {}
effectTags = {OptionEffectTag.NO_OP},
defaultValue = "null")
public List<String> accumulatingOption;

@Option(
name = "dummy_option",
documentationCategory = OptionDocumentationCategory.UNDOCUMENTED,
effectTags = {OptionEffectTag.NO_OP},
defaultValue = "internal_default",
implicitRequirements = {"--implicit_option=set_implicitly"})
public String dummyOption;

@Option(
name = "implicit_option",
documentationCategory = OptionDocumentationCategory.UNDOCUMENTED,
effectTags = {OptionEffectTag.NO_OP},
defaultValue = "implicit_default")
public String implicitOption;
}

/** Extra options for this test. */
Expand Down Expand Up @@ -288,11 +303,40 @@ public void parsingResultTransformMultiValueOption() throws Exception {
BuildOptions original = BuildOptions.of(BUILD_CONFIG_OPTIONS);

OptionsParser parser = OptionsParser.builder().optionsClasses(BUILD_CONFIG_OPTIONS).build();
parser.parse("--list_option=foo");
parser.parse("--list_option=foo,bar");

BuildOptions modified = original.applyParsingResult(parser);

assertThat(modified.get(DummyTestOptions.class).listOption)
.containsExactly("foo", "bar")
.inOrder();
}

@Test
public void parsingResultTransformImplicitOption() throws Exception {
BuildOptions original = BuildOptions.of(BUILD_CONFIG_OPTIONS);

OptionsParser parser = OptionsParser.builder().optionsClasses(BUILD_CONFIG_OPTIONS).build();
parser.parse("--dummy_option=direct");

BuildOptions modified = original.applyParsingResult(parser);

assertThat(modified.get(DummyTestOptions.class).dummyOption).isEqualTo("direct");
assertThat(modified.get(DummyTestOptions.class).implicitOption).isEqualTo("set_implicitly");
}

@Test
public void parsingResultTransform_accumulating() throws Exception {
BuildOptions original = BuildOptions.of(BUILD_CONFIG_OPTIONS);

OptionsParser parser = OptionsParser.builder().optionsClasses(BUILD_CONFIG_OPTIONS).build();
parser.parse("--accumulating_option=foo", "--accumulating_option=bar");

BuildOptions modified = original.applyParsingResult(parser);

assertThat(modified.get(DummyTestOptions.class).listOption).containsExactly("foo");
assertThat(modified.get(DummyTestOptions.class).accumulatingOption)
.containsExactly("foo", "bar")
.inOrder();
}

@Test
Expand Down

0 comments on commit 219f118

Please sign in to comment.