diff options
6 files changed, 75 insertions, 16 deletions
diff --git a/src/main/java/com/google/devtools/build/lib/rules/config/ConfigFeatureFlag.java b/src/main/java/com/google/devtools/build/lib/rules/config/ConfigFeatureFlag.java index 68bf177ce5..175588cc6d 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/config/ConfigFeatureFlag.java +++ b/src/main/java/com/google/devtools/build/lib/rules/config/ConfigFeatureFlag.java @@ -40,6 +40,7 @@ import com.google.devtools.build.lib.packages.AttributeMap; import com.google.devtools.build.lib.packages.RuleClass.ConfiguredTargetFactory.RuleErrorException; import com.google.devtools.build.lib.syntax.Printer; import java.util.List; +import java.util.Optional; /** * The implementation of the config_feature_flag rule for defining custom flags for Android rules. @@ -115,14 +116,17 @@ public class ConfigFeatureFlag implements RuleConfiguredTargetFactory { + Printer.repr(duplicates.build())); } - String defaultValue = ruleContext.attributes().get("default_value", STRING); - if (!isValidValue.apply(defaultValue)) { + Optional<String> defaultValue = + ruleContext.attributes().isAttributeValueExplicitlySpecified("default_value") + ? Optional.of(ruleContext.attributes().get("default_value", STRING)) + : Optional.empty(); + if (defaultValue.isPresent() && !isValidValue.apply(defaultValue.get())) { ruleContext.attributeError( "default_value", "must be one of " + Printer.repr(values.asList()) + ", but was " - + Printer.repr(defaultValue)); + + Printer.repr(defaultValue.get())); } if (ruleContext.hasErrors()) { @@ -131,22 +135,29 @@ public class ConfigFeatureFlag implements RuleConfiguredTargetFactory { return null; } - String value = + Optional<String> configuredValue = ruleContext .getFragment(ConfigFeatureFlagConfiguration.class) - .getFeatureFlagValue(ruleContext.getOwner()) - .or(defaultValue); + .getFeatureFlagValue(ruleContext.getOwner()); - if (!isValidValue.apply(value)) { + if (configuredValue.isPresent() && !isValidValue.apply(configuredValue.get())) { // TODO(mstaib): When configurationError is available, use that instead. ruleContext.ruleError( "value must be one of " + Printer.repr(values.asList()) + ", but was " - + Printer.repr(value)); + + Printer.repr(configuredValue.get())); return null; } + if (!configuredValue.isPresent() && !defaultValue.isPresent()) { + // TODO(mstaib): When configurationError is available, use that instead. + ruleContext.ruleError("flag has no default and must be set, but was not set"); + return null; + } + + String value = configuredValue.orElseGet(defaultValue::get); + ConfigFeatureFlagProvider provider = ConfigFeatureFlagProvider.create(value, isValidValue); return new RuleConfiguredTargetBuilder(ruleContext) diff --git a/src/main/java/com/google/devtools/build/lib/rules/config/ConfigFeatureFlagConfiguration.java b/src/main/java/com/google/devtools/build/lib/rules/config/ConfigFeatureFlagConfiguration.java index 9f34de2937..be93cb9353 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/config/ConfigFeatureFlagConfiguration.java +++ b/src/main/java/com/google/devtools/build/lib/rules/config/ConfigFeatureFlagConfiguration.java @@ -14,7 +14,6 @@ package com.google.devtools.build.lib.rules.config; -import com.google.common.base.Optional; import com.google.common.collect.ImmutableSet; import com.google.common.collect.ImmutableSortedMap; import com.google.common.hash.Hasher; @@ -32,6 +31,7 @@ import com.google.devtools.common.options.OptionDocumentationCategory; import com.google.devtools.common.options.OptionEffectTag; import com.google.devtools.common.options.OptionMetadataTag; import java.util.Map; +import java.util.Optional; import java.util.SortedMap; import javax.annotation.Nullable; @@ -141,7 +141,7 @@ public final class ConfigFeatureFlagConfiguration extends BuildConfiguration.Fra * ArtifactOwner for a rule, call {@code ruleContext.getOwner()}. */ public Optional<String> getFeatureFlagValue(ArtifactOwner owner) { - return Optional.fromNullable(flagValues.get(owner.getLabel())); + return Optional.ofNullable(flagValues.get(owner.getLabel())); } /** diff --git a/src/main/java/com/google/devtools/build/lib/rules/config/ConfigRuleClasses.java b/src/main/java/com/google/devtools/build/lib/rules/config/ConfigRuleClasses.java index 7ba3611dad..fb8c973973 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/config/ConfigRuleClasses.java +++ b/src/main/java/com/google/devtools/build/lib/rules/config/ConfigRuleClasses.java @@ -411,7 +411,6 @@ platform( .nonconfigurable(NONCONFIGURABLE_ATTRIBUTE_REASON)) .add( attr("default_value", STRING) - .mandatory() .nonconfigurable(NONCONFIGURABLE_ATTRIBUTE_REASON)) .add(ConfigFeatureFlag.getWhitelistAttribute(env)) .build(); diff --git a/src/test/java/com/google/devtools/build/lib/rules/config/BUILD b/src/test/java/com/google/devtools/build/lib/rules/config/BUILD index 4e115e79e0..feedac96ae 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/config/BUILD +++ b/src/test/java/com/google/devtools/build/lib/rules/config/BUILD @@ -33,6 +33,7 @@ java_library( "//third_party:jsr305", "//third_party:junit4", "//third_party:truth", + "//third_party:truth8", ], ) diff --git a/src/test/java/com/google/devtools/build/lib/rules/config/ConfigFeatureFlagConfigurationTest.java b/src/test/java/com/google/devtools/build/lib/rules/config/ConfigFeatureFlagConfigurationTest.java index 9f1e0c2d9f..09bf495e2b 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/config/ConfigFeatureFlagConfigurationTest.java +++ b/src/test/java/com/google/devtools/build/lib/rules/config/ConfigFeatureFlagConfigurationTest.java @@ -15,9 +15,9 @@ package com.google.devtools.build.lib.rules.config; import static com.google.common.truth.Truth.assertThat; +import static com.google.common.truth.Truth8.assertThat; import static org.junit.Assert.fail; -import com.google.common.base.Optional; import com.google.common.collect.ImmutableMap; import com.google.common.collect.Iterables; import com.google.common.testing.EqualsTester; @@ -26,6 +26,7 @@ import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.common.options.OptionsParser; import com.google.devtools.common.options.OptionsParsingException; import java.util.Map; +import java.util.Optional; import org.junit.Test; import org.junit.runner.RunWith; import org.junit.runners.JUnit4; @@ -152,11 +153,11 @@ public final class ConfigFeatureFlagConfigurationTest { } @Test - public void getFeatureFlagValue_returnsAbsentOptionalWhenRequestingUnsetFlag() throws Exception { + public void getFeatureFlagValue_returnsEmptyOptionalWhenRequestingUnsetFlag() throws Exception { Optional<String> flagValue = getConfigurationWithFlags(ImmutableMap.of(Label.parseAbsoluteUnchecked("//a:a"), "valued")) .getFeatureFlagValue(new LabelArtifactOwner(Label.parseAbsoluteUnchecked("//b:b"))); - assertThat(flagValue).isAbsent(); + assertThat(flagValue).isEmpty(); } @Test diff --git a/src/test/java/com/google/devtools/build/lib/rules/config/ConfigFeatureFlagTest.java b/src/test/java/com/google/devtools/build/lib/rules/config/ConfigFeatureFlagTest.java index 9629bbd54e..dcd80382f7 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/config/ConfigFeatureFlagTest.java +++ b/src/test/java/com/google/devtools/build/lib/rules/config/ConfigFeatureFlagTest.java @@ -76,6 +76,25 @@ public final class ConfigFeatureFlagTest extends SkylarkTestCase { "config_feature_flag(", " name = 'flag',", " allowed_values = ['default', 'configured', 'other'],", + ")"); + assertThat(ConfigFeatureFlagProvider.fromTarget(getConfiguredTarget("//test:top")).getValue()) + .isEqualTo("configured"); + } + + @Test + public void configFeatureFlagProvider_usesConfiguredValueOverDefault() throws Exception { + scratch.file( + "test/BUILD", + "feature_flag_setter(", + " name = 'top',", + " exports_flag = ':flag',", + " flag_values = {", + " ':flag': 'configured',", + " },", + ")", + "config_feature_flag(", + " name = 'flag',", + " allowed_values = ['default', 'configured', 'other'],", " default_value = 'default',", ")"); assertThat(ConfigFeatureFlagProvider.fromTarget(getConfiguredTarget("//test:top")).getValue()) @@ -250,6 +269,34 @@ public final class ConfigFeatureFlagTest extends SkylarkTestCase { } @Test + public void configFeatureFlagProvider_throwsErrorIfNeitherDefaultNorConfiguredValueSet() + throws Exception { + reporter.removeHandler(failFastHandler); // expecting an error + scratch.file( + "test/BUILD", + "feature_flag_setter(", + " name = 'top',", + " exports_flag = ':flag',", + " flag_values = {", + " ':other': 'configured',", + " },", + ")", + "config_feature_flag(", + " name = 'flag',", + " allowed_values = ['other', 'configured'],", + ")", + "config_feature_flag(", + " name = 'other',", + " allowed_values = ['default', 'configured', 'other'],", + " default_value = 'default',", + ")"); + assertThat(getConfiguredTarget("//test:flag")).isNull(); + assertContainsEvent( + "in config_feature_flag rule //test:flag: " + + "flag has no default and must be set, but was not set"); + } + + @Test public void allowedValuesAttribute_cannotBeEmpty() throws Exception { reporter.removeHandler(failFastHandler); // expecting an error scratch.file( @@ -282,7 +329,7 @@ public final class ConfigFeatureFlagTest extends SkylarkTestCase { } @Test - public void defaultValueAttribute_mustBeMemberOfAllowedValues() throws Exception { + public void defaultValueAttribute_mustBeMemberOfAllowedValuesIfPresent() throws Exception { reporter.removeHandler(failFastHandler); // expecting an error scratch.file( "test/BUILD", @@ -305,7 +352,7 @@ public final class ConfigFeatureFlagTest extends SkylarkTestCase { } @Test - public void configurationValue_mustBeMemberOfAllowedValues() throws Exception { + public void configurationValue_mustBeMemberOfAllowedValuesIfPresent() throws Exception { reporter.removeHandler(failFastHandler); // expecting an error scratch.file( "test/BUILD", |