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/skyframe/ActionExecutionFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java
index c143581..8b37d9f 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java
@@ -274,7 +274,7 @@
path.getParentDirectory(), "Must pass in files, not root directory");
Preconditions.checkArgument(!parent.isAbsolute(), path);
SkyKey depKey =
- ContainingPackageLookupValue.key(PackageIdentifier.createInDefaultRepo(parent));
+ ContainingPackageLookupValue.key(PackageIdentifier.createInMainRepo(parent));
depKeys.put(path, depKey);
keysRequested.add(depKey);
}
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ContainingPackageLookupValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/ContainingPackageLookupValue.java
index ffee11c..0405f63 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ContainingPackageLookupValue.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ContainingPackageLookupValue.java
@@ -39,6 +39,7 @@
public static SkyKey key(PackageIdentifier id) {
Preconditions.checkArgument(!id.getPackageFragment().isAbsolute(), id);
+ Preconditions.checkArgument(!id.getRepository().isDefault(), id);
return SkyKey.create(SkyFunctions.CONTAINING_PACKAGE_LOOKUP, id);
}
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/EnvironmentBackedRecursivePackageProvider.java b/src/main/java/com/google/devtools/build/lib/skyframe/EnvironmentBackedRecursivePackageProvider.java
index 8045c59..92888d2 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/EnvironmentBackedRecursivePackageProvider.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/EnvironmentBackedRecursivePackageProvider.java
@@ -120,7 +120,7 @@
}
List<Path> roots = new ArrayList<>();
- if (repository.isDefault()) {
+ if (repository.isMain()) {
roots.addAll(packageLocator.getPathEntries());
} else {
RepositoryDirectoryValue repositoryValue =
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/GraphBackedRecursivePackageProvider.java b/src/main/java/com/google/devtools/build/lib/skyframe/GraphBackedRecursivePackageProvider.java
index 5703c42..a306f4fe 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/GraphBackedRecursivePackageProvider.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/GraphBackedRecursivePackageProvider.java
@@ -171,7 +171,7 @@
}
List<Path> roots = new ArrayList<>();
- if (repository.isDefault()) {
+ if (repository.isMain()) {
roots.addAll(pkgPath.getPathEntries());
} else {
RepositoryDirectoryValue repositoryValue =
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 4021049..90aee2d 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
@@ -569,6 +569,8 @@
Environment env,
SkylarkImportLookupFunction skylarkImportLookupFunctionForInlining)
throws PackageFunctionException, InterruptedException {
+ Preconditions.checkArgument(!packageId.getRepository().isDefault());
+
ImmutableList<SkylarkImport> imports = buildFileAST.getImports();
Map<String, Extension> importMap = Maps.newHashMapWithExpectedSize(imports.size());
ImmutableList.Builder<SkylarkFileDependency> fileDependencies = ImmutableList.builder();
@@ -803,7 +805,7 @@
- label.getPackageFragment().segmentCount(),
labelNameFragment.segmentCount());
message += " (perhaps you meant to put the colon here: '";
- if (containingPkg.getRepository().isDefault()) {
+ if (containingPkg.getRepository().isDefault() || containingPkg.getRepository().isMain()) {
message += "//";
}
message += containingPkg + ":" + labelNameInContainingPackage + "'?)";
@@ -1245,7 +1247,7 @@
}
static boolean isDefaultsPackage(PackageIdentifier packageIdentifier) {
- return packageIdentifier.getRepository().isDefault()
+ return packageIdentifier.getRepository().isMain()
&& packageIdentifier.getPackageFragment().equals(DEFAULTS_PACKAGE_NAME);
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/PackageLookupFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/PackageLookupFunction.java
index 9a53dd3..e4d9dd5 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/PackageLookupFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/PackageLookupFunction.java
@@ -54,8 +54,7 @@
return PackageLookupValue.success(pkgLocator.getPathEntries().get(0));
}
- if (!packageKey.getRepository().equals(PackageIdentifier.MAIN_REPOSITORY_NAME)
- && !packageKey.getRepository().isDefault()) {
+ if (!packageKey.getRepository().isMain()) {
return computeExternalPackageLookupValue(skyKey, env, packageKey);
} else if (packageKey.equals(Label.EXTERNAL_PACKAGE_IDENTIFIER)) {
return computeWorkspaceLookupValue(env, packageKey);
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/PackageLookupValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/PackageLookupValue.java
index 9460b85..e9d5bf5 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/PackageLookupValue.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/PackageLookupValue.java
@@ -93,10 +93,11 @@
static SkyKey key(PathFragment directory) {
Preconditions.checkArgument(!directory.isAbsolute(), directory);
- return key(PackageIdentifier.createInDefaultRepo(directory));
+ return key(PackageIdentifier.createInMainRepo(directory));
}
public static SkyKey key(PackageIdentifier pkgIdentifier) {
+ Preconditions.checkArgument(!pkgIdentifier.getRepository().isDefault());
return SkyKey.create(SkyFunctions.PACKAGE_LOOKUP, pkgIdentifier);
}
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/PackageValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/PackageValue.java
index 840a4c8..d637a7f 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/PackageValue.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/PackageValue.java
@@ -54,6 +54,7 @@
}
public static SkyKey key(PackageIdentifier pkgIdentifier) {
+ Preconditions.checkArgument(!pkgIdentifier.getRepository().isDefault());
return SkyKey.create(SkyFunctions.PACKAGE, pkgIdentifier);
}
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/PrepareDepsOfPatternFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/PrepareDepsOfPatternFunction.java
index 2261b77..9caa115 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/PrepareDepsOfPatternFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/PrepareDepsOfPatternFunction.java
@@ -223,7 +223,7 @@
rulesOnly ? FilteringPolicies.RULES_ONLY : FilteringPolicies.NO_FILTER;
PathFragment pathFragment = TargetPatternResolverUtil.getPathFragment(directory);
List<Path> roots = new ArrayList<>();
- if (repository.isDefault()) {
+ if (repository.isMain()) {
roots.addAll(pkgPath.getPathEntries());
} else {
RepositoryDirectoryValue repositoryValue =
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/RecursiveDirectoryTraversalFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/RecursiveDirectoryTraversalFunction.java
index ce9a912..a32e88b 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/RecursiveDirectoryTraversalFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/RecursiveDirectoryTraversalFunction.java
@@ -152,7 +152,7 @@
PackageIdentifier packageId = PackageIdentifier.create(
recursivePkgKey.getRepository(), rootRelativePath);
- if (packageId.getRepository().isDefault()
+ if ((packageId.getRepository().isDefault() || packageId.getRepository().isMain())
&& fileValue.isSymlink()
&& fileValue.getUnresolvedLinkTarget().startsWith(directories.getOutputBase().asFragment())) {
// Symlinks back to the output base are not traversed so that we avoid convenience symlinks.
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/RecursivePkgValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/RecursivePkgValue.java
index b318519..9bec1ee 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/RecursivePkgValue.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/RecursivePkgValue.java
@@ -85,6 +85,7 @@
ImmutableSet<PathFragment> excludedPaths) {
PathFragment.checkAllPathsAreUnder(excludedPaths,
rootedPath.getRelativePath());
+ Preconditions.checkState(!repositoryName.isDefault());
this.repositoryName = repositoryName;
this.rootedPath = Preconditions.checkNotNull(rootedPath);
this.excludedPaths = Preconditions.checkNotNull(excludedPaths);
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
index f4ece81..b948490 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
@@ -710,7 +710,7 @@
execPath.getParentDirectory(), "Must pass in files, not root directory");
Preconditions.checkArgument(!parent.isAbsolute(), execPath);
packageKeys.add(ContainingPackageLookupValue.key(
- PackageIdentifier.createInDefaultRepo(parent)));
+ PackageIdentifier.createInMainRepo(parent)));
}
EvaluationResult<ContainingPackageLookupValue> result;
@@ -736,7 +736,7 @@
Map<PathFragment, Root> roots = new HashMap<>();
for (PathFragment execPath : execPaths) {
ContainingPackageLookupValue value = result.get(ContainingPackageLookupValue.key(
- PackageIdentifier.createInDefaultRepo(execPath.getParentDirectory())));
+ PackageIdentifier.createInMainRepo(execPath.getParentDirectory())));
if (value.hasContainingPackage()) {
roots.put(execPath, Root.asSourceRoot(value.getContainingPackageRoot()));
} else {
@@ -1552,6 +1552,8 @@
* Returns whether the given package should be consider deleted and thus should be ignored.
*/
public boolean isPackageDeleted(PackageIdentifier packageName) {
+ Preconditions.checkState(!packageName.getRepository().isDefault(),
+ "package must be absolute: %s", packageName);
return deletedPackages.get().contains(packageName);
}
@@ -1586,7 +1588,7 @@
workingDirectory),
packageCacheOptions.defaultVisibility, packageCacheOptions.showLoadingProgress,
packageCacheOptions.globbingThreads, defaultsPackageContents, commandId);
- setDeletedPackages(ImmutableSet.copyOf(packageCacheOptions.deletedPackages));
+ setDeletedPackages(packageCacheOptions.getDeletedPackages());
incrementalBuildMonitor = new SkyframeIncrementalBuildMonitor();
invalidateTransientErrors();
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkImportLookupFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkImportLookupFunction.java
index 2b1f76c..47ee3ef 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkImportLookupFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkImportLookupFunction.java
@@ -232,7 +232,7 @@
for (PathFragment importPath : pathsToLookup) {
PathFragment relativeImportPath = importPath.toRelative();
PackageIdentifier pkgToLookUp =
- PackageIdentifier.createInDefaultRepo(relativeImportPath.getParentDirectory());
+ PackageIdentifier.createInMainRepo(relativeImportPath.getParentDirectory());
lookupMap.put(ContainingPackageLookupValue.key(pkgToLookUp), importPath);
}
@@ -303,6 +303,8 @@
static ImmutableMap<String, Label> findLabelsForLoadStatements(
ImmutableCollection<SkylarkImport> imports, Label containingFileLabel, Environment env)
throws SkylarkImportFailedException {
+ Preconditions.checkArgument(
+ !containingFileLabel.getPackageIdentifier().getRepository().isDefault());
Map<String, Label> outputMap = Maps.newHashMapWithExpectedSize(imports.size());
// Filter relative vs. absolute paths.
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkImportLookupValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkImportLookupValue.java
index 2fbaf10..201e8fc 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkImportLookupValue.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkylarkImportLookupValue.java
@@ -69,6 +69,7 @@
public SkylarkImportLookupKey(Label importLabel, boolean inWorkspace) {
Preconditions.checkNotNull(importLabel);
+ Preconditions.checkArgument(!importLabel.getPackageIdentifier().getRepository().isDefault());
this.importLabel = importLabel;
this.inWorkspace = inWorkspace;
}
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/TargetMarkerValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/TargetMarkerValue.java
index 9c39849..f1aa03d 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/TargetMarkerValue.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/TargetMarkerValue.java
@@ -16,6 +16,7 @@
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe;
+import com.google.devtools.build.lib.util.Preconditions;
import com.google.devtools.build.skyframe.SkyKey;
import com.google.devtools.build.skyframe.SkyValue;
@@ -45,6 +46,7 @@
@ThreadSafe
public static SkyKey key(Label label) {
+ Preconditions.checkArgument(!label.getPackageIdentifier().getRepository().isDefault());
return SkyKey.create(SkyFunctions.TARGET_MARKER, label);
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/TransitiveTargetValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/TransitiveTargetValue.java
index d8e9b26..6fb4fcf 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/TransitiveTargetValue.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/TransitiveTargetValue.java
@@ -22,6 +22,7 @@
import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe;
import com.google.devtools.build.lib.packages.NoSuchTargetException;
+import com.google.devtools.build.lib.util.Preconditions;
import com.google.devtools.build.skyframe.SkyKey;
import com.google.devtools.build.skyframe.SkyValue;
@@ -170,6 +171,7 @@
@ThreadSafe
public static SkyKey key(Label label) {
+ Preconditions.checkArgument(!label.getPackageIdentifier().getRepository().isDefault());
return SkyKey.create(SkyFunctions.TRANSITIVE_TARGET, label);
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/TransitiveTraversalValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/TransitiveTraversalValue.java
index 59252d8..2070ff2 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/TransitiveTraversalValue.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/TransitiveTraversalValue.java
@@ -125,6 +125,7 @@
@ThreadSafe
public static SkyKey key(Label label) {
+ Preconditions.checkArgument(!label.getPackageIdentifier().getRepository().isDefault());
return SkyKey.create(SkyFunctions.TRANSITIVE_TRAVERSAL, label);
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/WorkspaceFileFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/WorkspaceFileFunction.java
index 8536558..ea6f4dc 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/WorkspaceFileFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/WorkspaceFileFunction.java
@@ -16,7 +16,7 @@
import com.google.common.collect.ImmutableMap;
import com.google.devtools.build.lib.analysis.BlazeDirectories;
-import com.google.devtools.build.lib.cmdline.Label;
+import com.google.devtools.build.lib.cmdline.PackageIdentifier;
import com.google.devtools.build.lib.packages.Package;
import com.google.devtools.build.lib.packages.Package.LegacyBuilder;
import com.google.devtools.build.lib.packages.Package.NameConflictException;
@@ -44,6 +44,7 @@
private final PackageFactory packageFactory;
private final BlazeDirectories directories;
private final RuleClassProvider ruleClassProvider;
+ private static final PackageIdentifier rootPackage = PackageIdentifier.createInMainRepo("");
public WorkspaceFileFunction(
RuleClassProvider ruleClassProvider,
@@ -105,7 +106,7 @@
BuildFileAST ast = workspaceASTValue.getASTs().get(key.getIndex());
PackageFunction.SkylarkImportResult importResult =
PackageFunction.fetchImportsFromBuildFile(
- repoWorkspace, Label.EXTERNAL_PACKAGE_IDENTIFIER, ast, env, null);
+ repoWorkspace, rootPackage, ast, env, null);
if (importResult == null) {
return null;
}
