From fb4a0c288ba5ee14e47c7879a6be3eefb44bfe9f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?X=C3=B9d=C5=8Dng=20Y=C3=A1ng?= Date: Wed, 5 Apr 2023 03:31:35 +1000 Subject: [PATCH] [6.2.0] TargetPattern parsing fixes (#17945) * Tests for TargetPattern parsing, and some sanity fixes - TargetPatternTest isn't testing much right now. This CL changes it to actually assert the result of parsing is as expected, and also test a few different parser setups (in main repo, with a relative directory, in a non-main repo). - This necessitated adding #toString methods to all TargetPattern subclasses. - Also rearranged the parameters in the constructors of all subclasses so that `originalPattern` is always the first parameter. - Removed the always-true `checkWildcardConflict` parameter in `TargetsInPackage`. - Removed the weak check before `interpretPathAsTarget` as it was not only weak, but wrong. It made parsing `Bar\\java` a failure at the repo root but a success in a subdirectory. Work towards https://github.com/bazelbuild/bazel/issues/4385 PiperOrigin-RevId: 520630430 Change-Id: Icee98f38134fe274ba800d2e949c83f0ae18c247 * Switch TargetPattern.Parser to use LabelParser - Target patterns and labels have very similar syntax, yet are parsed completely separately. Furthermore, the code for target pattern parsing wasn't written with repos in mind, causing some corner cases such as https://github.com/bazelbuild/bazel/issues/4385. - This CL augments LabelParser to deal with some syntax specific to target patterns (mostly the '...' thing), and switches TargetPattern.Parser to use LabelParser instead. - This fixes some inconsistencies between the two and eliminates the bug linked above. - Added LabelParserTest to make sure that the table in the javadoc of LabelParser.Parts#parse is actually accurate. - Switched LabelParser.Parts to use AutoValue instead to enable testing. - Some more cleanup in TargetPattern.Parser; removing outdated fields/methods/etc. Fixes https://github.com/bazelbuild/bazel/issues/4385. RELNOTES: `@foo` labels can now be used on the command line as the top-level target (that is, `bazel build @foo` now works). Double-dot syntax is now forbidden (`bazel build ../foo` will no longer work). PiperOrigin-RevId: 520902549 Change-Id: I38d6400381a3c4ac4c370360578702d5dd870909 --- .../devtools/build/lib/cmdline/Label.java | 32 +- .../build/lib/cmdline/LabelParser.java | 149 +++++---- .../build/lib/cmdline/PackageIdentifier.java | 4 +- .../build/lib/cmdline/TargetPattern.java | 286 +++++++--------- .../build/lib/cmdline/LabelParserTest.java | 69 ++++ .../build/lib/cmdline/TargetPatternTest.java | 306 ++++++++++++++---- .../lib/pkgcache/LoadingPhaseRunnerTest.java | 21 +- .../query2/testutil/AbstractQueryTest.java | 7 +- .../skyframe/WorkspaceFileFunctionTest.java | 2 +- .../shell/integration/loading_phase_test.sh | 12 - src/test/shell/integration/toolchain_test.sh | 2 +- 11 files changed, 537 insertions(+), 353 deletions(-) create mode 100644 src/test/java/com/google/devtools/build/lib/cmdline/LabelParserTest.java 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() {