Fix query command suggestion in error message with repo mappings
The package label in the query command suggested when a specified target isn't found in a package 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`.
Closes #16482.
PiperOrigin-RevId: 483358185
Change-Id: I5a6eeae9950c614d5268aed62474f5feb31a8ce6
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 82ea549..da4b7ae 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
@@ -178,6 +178,53 @@
}
/**
+ * 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 a label representation for this package that is suitable for display. The returned
+ * string is as simple as possible while referencing the current package when parsed in the
+ * context of the main repository.
+ *
+ * @param mainRepositoryMapping the {@link RepositoryMapping} of the main repository
+ * @return
+ * <dl>
+ * <dt><code>//some/pkg</code>
+ * <dd>if this package lives in the main repository
+ * <dt><code>@protobuf//some/pkg</code>
+ * <dd>if this package lives in a repository with "protobuf" as <code>name</code> of a
+ * repository in WORKSPACE or as apparent name of a Bzlmod dependency of the main module
+ * <dt><code>@@protobuf~3.19.2//some/pkg</code>
+ * <dd>only with Bzlmod if the current package belongs to a repository that is not visible
+ * from the main module
+ */
+ public String getDisplayForm(RepositoryMapping mainRepositoryMapping) {
+ Preconditions.checkArgument(
+ mainRepositoryMapping.ownerRepo() == null || mainRepositoryMapping.ownerRepo().isMain());
+ if (repository.isMain()) {
+ // Packages in the main repository can always use repo-relative form.
+ return "//" + getPackageFragment();
+ }
+ if (!mainRepositoryMapping.usesStrictDeps()) {
+ // If the main repository mapping is not using strict visibility, then Bzlmod is certainly
+ // disabled, which means that canonical and apparent names can be used interchangeably from
+ // the context of the main repository.
+ return repository.getNameWithAt() + "//" + getPackageFragment();
+ }
+ // If possible, represent the repository with a non-canonical label using the apparent name the
+ // main repository has for it, otherwise fall back to a canonical label.
+ return mainRepositoryMapping
+ .getInverse(repository)
+ .map(apparentName -> "@" + apparentName + "//" + getPackageFragment())
+ .orElseGet(this::getUnambiguousCanonicalForm);
+ }
+
+ /**
* Returns the package path, possibly qualified with a repository name.
*
* <p>Packages that live in the main repo are stringified without a "@" qualifier or "//"
diff --git a/src/main/java/com/google/devtools/build/lib/cmdline/RepositoryMapping.java b/src/main/java/com/google/devtools/build/lib/cmdline/RepositoryMapping.java
index e504ff9..3a2889c 100644
--- a/src/main/java/com/google/devtools/build/lib/cmdline/RepositoryMapping.java
+++ b/src/main/java/com/google/devtools/build/lib/cmdline/RepositoryMapping.java
@@ -19,6 +19,8 @@
import com.google.common.collect.ImmutableMap;
import java.util.HashMap;
import java.util.Map;
+import java.util.Map.Entry;
+import java.util.Optional;
import javax.annotation.Nullable;
/**
@@ -93,4 +95,22 @@
return RepositoryName.createUnvalidated(preMappingName).toNonVisible(ownerRepo());
}
}
+
+ /**
+ * Whether the repo with this mapping is subject to strict deps; when strict deps is off, unknown
+ * apparent names are silently treated as canonical names.
+ */
+ public boolean usesStrictDeps() {
+ return ownerRepo() != null;
+ }
+
+ /**
+ * Returns the first apparent name in this mapping that maps to the given canonical name, if any.
+ */
+ public Optional<String> getInverse(RepositoryName postMappingName) {
+ return repositoryMapping().entrySet().stream()
+ .filter(e -> e.getValue().equals(postMappingName))
+ .map(Entry::getKey)
+ .findFirst();
+ }
}
diff --git a/src/main/java/com/google/devtools/build/lib/packages/Package.java b/src/main/java/com/google/devtools/build/lib/packages/Package.java
index a73a3d3..ac0caaf 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/Package.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/Package.java
@@ -248,6 +248,13 @@
*/
private RepositoryMapping repositoryMapping;
+ /**
+ * The repository mapping of the main repository. This is only used internally to obtain
+ * user-friendly apparent names from canonical repository names in error message arising from this
+ * package.
+ */
+ private RepositoryMapping mainRepositoryMapping;
+
private Set<Label> defaultCompatibleWith = ImmutableSet.of();
private Set<Label> defaultRestrictedTo = ImmutableSet.of();
@@ -478,6 +485,7 @@
this.registeredExecutionPlatforms = ImmutableList.copyOf(builder.registeredExecutionPlatforms);
this.registeredToolchains = ImmutableList.copyOf(builder.registeredToolchains);
this.repositoryMapping = Preconditions.checkNotNull(builder.repositoryMapping);
+ this.mainRepositoryMapping = Preconditions.checkNotNull(builder.mainRepositoryMapping);
ImmutableMap.Builder<RepositoryName, ImmutableMap<String, RepositoryName>>
repositoryMappingsBuilder = ImmutableMap.builder();
if (!builder.externalPackageRepositoryMappings.isEmpty() && !builder.isRepoRulePackage()) {
@@ -731,7 +739,7 @@
String blazeQuerySuggestion =
String.format(
"Tip: use `query %s:*` to see all the targets in that package",
- packageIdentifier.getCanonicalForm());
+ packageIdentifier.getDisplayForm(mainRepositoryMapping));
return String.format(
" (%s)", Joiner.on(" ").skipNulls().join(targetSuggestion, blazeQuerySuggestion));
}
@@ -880,6 +888,7 @@
LabelConstants.EXTERNAL_PACKAGE_IDENTIFIER,
workspaceName,
starlarkSemantics.getBool(BuildLanguageOptions.INCOMPATIBLE_NO_IMPLICIT_FILE_EXPORT),
+ mainRepoMapping,
mainRepoMapping)
.setFilename(workspacePath);
}
@@ -894,7 +903,11 @@
basePackageId,
DUMMY_WORKSPACE_NAME_FOR_BZLMOD_PACKAGES,
starlarkSemantics.getBool(BuildLanguageOptions.INCOMPATIBLE_NO_IMPLICIT_FILE_EXPORT),
- repoMapping)
+ repoMapping,
+ // This mapping is *not* the main repository's mapping, but since it is only used to
+ // construct a query command in an error message and the package built here can't be
+ // seen by query, the particular value does not matter.
+ RepositoryMapping.ALWAYS_FALLBACK)
.setFilename(moduleFilePath);
}
@@ -974,6 +987,11 @@
* workspace.
*/
private final RepositoryMapping repositoryMapping;
+ /**
+ * The repository mapping of the main repository. This is only used to resolve user-friendly
+ * apparent names from canonical repository names in error message arising from this package.
+ */
+ private final RepositoryMapping mainRepositoryMapping;
/** Converts label literals to Label objects within this package. */
private final LabelConverter labelConverter;
@@ -1098,10 +1116,12 @@
PackageIdentifier id,
String workspaceName,
boolean noImplicitFileExport,
- RepositoryMapping repositoryMapping) {
+ RepositoryMapping repositoryMapping,
+ RepositoryMapping mainRepositoryMapping) {
this.pkg = new Package(id, workspaceName, packageSettings.succinctTargetNotFoundErrors());
this.noImplicitFileExport = noImplicitFileExport;
this.repositoryMapping = repositoryMapping;
+ this.mainRepositoryMapping = mainRepositoryMapping;
this.labelConverter = new LabelConverter(id, repositoryMapping);
if (pkg.getName().startsWith("javatests/")) {
setDefaultTestonly(true);
diff --git a/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java b/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java
index 911d668..3989b16e 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java
@@ -465,13 +465,15 @@
PackageIdentifier packageId,
String workspaceName,
StarlarkSemantics starlarkSemantics,
- RepositoryMapping repositoryMapping) {
+ RepositoryMapping repositoryMapping,
+ RepositoryMapping mainRepositoryMapping) {
return new Package.Builder(
packageSettings,
packageId,
workspaceName,
starlarkSemantics.getBool(BuildLanguageOptions.INCOMPATIBLE_NO_IMPLICIT_FILE_EXPORT),
- repositoryMapping);
+ repositoryMapping,
+ mainRepositoryMapping);
}
/** Returns a new {@link NonSkyframeGlobber}. */
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java
index 0e6f546..a8e4ee4 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java
@@ -33,6 +33,7 @@
import com.google.devtools.build.lib.cmdline.LabelSyntaxException;
import com.google.devtools.build.lib.cmdline.PackageIdentifier;
import com.google.devtools.build.lib.cmdline.RepositoryMapping;
+import com.google.devtools.build.lib.cmdline.RepositoryName;
import com.google.devtools.build.lib.collect.nestedset.NestedSet;
import com.google.devtools.build.lib.events.Event;
import com.google.devtools.build.lib.events.ExtendedEventHandler.Postable;
@@ -1215,6 +1216,8 @@
RepositoryMappingValue repositoryMappingValue =
(RepositoryMappingValue)
env.getValue(RepositoryMappingValue.key(packageId.getRepository()));
+ RepositoryMappingValue mainRepositoryMappingValue =
+ (RepositoryMappingValue) env.getValue(RepositoryMappingValue.key(RepositoryName.MAIN));
RootedPath buildFileRootedPath = packageLookupValue.getRootedPath(packageId);
FileValue buildFileValue = getBuildFileValue(env, buildFileRootedPath);
RuleVisibility defaultVisibility = PrecomputedValue.DEFAULT_VISIBILITY.get(env);
@@ -1346,7 +1349,8 @@
packageId,
workspaceName,
starlarkBuiltinsValue.starlarkSemantics,
- repositoryMapping)
+ repositoryMapping,
+ mainRepositoryMappingValue.getRepositoryMapping())
.setFilename(buildFileRootedPath)
.setDefaultVisibility(defaultVisibility)
// "defaultVisibility" comes from the command line.
diff --git a/src/test/java/com/google/devtools/build/lib/cmdline/PackageIdentifierTest.java b/src/test/java/com/google/devtools/build/lib/cmdline/PackageIdentifierTest.java
index d781332..3c80e34 100644
--- a/src/test/java/com/google/devtools/build/lib/cmdline/PackageIdentifierTest.java
+++ b/src/test/java/com/google/devtools/build/lib/cmdline/PackageIdentifierTest.java
@@ -16,14 +16,13 @@
import static com.google.common.truth.Truth.assertThat;
+import com.google.common.collect.ImmutableMap;
import com.google.devtools.build.lib.vfs.PathFragment;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.JUnit4;
-/**
- * Unit tests for {@link PackageIdentifier}.
- */
+/** Unit tests for {@link PackageIdentifier}. */
@RunWith(JUnit4.class)
public class PackageIdentifierTest {
@Test
@@ -93,4 +92,36 @@
assertThat(PackageIdentifier.create("", PathFragment.create("bar/baz")).getRunfilesPath())
.isEqualTo(PathFragment.create("bar/baz"));
}
+
+ @Test
+ public void testDisplayFormInMainRepository() throws Exception {
+ PackageIdentifier pkg =
+ PackageIdentifier.create(RepositoryName.MAIN, PathFragment.create("some/pkg"));
+
+ assertThat(pkg.getDisplayForm(RepositoryMapping.ALWAYS_FALLBACK)).isEqualTo("//some/pkg");
+ assertThat(
+ pkg.getDisplayForm(
+ RepositoryMapping.create(
+ ImmutableMap.of("foo", RepositoryName.create("bar")), RepositoryName.MAIN)))
+ .isEqualTo("//some/pkg");
+ }
+
+ @Test
+ public void testDisplayFormInExternalRepository() throws Exception {
+ RepositoryName repo = RepositoryName.create("canonical");
+ PackageIdentifier pkg = PackageIdentifier.create(repo, PathFragment.create("some/pkg"));
+
+ assertThat(pkg.getDisplayForm(RepositoryMapping.ALWAYS_FALLBACK))
+ .isEqualTo("@canonical//some/pkg");
+ assertThat(
+ pkg.getDisplayForm(
+ RepositoryMapping.create(ImmutableMap.of("local", repo), RepositoryName.MAIN)))
+ .isEqualTo("@local//some/pkg");
+ assertThat(
+ pkg.getDisplayForm(
+ RepositoryMapping.create(
+ ImmutableMap.of("local", RepositoryName.create("other_repo")),
+ RepositoryName.MAIN)))
+ .isEqualTo("@@canonical//some/pkg");
+ }
}
diff --git a/src/test/java/com/google/devtools/build/lib/packages/PackageTest.java b/src/test/java/com/google/devtools/build/lib/packages/PackageTest.java
index 3d33e49..6d8b81c 100644
--- a/src/test/java/com/google/devtools/build/lib/packages/PackageTest.java
+++ b/src/test/java/com/google/devtools/build/lib/packages/PackageTest.java
@@ -164,6 +164,7 @@
PackageIdentifier.createInMainRepo(name),
"workspace",
/*noImplicitFileExport=*/ true,
+ RepositoryMapping.ALWAYS_FALLBACK,
RepositoryMapping.ALWAYS_FALLBACK);
result.setFilename(
RootedPath.toRootedPath(
diff --git a/src/test/java/com/google/devtools/build/lib/packages/RuleClassTest.java b/src/test/java/com/google/devtools/build/lib/packages/RuleClassTest.java
index 541b95c..919d71a 100644
--- a/src/test/java/com/google/devtools/build/lib/packages/RuleClassTest.java
+++ b/src/test/java/com/google/devtools/build/lib/packages/RuleClassTest.java
@@ -265,6 +265,7 @@
PackageIdentifier.createInMainRepo(TEST_PACKAGE_NAME),
"TESTING",
StarlarkSemantics.DEFAULT,
+ RepositoryMapping.ALWAYS_FALLBACK,
RepositoryMapping.ALWAYS_FALLBACK)
.setFilename(RootedPath.toRootedPath(root, testBuildfilePath));
}
diff --git a/src/test/java/com/google/devtools/build/lib/packages/RuleFactoryTest.java b/src/test/java/com/google/devtools/build/lib/packages/RuleFactoryTest.java
index 06b8d38..558e2dd 100644
--- a/src/test/java/com/google/devtools/build/lib/packages/RuleFactoryTest.java
+++ b/src/test/java/com/google/devtools/build/lib/packages/RuleFactoryTest.java
@@ -58,7 +58,11 @@
private Package.Builder newBuilder(PackageIdentifier id, Path filename) {
return packageFactory
.newPackageBuilder(
- id, "TESTING", StarlarkSemantics.DEFAULT, RepositoryMapping.ALWAYS_FALLBACK)
+ id,
+ "TESTING",
+ StarlarkSemantics.DEFAULT,
+ RepositoryMapping.ALWAYS_FALLBACK,
+ RepositoryMapping.ALWAYS_FALLBACK)
.setFilename(RootedPath.toRootedPath(root, filename));
}
