Don't keep packages in the default repository around after loading.
Previously, this would get thrown when referring to the same package
from both the main and default repositories:
java.lang.IllegalArgumentException: Multiple entries with same key: tools/cpp=/home/brian/971-Robot-Code and tools/cpp=/home/brian/971-Robot-Code
at com.google.common.collect.ImmutableMap.checkNoConflict(ImmutableMap.java:136)
at com.google.common.collect.RegularImmutableMap.checkNoConflictInKeyBucket(RegularImmutableMap.java:98)
at com.google.common.collect.RegularImmutableMap.fromEntryArray(RegularImmutableMap.java:84)
at com.google.common.collect.ImmutableMap$Builder.build(ImmutableMap.java:295)
at com.google.devtools.build.lib.buildtool.BuildTool.transformPackageRoots(BuildTool.java:301)
at com.google.devtools.build.lib.buildtool.BuildTool.buildTargets(BuildTool.java:209)
at com.google.devtools.build.lib.buildtool.BuildTool.processRequest(BuildTool.java:334)
at com.google.devtools.build.lib.runtime.commands.TestCommand.doTest(TestCommand.java:119)
at com.google.devtools.build.lib.runtime.commands.TestCommand.exec(TestCommand.java:104)
at com.google.devtools.build.lib.runtime.BlazeCommandDispatcher.exec(BlazeCommandDispatcher.java:371)
at com.google.devtools.build.lib.runtime.BlazeRuntime$3.exec(BlazeRuntime.java:1016)
at com.google.devtools.build.lib.server.RPCService.executeRequest(RPCService.java:65)
at com.google.devtools.build.lib.server.RPCServer.executeRequest(RPCServer.java:434)
at com.google.devtools.build.lib.server.RPCServer.serve(RPCServer.java:229)
at com.google.devtools.build.lib.runtime.BlazeRuntime.serverMain(BlazeRuntime.java:975)
at com.google.devtools.build.lib.runtime.BlazeRuntime.main(BlazeRuntime.java:772)
at com.google.devtools.build.lib.bazel.BazelMain.main(BazelMain.java:55)
And this would get thrown for any packages in the main repository loaded
from other repositories:
java.lang.RuntimeException: Unrecoverable error while evaluating node 'PACKAGE:@//tools/build_rules/go/toolchain' (requested by nodes )
at com.google.devtools.build.skyframe.ParallelEvaluator$Evaluate.run(ParallelEvaluator.java:982)
at com.google.devtools.build.lib.concurrent.AbstractQueueVisitor$WrappedRunnable.run(AbstractQueueVisitor.java:499)
at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142)
at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617)
at java.lang.Thread.run(Thread.java:745)
Caused by: java.lang.IllegalArgumentException: Invalid BUILD file name for package '@//tools/build_rules/go/toolchain': /home/brian/bazel/tools/build_rules/go/toolchain/BUILD
at com.google.devtools.build.lib.packages.Package.finishInit(Package.java:299)
at com.google.devtools.build.lib.packages.Package$Builder.finishBuild(Package.java:1308)
at com.google.devtools.build.lib.skyframe.PackageFunction.compute(PackageFunction.java:501)
at com.google.devtools.build.skyframe.ParallelEvaluator$Evaluate.run(ParallelEvaluator.java:933)
... 4 more
Sponsor's comment: note the abundance of new Label.resolveRepositoryRelative() calls. They are ugly, but it's only making existing ugliness explicit. Yes, we should fix it, especially in the implementation of configurable attributes.
Refs #940
--
Change-Id: I8bd7f7b00bec58a7157507595421bc50c81b404c
Reviewed-on: https://bazel-review.googlesource.com/#/c/2591
MOS_MIGRATED_REVID=117429733
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 b1bcffa..030e689 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
@@ -48,7 +48,7 @@
* Package names that aren't made relative to the current repository because they mean special
* things to Bazel.
*/
- private static final ImmutableSet<PathFragment> ABSOLUTE_PACKAGE_NAMES = ImmutableSet.of(
+ public static final ImmutableSet<PathFragment> ABSOLUTE_PACKAGE_NAMES = ImmutableSet.of(
// Used for select
new PathFragment("conditions"),
// dependencies that are a function of the configuration
@@ -59,7 +59,7 @@
Label.EXTERNAL_PACKAGE_NAME);
public static final PackageIdentifier EXTERNAL_PACKAGE_IDENTIFIER =
- PackageIdentifier.createInDefaultRepo(EXTERNAL_PACKAGE_NAME);
+ PackageIdentifier.createInMainRepo(EXTERNAL_PACKAGE_NAME);
public static final String EXTERNAL_PATH_PREFIX = "external";
@@ -71,9 +71,28 @@
* {@literal @}foo//bar
* {@literal @}foo//bar:baz
* </pre>
+ *
+ * <p>Treats labels in the default repository as being in the main repository instead.
*/
public static Label parseAbsolute(String absName) throws LabelSyntaxException {
- String repo = PackageIdentifier.DEFAULT_REPOSITORY;
+ return parseAbsolute(absName, true);
+ }
+
+ /**
+ * Factory for Labels from absolute string form. e.g.
+ * <pre>
+ * //foo/bar
+ * //foo/bar:quux
+ * {@literal @}foo//bar
+ * {@literal @}foo//bar:baz
+ * </pre>
+ *
+ * @param defaultToMain Treat labels in the default repository as being in the main
+ * one instead.
+ */
+ public static Label parseAbsolute(String absName, boolean defaultToMain)
+ throws LabelSyntaxException {
+ String repo = defaultToMain ? "@" : PackageIdentifier.DEFAULT_REPOSITORY;
int packageStartPos = absName.indexOf("//");
if (packageStartPos > 0) {
repo = absName.substring(0, packageStartPos);
@@ -83,8 +102,12 @@
LabelValidator.PackageAndTarget labelParts = LabelValidator.parseAbsoluteLabel(absName);
PackageIdentifier pkgIdWithoutRepo =
validate(labelParts.getPackageName(), labelParts.getTargetName());
+ PathFragment packageFragment = pkgIdWithoutRepo.getPackageFragment();
+ if (repo.isEmpty() && ABSOLUTE_PACKAGE_NAMES.contains(packageFragment)) {
+ repo = "@";
+ }
return new Label(
- PackageIdentifier.create(repo, pkgIdWithoutRepo.getPackageFragment()),
+ PackageIdentifier.create(repo, packageFragment),
labelParts.getTargetName());
} catch (BadLabelException e) {
throw new LabelSyntaxException(e.getMessage());
@@ -98,14 +121,18 @@
*
* <p>Do not use this when the argument is not hard-wired.
*/
- public static Label parseAbsoluteUnchecked(String absName) {
+ public static Label parseAbsoluteUnchecked(String absName, boolean defaultToMain) {
try {
- return parseAbsolute(absName);
+ return parseAbsolute(absName, defaultToMain);
} catch (LabelSyntaxException e) {
throw new IllegalArgumentException(e);
}
}
+ public static Label parseAbsoluteUnchecked(String absName) {
+ return parseAbsoluteUnchecked(absName, true);
+ }
+
/**
* Factory for Labels from separate components.
*
@@ -243,7 +270,7 @@
}
private Object writeReplace() {
- return new LabelSerializationProxy(toString());
+ return new LabelSerializationProxy(getUnambiguousCanonicalForm());
}
private void readObject(ObjectInputStream stream) throws InvalidObjectException {
@@ -322,7 +349,7 @@
/**
* Renders this label in canonical form.
*
- * <p>invariant: {@code parseAbsolute(x.toString()).equals(x)}
+ * <p>invariant: {@code parseAbsolute(x.toString(), false).equals(x)}
*/
@Override
public String toString() {
@@ -332,21 +359,46 @@
/**
* Renders this label in canonical form.
*
- * <p>invariant: {@code parseAbsolute(x.toString()).equals(x)}
+ * <p>invariant: {@code parseAbsolute(x.getCanonicalForm(), false).equals(x)}
*/
public String getCanonicalForm() {
+ return getDefaultCanonicalForm();
+ }
+
+ public String getUnambiguousCanonicalForm() {
return packageIdentifier.getRepository() + "//" + packageIdentifier.getPackageFragment()
+ ":" + name;
}
/**
+ * Renders this label in canonical form, except with labels in the main and default
+ * repositories conflated.
+ */
+ public String getDefaultCanonicalForm() {
+ String repository;
+ if (packageIdentifier.getRepository().isMain()) {
+ repository = "";
+ } else {
+ repository = packageIdentifier.getRepository().getName();
+ }
+ return repository + "//" + packageIdentifier.getPackageFragment()
+ + ":" + name;
+ }
+
+ /**
* Renders this label in shorthand form.
*
* <p>Labels with canonical form {@code //foo/bar:bar} have the shorthand form {@code //foo/bar}.
* All other labels have identical shorthand and canonical forms.
*/
public String toShorthandString() {
- return packageIdentifier.getRepository() + (getPackageFragment().getBaseName().equals(name)
+ String repository;
+ if (packageIdentifier.getRepository().isMain()) {
+ repository = "";
+ } else {
+ repository = packageIdentifier.getRepository().getName();
+ }
+ return repository + (getPackageFragment().getBaseName().equals(name)
? "//" + getPackageFragment()
: toString());
}
@@ -391,7 +443,7 @@
}
if (LabelValidator.isAbsolute(relName)) {
- return resolveRepositoryRelative(parseAbsolute(relName));
+ return resolveRepositoryRelative(parseAbsolute(relName, false));
} else if (relName.equals(":")) {
throw new LabelSyntaxException("':' is not a valid package-relative label");
} else if (relName.charAt(0) == ':') {
@@ -407,11 +459,12 @@
* <p>This is necessary so that dependency edges in remote repositories do not need to explicitly
* mention their repository name. Otherwise, referring to e.g. <code>//a:b</code> in a remote
* repository would point back to the main repository, which is usually not what is intended.
+ *
+ * <p>The return value will not be in the default repository.
*/
public Label resolveRepositoryRelative(Label relative) {
if (packageIdentifier.getRepository().isDefault()
- || !relative.packageIdentifier.getRepository().isDefault()
- || ABSOLUTE_PACKAGE_NAMES.contains(relative.getPackageIdentifier().getPackageFragment())) {
+ || !relative.packageIdentifier.getRepository().isDefault()) {
return relative;
} else {
try {
diff --git a/src/main/java/com/google/devtools/build/lib/cmdline/LabelSerializationProxy.java b/src/main/java/com/google/devtools/build/lib/cmdline/LabelSerializationProxy.java
index 5d7e6d0..c28e185 100644
--- a/src/main/java/com/google/devtools/build/lib/cmdline/LabelSerializationProxy.java
+++ b/src/main/java/com/google/devtools/build/lib/cmdline/LabelSerializationProxy.java
@@ -44,6 +44,6 @@
}
private Object readResolve() {
- return Label.parseAbsoluteUnchecked(labelString);
+ return Label.parseAbsoluteUnchecked(labelString, false);
}
}
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 4c995eb..69fb31a 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
@@ -60,19 +60,22 @@
}
}
- // Temporary factory for identifiers without explicit repositories.
- // TODO(bazel-team): remove all usages of this.
+ /**
+ * This is only used by legacy callers. Actually creates the package identifier in the main
+ * repository, not the default one.
+ */
+ // TODO(lberki): Remove this method.
+ @Deprecated
public static PackageIdentifier createInDefaultRepo(String name) {
- return createInDefaultRepo(new PathFragment(name));
+ return create(MAIN_REPOSITORY_NAME, new PathFragment(name));
}
- public static PackageIdentifier createInDefaultRepo(PathFragment name) {
- try {
- return create(DEFAULT_REPOSITORY, name);
- } catch (LabelSyntaxException e) {
- throw new IllegalArgumentException("could not create package identifier for " + name
- + ": " + e.getMessage());
- }
+ public static PackageIdentifier createInMainRepo(String name) {
+ return createInMainRepo(new PathFragment(name));
+ }
+
+ public static PackageIdentifier createInMainRepo(PathFragment name) {
+ return create(MAIN_REPOSITORY_NAME, name);
}
/**
@@ -136,6 +139,14 @@
return repository.getPathFragment().getRelative(pkgName);
}
+ public PackageIdentifier makeAbsolute() {
+ if (!repository.isDefault()) {
+ return this;
+ }
+
+ return create(MAIN_REPOSITORY_NAME, pkgName);
+ }
+
/**
* Returns the name of this package.
*
@@ -145,7 +156,7 @@
*/
@Override
public String toString() {
- return (repository.isDefault() ? "" : repository + "//") + pkgName;
+ return (repository.isDefault() || repository.isMain() ? "" : repository + "//") + pkgName;
}
@Override
diff --git a/src/main/java/com/google/devtools/build/lib/cmdline/RepositoryName.java b/src/main/java/com/google/devtools/build/lib/cmdline/RepositoryName.java
index 6f5811d..9a4e5a7 100644
--- a/src/main/java/com/google/devtools/build/lib/cmdline/RepositoryName.java
+++ b/src/main/java/com/google/devtools/build/lib/cmdline/RepositoryName.java
@@ -178,6 +178,13 @@
}
/**
+ * Returns if this is the default repository, that is, {@link #name} is "@".
+ */
+ public boolean isMain() {
+ return name.equals("@");
+ }
+
+ /**
* Returns the repository name, with leading "{@literal @}" (or "" for the default repository).
*/
// TODO(bazel-team): Use this over toString()- easier to track its usage.
@@ -189,7 +196,7 @@
* Returns the path at which this repository is mapped within the exec root.
*/
public PathFragment getPathFragment() {
- return isDefault() || this.equals(PackageIdentifier.MAIN_REPOSITORY_NAME)
+ return isDefault() || isMain()
? PathFragment.EMPTY_FRAGMENT
: new PathFragment(Label.EXTERNAL_PATH_PREFIX).getRelative(strippedName());
}
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 f8c1a83..c405efa 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
@@ -270,7 +270,7 @@
Preconditions.checkArgument(excludedSubdirectories.isEmpty(),
"Target pattern \"%s\" of type %s cannot be evaluated with excluded subdirectories: %s.",
getOriginalPattern(), getType(), excludedSubdirectories);
- if (resolver.isPackage(PackageIdentifier.createInDefaultRepo(path))) {
+ if (resolver.isPackage(PackageIdentifier.createInMainRepo(path))) {
// User has specified a package name. lookout for default target.
callback.process(resolver.getExplicitTarget(label("//" + path)).getTargets());
} else {
@@ -281,7 +281,7 @@
// first BUILD file is found (i.e. longest prefix match).
for (int i = pieces.size() - 1; i > 0; i--) {
String packageName = SLASH_JOINER.join(pieces.subList(0, i));
- if (resolver.isPackage(PackageIdentifier.createInDefaultRepo(packageName))) {
+ if (resolver.isPackage(PackageIdentifier.createInMainRepo(packageName))) {
String targetName = SLASH_JOINER.join(pieces.subList(i, pieces.size()));
callback.process(
resolver
@@ -304,7 +304,7 @@
public PackageIdentifier getDirectory() {
int lastSlashIndex = path.lastIndexOf('/');
// The package name cannot be illegal because we verified it during target parsing
- return PackageIdentifier.createInDefaultRepo(
+ return PackageIdentifier.createInMainRepo(
lastSlashIndex < 0 ? "" : path.substring(0, lastSlashIndex));
}
@@ -591,7 +591,7 @@
String originalPattern = pattern;
final boolean includesRepo = pattern.startsWith("@");
- RepositoryName repository = PackageIdentifier.DEFAULT_REPOSITORY_NAME;
+ RepositoryName repository = null;
if (includesRepo) {
int pkgStart = pattern.indexOf("//");
if (pkgStart < 0) {
@@ -638,6 +638,14 @@
+ "' should not end in a slash");
}
+ if (repository == null) {
+ if (packagePart.startsWith(Label.EXTERNAL_PACKAGE_NAME.toString())) {
+ repository = PackageIdentifier.DEFAULT_REPOSITORY_NAME;
+ } else {
+ repository = PackageIdentifier.MAIN_REPOSITORY_NAME;
+ }
+ }
+
if (packagePart.endsWith("/...")) {
String realPackagePart = removeSuffix(packagePart, "/...");
PackageIdentifier packageIdentifier;
