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 b11ce30c63f4aa..7ff3ef9d36e0eb 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 @@ -121,11 +121,12 @@ abstract static class PackageContextImpl implements PackageContext {} */ public static Label parseCanonical(String raw) throws LabelSyntaxException { Parts parts = Parts.parse(raw); + parts.checkPkgDoesNotEndWithTripleDots(); parts.checkPkgIsAbsolute(); RepositoryName repoName = - parts.repo == null ? RepositoryName.MAIN : RepositoryName.createUnvalidated(parts.repo); + parts.repo() == null ? RepositoryName.MAIN : RepositoryName.createUnvalidated(parts.repo()); return createUnvalidated( - PackageIdentifier.create(repoName, PathFragment.create(parts.pkg)), parts.target); + PackageIdentifier.create(repoName, PathFragment.create(parts.pkg())), parts.target()); } public static Label parseCanonicalUnchecked(String raw) { @@ -139,18 +140,18 @@ public static Label parseCanonicalUnchecked(String raw) { /** Computes the repo name for the label, within the context of a current repo. */ private static RepositoryName computeRepoNameWithRepoContext( Parts parts, RepoContext repoContext) { - if (parts.repo == null) { + if (parts.repo() == null) { // Certain package names when used without a "@" part are always absolutely in the main repo, // disregarding the current repo and repo mappings. - return ABSOLUTE_PACKAGE_NAMES.contains(parts.pkg) + return ABSOLUTE_PACKAGE_NAMES.contains(parts.pkg()) ? RepositoryName.MAIN : repoContext.currentRepo(); } - if (parts.repoIsCanonical) { + if (parts.repoIsCanonical()) { // This label uses the canonical label literal syntax starting with two @'s ("@@foo//bar"). - return RepositoryName.createUnvalidated(parts.repo); + return RepositoryName.createUnvalidated(parts.repo()); } - return repoContext.repoMapping().get(parts.repo); + return repoContext.repoMapping().get(parts.repo()); } /** @@ -162,10 +163,11 @@ private static RepositoryName computeRepoNameWithRepoContext( public static Label parseWithRepoContext(String raw, RepoContext repoContext) throws LabelSyntaxException { Parts parts = Parts.parse(raw); + parts.checkPkgDoesNotEndWithTripleDots(); parts.checkPkgIsAbsolute(); RepositoryName repoName = computeRepoNameWithRepoContext(parts, repoContext); return createUnvalidated( - PackageIdentifier.create(repoName, PathFragment.create(parts.pkg)), parts.target); + PackageIdentifier.create(repoName, PathFragment.create(parts.pkg())), parts.target()); } /** @@ -178,14 +180,15 @@ public static Label parseWithRepoContext(String raw, RepoContext repoContext) public static Label parseWithPackageContext(String raw, PackageContext packageContext) throws LabelSyntaxException { Parts parts = Parts.parse(raw); + parts.checkPkgDoesNotEndWithTripleDots(); // pkg is either absolute or empty - if (!parts.pkg.isEmpty()) { + if (!parts.pkg().isEmpty()) { parts.checkPkgIsAbsolute(); } RepositoryName repoName = computeRepoNameWithRepoContext(parts, packageContext); PathFragment pkgFragment = - parts.pkgIsAbsolute ? PathFragment.create(parts.pkg) : packageContext.packageFragment(); - return createUnvalidated(PackageIdentifier.create(repoName, pkgFragment), parts.target); + parts.pkgIsAbsolute() ? PathFragment.create(parts.pkg()) : packageContext.packageFragment(); + return createUnvalidated(PackageIdentifier.create(repoName, pkgFragment), parts.target()); } /** @@ -251,7 +254,7 @@ public static Label parseAbsoluteUnchecked(String absName) { public static Label create(String packageName, String targetName) throws LabelSyntaxException { return createUnvalidated( PackageIdentifier.parse(packageName), - validateAndProcessTargetName(packageName, targetName)); + validateAndProcessTargetName(packageName, targetName, /* pkgEndsWithTripleDots= */ false)); } /** @@ -263,7 +266,10 @@ public static Label create(PackageIdentifier packageId, String targetName) throws LabelSyntaxException { return createUnvalidated( packageId, - validateAndProcessTargetName(packageId.getPackageFragment().getPathString(), targetName)); + validateAndProcessTargetName( + packageId.getPackageFragment().getPathString(), + targetName, + /* pkgEndsWithTripleDots= */ false)); } /** diff --git a/src/main/java/com/google/devtools/build/lib/cmdline/LabelParser.java b/src/main/java/com/google/devtools/build/lib/cmdline/LabelParser.java index 6dc3f6c8dff459..5666e1ab8fc55b 100644 --- a/src/main/java/com/google/devtools/build/lib/cmdline/LabelParser.java +++ b/src/main/java/com/google/devtools/build/lib/cmdline/LabelParser.java @@ -14,6 +14,8 @@ package com.google.devtools.build.lib.cmdline; +import com.google.auto.value.AutoValue; +import com.google.common.annotations.VisibleForTesting; import com.google.devtools.build.lib.util.StringUtilities; import com.google.errorprone.annotations.FormatMethod; import javax.annotation.Nullable; @@ -26,82 +28,83 @@ private LabelParser() {} * Contains the parsed elements of a label string. The parts are validated (they don't contain * invalid characters). See {@link #parse} for valid label patterns. */ - static final class Parts { + @AutoValue + abstract static class Parts { /** * The {@code @repo} or {@code @@canonical_repo} part of the string (sans any leading * {@literal @}s); can be null if it doesn't have such a part (i.e. if it doesn't start with a * {@literal @}). */ - @Nullable final String repo; + @Nullable + abstract String repo(); /** * Whether the repo part is using the canonical repo syntax (two {@literal @}s) or not (one * {@literal @}). If there is no repo part, this is false. */ - final boolean repoIsCanonical; + abstract boolean repoIsCanonical(); /** * Whether the package part of the string is prefixed by double-slash. This can only be false if * the repo part is missing. */ - final boolean pkgIsAbsolute; - /** The package part of the string (sans double-slash, if any). */ - final String pkg; + abstract boolean pkgIsAbsolute(); + /** + * The package part of the string (sans the leading double-slash, if present; also sans the + * final '...' segment, if present). + */ + abstract String pkg(); + /** Whether the package part of the string ends with a '...' segment. */ + abstract boolean pkgEndsWithTripleDots(); /** The target part of the string (sans colon). */ - final String target; + abstract String target(); /** The original unparsed raw string. */ - final String raw; + abstract String raw(); - private Parts( - @Nullable String repo, - boolean repoIsCanonical, - boolean pkgIsAbsolute, - String pkg, - String target, - String raw) { - this.repo = repo; - this.repoIsCanonical = repoIsCanonical; - this.pkgIsAbsolute = pkgIsAbsolute; - this.pkg = pkg; - this.target = target; - this.raw = raw; - } - - private static Parts validateAndCreate( + @VisibleForTesting + static Parts validateAndCreate( @Nullable String repo, boolean repoIsCanonical, boolean pkgIsAbsolute, String pkg, + boolean pkgEndsWithTripleDots, String target, String raw) throws LabelSyntaxException { validateRepoName(repo); validatePackageName(pkg, target); - return new Parts( + return new AutoValue_LabelParser_Parts( repo, repoIsCanonical, pkgIsAbsolute, pkg, - validateAndProcessTargetName(pkg, target), + pkgEndsWithTripleDots, + validateAndProcessTargetName(pkg, target, pkgEndsWithTripleDots), raw); } /** * Parses a raw label string into parts. The logic can be summarized by the following table: * - * {@code - * raw | repo | repoIsCanonical | pkgIsAbsolute | pkg | target - * ----------------------+--------+-----------------+---------------+-----------+----------- - * foo/bar | null | false | false | "" | "foo/bar" - * //foo/bar | null | false | true | "foo/bar" | "bar" - * @repo | "repo" | false | true | "" | "repo" - * @@repo | "repo" | true | true | "" | "repo" - * @repo//foo/bar | "repo" | false | true | "foo/bar" | "bar" - * @@repo//foo/bar | "repo" | true | true | "foo/bar" | "bar" - * :quux | null | false | false | "" | "quux" - * foo/bar:quux | null | false | false | "foo/bar" | "quux" - * //foo/bar:quux | null | false | true | "foo/bar" | "quux" - * @repo//foo/bar:quux | "repo" | false | true | "foo/bar" | "quux" - * @@repo//foo/bar:quux | "repo" | true | true | "foo/bar" | "quux" - * } + *
{@code
+     *  raw                  | repo   | repoIs-   | pkgIs-   | pkg       | pkgEndsWith- | target
+     *                       |        | Canonical | Absolute |           | TripleDots   |
+     * ----------------------+--------+-----------+----------+-----------+--------------+-----------
+     * "foo/bar"             | null   | false     | false    | ""        | false        | "foo/bar"
+     * "..."                 | null   | false     | false    | ""        | true         | ""
+     * "...:all"             | null   | false     | false    | ""        | true         | "all"
+     * "foo/..."             | null   | false     | false    | "foo"     | true         | ""
+     * "//foo/bar"           | null   | false     | true     | "foo/bar" | false        | "bar"
+     * "//foo/..."           | null   | false     | true     | "foo"     | true         | ""
+     * "//foo/...:all"       | null   | false     | true     | "foo"     | true         | "all"
+     * "//foo/all"           | null   | false     | true     | "foo/all" | false        | "all"
+     * "@repo"               | "repo" | false     | true     | ""        | false        | "repo"
+     * "@@repo"              | "repo" | true      | true     | ""        | false        | "repo"
+     * "@repo//foo/bar"      | "repo" | false     | true     | "foo/bar" | false        | "bar"
+     * "@@repo//foo/bar"     | "repo" | true      | true     | "foo/bar" | false        | "bar"
+     * ":quux"               | null   | false     | false    | ""        | false        | "quux"
+     * "foo/bar:quux"        | null   | false     | false    | "foo/bar" | false        | "quux"
+     * "//foo/bar:quux"      | null   | false     | true     | "foo/bar" | false        | "quux"
+     * "@repo//foo/bar:quux" | "repo" | false     | true     | "foo/bar" | false        | "quux"
+     * }
*/ static Parts parse(String rawLabel) throws LabelSyntaxException { @Nullable final String repo; @@ -116,9 +119,10 @@ static Parts parse(String rawLabel) throws LabelSyntaxException { return validateAndCreate( repo, repoIsCanonical, - /*pkgIsAbsolute=*/ true, - /*pkg=*/ "", - /*target=*/ repo, + /* pkgIsAbsolute= */ true, + /* pkg= */ "", + /* pkgEndsWithTripleDots= */ false, + /* target= */ repo, rawLabel); } else { repo = rawLabel.substring(repoIsCanonical ? 2 : 1, doubleSlashIndex); @@ -136,20 +140,39 @@ static Parts parse(String rawLabel) throws LabelSyntaxException { final String pkg; final String target; final int colonIndex = rawLabel.indexOf(':', startOfPackage); - if (colonIndex >= 0) { - pkg = rawLabel.substring(startOfPackage, colonIndex); - target = rawLabel.substring(colonIndex + 1); - } else if (pkgIsAbsolute) { - // Special case: the label "[@repo]//foo/bar" is synonymous with "[@repo]//foo/bar:bar". - pkg = rawLabel.substring(startOfPackage); - // The target name is the last package segment (works even if `pkg` contains no slash) - target = pkg.substring(pkg.lastIndexOf('/') + 1); - } else { + final String rawPkg = + rawLabel.substring(startOfPackage, colonIndex >= 0 ? colonIndex : rawLabel.length()); + final boolean pkgEndsWithTripleDots = rawPkg.endsWith("/...") || rawPkg.equals("..."); + if (colonIndex < 0 && pkgEndsWithTripleDots) { + // Special case: if the entire label ends in '...', the target name is empty. + pkg = stripTrailingTripleDots(rawPkg); + target = ""; + } else if (colonIndex < 0 && !pkgIsAbsolute) { // Special case: the label "foo/bar" is synonymous with ":foo/bar". pkg = ""; target = rawLabel.substring(startOfPackage); + } else { + pkg = stripTrailingTripleDots(rawPkg); + if (colonIndex >= 0) { + target = rawLabel.substring(colonIndex + 1); + } else { + // Special case: the label "[@repo]//foo/bar" is synonymous with "[@repo]//foo/bar:bar". + // The target name is the last package segment (works even if `pkg` contains no slash) + target = pkg.substring(pkg.lastIndexOf('/') + 1); + } + } + return validateAndCreate( + repo, repoIsCanonical, pkgIsAbsolute, pkg, pkgEndsWithTripleDots, target, rawLabel); + } + + private static String stripTrailingTripleDots(String pkg) { + if (pkg.endsWith("/...")) { + return pkg.substring(0, pkg.length() - 4); } - return validateAndCreate(repo, repoIsCanonical, pkgIsAbsolute, pkg, target, rawLabel); + if (pkg.equals("...")) { + return ""; + } + return pkg; } private static void validateRepoName(@Nullable String repo) throws LabelSyntaxException { @@ -167,8 +190,14 @@ private static void validatePackageName(String pkg, String target) throws LabelS } void checkPkgIsAbsolute() throws LabelSyntaxException { - if (!pkgIsAbsolute) { - throw syntaxErrorf("invalid label '%s': absolute label must begin with '@' or '//'", raw); + if (!pkgIsAbsolute()) { + throw syntaxErrorf("invalid label '%s': absolute label must begin with '@' or '//'", raw()); + } + } + + void checkPkgDoesNotEndWithTripleDots() throws LabelSyntaxException { + if (pkgEndsWithTripleDots()) { + throw syntaxErrorf("invalid label '%s': package name cannot contain '...'", raw()); } } } @@ -183,8 +212,12 @@ private static String perhapsYouMeantMessage(String pkg, String target) { return pkg.endsWith('/' + target) ? " (perhaps you meant \":" + target + "\"?)" : ""; } - static String validateAndProcessTargetName(String pkg, String target) - throws LabelSyntaxException { + static String validateAndProcessTargetName( + String pkg, String target, boolean pkgEndsWithTripleDots) throws LabelSyntaxException { + if (pkgEndsWithTripleDots && target.isEmpty()) { + // Allow empty target name if the package part ends in '...'. + return target; + } String targetError = LabelValidator.validateTargetName(target); if (targetError != null) { throw syntaxErrorf( 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 86a12c07b8e5d6..b945c6198381b0 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 @@ -121,8 +121,8 @@ public static PackageIdentifier parse(String input) throws LabelSyntaxException } LabelParser.Parts parts = LabelParser.Parts.parse(input + ":dummy_target"); RepositoryName repoName = - parts.repo == null ? RepositoryName.MAIN : RepositoryName.createUnvalidated(parts.repo); - return create(repoName, PathFragment.create(parts.pkg)); + parts.repo() == null ? RepositoryName.MAIN : RepositoryName.createUnvalidated(parts.repo()); + return create(repoName, PathFragment.create(parts.pkg())); } public RepositoryName getRepository() { diff --git a/src/main/java/com/google/devtools/build/lib/cmdline/TargetPattern.java b/src/main/java/com/google/devtools/build/lib/cmdline/TargetPattern.java index 1d623bb19c1fd7..d9886b3d60579d 100644 --- a/src/main/java/com/google/devtools/build/lib/cmdline/TargetPattern.java +++ b/src/main/java/com/google/devtools/build/lib/cmdline/TargetPattern.java @@ -19,6 +19,8 @@ import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Joiner; +import com.google.common.base.MoreObjects; +import com.google.common.base.MoreObjects.ToStringHelper; import com.google.common.base.Preconditions; import com.google.common.base.Splitter; import com.google.common.collect.ImmutableList; @@ -39,7 +41,6 @@ import java.util.Iterator; import java.util.List; import java.util.Objects; -import java.util.regex.Pattern; import javax.annotation.Nullable; import javax.annotation.concurrent.Immutable; @@ -257,11 +258,17 @@ public PackageIdentifier getDirectory() { */ public abstract boolean getRulesOnly(); - private static final class SingleTarget extends TargetPattern { + protected final ToStringHelper toStringHelper() { + return MoreObjects.toStringHelper(this).add("originalPattern", originalPattern); + } + + @VisibleForTesting + static final class SingleTarget extends TargetPattern { private final Label target; - private SingleTarget(Label target, String originalPattern) { + @VisibleForTesting + SingleTarget(String originalPattern, Label target) { super(originalPattern); this.target = Preconditions.checkNotNull(target); } @@ -318,12 +325,19 @@ public boolean equals(Object o) { public int hashCode() { return Objects.hash(getType(), target); } + + @Override + public String toString() { + return toStringHelper().add("target", target).toString(); + } } - private static final class InterpretPathAsTarget extends TargetPattern { + @VisibleForTesting + static final class InterpretPathAsTarget extends TargetPattern { private final String path; - private InterpretPathAsTarget(String path, String originalPattern) { + @VisibleForTesting + InterpretPathAsTarget(String originalPattern, String path) { super(originalPattern); this.path = normalize(Preconditions.checkNotNull(path)); } @@ -403,28 +417,32 @@ public boolean equals(Object o) { public int hashCode() { return Objects.hash(getType(), path); } + + @Override + public String toString() { + return toStringHelper().add("path", path).toString(); + } } - private static final class TargetsInPackage extends TargetPattern { + @VisibleForTesting + static final class TargetsInPackage extends TargetPattern { private final PackageIdentifier packageIdentifier; private final String suffix; private final boolean wasOriginallyAbsolute; private final boolean rulesOnly; - private final boolean checkWildcardConflict; - private TargetsInPackage( + @VisibleForTesting + TargetsInPackage( String originalPattern, PackageIdentifier packageIdentifier, String suffix, boolean wasOriginallyAbsolute, - boolean rulesOnly, - boolean checkWildcardConflict) { + boolean rulesOnly) { super(originalPattern); this.packageIdentifier = packageIdentifier; this.suffix = Preconditions.checkNotNull(suffix); this.wasOriginallyAbsolute = wasOriginallyAbsolute; this.rulesOnly = rulesOnly; - this.checkWildcardConflict = checkWildcardConflict; } @Override @@ -435,12 +453,10 @@ public void eval( BatchCallback callback, Class exceptionClass) throws TargetParsingException, E, InterruptedException, InconsistentFilesystemException { - if (checkWildcardConflict) { - ResolvedTargets targets = getWildcardConflict(resolver); - if (targets != null) { - callback.process(targets.getTargets()); - return; - } + ResolvedTargets targets = getWildcardConflict(resolver); + if (targets != null) { + callback.process(targets.getTargets()); + return; } callback.process( @@ -478,7 +494,6 @@ public boolean equals(Object o) { TargetsInPackage that = (TargetsInPackage) o; return wasOriginallyAbsolute == that.wasOriginallyAbsolute && rulesOnly == that.rulesOnly - && checkWildcardConflict == that.checkWildcardConflict && getOriginalPattern().equals(that.getOriginalPattern()) && packageIdentifier.equals(that.packageIdentifier) && suffix.equals(that.suffix); @@ -492,8 +507,17 @@ public int hashCode() { packageIdentifier, suffix, wasOriginallyAbsolute, - rulesOnly, - checkWildcardConflict); + rulesOnly); + } + + @Override + public String toString() { + return toStringHelper() + .add("packageIdentifier", packageIdentifier) + .add("suffix", suffix) + .add("wasOriginallyAbsolute", wasOriginallyAbsolute) + .add("rulesOnly", rulesOnly) + .toString(); } /** @@ -547,8 +571,8 @@ public static final class TargetsBelowDirectory extends TargetPattern { private final PackageIdentifier directory; private final boolean rulesOnly; - private TargetsBelowDirectory( - String originalPattern, PackageIdentifier directory, boolean rulesOnly) { + @VisibleForTesting + TargetsBelowDirectory(String originalPattern, PackageIdentifier directory, boolean rulesOnly) { super(originalPattern); this.directory = Preconditions.checkNotNull(directory); this.rulesOnly = rulesOnly; @@ -816,16 +840,15 @@ && getOriginalPattern().equals(that.getOriginalPattern()) public int hashCode() { return Objects.hash(getType(), getOriginalPattern(), directory, rulesOnly); } + + @Override + public String toString() { + return toStringHelper().add("directory", directory).add("rulesOnly", rulesOnly).toString(); + } } @Immutable public static final class Parser { - // A valid pattern either starts with exactly 0 slashes (relative pattern) or exactly two - // slashes (absolute pattern). - private static final Pattern VALID_SLASH_PREFIX = Pattern.compile("(//)?([^/]|$)"); - - // TODO(bazel-team): Merge the Label functionality that requires similar constants into this - // class. /** * The set of target-pattern suffixes which indicate wildcards over all rules in a * single package. @@ -839,34 +862,6 @@ public static final class Parser { private static final ImmutableList ALL_TARGETS_IN_SUFFIXES = ImmutableList.of("*", "all-targets"); - private static final List SUFFIXES; - - static { - SUFFIXES = - ImmutableList.builder() - .addAll(ALL_RULES_IN_SUFFIXES) - .addAll(ALL_TARGETS_IN_SUFFIXES) - .add("/...") - .build(); - } - - /** - * Returns whether the given pattern is simple, i.e., not starting with '-' and using none of - * the target matching suffixes. - */ - public static boolean isSimpleTargetPattern(String pattern) { - if (pattern.startsWith("-")) { - return false; - } - - for (String suffix : SUFFIXES) { - if (pattern.endsWith(":" + suffix)) { - return false; - } - } - return true; - } - /** * Directory prefix to use when resolving relative labels (rather than absolute ones). For * example, if the working directory is "/foo", then this should be "foo", which @@ -885,6 +880,9 @@ public static boolean isSimpleTargetPattern(String pattern) { /** Creates a new parser with the given offset for relative patterns. */ public Parser( PathFragment relativeDirectory, RepositoryName currentRepo, RepositoryMapping repoMapping) { + Preconditions.checkArgument( + currentRepo.isMain() || relativeDirectory.isEmpty(), + "parsing target patterns in a non-main repo with a relative directory is unsupported"); this.relativeDirectory = relativeDirectory; this.currentRepo = currentRepo; this.repoMapping = repoMapping; @@ -897,153 +895,85 @@ public Parser( * @throws TargetParsingException if the pattern is invalid */ public TargetPattern parse(String pattern) throws TargetParsingException { - // The structure of this method is by cases, according to the usage string - // constant (see lib/blaze/commands/target-syntax.txt). - - String originalPattern = pattern; - final boolean includesRepo = pattern.startsWith("@"); - RepositoryName repository; - if (!includesRepo) { - repository = currentRepo; - } else { - int pkgStart = pattern.indexOf("//"); - if (pkgStart < 0) { - throw new TargetParsingException( - "Couldn't find package in target " + pattern, TargetPatterns.Code.PACKAGE_NOT_FOUND); - } - boolean isCanonicalRepoName = pattern.startsWith("@@"); - String repoPart = pattern.substring(isCanonicalRepoName ? 2 : 1, pkgStart); - try { - RepositoryName.validate(repoPart); - } catch (LabelSyntaxException e) { - throw new TargetParsingException(e.getMessage(), TargetPatterns.Code.LABEL_SYNTAX_ERROR); - } - if (isCanonicalRepoName) { - repository = RepositoryName.createUnvalidated(repoPart); - } else { - repository = repoMapping.get(repoPart); - if (!repository.isVisible()) { - throw new TargetParsingException( - String.format( - "No repository visible as '@%s' from %s", - repository.getName(), repository.getOwnerRepoDisplayString()), - Code.PACKAGE_NOT_FOUND); - } - } - - pattern = pattern.substring(pkgStart); - } - - if (!VALID_SLASH_PREFIX.matcher(pattern).lookingAt()) { - throw new TargetParsingException( - "not a valid absolute pattern (absolute target patterns " - + "must start with exactly two slashes): '" - + pattern - + "'", - TargetPatterns.Code.ABSOLUTE_TARGET_PATTERN_INVALID); - } - - final boolean wasOriginallyAbsolute = pattern.startsWith("//"); - // We now ensure the relativeDirectory is applied to relative patterns. - pattern = absolutize(pattern).substring(2); - - if (pattern.isEmpty()) { - throw new TargetParsingException( - "the empty string is not a valid target", - TargetPatterns.Code.TARGET_CANNOT_BE_EMPTY_STRING); + LabelParser.Parts parts; + try { + parts = LabelParser.Parts.parse(pattern); + } catch (LabelSyntaxException e) { + throw new TargetParsingException(e.getMessage(), TargetPatterns.Code.LABEL_SYNTAX_ERROR); } - int colonIndex = pattern.lastIndexOf(':'); - String packagePart = colonIndex < 0 ? pattern : pattern.substring(0, colonIndex); - String targetPart = colonIndex < 0 ? "" : pattern.substring(colonIndex + 1); - - if (packagePart.equals("...")) { - packagePart = "/..."; // special case this for easier parsing + // Special case: For a target pattern that just looks like `foo/bar/baz`, we treat this as a + // file path. LabelParser parses it as `:foo/bar/baz`, so we need to distinguish this case by + // checking if the original pattern contains a colon. + if (!parts.pkgIsAbsolute() + && currentRepo.isMain() + && parts.pkg().isEmpty() + && !parts.pkgEndsWithTripleDots() + && !pattern.contains(":")) { + return new InterpretPathAsTarget( + pattern, relativeDirectory.getRelative(parts.target()).getPathString()); } - if (packagePart.endsWith("/")) { - throw new TargetParsingException( - "The package part of '" + originalPattern + "' should not end in a slash", - TargetPatterns.Code.PACKAGE_PART_CANNOT_END_IN_SLASH); - } - - if (packagePart.endsWith("/...")) { - String realPackagePart = packagePart.substring(0, packagePart.length() - "/...".length()); - PackageIdentifier packageIdentifier = createPackageIdentifier(repository, realPackagePart); - if (targetPart.isEmpty() || ALL_RULES_IN_SUFFIXES.contains(targetPart)) { - return new TargetsBelowDirectory(originalPattern, packageIdentifier, true); - } else if (ALL_TARGETS_IN_SUFFIXES.contains(targetPart)) { - return new TargetsBelowDirectory(originalPattern, packageIdentifier, false); + PackageIdentifier packageIdentifier = createPackageIdentifierFromParts(parts); + if (parts.pkgEndsWithTripleDots()) { + if (parts.target().isEmpty() || ALL_RULES_IN_SUFFIXES.contains(parts.target())) { + return new TargetsBelowDirectory(pattern, packageIdentifier, true); + } else if (ALL_TARGETS_IN_SUFFIXES.contains(parts.target())) { + return new TargetsBelowDirectory(pattern, packageIdentifier, false); } + throw new TargetParsingException( + "Invalid target pattern " + pattern + ": '...' can only be used with wildcard targets", + Code.LABEL_SYNTAX_ERROR); } - if (ALL_RULES_IN_SUFFIXES.contains(targetPart)) { + if (pattern.contains(":") && ALL_RULES_IN_SUFFIXES.contains(parts.target())) { return new TargetsInPackage( - originalPattern, - createPackageIdentifier(repository, packagePart), - targetPart, - wasOriginallyAbsolute, - true, - true); + pattern, packageIdentifier, parts.target(), parts.pkgIsAbsolute(), true); } - if (ALL_TARGETS_IN_SUFFIXES.contains(targetPart)) { + if (pattern.contains(":") && ALL_TARGETS_IN_SUFFIXES.contains(parts.target())) { return new TargetsInPackage( - originalPattern, - createPackageIdentifier(repository, packagePart), - targetPart, - wasOriginallyAbsolute, - false, - true); + pattern, packageIdentifier, parts.target(), parts.pkgIsAbsolute(), false); } - if (includesRepo || wasOriginallyAbsolute || pattern.contains(":")) { - Label label; - try { - label = Label.parseCanonical(repository.getNameWithAt() + "//" + pattern); - } catch (LabelSyntaxException e) { + return new SingleTarget(pattern, Label.createUnvalidated(packageIdentifier, parts.target())); + } + + private PackageIdentifier createPackageIdentifierFromParts(LabelParser.Parts parts) + throws TargetParsingException { + RepositoryName repo; + if (parts.repo() == null) { + repo = currentRepo; + } else if (parts.repoIsCanonical()) { + repo = RepositoryName.createUnvalidated(parts.repo()); + } else { + repo = repoMapping.get(parts.repo()); + if (!repo.isVisible()) { throw new TargetParsingException( - "invalid target format '" + originalPattern + "': " + e.getMessage(), - TargetPatterns.Code.TARGET_FORMAT_INVALID); + String.format( + "No repository visible as '@%s' from %s", + repo.getName(), repo.getOwnerRepoDisplayString()), + Code.PACKAGE_NOT_FOUND); } - return new SingleTarget(label, originalPattern); } - // This is a stripped-down version of interpretPathAsTarget that does no I/O. We have a basic - // relative path. e.g. "foo/bar/Wiz.java". The strictest correct check we can do here (without - // I/O) is just to ensure that there is *some* prefix that is a valid package-name. It's - // sufficient to test the first segment. This is really a rather weak check; perhaps we should - // just eliminate it. - int slashIndex = pattern.indexOf('/'); - String packageName = pattern; - if (slashIndex > 0) { - packageName = pattern.substring(0, slashIndex); - } - String pkgError = LabelValidator.validatePackageName(packageName); - if (pkgError != null) { - throw new TargetParsingException( - "Bad target pattern '" + originalPattern + "': " + pkgError, - TargetPatterns.Code.LABEL_SYNTAX_ERROR); - } - return new InterpretPathAsTarget(pattern, originalPattern); + PathFragment packagePathFragment = + parts.pkgIsAbsolute() + ? PathFragment.create(parts.pkg()) + : relativeDirectory.getRelative(parts.pkg()); + return PackageIdentifier.create(repo, packagePathFragment); } public RepositoryMapping getRepoMapping() { return repoMapping; } - public PathFragment getRelativeDirectory() { - return relativeDirectory; + public RepositoryName getCurrentRepo() { + return currentRepo; } - private PackageIdentifier createPackageIdentifier(RepositoryName repoName, String pkg) - throws TargetParsingException { - String pkgError = LabelValidator.validatePackageName(pkg); - if (pkgError != null) { - throw new TargetParsingException( - "Invalid package name '" + pkg + "': " + pkgError, Code.LABEL_SYNTAX_ERROR); - } - return PackageIdentifier.create(repoName, PathFragment.create(pkg)); + public PathFragment getRelativeDirectory() { + return relativeDirectory; } /** diff --git a/src/test/java/com/google/devtools/build/lib/cmdline/LabelParserTest.java b/src/test/java/com/google/devtools/build/lib/cmdline/LabelParserTest.java new file mode 100644 index 00000000000000..3039f001e42d41 --- /dev/null +++ b/src/test/java/com/google/devtools/build/lib/cmdline/LabelParserTest.java @@ -0,0 +1,69 @@ +// Copyright 2023 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. + +package com.google.devtools.build.lib.cmdline; + +import static com.google.common.truth.Truth.assertThat; +import static com.google.devtools.build.lib.cmdline.LabelParser.Parts.parse; +import static com.google.devtools.build.lib.cmdline.LabelParser.Parts.validateAndCreate; + +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +/** Tests for {@link LabelParser}. */ +@RunWith(JUnit4.class) +public class LabelParserTest { + + /** Checks that the javadoc of {@link LabelParser.Parts#parse} is accurate. */ + @Test + public void parserTable() throws Exception { + assertThat(parse("foo/bar")) + .isEqualTo(validateAndCreate(null, false, false, "", false, "foo/bar", "foo/bar")); + assertThat(parse("...")).isEqualTo(validateAndCreate(null, false, false, "", true, "", "...")); + assertThat(parse("...:all")) + .isEqualTo(validateAndCreate(null, false, false, "", true, "all", "...:all")); + assertThat(parse("foo/...")) + .isEqualTo(validateAndCreate(null, false, false, "foo", true, "", "foo/...")); + assertThat(parse("//foo/bar")) + .isEqualTo(validateAndCreate(null, false, true, "foo/bar", false, "bar", "//foo/bar")); + assertThat(parse("//foo/...")) + .isEqualTo(validateAndCreate(null, false, true, "foo", true, "", "//foo/...")); + assertThat(parse("//foo/...:all")) + .isEqualTo(validateAndCreate(null, false, true, "foo", true, "all", "//foo/...:all")); + assertThat(parse("//foo/all")) + .isEqualTo(validateAndCreate(null, false, true, "foo/all", false, "all", "//foo/all")); + assertThat(parse("@repo")) + .isEqualTo(validateAndCreate("repo", false, true, "", false, "repo", "@repo")); + assertThat(parse("@@repo")) + .isEqualTo(validateAndCreate("repo", true, true, "", false, "repo", "@@repo")); + assertThat(parse("@repo//foo/bar")) + .isEqualTo( + validateAndCreate("repo", false, true, "foo/bar", false, "bar", "@repo//foo/bar")); + assertThat(parse("@@repo//foo/bar")) + .isEqualTo( + validateAndCreate("repo", true, true, "foo/bar", false, "bar", "@@repo//foo/bar")); + assertThat(parse(":quux")) + .isEqualTo(validateAndCreate(null, false, false, "", false, "quux", ":quux")); + assertThat(parse("foo/bar:quux")) + .isEqualTo(validateAndCreate(null, false, false, "foo/bar", false, "quux", "foo/bar:quux")); + assertThat(parse("//foo/bar:quux")) + .isEqualTo( + validateAndCreate(null, false, true, "foo/bar", false, "quux", "//foo/bar:quux")); + assertThat(parse("@repo//foo/bar:quux")) + .isEqualTo( + validateAndCreate( + "repo", false, true, "foo/bar", false, "quux", "@repo//foo/bar:quux")); + } +} 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 1412e0ce8ffbfb..69345ff84d69e2 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 @@ -16,10 +16,15 @@ import static com.google.common.truth.Truth.assertThat; import static org.junit.Assert.assertThrows; +import static org.junit.Assert.fail; +import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; +import com.google.devtools.build.lib.cmdline.TargetPattern.InterpretPathAsTarget; +import com.google.devtools.build.lib.cmdline.TargetPattern.SingleTarget; import com.google.devtools.build.lib.cmdline.TargetPattern.TargetsBelowDirectory; import com.google.devtools.build.lib.cmdline.TargetPattern.TargetsBelowDirectory.ContainsResult; +import com.google.devtools.build.lib.cmdline.TargetPattern.TargetsInPackage; import com.google.devtools.build.lib.vfs.PathFragment; import org.junit.Test; import org.junit.runner.RunWith; @@ -28,38 +33,237 @@ /** Tests for {@link com.google.devtools.build.lib.cmdline.TargetPattern}. */ @RunWith(JUnit4.class) public class TargetPatternTest { - private void expectError(String pattern) { - assertThrows(TargetParsingException.class, () -> parse(pattern)); + private static Label label(String raw) { + return Label.parseCanonicalUnchecked(raw); + } + + private static PackageIdentifier pkg(String raw) { + try { + return PackageIdentifier.parse(raw); + } catch (LabelSyntaxException e) { + throw new RuntimeException(e); + } } @Test - public void testPassingValidations() throws TargetParsingException { - parse("foo:bar"); - parse("foo:all"); - parse("foo/...:all"); - parse("foo:*"); - - parse("//foo"); - parse("//foo:bar"); - parse("//foo:all"); - - parse("//foo/all"); - parse("java/com/google/foo/Bar.java"); - parse("//foo/...:all"); - - parse("//..."); - parse("@repo//foo:bar"); - parse("@repo//foo:all"); - parse("@repo//:bar"); - parse("@@repo//foo:all"); - parse("@@repo//:bar"); + public void validPatterns_mainRepo_atRepoRoot() throws TargetParsingException { + TargetPattern.Parser parser = + new TargetPattern.Parser( + PathFragment.EMPTY_FRAGMENT, + RepositoryName.MAIN, + RepositoryMapping.create( + ImmutableMap.of("repo", RepositoryName.createUnvalidated("canonical_repo")), + RepositoryName.MAIN)); + + assertThat(parser.parse(":foo")).isEqualTo(new SingleTarget(":foo", label("@@//:foo"))); + assertThat(parser.parse("foo:bar")) + .isEqualTo(new SingleTarget("foo:bar", label("@@//foo:bar"))); + assertThat(parser.parse("foo:all")) + .isEqualTo(new TargetsInPackage("foo:all", pkg("@@//foo"), "all", false, true)); + assertThat(parser.parse("foo/...:all")) + .isEqualTo(new TargetsBelowDirectory("foo/...:all", pkg("@@//foo"), true)); + assertThat(parser.parse("foo:*")) + .isEqualTo(new TargetsInPackage("foo:*", pkg("@@//foo"), "*", false, false)); + assertThat(parser.parse("foo")).isEqualTo(new InterpretPathAsTarget("foo", "foo")); + assertThat(parser.parse("...")).isEqualTo(new TargetsBelowDirectory("...", pkg("@@//"), true)); + assertThat(parser.parse("foo/bar")).isEqualTo(new InterpretPathAsTarget("foo/bar", "foo/bar")); + + assertThat(parser.parse("//foo")).isEqualTo(new SingleTarget("//foo", label("@@//foo:foo"))); + assertThat(parser.parse("//foo:bar")) + .isEqualTo(new SingleTarget("//foo:bar", label("@@//foo:bar"))); + assertThat(parser.parse("//foo:all")) + .isEqualTo(new TargetsInPackage("//foo:all", pkg("@@//foo"), "all", true, true)); + + assertThat(parser.parse("//foo/all")) + .isEqualTo(new SingleTarget("//foo/all", label("@@//foo/all:all"))); + assertThat(parser.parse("//foo/...:all")) + .isEqualTo(new TargetsBelowDirectory("//foo/...:all", pkg("@@//foo"), true)); + assertThat(parser.parse("//...")) + .isEqualTo(new TargetsBelowDirectory("//...", pkg("@@//"), true)); + + assertThat(parser.parse("@repo")) + .isEqualTo(new SingleTarget("@repo", label("@@canonical_repo//:repo"))); + assertThat(parser.parse("@repo//foo:bar")) + .isEqualTo(new SingleTarget("@repo//foo:bar", label("@@canonical_repo//foo:bar"))); + assertThat(parser.parse("@repo//foo:all")) + .isEqualTo( + new TargetsInPackage( + "@repo//foo:all", pkg("@@canonical_repo//foo"), "all", true, true)); + assertThat(parser.parse("@repo//:bar")) + .isEqualTo(new SingleTarget("@repo//:bar", label("@@canonical_repo//:bar"))); + assertThat(parser.parse("@repo//...")) + .isEqualTo(new TargetsBelowDirectory("@repo//...", pkg("@@canonical_repo//"), true)); + + assertThat(parser.parse("@@repo")) + .isEqualTo(new SingleTarget("@@repo", label("@@repo//:repo"))); + assertThat(parser.parse("@@repo//foo:all")) + .isEqualTo(new TargetsInPackage("@@repo//foo:all", pkg("@@repo//foo"), "all", true, true)); + assertThat(parser.parse("@@repo//:bar")) + .isEqualTo(new SingleTarget("@@repo//:bar", label("@@repo//:bar"))); } @Test - public void testInvalidPatterns() throws TargetParsingException { - expectError("Bar\\java"); - expectError(""); - expectError("\\"); + public void validPatterns_mainRepo_inSomeRelativeDirectory() throws TargetParsingException { + TargetPattern.Parser parser = + new TargetPattern.Parser( + PathFragment.create("base"), + RepositoryName.MAIN, + RepositoryMapping.create( + ImmutableMap.of("repo", RepositoryName.createUnvalidated("canonical_repo")), + RepositoryName.MAIN)); + + assertThat(parser.parse(":foo")).isEqualTo(new SingleTarget(":foo", label("@@//base:foo"))); + assertThat(parser.parse("foo:bar")) + .isEqualTo(new SingleTarget("foo:bar", label("@@//base/foo:bar"))); + assertThat(parser.parse("foo:all")) + .isEqualTo(new TargetsInPackage("foo:all", pkg("@@//base/foo"), "all", false, true)); + assertThat(parser.parse("foo/...:all")) + .isEqualTo(new TargetsBelowDirectory("foo/...:all", pkg("@@//base/foo"), true)); + assertThat(parser.parse("foo:*")) + .isEqualTo(new TargetsInPackage("foo:*", pkg("@@//base/foo"), "*", false, false)); + assertThat(parser.parse("foo")).isEqualTo(new InterpretPathAsTarget("foo", "base/foo")); + assertThat(parser.parse("...")) + .isEqualTo(new TargetsBelowDirectory("...", pkg("@@//base"), true)); + assertThat(parser.parse("foo/bar")) + .isEqualTo(new InterpretPathAsTarget("foo/bar", "base/foo/bar")); + + assertThat(parser.parse("//foo")).isEqualTo(new SingleTarget("//foo", label("@@//foo:foo"))); + assertThat(parser.parse("//foo:bar")) + .isEqualTo(new SingleTarget("//foo:bar", label("@@//foo:bar"))); + assertThat(parser.parse("//foo:all")) + .isEqualTo(new TargetsInPackage("//foo:all", pkg("@@//foo"), "all", true, true)); + + assertThat(parser.parse("//foo/all")) + .isEqualTo(new SingleTarget("//foo/all", label("@@//foo/all:all"))); + assertThat(parser.parse("//foo/...:all")) + .isEqualTo(new TargetsBelowDirectory("//foo/...:all", pkg("@@//foo"), true)); + assertThat(parser.parse("//...")) + .isEqualTo(new TargetsBelowDirectory("//...", pkg("@@//"), true)); + + assertThat(parser.parse("@repo")) + .isEqualTo(new SingleTarget("@repo", label("@@canonical_repo//:repo"))); + assertThat(parser.parse("@repo//foo:bar")) + .isEqualTo(new SingleTarget("@repo//foo:bar", label("@@canonical_repo//foo:bar"))); + assertThat(parser.parse("@repo//foo:all")) + .isEqualTo( + new TargetsInPackage( + "@repo//foo:all", pkg("@@canonical_repo//foo"), "all", true, true)); + assertThat(parser.parse("@repo//:bar")) + .isEqualTo(new SingleTarget("@repo//:bar", label("@@canonical_repo//:bar"))); + assertThat(parser.parse("@repo//...")) + .isEqualTo(new TargetsBelowDirectory("@repo//...", pkg("@@canonical_repo//"), true)); + + assertThat(parser.parse("@@repo")) + .isEqualTo(new SingleTarget("@@repo", label("@@repo//:repo"))); + assertThat(parser.parse("@@repo//foo:all")) + .isEqualTo(new TargetsInPackage("@@repo//foo:all", pkg("@@repo//foo"), "all", true, true)); + assertThat(parser.parse("@@repo//:bar")) + .isEqualTo(new SingleTarget("@@repo//:bar", label("@@repo//:bar"))); + } + + @Test + public void validPatterns_nonMainRepo() throws TargetParsingException { + TargetPattern.Parser parser = + new TargetPattern.Parser( + PathFragment.EMPTY_FRAGMENT, + RepositoryName.createUnvalidated("my_repo"), + RepositoryMapping.create( + ImmutableMap.of("repo", RepositoryName.createUnvalidated("canonical_repo")), + RepositoryName.createUnvalidated("my_repo"))); + + assertThat(parser.parse(":foo")).isEqualTo(new SingleTarget(":foo", label("@@my_repo//:foo"))); + assertThat(parser.parse("foo:bar")) + .isEqualTo(new SingleTarget("foo:bar", label("@@my_repo//foo:bar"))); + assertThat(parser.parse("foo:all")) + .isEqualTo(new TargetsInPackage("foo:all", pkg("@@my_repo//foo"), "all", false, true)); + assertThat(parser.parse("foo/...:all")) + .isEqualTo(new TargetsBelowDirectory("foo/...:all", pkg("@@my_repo//foo"), true)); + assertThat(parser.parse("foo:*")) + .isEqualTo(new TargetsInPackage("foo:*", pkg("@@my_repo//foo"), "*", false, false)); + assertThat(parser.parse("foo")).isEqualTo(new SingleTarget("foo", label("@@my_repo//:foo"))); + assertThat(parser.parse("...")) + .isEqualTo(new TargetsBelowDirectory("...", pkg("@@my_repo//"), true)); + assertThat(parser.parse("foo/bar")) + .isEqualTo(new SingleTarget("foo/bar", label("@@my_repo//:foo/bar"))); + + assertThat(parser.parse("//foo")) + .isEqualTo(new SingleTarget("//foo", label("@@my_repo//foo:foo"))); + assertThat(parser.parse("//foo:bar")) + .isEqualTo(new SingleTarget("//foo:bar", label("@@my_repo//foo:bar"))); + assertThat(parser.parse("//foo:all")) + .isEqualTo(new TargetsInPackage("//foo:all", pkg("@@my_repo//foo"), "all", true, true)); + + assertThat(parser.parse("//foo/all")) + .isEqualTo(new SingleTarget("//foo/all", label("@@my_repo//foo/all:all"))); + assertThat(parser.parse("//foo/...:all")) + .isEqualTo(new TargetsBelowDirectory("//foo/...:all", pkg("@@my_repo//foo"), true)); + assertThat(parser.parse("//...")) + .isEqualTo(new TargetsBelowDirectory("//...", pkg("@@my_repo//"), true)); + + assertThat(parser.parse("@repo")) + .isEqualTo(new SingleTarget("@repo", label("@@canonical_repo//:repo"))); + assertThat(parser.parse("@repo//foo:bar")) + .isEqualTo(new SingleTarget("@repo//foo:bar", label("@@canonical_repo//foo:bar"))); + assertThat(parser.parse("@repo//foo:all")) + .isEqualTo( + new TargetsInPackage( + "@repo//foo:all", pkg("@@canonical_repo//foo"), "all", true, true)); + assertThat(parser.parse("@repo//:bar")) + .isEqualTo(new SingleTarget("@repo//:bar", label("@@canonical_repo//:bar"))); + assertThat(parser.parse("@repo//...")) + .isEqualTo(new TargetsBelowDirectory("@repo//...", pkg("@@canonical_repo//"), true)); + + assertThat(parser.parse("@@repo")) + .isEqualTo(new SingleTarget("@@repo", label("@@repo//:repo"))); + assertThat(parser.parse("@@repo//foo:all")) + .isEqualTo(new TargetsInPackage("@@repo//foo:all", pkg("@@repo//foo"), "all", true, true)); + assertThat(parser.parse("@@repo//:bar")) + .isEqualTo(new SingleTarget("@@repo//:bar", label("@@repo//:bar"))); + } + + @Test + public void invalidPatterns() throws Exception { + ImmutableList badPatterns = + ImmutableList.of("//Bar\\java", "", "/foo", "///foo", "@", "@foo//", "@@"); + ImmutableMap repoMappingEntries = + ImmutableMap.of("repo", RepositoryName.createUnvalidated("canonical_repo")); + for (TargetPattern.Parser parser : + ImmutableList.of( + new TargetPattern.Parser( + PathFragment.EMPTY_FRAGMENT, + RepositoryName.MAIN, + RepositoryMapping.create(repoMappingEntries, RepositoryName.MAIN)), + new TargetPattern.Parser( + PathFragment.create("base"), + RepositoryName.MAIN, + RepositoryMapping.create(repoMappingEntries, RepositoryName.MAIN)), + new TargetPattern.Parser( + PathFragment.EMPTY_FRAGMENT, + RepositoryName.create("my_repo"), + RepositoryMapping.create(repoMappingEntries, RepositoryName.create("my_repo"))))) { + for (String pattern : badPatterns) { + try { + TargetPattern parsed = parser.parse(pattern); + fail( + String.format( + "parsing should have failed for pattern \"%s\" with parser in repo %s at" + + " relative directory [%s], but succeeded with the result:\n%s", + pattern, parser.getCurrentRepo(), parser.getRelativeDirectory(), parsed)); + } catch (TargetParsingException expected) { + } + } + } + } + + @Test + public void invalidParser_nonMainRepo_nonEmptyRelativeDirectory() throws Exception { + assertThrows( + IllegalArgumentException.class, + () -> + new TargetPattern.Parser( + PathFragment.create("base"), + RepositoryName.create("my_repo"), + RepositoryMapping.ALWAYS_FALLBACK)); } @Test @@ -89,6 +293,13 @@ public void testNormalize() { assertThat(TargetPattern.normalize("a/../../../b")).isEqualTo("../../b"); } + private static TargetsBelowDirectory parseAsTBD(String pattern) throws TargetParsingException { + TargetPattern parsedPattern = TargetPattern.defaultParser().parse(pattern); + assertThat(parsedPattern.getType()).isEqualTo(TargetPattern.Type.TARGETS_BELOW_DIRECTORY); + assertThat(parsedPattern.getOriginalPattern()).isEqualTo(pattern); + return (TargetsBelowDirectory) parsedPattern; + } + @Test public void testTargetsBelowDirectoryContainsColonStar() throws Exception { // Given an outer pattern '//foo/...', that matches rules only, @@ -166,47 +377,4 @@ public void testDepotRootTargetsBelowDirectoryContainsPatterns() throws Exceptio .isEqualTo(ContainsResult.DIRECTORY_EXCLUSION_WOULD_BE_EXACT); assertThat(tbdFoo.contains(tbdDepot)).isEqualTo(ContainsResult.NOT_CONTAINED); } - - @Test - public void testRenameRepository() throws Exception { - RepositoryMapping renaming = - RepositoryMapping.createAllowingFallback( - ImmutableMap.of( - "foo", RepositoryName.create("bar"), - "myworkspace", RepositoryName.create(""))); - TargetPattern.Parser parser = - new TargetPattern.Parser( - PathFragment.EMPTY_FRAGMENT, RepositoryName.createUnvalidated("myrepo"), renaming); - - // Expecting renaming - assertThat(parser.parse("@foo//package:target").getRepository().getName()).isEqualTo("bar"); - assertThat(parser.parse("@myworkspace//package:target").getRepository().isMain()).isTrue(); - assertThat(parser.parse("@foo//foo/...").getRepository().getName()).isEqualTo("bar"); - assertThat(parser.parse("@myworkspace//foo/...").getRepository().isMain()).isTrue(); - - // No renaming should occur - assertThat(parser.parse("@//package:target").getRepository().isMain()).isTrue(); - assertThat(parser.parse("@@foo//package:target").getRepository().getName()).isEqualTo("foo"); - assertThat(parser.parse("@unrelated//package:target").getRepository().getName()) - .isEqualTo("unrelated"); - assertThat(parser.parse("foo/package:target").getRepository().getName()).isEqualTo("myrepo"); - assertThat(parser.parse("foo/...").getRepository().getName()).isEqualTo("myrepo"); - } - - private static TargetPattern parse(String pattern) throws TargetParsingException { - return TargetPattern.defaultParser().parse(pattern); - } - - private static TargetPattern parseAsExpectedType(String pattern, TargetPattern.Type expectedType) - throws TargetParsingException { - TargetPattern parsedPattern = parse(pattern); - assertThat(parsedPattern.getType()).isEqualTo(expectedType); - assertThat(parsedPattern.getOriginalPattern()).isEqualTo(pattern); - return parsedPattern; - } - - private static TargetsBelowDirectory parseAsTBD(String pattern) throws TargetParsingException { - return (TargetsBelowDirectory) - parseAsExpectedType(pattern, TargetPattern.Type.TARGETS_BELOW_DIRECTORY); - } } diff --git a/src/test/java/com/google/devtools/build/lib/pkgcache/LoadingPhaseRunnerTest.java b/src/test/java/com/google/devtools/build/lib/pkgcache/LoadingPhaseRunnerTest.java index db27286523474f..f13def7bee7dd6 100644 --- a/src/test/java/com/google/devtools/build/lib/pkgcache/LoadingPhaseRunnerTest.java +++ b/src/test/java/com/google/devtools/build/lib/pkgcache/LoadingPhaseRunnerTest.java @@ -248,8 +248,7 @@ public void testMistypedTarget() { assertThat(e) .hasMessageThat() .contains( - "invalid target format 'foo//bar:missing': invalid package name 'foo//bar': package" - + " names may not contain '//' path separators"); + "invalid package name 'foo//bar': package names may not contain '//' path separators"); ParsingFailedEvent err = tester.findPostOnce(ParsingFailedEvent.class); assertThat(err.getPattern()).isEqualTo("foo//bar:missing"); } @@ -257,7 +256,7 @@ public void testMistypedTarget() { @Test public void testEmptyTarget() { TargetParsingException e = assertThrows(TargetParsingException.class, () -> tester.load("")); - assertThat(e).hasMessageThat().contains("the empty string is not a valid target"); + assertThat(e).hasMessageThat().contains("invalid target name '': empty target name"); } @Test @@ -265,8 +264,7 @@ public void testMistypedTargetKeepGoing() throws Exception { TargetPatternPhaseValue result = tester.loadKeepGoing("foo//bar:missing"); assertThat(result.hasError()).isTrue(); tester.assertContainsError( - "invalid target format 'foo//bar:missing': invalid package name 'foo//bar': package names" - + " may not contain '//' path separators"); + "invalid package name 'foo//bar': package names may not contain '//' path separators"); ParsingFailedEvent err = tester.findPostOnce(ParsingFailedEvent.class); assertThat(err.getPattern()).isEqualTo("foo//bar:missing"); } @@ -1142,8 +1140,7 @@ private void expectError(String pattern, String message) { public void testPatternWithSingleSlashIsError() { expectError( "/single/slash", - "not a valid absolute pattern (absolute target patterns must start with exactly " - + "two slashes): '/single/slash'"); + "invalid target name '/single/slash': target names may not start with '/'"); } @Test @@ -1151,28 +1148,26 @@ public void testPatternWithSingleSlashIsErrorAndOffset() { tester.setRelativeWorkingDirectory("base"); expectError( "/single/slash", - "not a valid absolute pattern (absolute target patterns must start with exactly " - + "two slashes): '/single/slash'"); + "invalid target name '/single/slash': target names may not start with '/'"); } @Test public void testPatternWithTripleSlashIsError() { expectError( "///triple/slash", - "not a valid absolute pattern (absolute target patterns must start with exactly " - + "two slashes): '///triple/slash'"); + "invalid package name '/triple/slash': package names may not start with '/'"); } @Test public void testPatternEndingWithSingleSlashIsError() { - expectError("foo/", "The package part of 'foo/' should not end in a slash"); + expectError("foo/", "invalid target name 'foo/': target names may not end with '/'"); } @Test public void testPatternStartingWithDotDotSlash() { expectError( "../foo", - "Bad target pattern '../foo': package name component contains only '.' characters"); + "invalid target name '../foo': target names may not contain up-level references '..'"); } private void runTestPackageLoadingError(boolean keepGoing, String... patterns) throws Exception { diff --git a/src/test/java/com/google/devtools/build/lib/query2/testutil/AbstractQueryTest.java b/src/test/java/com/google/devtools/build/lib/query2/testutil/AbstractQueryTest.java index 15e34d881d465e..dfaccbdc3904fb 100644 --- a/src/test/java/com/google/devtools/build/lib/query2/testutil/AbstractQueryTest.java +++ b/src/test/java/com/google/devtools/build/lib/query2/testutil/AbstractQueryTest.java @@ -83,11 +83,6 @@ public abstract class AbstractQueryTest { private static final String DEFAULT_UNIVERSE = "//...:*"; - protected static final String BAD_PACKAGE_NAME = - "package names may contain " - + "A-Z, a-z, 0-9, or any of ' !\"#$%&'()*+,-./;<=>?[]^_`{|}~' " - + "(most 7-bit ascii characters except 0-31, 127, ':', or '\\')"; - protected MockToolsConfig mockToolsConfig; protected QueryHelper helper; protected AnalysisMock analysisMock; @@ -324,7 +319,7 @@ public void testBadTargetLiterals() throws Exception { protected final void checkResultofBadTargetLiterals(String message, FailureDetail failureDetail) { assertThat(failureDetail.getTargetPatterns().getCode()) .isEqualTo(TargetPatterns.Code.LABEL_SYNTAX_ERROR); - assertThat(message).isEqualTo("Invalid package name 'bad:*': " + BAD_PACKAGE_NAME); + assertThat(message).isEqualTo("invalid target name '*:*': target names may not contain ':'"); } @Test diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/WorkspaceFileFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/WorkspaceFileFunctionTest.java index 929c23a7bea459..93d2ce512af385 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/WorkspaceFileFunctionTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/WorkspaceFileFunctionTest.java @@ -259,7 +259,7 @@ public void testRegisterToolchainsInvalidPattern() throws Exception { EvaluationResult evaluationResult = eval(key); Package pkg = evaluationResult.get(key).getPackage(); assertThat(pkg.containsErrors()).isTrue(); - assertContainsEvent("not a valid absolute pattern"); + assertContainsEvent("error parsing target pattern"); } @Test diff --git a/src/test/shell/integration/loading_phase_test.sh b/src/test/shell/integration/loading_phase_test.sh index 478410cd79427c..da44fb8589b24c 100755 --- a/src/test/shell/integration/loading_phase_test.sh +++ b/src/test/shell/integration/loading_phase_test.sh @@ -597,18 +597,6 @@ EOF assert_contains "^${filename}$" $(bazel info "${PRODUCT_NAME}-bin")/$pkg/paths.txt } -function test_path_from_subdir() { - local -r pkg="${FUNCNAME}" - mkdir -p "$pkg/subdir" || fail "could not create \"$pkg/subdir\"" - touch "$pkg/subdir/BUILD" || fail "Could not touch" - echo 'filegroup(name = "foo", srcs = [])' > "$pkg/BUILD" || fail "echo" - cd "$pkg/subdir" - bazel query '../BUILD + ../foo' >output 2> "$TEST_log" \ - || fail "Expected success" - assert_contains "^//$pkg:BUILD" output - assert_contains "^//$pkg:foo" output -} - function test_target_with_BUILD() { local -r pkg="${FUNCNAME}" mkdir -p "$pkg" || fail "could not create \"$pkg\"" diff --git a/src/test/shell/integration/toolchain_test.sh b/src/test/shell/integration/toolchain_test.sh index 24623e24840f8c..035802af91cde3 100755 --- a/src/test/shell/integration/toolchain_test.sh +++ b/src/test/shell/integration/toolchain_test.sh @@ -888,7 +888,7 @@ use_toolchain( EOF bazel build "//${pkg}/demo:use" &> $TEST_log && fail "Build failure expected" - expect_log "error parsing target pattern \"/:invalid:label:syntax\": not a valid absolute pattern" + expect_log "error parsing target pattern \"/:invalid:label:syntax\": invalid package name '/': package names may not start with '/'" } function test_register_toolchain_error_invalid_target() {