Allow ' ', '(', ')' and '$' in labels
This just add the special characters in labels and fixes the
associated tests, left is the hard part to test adding
those characters everywhere.
This is experimental and several characters will break at several
location especial in the runfiles manifest file.
Follow-ups: Resolve quoting then test, test more and add even more tests.
Issue found during development:
Parentheses are not accepted in exclude pattern in globs
Building a binary includes build-runfiles that relies on the runfiles
manifest format so the added test would fails with a java_binary
instead of a library.
--
Change-Id: I9c87273a90318b931c61bdb86f1066962819960a
Reviewed-on: https://cr.bazel.build/9055
PiperOrigin-RevId: 149108027
MOS_MIGRATED_REVID=149108027
diff --git a/src/test/java/com/google/devtools/build/lib/cmdline/LabelTest.java b/src/test/java/com/google/devtools/build/lib/cmdline/LabelTest.java
index 3766a76..6980505 100644
--- a/src/test/java/com/google/devtools/build/lib/cmdline/LabelTest.java
+++ b/src/test/java/com/google/devtools/build/lib/cmdline/LabelTest.java
@@ -279,8 +279,6 @@
@Test
public void testBadCharacters() throws Exception {
- assertSyntaxError("package names may contain only",
- "//foo/bar baz");
assertSyntaxError("target names may not contain ':'",
"//foo:bar:baz");
assertSyntaxError("target names may not contain ':'",
@@ -289,12 +287,6 @@
"//foo/bar::");
assertSyntaxError("target names may not contain '&'",
"//foo:bar&");
- assertSyntaxError("target names may not contain '$'",
- "//foo/bar:baz$a");
- assertSyntaxError("target names may not contain '('",
- "//foo/bar:baz(foo)");
- assertSyntaxError("target names may not contain ')'",
- "//foo/bar:bazfoo)");
// Warning: if these assertions are false, tools that assume that they can safely quote labels
// may need to be fixed. Please consult with bazel-dev before loosening these restrictions.
assertSyntaxError("target names may not contain '''", "//foo/bar:baz'foo");
@@ -357,6 +349,7 @@
Label.parseAbsolute("//package:foo.bar");
Label.parseAbsolute("//package:foo@bar");
Label.parseAbsolute("//package:foo~bar");
+ Label.parseAbsolute("//$( ):$( )");
}
/**
diff --git a/src/test/java/com/google/devtools/build/lib/cmdline/LabelValidatorTest.java b/src/test/java/com/google/devtools/build/lib/cmdline/LabelValidatorTest.java
index fcf887a..17e6ba5 100644
--- a/src/test/java/com/google/devtools/build/lib/cmdline/LabelValidatorTest.java
+++ b/src/test/java/com/google/devtools/build/lib/cmdline/LabelValidatorTest.java
@@ -53,17 +53,15 @@
assertNull(LabelValidator.validatePackageName("a/.b"));
assertNull(LabelValidator.validatePackageName("a/b."));
assertNull(LabelValidator.validatePackageName("a/b.."));
+ assertNull(LabelValidator.validatePackageName("a$( )/b.."));
// Bad:
assertEquals(
"package names may not start with '/'", LabelValidator.validatePackageName("/foo"));
assertEquals("package names may not end with '/'", LabelValidator.validatePackageName("foo/"));
- assertEquals(LabelValidator.PACKAGE_NAME_ERROR, LabelValidator.validatePackageName("bar baz"));
assertEquals(LabelValidator.PACKAGE_NAME_ERROR, LabelValidator.validatePackageName("foo:bar"));
assertEquals(
LabelValidator.PACKAGE_NAME_ERROR, LabelValidator.validatePackageName("baz@12345"));
- assertEquals(LabelValidator.PACKAGE_NAME_ERROR, LabelValidator.validatePackageName("baz(foo)"));
- assertEquals(LabelValidator.PACKAGE_NAME_ERROR, LabelValidator.validatePackageName("bazfoo)"));
assertEquals(
LabelValidator.PACKAGE_NAME_DOT_ERROR, LabelValidator.validatePackageName("bar/../baz"));
@@ -97,20 +95,12 @@
assertEquals("target names may not end with '/'",
LabelValidator.validateTargetName("foo/"));
- assertEquals("target names may not contain ' '",
- LabelValidator.validateTargetName("bar baz"));
assertEquals("target names may not contain ':'",
LabelValidator.validateTargetName("bar:baz"));
assertEquals("target names may not contain ':'",
LabelValidator.validateTargetName("bar:"));
assertEquals("target names may not contain '&'",
LabelValidator.validateTargetName("bar&"));
- assertEquals("target names may not contain '$'",
- LabelValidator.validateTargetName("baz$a"));
- assertEquals("target names may not contain '('",
- LabelValidator.validateTargetName("baz(foo)"));
- assertEquals("target names may not contain ')'",
- LabelValidator.validateTargetName("bazfoo)"));
}
@Test
@@ -122,6 +112,13 @@
LabelValidator.validateAbsoluteLabel("@repo//foo:bar"));
assertEquals(new PackageAndTarget("foo", "bar"),
LabelValidator.validateAbsoluteLabel("@//foo:bar"));
+ emptyPackage = new PackageAndTarget("", "b$() ar");
+ assertEquals(emptyPackage, LabelValidator.validateAbsoluteLabel("//:b$() ar"));
+ assertEquals(emptyPackage, LabelValidator.validateAbsoluteLabel("@repo//:b$() ar"));
+ assertEquals(new PackageAndTarget("f$( )oo", "b$() ar"),
+ LabelValidator.validateAbsoluteLabel("@repo//f$( )oo:b$() ar"));
+ assertEquals(new PackageAndTarget("f$( )oo", "b$() ar"),
+ LabelValidator.validateAbsoluteLabel("@//f$( )oo:b$() ar"));
}
@Test
diff --git a/src/test/java/com/google/devtools/build/lib/packages/BuildTypeTest.java b/src/test/java/com/google/devtools/build/lib/packages/BuildTypeTest.java
index 978f203..c5b0d31 100644
--- a/src/test/java/com/google/devtools/build/lib/packages/BuildTypeTest.java
+++ b/src/test/java/com/google/devtools/build/lib/packages/BuildTypeTest.java
@@ -294,14 +294,13 @@
@Test
public void testSelectorWrongType() throws Exception {
ImmutableMap<String, String> input = ImmutableMap.of(
- "//conditions:a", "not a label",
- "//conditions:b", "also not a label",
+ "//conditions:a", "not a/../label", "//conditions:b", "also not a/../label",
BuildType.Selector.DEFAULT_CONDITION_KEY, "whatever");
try {
new Selector<>(input, null, currentRule, BuildType.LABEL);
fail("Expected Selector instantiation to fail since the input isn't a selection of labels");
} catch (ConversionException e) {
- assertThat(e.getMessage()).contains("invalid label 'not a label'");
+ assertThat(e.getMessage()).contains("invalid label 'not a/../label'");
}
}
@@ -311,13 +310,13 @@
@Test
public void testSelectorKeyIsNotALabel() throws Exception {
ImmutableMap<String, String> input = ImmutableMap.of(
- "not a label", "//a:a",
+ "not a/../label", "//a:a",
BuildType.Selector.DEFAULT_CONDITION_KEY, "whatever");
try {
new Selector<>(input, null, currentRule, BuildType.LABEL);
fail("Expected Selector instantiation to fail since the key isn't a label");
} catch (ConversionException e) {
- assertThat(e.getMessage()).contains("invalid label 'not a label'");
+ assertThat(e.getMessage()).contains("invalid label 'not a/../label'");
}
}
diff --git a/src/test/java/com/google/devtools/build/lib/packages/PackageFactoryTest.java b/src/test/java/com/google/devtools/build/lib/packages/PackageFactoryTest.java
index 836ed6c..3573827 100644
--- a/src/test/java/com/google/devtools/build/lib/packages/PackageFactoryTest.java
+++ b/src/test/java/com/google/devtools/build/lib/packages/PackageFactoryTest.java
@@ -114,13 +114,16 @@
@Test
public void testBadPackageName() throws Exception {
try {
- packages.createPackage("not even a legal label", emptyBuildFile("not even a legal label"));
+ // PathFragment parsing de-double slashes, and normalization of the path fragment removes
+ // up reference (/../), so use triple dot /.../ that will always be a forbidden package name.
+ packages.createPackage("not even a legal/.../label",
+ emptyBuildFile("not even a legal/.../label"));
fail();
} catch (NoSuchPackageException e) {
assertThat(e.getMessage())
.contains(
- "no such package 'not even a legal label': "
- + "illegal package name: 'not even a legal label' ");
+ "no such package 'not even a legal/.../label': "
+ + "illegal package name: 'not even a legal/.../label' ");
}
}
diff --git a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleClassFunctionsTest.java b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleClassFunctionsTest.java
index 2966fc8..f8b3d16 100644
--- a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleClassFunctionsTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleClassFunctionsTest.java
@@ -801,8 +801,8 @@
@Test
public void testLabelGetRelativeSyntaxError() throws Exception {
checkErrorContains(
- "invalid target name 'bad syntax': target names may not contain ' '",
- "Label('//foo:bar').relative('bad syntax')");
+ "invalid target name 'bad//syntax': target names may not contain '//' path separators",
+ "Label('//foo:bar').relative('bad//syntax')");
}
@Test
diff --git a/src/test/java/com/google/devtools/build/lib/syntax/TypeTest.java b/src/test/java/com/google/devtools/build/lib/syntax/TypeTest.java
index a4e42c7..26d9a39 100644
--- a/src/test/java/com/google/devtools/build/lib/syntax/TypeTest.java
+++ b/src/test/java/com/google/devtools/build/lib/syntax/TypeTest.java
@@ -256,10 +256,10 @@
@Test
public void testInvalidLabel() throws Exception {
try {
- BuildType.LABEL.convert("not a label", null, currentRule);
+ BuildType.LABEL.convert("not//a label", null, currentRule);
fail();
} catch (Type.ConversionException e) {
- MoreAsserts.assertContainsWordsWithQuotes(e.getMessage(), "not a label");
+ MoreAsserts.assertContainsWordsWithQuotes(e.getMessage(), "not//a label");
}
}
