From c4f2d80270f1ce947fcf7fb0a4e5f0afb3a7062d Mon Sep 17 00:00:00 2001 From: twerth Date: Tue, 15 May 2018 05:02:31 -0700 Subject: [PATCH] Allow more characters in labels. Partly addresses #374. Specifically allow !%^`"'&;<>?[]{|} in target and package names. It's actually simpler now to declare what we don't allow. In target names: 0-31 (control characters) 58 ':' (colon) 92 '\' (backslash) 127 (delete) In package names: 0-31 (control characters) 58 ':' (colon) 64 '@' (at-sign) 92 '\' (backslash) 127 (delete) - '\' is a path segment separator on Windows, and allowing it can lead to silent output file conflicts and - therefore - data corruption. We may be able to allow it in the future, but I didn't want to make that call. - ':' is a special character that Bazel interprets as the package name / target name separator. - '@' in package names can probably be allowed; at the beginning of a label it indicates a workspace name, but not within a segment. We actually have some tests that disallow it specifically, but those can probably just be deleted; however, it does require a bit of investigation, so I decided to delay that change. It is possible that we don't correctly escape filenames in all cases. Also note that the shell may require escaping for specific characters, and that Bazel treats a single '*' (star) target name specially when given on the command line. RELNOTES: Bazel now allows almost all 7-bit ASCII characters in labels. PiperOrigin-RevId: 196650651 --- .../build/lib/cmdline/LabelValidator.java | 28 +++++--- .../build/lib/packages/GlobCache.java | 4 ++ .../devtools/build/lib/cmdline/LabelTest.java | 11 +--- .../build/lib/cmdline/LabelValidatorTest.java | 66 +++++++++++++++++-- .../build/lib/cmdline/TargetPatternTest.java | 2 +- .../build/lib/packages/OutputFileTest.java | 4 +- .../build/lib/pkgcache/PackageCacheTest.java | 6 +- .../pkgcache/TargetPatternEvaluatorTest.java | 2 - .../skyframe/PackageLookupFunctionTest.java | 4 +- 9 files changed, 94 insertions(+), 33 deletions(-) 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 a2f419eb2303cb..633946c60fd736 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 @@ -24,8 +24,14 @@ */ public final class LabelValidator { + // Target names allow all 7-bit ASCII characters except + // 0-31 (control characters) + // 58 ':' (colon) + // 92 '\' (backslash) - directory separator (on Windows); may be allowed in the future + // 127 (delete) /** 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. @@ -33,16 +39,21 @@ public final class LabelValidator { * Note that . is also allowed in target names, and doesn't require quoting, but has restrictions * on its surrounding characters; see {@link #validateTargetName(String)}. */ - private static final CharMatcher PUNCTUATION_NOT_REQUIRING_QUOTING = CharMatcher.anyOf("_@-"); - - /** - * Matches characters allowed in package name. - */ + private static final CharMatcher PUNCTUATION_NOT_REQUIRING_QUOTING = + CharMatcher.anyOf("!%-@^_`"); + + // Package names allow all 7-bit ASCII characters except + // 0-31 (control characters) + // 58 ':' (colon) - target name separator + // 64 '@' (at-sign) - workspace name prefix + // 92 '\' (backslash) - directory separator (on Windows); may be allowed in the future + // 127 (delete) + /** 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 +70,8 @@ public final class LabelValidator { @VisibleForTesting static final String PACKAGE_NAME_ERROR = - "package names may contain only A-Z, a-z, 0-9, '/', '-', '.', ' ', '$', '(', ')' and '_'"; + "package names may contain A-Z, a-z, 0-9, or any of ' !\"#$%&'()*+,-./;<=>?[]^_`{|}~'" + + " (most 127-bit ascii characters except 0-31, 127, ':', '@', or '\\')"; @VisibleForTesting static final String PACKAGE_NAME_DOT_ERROR = diff --git a/src/main/java/com/google/devtools/build/lib/packages/GlobCache.java b/src/main/java/com/google/devtools/build/lib/packages/GlobCache.java index a486ddbf004b69..f9d885ded0ad8b 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/GlobCache.java +++ b/src/main/java/com/google/devtools/build/lib/packages/GlobCache.java @@ -174,6 +174,10 @@ protected List getGlobUnsorted(String pattern, boolean excludeDirs) // invalid as a label, plus users should say explicitly if they // really want to name the package directory. if (!relative.isEmpty()) { + if (relative.charAt(0) == '@') { + // Add explicit colon to disambiguate from external repository. + relative = ":" + relative; + } result.add(relative); } } 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 7e34cc64e8fcc4..be61b9472a6309 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 @@ -30,7 +30,8 @@ @RunWith(JUnit4.class) public class LabelTest { - private static final String BAD_PACKAGE_CHARS = "package names may contain only"; + private static final String BAD_PACKAGE_CHARS = + "package names may contain A-Z, a-z, 0-9, or any of"; private static final String INVALID_TARGET_NAME = "invalid target name"; private static final String INVALID_PACKAGE_NAME = "invalid package name"; @@ -89,7 +90,7 @@ public void testLabelResolutionAbsolutePath() throws Exception { @Test public void testLabelResolutionBadSyntax() throws Exception { try { - parseCommandLine("//absolute:A+bad%syntax", ""); + parseCommandLine("//absolute:A+bad:syntax", ""); fail(); } catch (LabelSyntaxException e) { // Expected exception @@ -303,12 +304,6 @@ public void testBadCharacters() throws Exception { "//foo:bar:"); assertSyntaxError("target names may not contain ':'", "//foo/bar::"); - assertSyntaxError("target names may not contain '&'", - "//foo:bar&"); - // 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"); - assertSyntaxError("target names may not contain '\"'", "//foo/bar:baz\"foo"); } @Test 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 2156ddf84d12b3..fcb187f268ffc6 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,6 +53,37 @@ public void testValidatePackageName() throws Exception { assertThat(LabelValidator.validatePackageName("a/b..")).isNull(); assertThat(LabelValidator.validatePackageName("a$( )/b..")).isNull(); + // These are in ascii code order. + assertThat(LabelValidator.validatePackageName("foo!bar")).isNull(); + assertThat(LabelValidator.validatePackageName("foo\"bar")).isNull(); + assertThat(LabelValidator.validatePackageName("foo#bar")).isNull(); + assertThat(LabelValidator.validatePackageName("foo$bar")).isNull(); + assertThat(LabelValidator.validatePackageName("foo%bar")).isNull(); + assertThat(LabelValidator.validatePackageName("foo&bar")).isNull(); + assertThat(LabelValidator.validatePackageName("foo'bar")).isNull(); + assertThat(LabelValidator.validatePackageName("foo(bar")).isNull(); + assertThat(LabelValidator.validatePackageName("foo)bar")).isNull(); + assertThat(LabelValidator.validatePackageName("foo*bar")).isNull(); + assertThat(LabelValidator.validatePackageName("foo+bar")).isNull(); + assertThat(LabelValidator.validatePackageName("foo,bar")).isNull(); + assertThat(LabelValidator.validatePackageName("foo-bar")).isNull(); + assertThat(LabelValidator.validatePackageName("foo.bar")).isNull(); + assertThat(LabelValidator.validatePackageName("foo+bar")).isNull(); + assertThat(LabelValidator.validatePackageName("foo;bar")).isNull(); + assertThat(LabelValidator.validatePackageName("foobar")).isNull(); + assertThat(LabelValidator.validatePackageName("foo?bar")).isNull(); + assertThat(LabelValidator.validatePackageName("foo[bar")).isNull(); + assertThat(LabelValidator.validatePackageName("foo]bar")).isNull(); + assertThat(LabelValidator.validatePackageName("foo^bar")).isNull(); + assertThat(LabelValidator.validatePackageName("foo_bar")).isNull(); + assertThat(LabelValidator.validatePackageName("foo`bar")).isNull(); + assertThat(LabelValidator.validatePackageName("foo{bar")).isNull(); + assertThat(LabelValidator.validatePackageName("foo|bar")).isNull(); + assertThat(LabelValidator.validatePackageName("foo}bar")).isNull(); + assertThat(LabelValidator.validatePackageName("foo~bar")).isNull(); + // Bad: assertThat(LabelValidator.validatePackageName("/foo")) .isEqualTo("package names may not start with '/'"); @@ -82,16 +113,39 @@ public void testValidatePackageName() throws Exception { @Test public void testValidateTargetName() throws Exception { - assertThat(LabelValidator.validateTargetName("foo")).isNull(); + assertThat(LabelValidator.validateTargetName("foo!bar")).isNull(); + assertThat(LabelValidator.validateTargetName("foo\"bar")).isNull(); + assertThat(LabelValidator.validateTargetName("foo#bar")).isNull(); + assertThat(LabelValidator.validateTargetName("foo$bar")).isNull(); + assertThat(LabelValidator.validateTargetName("foo%bar")).isNull(); + assertThat(LabelValidator.validateTargetName("foo&bar")).isNull(); + assertThat(LabelValidator.validateTargetName("foo'bar")).isNull(); + assertThat(LabelValidator.validateTargetName("foo(bar")).isNull(); + assertThat(LabelValidator.validateTargetName("foo)bar")).isNull(); + assertThat(LabelValidator.validateTargetName("foo*bar")).isNull(); assertThat(LabelValidator.validateTargetName("foo+bar")).isNull(); - assertThat(LabelValidator.validateTargetName("foo_bar")).isNull(); - assertThat(LabelValidator.validateTargetName("foo=bar")).isNull(); + assertThat(LabelValidator.validateTargetName("foo,bar")).isNull(); assertThat(LabelValidator.validateTargetName("foo-bar")).isNull(); assertThat(LabelValidator.validateTargetName("foo.bar")).isNull(); - assertThat(LabelValidator.validateTargetName("foo@bar")).isNull(); + assertThat(LabelValidator.validateTargetName("foo+bar")).isNull(); + assertThat(LabelValidator.validateTargetName("foo;bar")).isNull(); + assertThat(LabelValidator.validateTargetName("foobar")).isNull(); + assertThat(LabelValidator.validateTargetName("foo?bar")).isNull(); + assertThat(LabelValidator.validateTargetName("foo[bar")).isNull(); + assertThat(LabelValidator.validateTargetName("foo]bar")).isNull(); + assertThat(LabelValidator.validateTargetName("foo^bar")).isNull(); + assertThat(LabelValidator.validateTargetName("foo_bar")).isNull(); + assertThat(LabelValidator.validateTargetName("foo`bar")).isNull(); + assertThat(LabelValidator.validateTargetName("foo{bar")).isNull(); + assertThat(LabelValidator.validateTargetName("foo|bar")).isNull(); + assertThat(LabelValidator.validateTargetName("foo}bar")).isNull(); assertThat(LabelValidator.validateTargetName("foo~bar")).isNull(); - assertThat(LabelValidator.validateTargetName("foo#bar")).isNull(); + + assertThat(LabelValidator.validateTargetName("foo/bar")).isNull(); + assertThat(LabelValidator.validateTargetName("foo@bar")).isNull(); assertThat(LabelValidator.validateTargetName("foo/")) .isEqualTo("target names may not end with '/'"); @@ -99,8 +153,6 @@ public void testValidateTargetName() throws Exception { .isEqualTo("target names may not contain ':'"); assertThat(LabelValidator.validateTargetName("bar:")) .isEqualTo("target names may not contain ':'"); - assertThat(LabelValidator.validateTargetName("bar&")) - .isEqualTo("target names may not contain '&'"); } @Test diff --git a/src/test/java/com/google/devtools/build/lib/cmdline/TargetPatternTest.java b/src/test/java/com/google/devtools/build/lib/cmdline/TargetPatternTest.java index 012e120139a627..e33d1f37c374c0 100644 --- a/src/test/java/com/google/devtools/build/lib/cmdline/TargetPatternTest.java +++ b/src/test/java/com/google/devtools/build/lib/cmdline/TargetPatternTest.java @@ -50,7 +50,7 @@ public void testPassingValidations() throws TargetParsingException { @Test public void testInvalidPatterns() throws TargetParsingException { try { - parse("Bar&&&java"); + parse("Bar@java"); fail(); } catch (TargetParsingException expected) { } diff --git a/src/test/java/com/google/devtools/build/lib/packages/OutputFileTest.java b/src/test/java/com/google/devtools/build/lib/packages/OutputFileTest.java index 57b7c59090ca36..c422f77e2f0a47 100644 --- a/src/test/java/com/google/devtools/build/lib/packages/OutputFileTest.java +++ b/src/test/java/com/google/devtools/build/lib/packages/OutputFileTest.java @@ -167,7 +167,7 @@ public void testOutputFileWithIllegalName() throws Exception { "bad_out_name/BUILD", "genrule(name='a',", " cmd='ls',", - " outs=['!@#'])"); + " outs=['!@#:'])"); reporter.removeHandler(failFastHandler); packageFactory.createPackageForTesting( @@ -175,7 +175,7 @@ public void testOutputFileWithIllegalName() throws Exception { buildfile, getPackageManager(), reporter); - assertContainsEvent("illegal output file name '!@#' in rule //bad_out_name:a"); + assertContainsEvent("illegal output file name '!@#:' in rule //bad_out_name:a"); } @Test diff --git a/src/test/java/com/google/devtools/build/lib/pkgcache/PackageCacheTest.java b/src/test/java/com/google/devtools/build/lib/pkgcache/PackageCacheTest.java index e4729d3516504c..ba07b6841ad316 100644 --- a/src/test/java/com/google/devtools/build/lib/pkgcache/PackageCacheTest.java +++ b/src/test/java/com/google/devtools/build/lib/pkgcache/PackageCacheTest.java @@ -236,10 +236,10 @@ public void testGetNonexistentPackage() throws Exception { @Test public void testGetPackageWithInvalidName() throws Exception { - scratch.file("invalidpackagename&42/BUILD", "cc_library(name = 'foo') # a BUILD file"); + scratch.file("invalidpackagename:42/BUILD", "cc_library(name = 'foo') # a BUILD file"); checkGetPackageFails( - "invalidpackagename&42", - "no such package 'invalidpackagename&42': Invalid package name 'invalidpackagename&42'"); + "invalidpackagename:42", + "no such package 'invalidpackagename:42': Invalid package name 'invalidpackagename:42'"); } @Test diff --git a/src/test/java/com/google/devtools/build/lib/pkgcache/TargetPatternEvaluatorTest.java b/src/test/java/com/google/devtools/build/lib/pkgcache/TargetPatternEvaluatorTest.java index f6ec361cd6c16e..a7f81a23e0a4ae 100644 --- a/src/test/java/com/google/devtools/build/lib/pkgcache/TargetPatternEvaluatorTest.java +++ b/src/test/java/com/google/devtools/build/lib/pkgcache/TargetPatternEvaluatorTest.java @@ -712,8 +712,6 @@ public void testPassingValidations() { public void testFailingValidations() { expectValidationFail(""); expectValidationFail("\\"); - expectValidationFail("foo:**"); - expectValidationFail("//foo/*"); } private void expectValidationFail(String target) { diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/PackageLookupFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/PackageLookupFunctionTest.java index 195d987fcad1cf..4d6339922fd79c 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/PackageLookupFunctionTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/PackageLookupFunctionTest.java @@ -243,8 +243,8 @@ public void testBlacklistedPackage() throws Exception { @Test public void testInvalidPackageName() throws Exception { - scratch.file("parentpackage/invalidpackagename%42/BUILD"); - PackageLookupValue packageLookupValue = lookupPackage("parentpackage/invalidpackagename%42"); + scratch.file("parentpackage/invalidpackagename:42/BUILD"); + PackageLookupValue packageLookupValue = lookupPackage("parentpackage/invalidpackagename:42"); assertThat(packageLookupValue.packageExists()).isFalse(); assertThat(packageLookupValue.getErrorReason()).isEqualTo(ErrorReason.INVALID_PACKAGE_NAME); assertThat(packageLookupValue.getErrorMsg()).isNotNull();