diff options
| -rw-r--r-- | src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java | 21 | ||||
| -rw-r--r-- | src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleContextTest.java | 17 |
2 files changed, 36 insertions, 2 deletions
diff --git a/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java b/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java index 824ecc1377..45d1e0e143 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java +++ b/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java @@ -892,10 +892,12 @@ public final class PackageFactory { * Returns null if we don't want to export the value. * * <p>All of the types returned are immutable. If we want, we can change this to - * immutable in the future, but this is the safe choice for now.o + * immutable in the future, but this is the safe choice for now. */ @Nullable private static Object skylarkifyValue(Object val, Package pkg) throws NotRepresentableException { + // TODO(bazel-team): the location of this function is ad-hoc. Arguably, the conversion + // from Java native types to Skylark types should be part of the Type class hierarchy, if (val == null) { return null; } @@ -909,7 +911,6 @@ public final class PackageFactory { return val; } - // Maybe we should have an interface for types so they can represent themselves to skylark? if (val instanceof TriState) { switch ((TriState) val) { case AUTO: @@ -972,6 +973,22 @@ public final class PackageFactory { return null; } + if (val instanceof BuildType.SelectorList) { + // This is terrible: + // 1) this value is opaque, and not a BUILD value, so it cannot be used in rule arguments + // 2) its representation has a pointer address, so it breaks hermeticity. + // + // Even though this is clearly imperfect, we return this value because otherwise + // native.rules() fails if there is any rule using a select() in the BUILD file. + // + // To remedy this, we should return a syntax.SelectorList. To do so, we have to + // 1) recurse into the Selector contents of SelectorList, so those values are skylarkified too + // 2) get the right Class<?> value. We could probably get at that by looking at + // ((SelectorList)val).getSelectors().first().getEntries().first().getClass(). + + return val; + } + // We are explicit about types we don't understand so we minimize changes to existing callers // if we add more types that we can represent. throw new NotRepresentableException( diff --git a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleContextTest.java b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleContextTest.java index 088a542c89..240b5878b1 100644 --- a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleContextTest.java +++ b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleContextTest.java @@ -354,6 +354,23 @@ public class SkylarkRuleContextTest extends SkylarkTestCase { } @Test + public void testGetRuleSelect() throws Exception { + scratch.file("test/skylark/BUILD"); + scratch.file("test/skylark/rulestr.bzl", "def rule_dict(name):", " return native.rule(name)"); + + scratch.file( + "test/getrule/BUILD", + "load('/test/skylark/rulestr', 'rule_dict')", + "cc_library(name ='x', ", + " srcs = select({'//conditions:default': []})", + ")", + "rule_dict('x')"); + + // Parse the BUILD file, to make sure select() makes it out of native.rule(). + createRuleContext("//test/getrule:x"); + } + + @Test public void testGetRule() throws Exception { scratch.file("test/skylark/BUILD"); scratch.file( |