Automated rollback of commit 5a9befc5602e71f7512074c303afbdcff5617cca. RELNOTES: None *** Reason for rollback *** PiperOrigin-RevId: 201686843
diff --git a/src/main/java/com/google/devtools/build/lib/cmdline/Label.java b/src/main/java/com/google/devtools/build/lib/cmdline/Label.java index 91106b2..9899f2c 100644 --- a/src/main/java/com/google/devtools/build/lib/cmdline/Label.java +++ b/src/main/java/com/google/devtools/build/lib/cmdline/Label.java
@@ -155,22 +155,17 @@ repo = absName; absName = "//:" + absName.substring(1); } - String error = RepositoryName.validate(repo); - if (error != null) { - throw new LabelSyntaxException( - "invalid repository name '" + StringUtilities.sanitizeControlChars(repo) + "': " + error); - } try { LabelValidator.PackageAndTarget labelParts = LabelValidator.parseAbsoluteLabel(absName); - PackageIdentifier pkgId = - validatePackageName( - labelParts.getPackageName(), labelParts.getTargetName(), repo, repositoryMapping); - PathFragment packageFragment = pkgId.getPackageFragment(); + PackageIdentifier pkgIdWithoutRepo = + validatePackageName(labelParts.getPackageName(), labelParts.getTargetName()); + PathFragment packageFragment = pkgIdWithoutRepo.getPackageFragment(); if (repo.isEmpty() && ABSOLUTE_PACKAGE_NAMES.contains(packageFragment)) { - pkgId = - PackageIdentifier.create(getGlobalRepoName("@", repositoryMapping), packageFragment); + repo = "@"; } - return create(pkgId, labelParts.getTargetName()); + RepositoryName globalRepoName = getGlobalRepoName(repo, repositoryMapping); + return create( + PackageIdentifier.create(globalRepoName, packageFragment), labelParts.getTargetName()); } catch (BadLabelException e) { throw new LabelSyntaxException(e.getMessage()); } @@ -294,25 +289,15 @@ return name; } - private static PackageIdentifier validatePackageName(String packageIdentifier, String name) - throws LabelSyntaxException { - return validatePackageName( - packageIdentifier, name, /* repo= */ null, /* repositoryMapping= */ null); - } - /** * Validates the given package name and returns a canonical {@link PackageIdentifier} instance if * it is valid. Otherwise it throws a SyntaxException. */ - private static PackageIdentifier validatePackageName( - String packageIdentifier, - String name, - String repo, - ImmutableMap<RepositoryName, RepositoryName> repositoryMapping) + private static PackageIdentifier validatePackageName(String packageIdentifier, String name) throws LabelSyntaxException { String error = null; try { - return PackageIdentifier.parse(packageIdentifier, repo, repositoryMapping); + return PackageIdentifier.parse(packageIdentifier); } catch (LabelSyntaxException e) { error = e.getMessage(); error = "invalid package name '" + packageIdentifier + "': " + error;
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 e63b096..633946c 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
@@ -45,6 +45,7 @@ // 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. */ @@ -52,7 +53,7 @@ CharMatcher.inRange('0', '9') .or(CharMatcher.inRange('a', 'z')) .or(CharMatcher.inRange('A', 'Z')) - .or(CharMatcher.anyOf(" !\"#$%&'()*+,-./;<=>?@[]^_`{|}~")) + .or(CharMatcher.anyOf(" !\"#$%&'()*+,-./;<=>?[]^_`{|}~")) .precomputed(); /** @@ -70,7 +71,7 @@ @VisibleForTesting static final String PACKAGE_NAME_ERROR = "package names may contain A-Z, a-z, 0-9, or any of ' !\"#$%&'()*+,-./;<=>?[]^_`{|}~'" - + " (most 127-bit ascii characters except 0-31, 127, ':', or '\\')"; + + " (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/cmdline/PackageIdentifier.java b/src/main/java/com/google/devtools/build/lib/cmdline/PackageIdentifier.java index 732e6a0..8d7ad19 100644 --- a/src/main/java/com/google/devtools/build/lib/cmdline/PackageIdentifier.java +++ b/src/main/java/com/google/devtools/build/lib/cmdline/PackageIdentifier.java
@@ -16,7 +16,6 @@ import com.google.common.base.Preconditions; import com.google.common.collect.ComparisonChain; -import com.google.common.collect.ImmutableMap; import com.google.common.collect.Interner; import com.google.devtools.build.lib.concurrent.BlazeInterners; import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec; @@ -120,17 +119,10 @@ } public static PackageIdentifier parse(String input) throws LabelSyntaxException { - return parse(input, /* repo= */ null, /* repositoryMapping= */ null); - } - - public static PackageIdentifier parse( - String input, String repo, ImmutableMap<RepositoryName, RepositoryName> repositoryMapping) - throws LabelSyntaxException { + String repo; String packageName; int packageStartPos = input.indexOf("//"); - if (repo != null) { - packageName = input; - } else if (input.startsWith("@") && packageStartPos > 0) { + if (input.startsWith("@") && packageStartPos > 0) { repo = input.substring(0, packageStartPos); packageName = input.substring(packageStartPos + 2); } else if (input.startsWith("@")) { @@ -153,13 +145,7 @@ throw new LabelSyntaxException(error); } - if (repositoryMapping != null) { - RepositoryName repositoryName = RepositoryName.create(repo); - repositoryName = repositoryMapping.getOrDefault(repositoryName, repositoryName); - return create(repositoryName, PathFragment.create(packageName)); - } else { - return create(repo, PathFragment.create(packageName)); - } + return create(repo, PathFragment.create(packageName)); } public RepositoryName getRepository() {
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 43875b2..4465a04 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
@@ -31,6 +31,8 @@ @RunWith(JUnit4.class) public class LabelTest { + 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"; @@ -57,18 +59,6 @@ assertThat(l.getPackageName()).isEmpty(); assertThat(l.getName()).isEqualTo("foo"); } - { - Label l = Label.parseAbsolute("//@foo"); - assertThat(l.getPackageIdentifier().getRepository().getName()).isEqualTo("@"); - assertThat(l.getPackageName()).isEqualTo("@foo"); - assertThat(l.getName()).isEqualTo("@foo"); - } - { - Label l = Label.parseAbsolute("//xyz/@foo:abc"); - assertThat(l.getPackageIdentifier().getRepository().getName()).isEqualTo("@"); - assertThat(l.getPackageName()).isEqualTo("xyz/@foo"); - assertThat(l.getName()).isEqualTo("abc"); - } } private static String parseCommandLine(String label, String prefix) throws LabelSyntaxException { @@ -374,6 +364,16 @@ Label.parseAbsolute("//$( ):$( )"); } + /** + * Regression test: we previously expanded the set of characters which are considered label chars + * to include "@" (see test above). An unexpected side-effect is that "@D" in genrule(cmd) was + * considered to be a valid relative label! The fix is to forbid "@x" in package names. + */ + @Test + public void testAtVersionIsIllegal() throws Exception { + assertSyntaxError(BAD_PACKAGE_CHARS, "//foo/bar@123:baz"); + } + @Test public void testDoubleSlashPathSeparator() throws Exception { assertSyntaxError("package names may not contain '//' path separators", @@ -443,21 +443,8 @@ Label.parseAbsolute("foo//bar/baz:bat/boo"); fail(); } catch (LabelSyntaxException e) { - assertThat(e) - .hasMessageThat() - .isEqualTo("invalid repository name 'foo': workspace names must start with '@'"); - } - } - - @Test - public void testInvalidRepoWithColon() throws Exception { - try { - Label.parseAbsolute("@foo:xyz"); - fail(); - } catch (LabelSyntaxException e) { - assertThat(e) - .hasMessageThat() - .containsMatch("invalid repository name '@foo:xyz': workspace names may contain only"); + assertThat(e).hasMessage( + "invalid repository name 'foo': workspace names must start with '@'"); } }
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 3ec7d01..fcb187f 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
@@ -74,7 +74,6 @@ 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(); @@ -92,6 +91,8 @@ .isEqualTo("package names may not end with '/'"); assertThat(LabelValidator.validatePackageName("foo:bar")) .isEqualTo(LabelValidator.PACKAGE_NAME_ERROR); + assertThat(LabelValidator.validatePackageName("baz@12345")) + .isEqualTo(LabelValidator.PACKAGE_NAME_ERROR); assertThat(LabelValidator.validatePackageName("bar/../baz")) .isEqualTo(LabelValidator.PACKAGE_NAME_DOT_ERROR); @@ -170,12 +171,6 @@ .isEqualTo(new PackageAndTarget("f$( )oo", "b$() ar")); assertThat(LabelValidator.validateAbsoluteLabel("@//f$( )oo:b$() ar")) .isEqualTo(new PackageAndTarget("f$( )oo", "b$() ar")); - assertThat(LabelValidator.validateAbsoluteLabel("//f@oo")) - .isEqualTo(new PackageAndTarget("f@oo", "f@oo")); - assertThat(LabelValidator.validateAbsoluteLabel("//@foo")) - .isEqualTo(new PackageAndTarget("@foo", "@foo")); - assertThat(LabelValidator.validateAbsoluteLabel("//@foo:@bar")) - .isEqualTo(new PackageAndTarget("@foo", "@bar")); } @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 1506dfd..e33d1f3 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 @@ @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/rules/cpp/CcToolchainTest.java b/src/test/java/com/google/devtools/build/lib/rules/cpp/CcToolchainTest.java index 8f0e8ec..8cb1ba8 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/cpp/CcToolchainTest.java +++ b/src/test/java/com/google/devtools/build/lib/rules/cpp/CcToolchainTest.java
@@ -499,8 +499,7 @@ @Test public void testInvalidIncludeDirectory() throws Exception { assertInvalidIncludeDirectoryMessage("%package(//a", "has an unrecognized %prefix%"); - assertInvalidIncludeDirectoryMessage( - "%package(//a:@@a)%", "The package '//a:@@a' is not valid"); + assertInvalidIncludeDirectoryMessage("%package(//a@@a)%", "The package '//a@@a' is not valid"); assertInvalidIncludeDirectoryMessage( "%package(//a)%foo", "The path in the package.*is not valid"); assertInvalidIncludeDirectoryMessage(