Give --define a new Starlark friendly converter.
This allow us to read --define in starlark transitions.
RELNOTES: None.
PiperOrigin-RevId: 243342166
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfiguration.java b/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfiguration.java
index 73fbc53..cbc7447 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfiguration.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfiguration.java
@@ -37,6 +37,7 @@
import com.google.devtools.build.lib.analysis.BlazeDirectories;
import com.google.devtools.build.lib.analysis.ConfiguredRuleClassProvider;
import com.google.devtools.build.lib.analysis.actions.FileWriteAction;
+import com.google.devtools.build.lib.analysis.skylark.StarlarkConverter;
import com.google.devtools.build.lib.buildeventstream.BuildEventId;
import com.google.devtools.build.lib.buildeventstream.BuildEventStreamProtos;
import com.google.devtools.build.lib.cmdline.Label;
@@ -50,6 +51,7 @@
import com.google.devtools.build.lib.skylarkbuildapi.BuildConfigurationApi;
import com.google.devtools.build.lib.skylarkinterface.SkylarkInterfaceUtils;
import com.google.devtools.build.lib.skylarkinterface.SkylarkModule;
+import com.google.devtools.build.lib.syntax.SkylarkDict;
import com.google.devtools.build.lib.util.OS;
import com.google.devtools.build.lib.util.RegexFilter;
import com.google.devtools.build.lib.vfs.Path;
@@ -321,15 +323,28 @@
help = "If true, the genfiles directory is folded into the bin directory.")
public boolean mergeGenfilesDirectory;
+ /**
+ * AssignmentConverter that also supports Starlarkization. For use with options that use {@link
+ * com.google.devtools.common.options.Converters.AssignmentConverter} and
+ * {@link @Option#allowMultiple}.
+ */
+ public static class StarlarkAssignmentConverter extends Converters.AssignmentConverter
+ implements StarlarkConverter<List<Map.Entry<String, String>>, Map.Entry<String, String>> {
+ @Override
+ public SkylarkDict<String, String> convertToStarlark(
+ List<Map.Entry<String, String>> entries) {
+ return SkylarkDict.copyOf(null, ImmutableMap.copyOf(entries));
+ }
+ }
+
@Option(
- name = "define",
- converter = Converters.AssignmentConverter.class,
- defaultValue = "",
- allowMultiple = true,
- documentationCategory = OptionDocumentationCategory.OUTPUT_PARAMETERS,
- effectTags = {OptionEffectTag.CHANGES_INPUTS, OptionEffectTag.AFFECTS_OUTPUTS},
- help = "Each --define option specifies an assignment for a build variable."
- )
+ name = "define",
+ converter = StarlarkAssignmentConverter.class,
+ defaultValue = "",
+ allowMultiple = true,
+ documentationCategory = OptionDocumentationCategory.OUTPUT_PARAMETERS,
+ effectTags = {OptionEffectTag.CHANGES_INPUTS, OptionEffectTag.AFFECTS_OUTPUTS},
+ help = "Each --define option specifies an assignment for a build variable.")
public List<Map.Entry<String, String>> commandLineBuildVariables;
@Option(
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/skylark/FunctionTransitionUtil.java b/src/main/java/com/google/devtools/build/lib/analysis/skylark/FunctionTransitionUtil.java
index c014b8a6..d074579 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/skylark/FunctionTransitionUtil.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/skylark/FunctionTransitionUtil.java
@@ -17,6 +17,7 @@
import static java.nio.charset.StandardCharsets.US_ASCII;
import com.google.common.base.Joiner;
+import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
@@ -34,6 +35,8 @@
import com.google.devtools.build.lib.syntax.Runtime.NoneType;
import com.google.devtools.build.lib.syntax.SkylarkDict;
import com.google.devtools.build.lib.syntax.SkylarkList;
+import com.google.devtools.build.lib.syntax.SkylarkType;
+import com.google.devtools.common.options.Converter;
import com.google.devtools.common.options.OptionDefinition;
import com.google.devtools.common.options.OptionsParser;
import com.google.devtools.common.options.OptionsParsingException;
@@ -153,7 +156,7 @@
* @throws EvalException if any of the specified transition inputs do not correspond to a valid
* build setting
*/
- static SkylarkDict<String, Object> buildSettings(
+ static <S, T> SkylarkDict<String, Object> buildSettings(
BuildOptions buildOptions,
Map<String, OptionInfo> optionInfoMap,
StarlarkDefinedConfigTransition starlarkTransition)
@@ -178,10 +181,29 @@
Field field = optionInfo.getDefinition().getField();
FragmentOptions options = buildOptions.get(optionInfo.getOptionClass());
Object optionValue = field.get(options);
-
- dict.put(optionKey, optionValue == null ? Runtime.NONE : optionValue, null, mutability);
+ Converter<?> converter = optionInfo.getDefinition().getConverter();
+ if (converter instanceof StarlarkConverter) {
+ @SuppressWarnings("unchecked") // Type erasure
+ Object convertedValue =
+ ((StarlarkConverter<S, T>) converter).convertToStarlark((S) optionValue);
+ Preconditions.checkNotNull(convertedValue);
+ optionValue = convertedValue;
+ } else if (optionValue == null) {
+ optionValue = Runtime.NONE;
+ } else {
+ optionValue = SkylarkType.convertToSkylark(optionValue, mutability);
+ if (optionValue != null && !SkylarkType.isSkylarkAcceptable(optionValue.getClass())) {
+ throw new EvalException(
+ starlarkTransition.getLocationForErrorReporting(),
+ String.format(
+ "Unable to read option '%s' - option '%s' is Starlark incompatible.",
+ optionKey, optionKey));
+ }
+ }
+ dict.put(optionKey, optionValue, null, mutability);
} catch (IllegalAccessException e) {
- // These exceptions should not happen, but if they do, throw a RuntimeException.
+ // These exceptions should not happen since all options should be public, but if they
+ // do, throw a RuntimeException.
throw new RuntimeException(e);
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/skylark/StarlarkConverter.java b/src/main/java/com/google/devtools/build/lib/analysis/skylark/StarlarkConverter.java
new file mode 100644
index 0000000..6798fab
--- /dev/null
+++ b/src/main/java/com/google/devtools/build/lib/analysis/skylark/StarlarkConverter.java
@@ -0,0 +1,44 @@
+// Copyright 2019 The Bazel Authors. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.devtools.build.lib.analysis.skylark;
+
+import com.google.devtools.build.lib.skylarkinterface.SkylarkValue;
+import com.google.devtools.common.options.Converter;
+
+/**
+ * A converter that also knows how to turn a native object into a Starlark object
+ *
+ * <p>This interface declares/inherits two converting methods:
+ *
+ * <p>For converting from command line to Java (inherited)
+ *
+ * <p>{@code convert} String -> Object<T>
+ *
+ * <p>For converting from Java to Starlark
+ *
+ * <p>{@code convertToStarlark} Object<S> -> SkylarkValue
+ *
+ * <p>In practice, <T> and <S> will almost always be the same type. An example of an exception to
+ * this rule is for options with {@code allowMultiple = true} for which <S> is a list of <T>
+ * objects.
+ */
+// TODO(juliexxia): Add Starlark -> Java converter method to this interface.
+public interface StarlarkConverter<S, T> extends Converter<T> {
+
+ /** Convert a java object of type S into a Starlark friendly value. */
+ // TODO(bazel-team): exapnd this interface to allow "primitive" types to also be returned here.
+ // Example case of where this would be useful is for {@code enum}s.
+ SkylarkValue convertToStarlark(S input);
+}
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/EvalUtils.java b/src/main/java/com/google/devtools/build/lib/syntax/EvalUtils.java
index 38e7710..d8583b4 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/EvalUtils.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/EvalUtils.java
@@ -25,7 +25,6 @@
import com.google.devtools.build.lib.skylarkinterface.SkylarkInterfaceUtils;
import com.google.devtools.build.lib.skylarkinterface.SkylarkModule;
import com.google.devtools.build.lib.skylarkinterface.SkylarkValue;
-import com.google.devtools.build.lib.vfs.PathFragment;
import java.util.Collection;
import java.util.List;
import java.util.Map;
@@ -147,21 +146,6 @@
|| c.equals(Boolean.class);
}
- /**
- * Returns true if the type is acceptable to be returned to the Skylark language.
- */
- public static boolean isSkylarkAcceptable(Class<?> c) {
- return SkylarkValue.class.isAssignableFrom(c) // implements SkylarkValue
- || c.equals(String.class) // basic values
- || c.equals(Integer.class)
- || c.equals(Boolean.class)
- // there is a registered Skylark ancestor class (useful e.g. when using AutoValue)
- || SkylarkInterfaceUtils.getSkylarkModule(c) != null
- || ImmutableMap.class.isAssignableFrom(c) // will be converted to SkylarkDict
- || NestedSet.class.isAssignableFrom(c) // will be converted to SkylarkNestedSet
- || PathFragment.class.isAssignableFrom(c); // other known class
- }
-
// TODO(bazel-team): move the following few type-related functions to SkylarkType
/**
* Return the Skylark-type of {@code c}
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/MethodDescriptor.java b/src/main/java/com/google/devtools/build/lib/syntax/MethodDescriptor.java
index b322d23..4bf6281 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/MethodDescriptor.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/MethodDescriptor.java
@@ -148,7 +148,7 @@
}
// TODO(bazel-team): get rid of this, by having everyone use the Skylark data structures
result = SkylarkType.convertToSkylark(result, method, env);
- if (result != null && !EvalUtils.isSkylarkAcceptable(result.getClass())) {
+ if (result != null && !SkylarkType.isSkylarkAcceptable(result.getClass())) {
throw new EvalException(
loc,
Printer.format(
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkType.java b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkType.java
index a714782..84afa81 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkType.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkType.java
@@ -26,9 +26,11 @@
import com.google.devtools.build.lib.events.Location;
import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec;
import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec.VisibleForSerialization;
+import com.google.devtools.build.lib.skylarkinterface.SkylarkInterfaceUtils;
import com.google.devtools.build.lib.skylarkinterface.SkylarkValue;
import com.google.devtools.build.lib.syntax.SkylarkList.MutableList;
import com.google.devtools.build.lib.syntax.SkylarkList.Tuple;
+import com.google.devtools.build.lib.vfs.PathFragment;
import java.io.Serializable;
import java.lang.reflect.Method;
import java.lang.reflect.ParameterizedType;
@@ -663,6 +665,19 @@
return true;
}
+ /** Returns true if the type is acceptable to be returned to the Skylark language. */
+ public static boolean isSkylarkAcceptable(Class<?> c) {
+ return SkylarkValue.class.isAssignableFrom(c) // implements SkylarkValue
+ || c.equals(String.class) // basic values
+ || c.equals(Integer.class)
+ || c.equals(Boolean.class)
+ // there is a registered Skylark ancestor class (useful e.g. when using AutoValue)
+ || SkylarkInterfaceUtils.getSkylarkModule(c) != null
+ || ImmutableMap.class.isAssignableFrom(c) // will be converted to SkylarkDict
+ || NestedSet.class.isAssignableFrom(c) // will be converted to SkylarkNestedSet
+ || PathFragment.class.isAssignableFrom(c); // other known class
+ }
+
/**
* Throws EvalException if the type of the object is not allowed to be present in Skylark.
*/
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/StarlarkRuleTransitionProviderTest.java b/src/test/java/com/google/devtools/build/lib/analysis/StarlarkRuleTransitionProviderTest.java
index c56d4eb..03ef28f 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/StarlarkRuleTransitionProviderTest.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/StarlarkRuleTransitionProviderTest.java
@@ -21,6 +21,7 @@
import com.google.devtools.build.lib.analysis.config.BuildConfiguration;
import com.google.devtools.build.lib.analysis.config.BuildConfiguration.EmptyToNullLabelConverter;
import com.google.devtools.build.lib.analysis.config.BuildConfiguration.Fragment;
+import com.google.devtools.build.lib.analysis.config.BuildConfiguration.Options.StarlarkAssignmentConverter;
import com.google.devtools.build.lib.analysis.config.BuildOptions;
import com.google.devtools.build.lib.analysis.config.ConfigurationFragmentFactory;
import com.google.devtools.build.lib.analysis.config.FragmentOptions;
@@ -29,10 +30,11 @@
import com.google.devtools.build.lib.analysis.util.BuildViewTestCase;
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.testutil.TestRuleClassProvider;
-import com.google.devtools.common.options.Converters;
+import com.google.devtools.common.options.Converter;
import com.google.devtools.common.options.Option;
import com.google.devtools.common.options.OptionDocumentationCategory;
import com.google.devtools.common.options.OptionEffectTag;
+import com.google.devtools.common.options.OptionsParsingException;
import java.util.List;
import java.util.Map;
import org.junit.Test;
@@ -61,13 +63,46 @@
@Option(
name = "allow_multiple",
- converter = Converters.AssignmentConverter.class,
+ converter = StarlarkAssignmentConverter.class,
defaultValue = "",
allowMultiple = true,
- documentationCategory = OptionDocumentationCategory.OUTPUT_PARAMETERS,
- effectTags = {OptionEffectTag.CHANGES_INPUTS, OptionEffectTag.AFFECTS_OUTPUTS},
- help = "Each --define option specifies an assignment for a build variable.")
+ documentationCategory = OptionDocumentationCategory.UNDOCUMENTED,
+ effectTags = {OptionEffectTag.NO_OP},
+ help = "An option that mimics the behavior of --define (allowMultiple=true + converter).")
public List<Map.Entry<String, String>> allowMultiple;
+
+ @Option(
+ name = "foo",
+ defaultValue = "",
+ documentationCategory = OptionDocumentationCategory.UNDOCUMENTED,
+ effectTags = {OptionEffectTag.NO_OP},
+ help = "A simple option for basic testing.")
+ public String foo;
+
+ /** TestObject to expose to Starlark. */
+ static class FooObject {}
+
+ /** A converter from String -> FooObject for foo_object option */
+ public static class FooConverter implements Converter<FooObject> {
+ @Override
+ public FooObject convert(String input) throws OptionsParsingException {
+ return new FooObject();
+ }
+
+ @Override
+ public String getTypeDescription() {
+ return "";
+ }
+ }
+
+ @Option(
+ name = "foo_object",
+ converter = FooConverter.class,
+ defaultValue = "",
+ documentationCategory = OptionDocumentationCategory.UNDOCUMENTED,
+ effectTags = {OptionEffectTag.NO_OP},
+ help = "An option with a non-Starlark friendly java form.")
+ public FooObject fooObject;
}
/** Loads a new {link @DummyTestFragment} instance. */
@@ -94,6 +129,7 @@
ConfiguredRuleClassProvider.Builder builder = new ConfiguredRuleClassProvider.Builder();
TestRuleClassProvider.addStandardRules(builder);
builder.addConfigurationFragment(new DummyTestLoader());
+ builder.addConfigurationOptions(BuildConfiguration.Options.class);
return builder.build();
}
@@ -927,4 +963,123 @@
.containsExactly(
Maps.immutableEntry("APRIL", "SHOWERS"), Maps.immutableEntry("MAY", "FLOWERS"));
}
+
+ @Test
+ public void testReadNativeOption_allowMultipleOptions() throws Exception {
+ writeWhitelistFile();
+ scratch.file(
+ "test/transitions.bzl",
+ "def _impl(settings, attr):",
+ " if settings['//command_line_option:allow_multiple']['APRIL'] == 'FOOLS':",
+ " return {",
+ " '//command_line_option:foo': 'post-transition'",
+ " }",
+ " else:",
+ " return {",
+ " '//command_line_option:foo': ''",
+ " }",
+ "my_transition = transition(",
+ " implementation = _impl,",
+ " inputs = ['//command_line_option:allow_multiple'],",
+ " outputs = ['//command_line_option:foo'])");
+ scratch.file(
+ "test/rules.bzl",
+ "load('//test:transitions.bzl', 'my_transition')",
+ "def _impl(ctx):",
+ " return []",
+ "my_rule = rule(",
+ " implementation = _impl,",
+ " cfg = my_transition,",
+ " attrs = {",
+ " '_whitelist_function_transition': attr.label(",
+ " default = '//tools/whitelists/function_transition_whitelist',",
+ " ),",
+ " })");
+ scratch.file("test/BUILD", "load('//test:rules.bzl', 'my_rule')", "my_rule(name = 'test')");
+
+ useConfiguration("--allow_multiple=APRIL=FOOLS");
+
+ BuildConfiguration configuration = getConfiguration(getConfiguredTarget("//test"));
+ assertThat(configuration.getOptions().get(DummyTestOptions.class).foo)
+ .isEqualTo("post-transition");
+ }
+
+ @Test
+ public void testReadNativeOption_testDefine() throws Exception {
+ writeWhitelistFile();
+ scratch.file(
+ "test/transitions.bzl",
+ "def _impl(settings, attr):",
+ " if settings['//command_line_option:define']['APRIL'] == 'SHOWERS' and "
+ + "settings['//command_line_option:define']['MAY'] == 'FLOWERS':",
+ " return {'//command_line_option:foo': 'post-transition'}",
+ " else:",
+ " return {",
+ " '//command_line_option:foo': ''",
+ " }",
+ "my_transition = transition(",
+ " implementation = _impl,",
+ " inputs = ['//command_line_option:define'],",
+ " outputs = ['//command_line_option:foo'])");
+ scratch.file(
+ "test/rules.bzl",
+ "load('//test:transitions.bzl', 'my_transition')",
+ "def _impl(ctx):",
+ " return []",
+ "my_rule = rule(",
+ " implementation = _impl,",
+ " cfg = my_transition,",
+ " attrs = {",
+ " '_whitelist_function_transition': attr.label(",
+ " default = '//tools/whitelists/function_transition_whitelist',",
+ " ),",
+ " })");
+ scratch.file("test/BUILD", "load('//test:rules.bzl', 'my_rule')", "my_rule(name = 'test')");
+
+ useConfiguration("--define=APRIL=SHOWERS", "--define=MAY=MORE_SHOWERS");
+
+ BuildConfiguration configuration = getConfiguration(getConfiguredTarget("//test"));
+ assertThat(configuration.getOptions().get(DummyTestOptions.class).foo).isEmpty();
+
+ useConfiguration("--define=APRIL=SHOWERS", "--define=MAY=FLOWERS");
+
+ configuration = getConfiguration(getConfiguredTarget("//test"));
+ assertThat(configuration.getOptions().get(DummyTestOptions.class).foo)
+ .isEqualTo("post-transition");
+ }
+
+ @Test
+ public void testReadNativeOption_noStarlarkConverter() throws Exception {
+ writeWhitelistFile();
+ scratch.file(
+ "test/transitions.bzl",
+ "def _impl(settings, attr):",
+ " return {",
+ " '//command_line_option:foo_object': settings['//command_line_option:foo_object'] ",
+ " }",
+ "my_transition = transition(",
+ " implementation = _impl,",
+ " inputs = ['//command_line_option:foo_object'],",
+ " outputs = ['//command_line_option:foo_object'])");
+ scratch.file(
+ "test/rules.bzl",
+ "load('//test:transitions.bzl', 'my_transition')",
+ "def _impl(ctx):",
+ " return []",
+ "my_rule = rule(",
+ " implementation = _impl,",
+ " cfg = my_transition,",
+ " attrs = {",
+ " '_whitelist_function_transition': attr.label(",
+ " default = '//tools/whitelists/function_transition_whitelist',",
+ " ),",
+ " })");
+ scratch.file("test/BUILD", "load('//test:rules.bzl', 'my_rule')", "my_rule(name = 'test')");
+
+ reporter.removeHandler(failFastHandler);
+ getConfiguredTarget("//test");
+ assertContainsEvent(
+ "Unable to read option '//command_line_option:foo_object' - option"
+ + " '//command_line_option:foo_object' is Starlark incompatible");
+ }
}
