Automated rollback of commit 1615da781db78b7a910daf89720189c2b2e73dbe.
*** Reason for rollback ***
PiperOrigin-RevId: 200605975
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 6f23842..9d14e6d 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 6f6d444..be61b94 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,6 +30,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";
@@ -56,18 +58,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 {
@@ -373,6 +363,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",
@@ -442,21 +442,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 253ae5c..d4e6cb5 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
@@ -546,8 +546,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(
