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/main/java/com/google/devtools/build/lib/cmdline/LabelValidator.java b/src/main/java/com/google/devtools/build/lib/cmdline/LabelValidator.java
index 035bfbb..a2f419e 100644
--- a/src/main/java/com/google/devtools/build/lib/cmdline/LabelValidator.java
+++ b/src/main/java/com/google/devtools/build/lib/cmdline/LabelValidator.java
@@ -25,7 +25,7 @@
public final class LabelValidator {
/** Matches punctuation in target names which requires quoting in a blaze query. */
- private static final CharMatcher PUNCTUATION_REQUIRING_QUOTING = CharMatcher.anyOf("+,=~#");
+ private static final CharMatcher PUNCTUATION_REQUIRING_QUOTING = CharMatcher.anyOf("+,=~# ()$");
/**
* Matches punctuation in target names which doesn't require quoting in a blaze query.
@@ -36,13 +36,13 @@
private static final CharMatcher PUNCTUATION_NOT_REQUIRING_QUOTING = CharMatcher.anyOf("_@-");
/**
- * Matches characters allowed in package name (allowed are A-Z, a-z, 0-9, '/', '-', '.' and '_')
+ * Matches characters allowed in package name.
*/
private static final CharMatcher ALLOWED_CHARACTERS_IN_PACKAGE_NAME =
CharMatcher.inRange('0', '9')
.or(CharMatcher.inRange('a', 'z'))
.or(CharMatcher.inRange('A', 'Z'))
- .or(CharMatcher.anyOf("/-._"))
+ .or(CharMatcher.anyOf("/-._ $()"))
.precomputed();
/**
@@ -59,7 +59,7 @@
@VisibleForTesting
static final String PACKAGE_NAME_ERROR =
- "package names may contain only A-Z, a-z, 0-9, '/', '-', '.' and '_'";
+ "package names may contain only A-Z, a-z, 0-9, '/', '-', '.', ' ', '$', '(', ')' and '_'";
@VisibleForTesting
static final String PACKAGE_NAME_DOT_ERROR =
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");
}
}
diff --git a/src/test/shell/bazel/BUILD b/src/test/shell/bazel/BUILD
index 0e73552..38029e0 100644
--- a/src/test/shell/bazel/BUILD
+++ b/src/test/shell/bazel/BUILD
@@ -82,6 +82,14 @@
)
sh_test(
+ name = "bazel_random_characters_test",
+ size = "large",
+ srcs = ["bazel_random_characters_test.sh"],
+ data = [":test-deps"],
+ tags = ["nowindows"],
+)
+
+sh_test(
name = "bazel_java_test",
size = "large",
srcs = ["bazel_java_test.sh"],
diff --git a/src/test/shell/bazel/bazel_random_characters_test.sh b/src/test/shell/bazel/bazel_random_characters_test.sh
new file mode 100755
index 0000000..59f4b89
--- /dev/null
+++ b/src/test/shell/bazel/bazel_random_characters_test.sh
@@ -0,0 +1,63 @@
+#!/bin/bash
+#
+# Copyright 2016 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.
+#
+# Tests the examples provided in Bazel
+#
+
+# Load the test setup defined in the parent directory
+CURRENT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
+source "${CURRENT_DIR}/../integration_test_setup.sh" \
+ || { echo "integration_test_setup.sh not found!" >&2; exit 1; }
+
+function basic_glob_scenario_test_template() {
+ local chars="$1"
+ local pkg="pkg${chars}"
+ echo "chars = ${chars}, pkg = ${pkg}"
+ mkdir -p "${pkg}/resources"
+ cat >"${pkg}/BUILD" <<EOF
+java_library(name = 'main',
+ resources = glob(["resources/**"]),
+ srcs = ['Main.java'])
+EOF
+
+ for i in $(seq 1 10); do
+ cat >"${pkg}/resources/file${chars}$i" <<EOF
+file${chars}$i
+EOF
+ done
+
+ cat >"$pkg/Main.java" <<'EOF'
+public class Main {
+ public static void main(String[] args) {
+ System.out.println("Hello, World!");
+ }
+}
+EOF
+
+ bazel build "//${pkg}:main" &>"${TEST_log}" \
+ || fail "Failed to build java target"
+
+ nb_files="$(unzip -l "bazel-bin/${pkg}/libmain.jar" \
+ | grep -F "file${chars}" | tee "${TEST_log}" | wc -l | xargs echo)"
+ [ "10" = "${nb_files}" ] || fail "Expected 10 files, got ${nb_files}"
+}
+
+function test_space_dollar_and_parentheses() {
+ basic_glob_scenario_test_template '$( )'
+}
+
+run_suite "Integration tests for handling of special characters"
+
