Make ExternalPackageFunction's key a singleton
PackageFunction finds the path to the workspace file and calls ExternalPackageFunction with it, who promptly discards it and finds the path itself again. So we just make PackageFunction directly call ExternalPackageFunction without any arguments (i.e. making ExternalPackageFunction.key a singleton).
PiperOrigin-RevId: 362939149
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ExternalPackageFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/ExternalPackageFunction.java
index 29568f4..43bef3d 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ExternalPackageFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ExternalPackageFunction.java
@@ -14,15 +14,11 @@
package com.google.devtools.build.lib.skyframe;
-import com.google.common.collect.Interner;
-import com.google.devtools.build.lib.concurrent.BlazeInterners;
import com.google.devtools.build.lib.packages.WorkspaceFileValue;
import com.google.devtools.build.lib.repository.ExternalPackageHelper;
import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec;
import com.google.devtools.build.lib.vfs.RootedPath;
-import com.google.devtools.build.skyframe.AbstractSkyKey;
import com.google.devtools.build.skyframe.SkyFunction;
-import com.google.devtools.build.skyframe.SkyFunctionName;
import com.google.devtools.build.skyframe.SkyKey;
import com.google.devtools.build.skyframe.SkyValue;
import javax.annotation.Nullable;
@@ -34,6 +30,8 @@
* that will contain all the bind statements from the WORKSPACE file.
*/
public class ExternalPackageFunction implements SkyFunction {
+ @AutoCodec @AutoCodec.VisibleForSerialization
+ static final SkyKey KEY = () -> SkyFunctions.EXTERNAL_PACKAGE;
private final ExternalPackageHelper externalPackageHelper;
@@ -72,29 +70,8 @@
return null;
}
- /** Returns a {@link Key} to find the WORKSPACE file at the given path. */
- public static SkyKey key(RootedPath workspacePath) {
- return Key.create(workspacePath);
- }
-
- @AutoCodec.VisibleForSerialization
- @AutoCodec
- static class Key extends AbstractSkyKey<RootedPath> {
- private static final Interner<Key> interner = BlazeInterners.newWeakInterner();
-
- private Key(RootedPath arg) {
- super(arg);
- }
-
- @AutoCodec.VisibleForSerialization
- @AutoCodec.Instantiator
- static Key create(RootedPath arg) {
- return interner.intern(new Key(arg));
- }
-
- @Override
- public SkyFunctionName functionName() {
- return SkyFunctions.EXTERNAL_PACKAGE;
- }
+ /** Returns the singleton {@link SkyKey} for the external package. */
+ public static SkyKey key() {
+ return KEY;
}
}
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 e4d2d55..f60b1a0 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
@@ -54,7 +54,6 @@
import com.google.devtools.build.lib.profiler.Profiler;
import com.google.devtools.build.lib.profiler.ProfilerTask;
import com.google.devtools.build.lib.profiler.SilentCloseable;
-import com.google.devtools.build.lib.repository.ExternalPackageHelper;
import com.google.devtools.build.lib.server.FailureDetails.FailureDetail;
import com.google.devtools.build.lib.server.FailureDetails.PackageLoading;
import com.google.devtools.build.lib.server.FailureDetails.PackageLoading.Code;
@@ -110,7 +109,6 @@
private final AtomicBoolean showLoadingProgress;
private final AtomicInteger numPackagesLoaded;
@Nullable private final PackageProgressReceiver packageProgress;
- private final ExternalPackageHelper externalPackageHelper;
// Not final only for testing.
@Nullable private BzlLoadFunction bzlLoadFunctionForInlining;
@@ -178,8 +176,7 @@
@Nullable BzlLoadFunction bzlLoadFunctionForInlining,
@Nullable PackageProgressReceiver packageProgress,
ActionOnIOExceptionReadingBuildFile actionOnIOExceptionReadingBuildFile,
- IncrementalityIntent incrementalityIntent,
- ExternalPackageHelper externalPackageHelper) {
+ IncrementalityIntent incrementalityIntent) {
this.bzlLoadFunctionForInlining = bzlLoadFunctionForInlining;
this.packageFactory = packageFactory;
this.packageLocator = pkgLocator;
@@ -190,7 +187,6 @@
this.packageProgress = packageProgress;
this.actionOnIOExceptionReadingBuildFile = actionOnIOExceptionReadingBuildFile;
this.incrementalityIntent = incrementalityIntent;
- this.externalPackageHelper = externalPackageHelper;
}
@VisibleForTesting
@@ -201,8 +197,7 @@
Cache<PackageIdentifier, LoadedPackageCacheEntry> packageFunctionCache,
Cache<PackageIdentifier, CompiledBuildFile> compiledBuildFileCache,
AtomicInteger numPackagesLoaded,
- @Nullable BzlLoadFunction bzlLoadFunctionForInlining,
- ExternalPackageHelper externalPackageHelper) {
+ @Nullable BzlLoadFunction bzlLoadFunctionForInlining) {
this(
packageFactory,
pkgLocator,
@@ -213,8 +208,7 @@
bzlLoadFunctionForInlining,
/*packageProgress=*/ null,
ActionOnIOExceptionReadingBuildFile.UseOriginalIOException.INSTANCE,
- IncrementalityIntent.INCREMENTAL,
- externalPackageHelper);
+ IncrementalityIntent.INCREMENTAL);
}
public void setBzlLoadFunctionForInliningForTesting(BzlLoadFunction bzlLoadFunctionForInlining) {
@@ -354,12 +348,11 @@
private SkyValue getExternalPackage(Environment env)
throws PackageFunctionException, InterruptedException {
StarlarkSemantics starlarkSemantics = PrecomputedValue.STARLARK_SEMANTICS.get(env);
- RootedPath workspacePath = externalPackageHelper.findWorkspaceFile(env);
if (env.valuesMissing()) {
return null;
}
- SkyKey workspaceKey = ExternalPackageFunction.key(workspacePath);
+ SkyKey workspaceKey = ExternalPackageFunction.key();
PackageValue workspace = null;
try {
// This may throw a NoSuchPackageException if the WORKSPACE file was malformed or had other
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 175b1ea..9043195 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
@@ -554,8 +554,7 @@
actionOnIOExceptionReadingBuildFile,
tracksStateForIncrementality()
? IncrementalityIntent.INCREMENTAL
- : IncrementalityIntent.NON_INCREMENTAL,
- externalPackageHelper));
+ : IncrementalityIntent.NON_INCREMENTAL));
map.put(SkyFunctions.PACKAGE_ERROR, new PackageErrorFunction());
map.put(SkyFunctions.PACKAGE_ERROR_MESSAGE, new PackageErrorMessageFunction());
map.put(SkyFunctions.TARGET_PATTERN_ERROR, new TargetPatternErrorFunction());
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/packages/AbstractPackageLoader.java b/src/main/java/com/google/devtools/build/lib/skyframe/packages/AbstractPackageLoader.java
index 99b4d66..0267d3d 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/packages/AbstractPackageLoader.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/packages/AbstractPackageLoader.java
@@ -457,7 +457,6 @@
return pkgLocatorRef.get().getPackageBuildFileNullable(packageName, syscallCacheRef);
}
};
- ExternalPackageHelper externalPackageHelper = getExternalPackageHelper();
ImmutableMap.Builder<SkyFunctionName, SkyFunction> builder = ImmutableMap.builder();
builder
.put(SkyFunctions.PRECOMPUTED, new PrecomputedFunction())
@@ -507,8 +506,7 @@
/*packageProgress=*/ null,
getActionOnIOExceptionReadingBuildFile(),
// Tell PackageFunction to optimize for our use-case of no incrementality.
- IncrementalityIntent.NON_INCREMENTAL,
- externalPackageHelper))
+ IncrementalityIntent.NON_INCREMENTAL))
.putAll(extraSkyFunctions);
return builder.build();
}
diff --git a/src/test/java/com/google/devtools/build/lib/repository/ExternalPackageHelperTest.java b/src/test/java/com/google/devtools/build/lib/repository/ExternalPackageHelperTest.java
index d16844b..34ed81f 100644
--- a/src/test/java/com/google/devtools/build/lib/repository/ExternalPackageHelperTest.java
+++ b/src/test/java/com/google/devtools/build/lib/repository/ExternalPackageHelperTest.java
@@ -152,16 +152,7 @@
directories,
/*bzlLoadFunctionForInlining=*/ null));
skyFunctions.put(
- SkyFunctions.PACKAGE,
- new PackageFunction(
- null,
- null,
- null,
- null,
- null,
- null,
- null,
- BazelSkyframeExecutorConstants.EXTERNAL_PACKAGE_HELPER));
+ SkyFunctions.PACKAGE, new PackageFunction(null, null, null, null, null, null, null));
skyFunctions.put(
SkyFunctions.EXTERNAL_PACKAGE,
new ExternalPackageFunction(BazelSkyframeExecutorConstants.EXTERNAL_PACKAGE_HELPER));
diff --git a/src/test/java/com/google/devtools/build/lib/rules/repository/RepositoryDelegatorTest.java b/src/test/java/com/google/devtools/build/lib/rules/repository/RepositoryDelegatorTest.java
index a845a3a..362d828 100644
--- a/src/test/java/com/google/devtools/build/lib/rules/repository/RepositoryDelegatorTest.java
+++ b/src/test/java/com/google/devtools/build/lib/rules/repository/RepositoryDelegatorTest.java
@@ -170,7 +170,7 @@
.put(SkyFunctions.REPOSITORY_DIRECTORY, delegatorFunction)
.put(
SkyFunctions.PACKAGE,
- new PackageFunction(null, null, null, null, null, null, null, null))
+ new PackageFunction(null, null, null, null, null, null, null))
.put(
SkyFunctions.PACKAGE_LOOKUP,
new PackageLookupFunction(
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/ArtifactFunctionTestCase.java b/src/test/java/com/google/devtools/build/lib/skyframe/ArtifactFunctionTestCase.java
index 6ceda54..3ec660f 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/ArtifactFunctionTestCase.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/ArtifactFunctionTestCase.java
@@ -113,7 +113,7 @@
.put(SkyFunctions.ACTION_EXECUTION, new SimpleActionExecutionFunction())
.put(
SkyFunctions.PACKAGE,
- new PackageFunction(null, null, null, null, null, null, null, null))
+ new PackageFunction(null, null, null, null, null, null, null))
.put(
SkyFunctions.PACKAGE_LOOKUP,
new PackageLookupFunction(
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/ContainingPackageLookupFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/ContainingPackageLookupFunctionTest.java
index 2db4032..c687dbb 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/ContainingPackageLookupFunctionTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/ContainingPackageLookupFunctionTest.java
@@ -110,7 +110,7 @@
BazelSkyframeExecutorConstants.BUILD_FILES_BY_PRIORITY,
BazelSkyframeExecutorConstants.EXTERNAL_PACKAGE_HELPER));
skyFunctions.put(
- SkyFunctions.PACKAGE, new PackageFunction(null, null, null, null, null, null, null, null));
+ SkyFunctions.PACKAGE, new PackageFunction(null, null, null, null, null, null, null));
skyFunctions.put(
SkyFunctions.IGNORED_PACKAGE_PREFIXES,
new IgnoredPackagePrefixesFunction(
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/FileFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/FileFunctionTest.java
index 77c2e6c..749ce07 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/FileFunctionTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/FileFunctionTest.java
@@ -170,7 +170,7 @@
.put(FileValue.FILE, new FileFunction(pkgLocatorRef))
.put(
SkyFunctions.PACKAGE,
- new PackageFunction(null, null, null, null, null, null, null, null))
+ new PackageFunction(null, null, null, null, null, null, null))
.put(
SkyFunctions.PACKAGE_LOOKUP,
new PackageLookupFunction(
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/FilesystemValueCheckerTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/FilesystemValueCheckerTest.java
index 47c2d8f..e6c792e 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/FilesystemValueCheckerTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/FilesystemValueCheckerTest.java
@@ -143,7 +143,7 @@
SkyFunctions.FILE_SYMLINK_INFINITE_EXPANSION_UNIQUENESS,
new FileSymlinkInfiniteExpansionUniquenessFunction());
skyFunctions.put(
- SkyFunctions.PACKAGE, new PackageFunction(null, null, null, null, null, null, null, null));
+ SkyFunctions.PACKAGE, new PackageFunction(null, null, null, null, null, null, null));
skyFunctions.put(
SkyFunctions.PACKAGE_LOOKUP,
new PackageLookupFunction(
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/PackageLookupFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/PackageLookupFunctionTest.java
index 36adb8f..73aeb14 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/PackageLookupFunctionTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/PackageLookupFunctionTest.java
@@ -118,7 +118,7 @@
BazelSkyframeExecutorConstants.BUILD_FILES_BY_PRIORITY,
BazelSkyframeExecutorConstants.EXTERNAL_PACKAGE_HELPER));
skyFunctions.put(
- SkyFunctions.PACKAGE, new PackageFunction(null, null, null, null, null, null, null, null));
+ SkyFunctions.PACKAGE, new PackageFunction(null, null, null, null, null, null, null));
skyFunctions.put(
FileStateValue.FILE_STATE,
new FileStateFunction(
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/RecursiveFilesystemTraversalFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/RecursiveFilesystemTraversalFunctionTest.java
index 8d653ba..0d0a332 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/RecursiveFilesystemTraversalFunctionTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/RecursiveFilesystemTraversalFunctionTest.java
@@ -164,7 +164,7 @@
new IgnoredPackagePrefixesFunction(
/*ignoredPackagePrefixesFile=*/ PathFragment.EMPTY_FRAGMENT));
skyFunctions.put(
- SkyFunctions.PACKAGE, new PackageFunction(null, null, null, null, null, null, null, null));
+ SkyFunctions.PACKAGE, new PackageFunction(null, null, null, null, null, null, null));
skyFunctions.put(
WorkspaceFileValue.WORKSPACE_FILE,
new WorkspaceFileFunction(
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/TimestampBuilderTestCase.java b/src/test/java/com/google/devtools/build/lib/skyframe/TimestampBuilderTestCase.java
index 54edfbf..baa8d72 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/TimestampBuilderTestCase.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/TimestampBuilderTestCase.java
@@ -271,7 +271,7 @@
new ActionExecutionFunction(skyframeActionExecutor, directories, tsgmRef))
.put(
SkyFunctions.PACKAGE,
- new PackageFunction(null, null, null, null, null, null, null, null))
+ new PackageFunction(null, null, null, null, null, null, null))
.put(
SkyFunctions.PACKAGE_LOOKUP,
new PackageLookupFunction(
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 850ec88..5bacda7 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
@@ -361,8 +361,8 @@
// Test intentionally introduces errors.
reporter.removeHandler(failFastHandler);
- RootedPath workspacePath = createWorkspaceFile("workspace(name = 'foo$')");
- SkyKey key = ExternalPackageFunction.key(workspacePath);
+ createWorkspaceFile("workspace(name = 'foo$')");
+ SkyKey key = ExternalPackageFunction.key();
EvaluationResult<PackageValue> evaluationResult = eval(key);
Package pkg = evaluationResult.get(key).getPackage();
assertThat(pkg.containsErrors()).isTrue();
@@ -372,9 +372,9 @@
@Test
public void testBindFunction() throws Exception {
String[] lines = {"bind(name = 'foo/bar',", "actual = '//foo:bar')"};
- RootedPath workspacePath = createWorkspaceFile(lines);
+ createWorkspaceFile(lines);
- SkyKey key = ExternalPackageFunction.key(workspacePath);
+ SkyKey key = ExternalPackageFunction.key();
EvaluationResult<PackageValue> evaluationResult = eval(key);
Package pkg = evaluationResult.get(key).getPackage();
assertThat(getLabelMapping(pkg, "foo/bar"))
@@ -385,9 +385,9 @@
@Test
public void testBindArgsReversed() throws Exception {
String[] lines = {"bind(actual = '//foo:bar', name = 'foo/bar')"};
- RootedPath workspacePath = createWorkspaceFile(lines);
+ createWorkspaceFile(lines);
- SkyKey key = ExternalPackageFunction.key(workspacePath);
+ SkyKey key = ExternalPackageFunction.key();
EvaluationResult<PackageValue> evaluationResult = eval(key);
Package pkg = evaluationResult.get(key).getPackage();
assertThat(getLabelMapping(pkg, "foo/bar"))
@@ -402,9 +402,9 @@
// name must be a valid label name.
String[] lines = {"bind(name = 'foo:bar', actual = '//bar/baz')"};
- RootedPath workspacePath = createWorkspaceFile(lines);
+ createWorkspaceFile(lines);
- SkyKey key = ExternalPackageFunction.key(workspacePath);
+ SkyKey key = ExternalPackageFunction.key();
EvaluationResult<PackageValue> evaluationResult = eval(key);
Package pkg = evaluationResult.get(key).getPackage();
assertThat(pkg.containsErrors()).isTrue();
@@ -418,9 +418,9 @@
// //external:bar:baz is not a legal package.
String[] lines = {"bind(name = 'foo/bar', actual = '//external:bar:baz')"};
- RootedPath workspacePath = createWorkspaceFile(lines);
+ createWorkspaceFile(lines);
- SkyKey key = ExternalPackageFunction.key(workspacePath);
+ SkyKey key = ExternalPackageFunction.key();
EvaluationResult<PackageValue> evaluationResult = eval(key);
Package pkg = evaluationResult.get(key).getPackage();
assertThat(pkg.containsErrors()).isTrue();
@@ -433,7 +433,7 @@
RootedPath workspacePath = createWorkspaceFile();
workspacePath.asPath().delete();
- SkyKey key = ExternalPackageFunction.key(workspacePath);
+ SkyKey key = ExternalPackageFunction.key();
EvaluationResult<PackageValue> evaluationResult = eval(key);
Package pkg = evaluationResult.get(key).getPackage();
assertThat(pkg.containsErrors()).isFalse();
@@ -445,9 +445,9 @@
String[] lines = {
"L = ['foo', 'bar']", "bind(name = '%s/%s' % (L[0], L[1]),", "actual = '//foo:bar')"
};
- RootedPath workspacePath = createWorkspaceFile(lines);
+ createWorkspaceFile(lines);
- SkyKey key = ExternalPackageFunction.key(workspacePath);
+ SkyKey key = ExternalPackageFunction.key();
EvaluationResult<PackageValue> evaluationResult = eval(key);
Package pkg = evaluationResult.get(key).getPackage();
assertThat(getLabelMapping(pkg, "foo/bar"))
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/WorkspaceNameFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/WorkspaceNameFunctionTest.java
index 85a7d14..f04433b 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/WorkspaceNameFunctionTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/WorkspaceNameFunctionTest.java
@@ -71,7 +71,8 @@
assertThatEvaluationResult(eval())
.hasErrorEntryForKeyThat(key)
.hasExceptionThat()
- .isInstanceOf(FileSymlinkCycleException.class);
+ .isInstanceOf(NoSuchPackageException.class);
+ assertContainsEvent("circular symlinks detected");
}
@Test
