Skip to content

Commit

Permalink
Fix query command suggestion in error message with repo mappings
Browse files Browse the repository at this point in the history
The package label in the query command suggested when a specified target
isn't found in a packge looked like `@rules_cc~1.0.0//pkg`, which
doesn't work as the repo name is canonical, but the label isn't.

This is fixed by reusing the logic from
`Label#getUnambiguousCanonicalForm`.
  • Loading branch information
fmeum committed Oct 16, 2022
1 parent b12f3a9 commit 95bda29
Show file tree
Hide file tree
Showing 9 changed files with 21 additions and 14 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -416,9 +416,7 @@ public String getCanonicalForm() {
* Label.parse*(x.getUnambiguousCanonicalForm(), ...).equals(x)}).
*/
public String getUnambiguousCanonicalForm() {
return String.format(
"@@%s//%s:%s",
packageIdentifier.getRepository().getName(), packageIdentifier.getPackageFragment(), name);
return packageIdentifier.getUnambiguousCanonicalForm() + ":" + name;
}

/** Return the name of the repository label refers to without the leading `at` symbol. */
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -177,6 +177,15 @@ public String getCanonicalForm() {
return repository.getCanonicalForm() + "//" + getPackageFragment();
}

/**
* Returns an absolutely unambiguous canonical form for this package in label form. Parsing this
* string in any environment, even when subject to repository mapping, should identify the same
* package.
*/
public String getUnambiguousCanonicalForm() {
return String.format("@@%s//%s", getRepository().getName(), getPackageFragment());
}

/**
* Returns the package path, possibly qualified with a repository name.
*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -731,7 +731,7 @@ private String getAlternateTargetSuggestion(String targetName) {
String blazeQuerySuggestion =
String.format(
"Tip: use `query %s:*` to see all the targets in that package",
packageIdentifier.getCanonicalForm());
packageIdentifier.getUnambiguousCanonicalForm());
return String.format(
" (%s)", Joiner.on(" ").skipNulls().join(targetSuggestion, blazeQuerySuggestion));
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -699,8 +699,8 @@ public void testHelpfulErrorForWrongPackageLabels() throws Exception {
assertThat(result.hasError()).isTrue();
assertContainsEvent(
"no such target '//x:z': target 'z' not declared in package 'x' defined by"
+ " /workspace/x/BUILD (Tip: use `query //x:*` to see all the targets in that package)"
+ " and referenced by '//y:y'");
+ " /workspace/x/BUILD (Tip: use `query @@//x:*` to see all the targets in that"
+ " package) and referenced by '//y:y'");
}

@Test
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ public void testFileThatsNotRegisteredYieldsUnknownTargetException() throws Exce
.isEqualTo(
"no such target '//pkg:baz.txt': target 'baz.txt' not declared in package 'pkg' "
+ "defined by /workspace/pkg/BUILD (did you mean 'bar.txt'? Tip: use `query "
+ "//pkg:*` to see all the targets in that package)");
+ "@@//pkg:*` to see all the targets in that package)");
}

@Test
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -284,7 +284,7 @@ public void testCreationOfInputFiles() throws Exception {
.hasMessageThat()
.isEqualTo(
"no such target '//foo:A': target 'A' not declared in package 'foo' defined by"
+ " /workspace/foo/BUILD (Tip: use `query //foo:*` to see all the targets in that"
+ " /workspace/foo/BUILD (Tip: use `query @@//foo:*` to see all the targets in that"
+ " package)");

// These are the only input files: BUILD, Z
Expand Down Expand Up @@ -423,8 +423,8 @@ public void testHelpfulErrorForMissingExportsFiles() throws Exception {
.hasMessageThat()
.isEqualTo(
"no such target '//x:z.cc': target 'z.cc' not declared in package 'x' defined by"
+ " /workspace/x/BUILD (did you mean 'x.cc'? Tip: use `query //x:*` to see all the"
+ " targets in that package)");
+ " /workspace/x/BUILD (did you mean 'x.cc'? Tip: use `query @@//x:*` to see all"
+ " the targets in that package)");

e = assertThrows(NoSuchTargetException.class, () -> pkg.getTarget("dir"));
assertThat(e)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -160,8 +160,8 @@ public void testCompileOneDepOnMissingFile() throws Exception {
.hasMessageThat()
.isEqualTo(
"no such target '//foo:missing.cc': target 'missing.cc' not declared in package "
+ "'foo' defined by /workspace/foo/BUILD (Tip: use `query //foo:*` to see all the "
+ "targets in that package)");
+ "'foo' defined by /workspace/foo/BUILD (Tip: use `query @@//foo:*` to see all "
+ "the targets in that package)");

// Also, try a valid input file which has no dependent rules in its package.
e = assertThrows(TargetParsingException.class, () -> parseCompileOneDep("//foo:baz/bang"));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -237,7 +237,7 @@ public void testGetNonexistentTarget() throws Exception {
.isEqualTo(
"no such target '//pkg1:not-there': target 'not-there' "
+ "not declared in package 'pkg1' defined by /workspace/pkg1/BUILD (Tip: use "
+ "`query //pkg1:*` to see all the targets in that package)");
+ "`query @@//pkg1:*` to see all the targets in that package)");
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -290,7 +290,7 @@ protected final void checkResultOfTargetLiteralWithMissingTargets(
"no such target '//a:b': target 'b' not declared in package 'a' "
+ "defined by "
+ helper.getRootDirectory().getPathString()
+ "/a/BUILD (Tip: use `query //a:*` to see all the targets in that package)");
+ "/a/BUILD (Tip: use `query @@//a:*` to see all the targets in that package)");
assertThat(failureDetail.getPackageLoading().getCode())
.isEqualTo(FailureDetails.PackageLoading.Code.TARGET_MISSING);
}
Expand Down

0 comments on commit 95bda29

Please sign in to comment.