Skylark no longer crashes when the default value of an attribute is a label string that points to a remote repository.
Fixes #1442.
--
MOS_MIGRATED_REVID=132320130
diff --git a/src/main/java/com/google/devtools/build/lib/packages/Attribute.java b/src/main/java/com/google/devtools/build/lib/packages/Attribute.java
index fed7bd4..80bba9b 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/Attribute.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/Attribute.java
@@ -620,14 +620,24 @@
/**
* See value(TYPE) above. This method is only meant for Skylark usage.
+ *
+ * <p>The parameter {@code context} is relevant iff the default value is a Label string.
+ * In this case, {@code context} must point to the parent Label in order to be able to convert
+ * the default value string to a proper Label.
*/
- public Builder<TYPE> defaultValue(Object defaultValue) throws ConversionException {
+ public Builder<TYPE> defaultValue(Object defaultValue, Object context)
+ throws ConversionException {
Preconditions.checkState(!valueSet, "the default value is already set");
- value = type.convert(defaultValue, "attribute " + name);
+ value = type.convert(defaultValue, "attribute " + name, context);
valueSet = true;
return this;
}
+ /** See value(TYPE) above. This method is only meant for Skylark usage. */
+ public Builder<TYPE> defaultValue(Object defaultValue) throws ConversionException {
+ return defaultValue(defaultValue, null);
+ }
+
public boolean isValueSet() {
return valueSet;
}
diff --git a/src/main/java/com/google/devtools/build/lib/rules/SkylarkAttr.java b/src/main/java/com/google/devtools/build/lib/rules/SkylarkAttr.java
index ea52e11..2e4bbb6 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/SkylarkAttr.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/SkylarkAttr.java
@@ -172,7 +172,7 @@
new SkylarkComputedDefaultTemplate(
type, callback.getParameterNames(), callback, ast.getLocation()));
} else {
- builder.defaultValue(defaultValue);
+ builder.defaultValue(defaultValue, env.getGlobals().label());
}
}
diff --git a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkIntegrationTest.java b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkIntegrationTest.java
index 6d156f2..3387941 100644
--- a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkIntegrationTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkIntegrationTest.java
@@ -64,6 +64,28 @@
}
@Test
+ public void testRemoteLabelAsDefaultAttributeValue() throws Exception {
+ scratch.file(
+ "test/skylark/extension.bzl",
+ "def _impl(ctx):",
+ " pass",
+ "my_rule = rule(implementation = _impl,",
+ " attrs = { 'dep' : attr.label_list(default=[\"@r//:t\"]) })");
+
+ // We are only interested in whether the label string in the default value can be converted
+ // to a proper Label without an exception (see GitHub issue #1442).
+ // Consequently, we expect getTarget() to fail later since the repository does not exist.
+ checkError(
+ "test/skylark",
+ "the_rule",
+ "no such package '@r//': error loading package 'external': "
+ + "The repository named 'r' could not be resolved",
+ "load('/test/skylark/extension', 'my_rule')",
+ "",
+ "my_rule(name='the_rule')");
+ }
+
+ @Test
public void testSameMethodNames() throws Exception {
// The alias feature of load() may hide the fact that two methods in the stack trace have the
// same name. This is perfectly legal as long as these two methods are actually distinct.
