Automated rollback of commit 493b59e6a5eb171c5710c4751c8f4feafddd6adb.
*** Reason for rollback ***
Rollback back because we're not positive we want to support --define (2/4)
*** Original change description ***
Give --define a new Starlark friendly converter.
This allow us to read --define in starlark transitions.
RELNOTES: None.
PiperOrigin-RevId: 244241515
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 3d9729c..83aa1fd 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,7 +37,6 @@
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;
@@ -51,7 +50,6 @@
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;
@@ -323,28 +321,15 @@
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 = 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.")
+ 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."
+ )
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 d074579..c014b8a6 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,7 +17,6 @@
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;
@@ -35,8 +34,6 @@
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;
@@ -156,7 +153,7 @@
* @throws EvalException if any of the specified transition inputs do not correspond to a valid
* build setting
*/
- static <S, T> SkylarkDict<String, Object> buildSettings(
+ static SkylarkDict<String, Object> buildSettings(
BuildOptions buildOptions,
Map<String, OptionInfo> optionInfoMap,
StarlarkDefinedConfigTransition starlarkTransition)
@@ -181,29 +178,10 @@
Field field = optionInfo.getDefinition().getField();
FragmentOptions options = buildOptions.get(optionInfo.getOptionClass());
Object optionValue = field.get(options);
- 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);
+
+ dict.put(optionKey, optionValue == null ? Runtime.NONE : optionValue, null, mutability);
} catch (IllegalAccessException e) {
- // These exceptions should not happen since all options should be public, but if they
- // do, throw a RuntimeException.
+ // These exceptions should not happen, 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
deleted file mode 100644
index 6798fab..0000000
--- a/src/main/java/com/google/devtools/build/lib/analysis/skylark/StarlarkConverter.java
+++ /dev/null
@@ -1,44 +0,0 @@
-// 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 d8583b4..38e7710 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,6 +25,7 @@
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;
@@ -146,6 +147,21 @@
|| 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 4bf6281..b322d23 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 && !SkylarkType.isSkylarkAcceptable(result.getClass())) {
+ if (result != null && !EvalUtils.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 84afa81..a714782 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,11 +26,9 @@
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;
@@ -665,19 +663,6 @@
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 03ef28f..c56d4eb 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,7 +21,6 @@
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;
@@ -30,11 +29,10 @@
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.Converter;
+import com.google.devtools.common.options.Converters;
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;
@@ -63,46 +61,13 @@
@Option(
name = "allow_multiple",
- converter = StarlarkAssignmentConverter.class,
+ converter = Converters.AssignmentConverter.class,
defaultValue = "",
allowMultiple = true,
- documentationCategory = OptionDocumentationCategory.UNDOCUMENTED,
- effectTags = {OptionEffectTag.NO_OP},
- help = "An option that mimics the behavior of --define (allowMultiple=true + converter).")
+ 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>> 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. */
@@ -129,7 +94,6 @@
ConfiguredRuleClassProvider.Builder builder = new ConfiguredRuleClassProvider.Builder();
TestRuleClassProvider.addStandardRules(builder);
builder.addConfigurationFragment(new DummyTestLoader());
- builder.addConfigurationOptions(BuildConfiguration.Options.class);
return builder.build();
}
@@ -963,123 +927,4 @@
.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");
- }
}
