From d24f94734266c4331ba8324a470d60ae68c26edc Mon Sep 17 00:00:00 2001 From: Googler Date: Thu, 29 Aug 2024 12:31:18 -0700 Subject: [PATCH] Decouple Skyframe worker thread stuff from repo fetching, part 1 Today, the code we have for using worker threads for repo fetching is tightly coupled with repo fetching. This doesn't need to be the case. This CL refactors some code in repo fetching so that eventually we can reuse the worker thread stuff for other use cases (for example, module extension eval). Most notably, `RepoFetchingSkyKeyComputeState` has a `recordedInputValues` field. This is conceptually an "output" of the repo fetching stage. By making `RepositoryFunction#fetch` _return_ a map of `recordedInputValues`, rather than taking such a map as an argument and writing into it, we can completely remove the `recordedInputValues` field from `RepoFetchingSkyKeyComputeState`. This allows us to parameterize the return type of the worker thread future, and thus rename `RepoFetchingSkyKeyComputeState` to just `WorkerSkyKeyComputeState`, and `RepoFetchingWorkerSkyFunctionEnvironment` to just `WorkerSkyFunctionEnvironment`. At this point, these two classes already have nothing to do with repo fetching, and can be moved to a shareable location. In a follow-up CL, I plan to refactor the code further such that some of the code interacting with these two classes in `StarlarkRepositoryFunction#fetch` can be moved into the two `Worker*` classes as well, so that the user of worker threads can enjoy a cleaner API. Work towards https://github.com/bazelbuild/bazel/issues/22729 PiperOrigin-RevId: 669024825 Change-Id: Ic1529a39ca81096812eff5e0fd74622b2ef0b68f --- .../LocalConfigPlatformFunction.java | 14 ++-- .../starlark/StarlarkRepositoryFunction.java | 65 +++++++------------ ...java => WorkerSkyFunctionEnvironment.java} | 21 +++--- ...ate.java => WorkerSkyKeyComputeState.java} | 45 +++++-------- .../android/AndroidSdkRepositoryFunction.java | 30 ++++----- .../repository/LocalRepositoryFunction.java | 16 ++--- .../NewLocalRepositoryFunction.java | 30 ++++----- .../RepositoryDelegatorFunction.java | 51 ++++++--------- .../rules/repository/RepositoryFunction.java | 56 +++++----------- 9 files changed, 125 insertions(+), 203 deletions(-) rename src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/{RepoFetchingWorkerSkyFunctionEnvironment.java => WorkerSkyFunctionEnvironment.java} (88%) rename src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/{RepoFetchingSkyKeyComputeState.java => WorkerSkyKeyComputeState.java} (75%) diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/LocalConfigPlatformFunction.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/LocalConfigPlatformFunction.java index e9ae1ed20bfab7..d8e74116886dc6 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/repository/LocalConfigPlatformFunction.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/repository/LocalConfigPlatformFunction.java @@ -20,7 +20,6 @@ import com.google.devtools.build.lib.bazel.ResolvedEvent; import com.google.devtools.build.lib.packages.Rule; import com.google.devtools.build.lib.packages.semantics.BuildLanguageOptions; -import com.google.devtools.build.lib.rules.repository.RepoRecordedInput; import com.google.devtools.build.lib.rules.repository.RepositoryDirectoryValue; import com.google.devtools.build.lib.rules.repository.RepositoryFunction; import com.google.devtools.build.lib.rules.repository.ResolvedFileValue; @@ -31,7 +30,6 @@ import com.google.devtools.build.skyframe.SkyFunctionException.Transience; import com.google.devtools.build.skyframe.SkyKey; import java.io.IOException; -import java.util.Map; import net.starlark.java.eval.StarlarkSemantics; /** Create a local repository that describes the auto-detected host platform. */ @@ -48,13 +46,8 @@ public Class getRuleDefinition() { } @Override - public RepositoryDirectoryValue.Builder fetch( - Rule rule, - Path outputDirectory, - BlazeDirectories directories, - Environment env, - Map recordedInputValues, - SkyKey key) + public FetchResult fetch( + Rule rule, Path outputDirectory, BlazeDirectories directories, Environment env, SkyKey key) throws RepositoryFunctionException, InterruptedException { ensureNativeRepoRuleEnabled(rule, env, "the platform defined at @platforms//host"); StarlarkSemantics starlarkSemantics = PrecomputedValue.STARLARK_SEMANTICS.get(env); @@ -104,7 +97,8 @@ public Object getResolvedInformation(XattrProvider xattrProvider) { }); // Return the needed info. - return RepositoryDirectoryValue.builder().setPath(outputDirectory); + return new FetchResult( + RepositoryDirectoryValue.builder().setPath(outputDirectory), ImmutableMap.of()); } private static String workspaceFileContent(String repositoryName) { diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryFunction.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryFunction.java index 0340faddb0824b..0096a0de81dbda 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryFunction.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryFunction.java @@ -59,7 +59,9 @@ import com.google.devtools.build.skyframe.SkyFunctionException.Transience; import com.google.devtools.build.skyframe.SkyKey; import java.io.IOException; +import java.util.LinkedHashMap; import java.util.Map; +import java.util.Optional; import java.util.concurrent.CancellationException; import java.util.concurrent.ExecutionException; import javax.annotation.Nullable; @@ -121,40 +123,31 @@ public void reportSkyframeRestart(Environment env, RepositoryName repoName) { } private record FetchArgs( - Rule rule, - Path outputDirectory, - BlazeDirectories directories, - Environment env, - Map recordedInputValues, - SkyKey key) { - FetchArgs toWorkerArgs(Environment env, Map recordedInputValues) { - return new FetchArgs(rule, outputDirectory, directories, env, recordedInputValues, key); + Rule rule, Path outputDirectory, BlazeDirectories directories, Environment env, SkyKey key) { + FetchArgs toWorkerArgs(Environment env) { + return new FetchArgs(rule, outputDirectory, directories, env, key); } } @Nullable @Override - public RepositoryDirectoryValue.Builder fetch( - Rule rule, - Path outputDirectory, - BlazeDirectories directories, - Environment env, - Map recordedInputValues, - SkyKey key) + public FetchResult fetch( + Rule rule, Path outputDirectory, BlazeDirectories directories, Environment env, SkyKey key) throws RepositoryFunctionException, InterruptedException { - var args = new FetchArgs(rule, outputDirectory, directories, env, recordedInputValues, key); + var args = new FetchArgs(rule, outputDirectory, directories, env, key); if (!useWorkers) { return fetchInternal(args); } // See below (the `catch CancellationException` clause) for why there's a `while` loop here. while (true) { - var state = env.getState(() -> new RepoFetchingSkyKeyComputeState(rule.getName())); - ListenableFuture workerFuture = + var state = env.getState(WorkerSkyKeyComputeState::new); + ListenableFuture workerFuture = state.getOrStartWorker( + "starlark-repository-" + rule.getName(), () -> { - Environment workerEnv = new RepoFetchingWorkerSkyFunctionEnvironment(state); + Environment workerEnv = new WorkerSkyFunctionEnvironment(state); setupRepoRoot(outputDirectory); - return fetchInternal(args.toWorkerArgs(workerEnv, state.recordedInputValues)); + return fetchInternal(args.toWorkerArgs(workerEnv)); }); try { state.delegateEnvQueue.put(env); @@ -164,9 +157,7 @@ public RepositoryDirectoryValue.Builder fetch( // null to trigger a Skyframe restart, but *don't* shut down the worker executor. return null; } - RepositoryDirectoryValue.Builder result = workerFuture.get(); - recordedInputValues.putAll(state.recordedInputValues); - return result; + return workerFuture.get(); } catch (ExecutionException e) { Throwables.throwIfInstanceOf(e.getCause(), RepositoryFunctionException.class); Throwables.throwIfUnchecked(e.getCause()); @@ -196,32 +187,22 @@ public RepositoryDirectoryValue.Builder fetch( } @Nullable - private RepositoryDirectoryValue.Builder fetchInternal(FetchArgs args) + private FetchResult fetchInternal(FetchArgs args) throws RepositoryFunctionException, InterruptedException { - return fetchInternal( - args.rule, - args.outputDirectory, - args.directories, - args.env, - args.recordedInputValues, - args.key); + return fetchInternal(args.rule, args.outputDirectory, args.directories, args.env, args.key); } @Nullable - private RepositoryDirectoryValue.Builder fetchInternal( - Rule rule, - Path outputDirectory, - BlazeDirectories directories, - Environment env, - Map recordedInputValues, - SkyKey key) + private FetchResult fetchInternal( + Rule rule, Path outputDirectory, BlazeDirectories directories, Environment env, SkyKey key) throws RepositoryFunctionException, InterruptedException { String defInfo = RepositoryResolvedEvent.getRuleDefinitionInformation(rule); env.getListener().post(new StarlarkRepositoryDefinitionLocationEvent(rule.getName(), defInfo)); StarlarkCallable function = rule.getRuleClassObject().getConfiguredTargetFunction(); - if (declareEnvironmentDependencies(recordedInputValues, env, getEnviron(rule)) == null) { + ImmutableMap> envVarValues = getEnvVarValues(env, getEnviron(rule)); + if (envVarValues == null) { return null; } StarlarkSemantics starlarkSemantics = PrecomputedValue.STARLARK_SEMANTICS.get(env); @@ -262,6 +243,7 @@ private RepositoryDirectoryValue.Builder fetchInternal( } ImmutableSet ignoredPatterns = checkNotNull(ignoredPackagesValue).getPatterns(); + Map recordedInputValues = new LinkedHashMap<>(); try (Mutability mu = Mutability.create("Starlark repository"); StarlarkRepositoryContext starlarkRepositoryContext = new StarlarkRepositoryContext( @@ -331,6 +313,8 @@ private RepositoryDirectoryValue.Builder fetchInternal( // Modify marker data to include the files/dirents/env vars used by the rule's implementation // function. + recordedInputValues.putAll( + Maps.transformValues(RepoRecordedInput.EnvVar.wrap(envVarValues), v -> v.orElse(null))); recordedInputValues.putAll(starlarkRepositoryContext.getRecordedFileInputs()); recordedInputValues.putAll(starlarkRepositoryContext.getRecordedDirentsInputs()); recordedInputValues.putAll(starlarkRepositoryContext.getRecordedDirTreeInputs()); @@ -395,7 +379,8 @@ private RepositoryDirectoryValue.Builder fetchInternal( } } - return RepositoryDirectoryValue.builder().setPath(outputDirectory); + return new FetchResult( + RepositoryDirectoryValue.builder().setPath(outputDirectory), recordedInputValues); } @SuppressWarnings("unchecked") diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/RepoFetchingWorkerSkyFunctionEnvironment.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/WorkerSkyFunctionEnvironment.java similarity index 88% rename from src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/RepoFetchingWorkerSkyFunctionEnvironment.java rename to src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/WorkerSkyFunctionEnvironment.java index 4d27a218728807..1cdedfea37a072 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/RepoFetchingWorkerSkyFunctionEnvironment.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/WorkerSkyFunctionEnvironment.java @@ -27,25 +27,24 @@ import javax.annotation.Nullable; /** - * A {@link SkyFunction.Environment} implementation designed to be used in a different thread than - * the corresponding SkyFunction runs in. It relies on a delegate Environment object to do - * underlying work. Its {@link #getValue} and {@link #getValueOrThrow} methods do not return {@code - * null} when the {@link SkyValue} in question is not available. Instead, it blocks and waits for - * the host Skyframe thread to restart, and replaces the delegate Environment with a fresh one from - * the restarted SkyFunction before continuing. (Note that those methods do return {@code - * null} if the SkyValue was evaluated but found to be in error.) + * A {@link SkyFunction.Environment} implementation designed to be used in a different thread (the + * "worker thread") than the corresponding SkyFunction runs in. It relies on a delegate Environment + * object to do underlying work. Its {@link #getValue} and {@link #getValueOrThrow} methods do not + * return {@code null} when the {@link SkyValue} in question is not available. Instead, it blocks + * and waits for the host Skyframe thread to restart, and replaces the delegate Environment with a + * fresh one from the restarted SkyFunction before continuing. (Note that those methods do + * return {@code null} if the SkyValue was evaluated but found to be in error.) * *

Crucially, the delegate Environment object must not be used by multiple threads at the same * time. In effect, this is guaranteed by only one of the worker thread and host thread being active * at any given time. */ -class RepoFetchingWorkerSkyFunctionEnvironment +class WorkerSkyFunctionEnvironment implements SkyFunction.Environment, ExtendedEventHandler, SkyframeLookupResult { - private final RepoFetchingSkyKeyComputeState state; + private final WorkerSkyKeyComputeState state; private SkyFunction.Environment delegate; - RepoFetchingWorkerSkyFunctionEnvironment(RepoFetchingSkyKeyComputeState state) - throws InterruptedException { + WorkerSkyFunctionEnvironment(WorkerSkyKeyComputeState state) throws InterruptedException { this.state = state; this.delegate = state.delegateEnvQueue.take(); } diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/RepoFetchingSkyKeyComputeState.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/WorkerSkyKeyComputeState.java similarity index 75% rename from src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/RepoFetchingSkyKeyComputeState.java rename to src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/WorkerSkyKeyComputeState.java index 6bd36490c02093..d1bcf992376d66 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/RepoFetchingSkyKeyComputeState.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/WorkerSkyKeyComputeState.java @@ -19,12 +19,8 @@ import com.google.common.util.concurrent.ListenableFuture; import com.google.common.util.concurrent.ListeningExecutorService; import com.google.common.util.concurrent.MoreExecutors; -import com.google.devtools.build.lib.rules.repository.RepoRecordedInput; -import com.google.devtools.build.lib.rules.repository.RepositoryDirectoryValue; import com.google.devtools.build.skyframe.SkyFunction; import com.google.devtools.build.skyframe.SkyFunction.Environment.SkyKeyComputeState; -import java.util.Map; -import java.util.TreeMap; import java.util.concurrent.ArrayBlockingQueue; import java.util.concurrent.BlockingQueue; import java.util.concurrent.Callable; @@ -34,15 +30,20 @@ import javax.annotation.concurrent.GuardedBy; /** - * Captures state that persists across different invocations of {@link - * com.google.devtools.build.lib.rules.repository.RepositoryDelegatorFunction}, specifically {@link - * StarlarkRepositoryFunction}. + * A {@link SkyKeyComputeState} that manages a non-Skyframe virtual worker thread that persists + * across different invocations of a SkyFunction. * - *

This class is used to hold on to a worker thread when fetching repos using a worker thread is - * enabled. The worker thread uses a {@link SkyFunction.Environment} object acquired from the host - * thread, and can signal the host thread to restart to get a fresh environment object. + *

The worker thread uses a {@link SkyFunction.Environment} object acquired from the host thread. + * When a new Skyframe dependency is needed, the worker thread itself does not need to restart; + * instead, it can signal the host thread to restart to get a fresh Environment object. + * + *

Similar to other implementations of {@link SkyKeyComputeState}, this avoids redoing expensive + * work when a new Skyframe dependency is needed; but because it holds on to an entire worker + * thread, this class is more suited to cases where the intermediate result of expensive work cannot + * be easily serialized (in particular, if there's an ongoing Starlark evaluation, as is the case in + * repo fetching). */ -class RepoFetchingSkyKeyComputeState implements SkyKeyComputeState { +class WorkerSkyKeyComputeState implements SkyKeyComputeState { /** * A semaphore with 0 or 1 permit. The worker can release a permit either when it's finished @@ -69,7 +70,7 @@ class RepoFetchingSkyKeyComputeState implements SkyKeyComputeState { */ @GuardedBy("this") @Nullable - private ListenableFuture workerFuture = null; + private ListenableFuture workerFuture = null; /** The executor service that manages the worker thread. */ // We hold on to this alongside `workerFuture` because it offers a convenient mechanism to make @@ -78,20 +79,6 @@ class RepoFetchingSkyKeyComputeState implements SkyKeyComputeState { @Nullable private ListeningExecutorService workerExecutorService = null; - private final String repoName; - - /** - * This is where the recorded inputs & values for the whole invocation is collected. - * - *

{@link com.google.devtools.build.lib.rules.repository.RepositoryDelegatorFunction} creates a - * new map on each restart, so we can't simply plumb that in. - */ - final Map recordedInputValues = new TreeMap<>(); - - RepoFetchingSkyKeyComputeState(String repoName) { - this.repoName = repoName; - } - /** * Releases a permit on the {@code signalSemaphore} and immediately expect a fresh Environment * back. This may only be called from the worker thread. @@ -107,8 +94,7 @@ SkyFunction.Environment signalForFreshEnv() throws InterruptedException { * when the worker finishes, successfully or otherwise. This may only be called from the host * Skyframe thread. */ - synchronized ListenableFuture getOrStartWorker( - Callable c) { + synchronized ListenableFuture getOrStartWorker(String workerThreadName, Callable c) { if (workerFuture != null) { return workerFuture; } @@ -119,10 +105,9 @@ synchronized ListenableFuture getOrStartWorker workerExecutorService = MoreExecutors.listeningDecorator( Executors.newThreadPerTaskExecutor( - Thread.ofVirtual().name("starlark-repository-" + repoName).factory())); + Thread.ofVirtual().name(workerThreadName).factory())); signalSemaphore.drainPermits(); delegateEnvQueue.clear(); - recordedInputValues.clear(); // Start the worker. workerFuture = workerExecutorService.submit(c); diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/android/AndroidSdkRepositoryFunction.java b/src/main/java/com/google/devtools/build/lib/bazel/rules/android/AndroidSdkRepositoryFunction.java index b75e9963aea127..52f67a90a9e25d 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/rules/android/AndroidSdkRepositoryFunction.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/rules/android/AndroidSdkRepositoryFunction.java @@ -23,6 +23,7 @@ import com.google.common.collect.ImmutableSortedSet; import com.google.common.collect.Iterables; import com.google.common.collect.Lists; +import com.google.common.collect.Maps; import com.google.devtools.build.lib.actions.FileValue; import com.google.devtools.build.lib.analysis.BlazeDirectories; import com.google.devtools.build.lib.analysis.RuleDefinition; @@ -53,6 +54,7 @@ import java.io.InputStream; import java.util.Iterator; import java.util.Map; +import java.util.Optional; import java.util.Properties; import javax.annotation.Nullable; import net.starlark.java.eval.EvalException; @@ -193,17 +195,15 @@ public boolean verifyRecordedInputs( @Override @Nullable - public RepositoryDirectoryValue.Builder fetch( + public FetchResult fetch( Rule rule, final Path outputDirectory, BlazeDirectories directories, Environment env, - Map recordedInputValues, SkyKey key) throws RepositoryFunctionException, InterruptedException { ensureNativeRepoRuleEnabled(rule, env, "https://github.com/bazelbuild/rules_android"); - Map environ = - declareEnvironmentDependencies(recordedInputValues, env, PATH_ENV_VAR_AS_SET); + ImmutableMap> environ = getEnvVarValues(env, PATH_ENV_VAR_AS_SET); if (environ == null) { return null; } @@ -216,19 +216,22 @@ public RepositoryDirectoryValue.Builder fetch( WorkspaceAttributeMapper attributes = WorkspaceAttributeMapper.of(rule); FileSystem fs = directories.getOutputBase().getFileSystem(); Path androidSdkPath; - String userDefinedPath = null; + String userDefinedPath; if (attributes.isAttributeValueExplicitlySpecified("path")) { userDefinedPath = getPathAttr(rule); androidSdkPath = fs.getPath(getTargetPath(userDefinedPath, directories.getWorkspace())); - } else if (environ.get(PATH_ENV_VAR) != null) { - userDefinedPath = environ.get(PATH_ENV_VAR); + } else if (environ.getOrDefault(PATH_ENV_VAR, Optional.empty()).isPresent()) { + userDefinedPath = environ.get(PATH_ENV_VAR).get(); + Path workspace = directories.getWorkspace(); androidSdkPath = - fs.getPath(getAndroidHomeEnvironmentVar(directories.getWorkspace(), environ)); + fs.getPath(workspace.getRelative(PathFragment.create(userDefinedPath)).asFragment()); } else { // Write an empty BUILD file that declares errors when referred to. String buildFile = getStringResource("android_sdk_repository_empty_template.txt"); writeBuildFile(outputDirectory, buildFile); - return RepositoryDirectoryValue.builder().setPath(outputDirectory); + return new FetchResult( + RepositoryDirectoryValue.builder().setPath(outputDirectory), + Maps.transformValues(RepoRecordedInput.EnvVar.wrap(environ), v -> v.orElse(null))); } if (!symlinkLocalRepositoryContents(outputDirectory, androidSdkPath, userDefinedPath)) { @@ -355,7 +358,9 @@ public RepositoryDirectoryValue.Builder fetch( } writeBuildFile(outputDirectory, buildFile); - return RepositoryDirectoryValue.builder().setPath(outputDirectory); + return new FetchResult( + RepositoryDirectoryValue.builder().setPath(outputDirectory), + Maps.transformValues(RepoRecordedInput.EnvVar.wrap(environ), v -> v.orElse(null))); } @Override @@ -363,11 +368,6 @@ public Class getRuleDefinition() { return AndroidSdkRepositoryRule.class; } - private static PathFragment getAndroidHomeEnvironmentVar( - Path workspace, Map env) { - return workspace.getRelative(PathFragment.create(env.get(PATH_ENV_VAR))).asFragment(); - } - private static String getStringResource(String name) { try { return ResourceFileLoader.loadResource(AndroidSdkRepositoryFunction.class, name); diff --git a/src/main/java/com/google/devtools/build/lib/rules/repository/LocalRepositoryFunction.java b/src/main/java/com/google/devtools/build/lib/rules/repository/LocalRepositoryFunction.java index 62942bf7f5941b..6e811be73e8fa7 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/repository/LocalRepositoryFunction.java +++ b/src/main/java/com/google/devtools/build/lib/rules/repository/LocalRepositoryFunction.java @@ -24,12 +24,9 @@ import com.google.devtools.build.lib.vfs.XattrProvider; import com.google.devtools.build.skyframe.SkyFunction.Environment; import com.google.devtools.build.skyframe.SkyKey; -import java.util.Map; import net.starlark.java.eval.Starlark; -/** - * Access a repository on the local filesystem. - */ +/** Access a repository on the local filesystem. */ public class LocalRepositoryFunction extends RepositoryFunction { @Override @@ -43,13 +40,8 @@ protected void setupRepoRootBeforeFetching(Path repoRoot) throws RepositoryFunct } @Override - public RepositoryDirectoryValue.Builder fetch( - Rule rule, - Path outputDirectory, - BlazeDirectories directories, - Environment env, - Map recordedInputValues, - SkyKey key) + public FetchResult fetch( + Rule rule, Path outputDirectory, BlazeDirectories directories, Environment env, SkyKey key) throws InterruptedException, RepositoryFunctionException { ensureNativeRepoRuleEnabled( rule, env, "load(\"@bazel_tools//tools/build_defs/repo:local.bzl\", \"local_repository\")"); @@ -63,7 +55,7 @@ public RepositoryDirectoryValue.Builder fetch( if (result != null) { env.getListener().post(resolve(rule, directories)); } - return result; + return new FetchResult(result, ImmutableMap.of()); } @Override diff --git a/src/main/java/com/google/devtools/build/lib/rules/repository/NewLocalRepositoryFunction.java b/src/main/java/com/google/devtools/build/lib/rules/repository/NewLocalRepositoryFunction.java index 4907e2ea3fb3dd..1c4a0e894b94f6 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/repository/NewLocalRepositoryFunction.java +++ b/src/main/java/com/google/devtools/build/lib/rules/repository/NewLocalRepositoryFunction.java @@ -38,13 +38,12 @@ import com.google.devtools.build.skyframe.SkyValue; import com.google.devtools.build.skyframe.SkyframeLookupResult; import java.io.IOException; +import java.util.LinkedHashMap; import java.util.Map; import javax.annotation.Nullable; import net.starlark.java.eval.Starlark; -/** - * Create a repository from a directory on the local filesystem. - */ +/** Create a repository from a directory on the local filesystem. */ public class NewLocalRepositoryFunction extends RepositoryFunction { @Override @@ -54,13 +53,8 @@ public boolean isLocal(Rule rule) { @Override @Nullable - public RepositoryDirectoryValue.Builder fetch( - Rule rule, - Path outputDirectory, - BlazeDirectories directories, - Environment env, - Map recordedInputValues, - SkyKey key) + public FetchResult fetch( + Rule rule, Path outputDirectory, BlazeDirectories directories, Environment env, SkyKey key) throws InterruptedException, RepositoryFunctionException { ensureNativeRepoRuleEnabled( rule, @@ -128,8 +122,9 @@ public RepositoryDirectoryValue.Builder fetch( SkyKey dirKey = DirectoryListingValue.key(dirPath); DirectoryListingValue directoryValue; try { - directoryValue = (DirectoryListingValue) env.getValueOrThrow( - dirKey, InconsistentFilesystemException.class); + directoryValue = + (DirectoryListingValue) + env.getValueOrThrow(dirKey, InconsistentFilesystemException.class); } catch (InconsistentFilesystemException e) { throw new RepositoryFunctionException(new IOException(e), Transience.PERSISTENT); } @@ -167,13 +162,16 @@ public RepositoryDirectoryValue.Builder fetch( return null; } + Map recordedInputValues = new LinkedHashMap<>(); fileHandler.finishFile(rule, outputDirectory, recordedInputValues); env.getListener().post(resolve(rule)); - return RepositoryDirectoryValue.builder() - .setPath(outputDirectory) - .setSourceDir(directoryValue) - .setFileValues(fileValues.buildOrThrow()); + return new FetchResult( + RepositoryDirectoryValue.builder() + .setPath(outputDirectory) + .setSourceDir(directoryValue) + .setFileValues(fileValues.buildOrThrow()), + recordedInputValues); } @Override diff --git a/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryDelegatorFunction.java b/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryDelegatorFunction.java index 51eaa6b0ce2ebc..d85275579d71fd 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryDelegatorFunction.java +++ b/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryDelegatorFunction.java @@ -21,7 +21,6 @@ import com.google.common.base.Preconditions; import com.google.common.base.Splitter; import com.google.common.collect.ImmutableMap; -import com.google.common.collect.Maps; import com.google.devtools.build.lib.actions.FileValue; import com.google.devtools.build.lib.analysis.BlazeDirectories; import com.google.devtools.build.lib.bazel.bzlmod.BzlmodRepoRuleValue; @@ -68,8 +67,7 @@ /** * A {@link SkyFunction} that implements delegation to the correct repository fetcher. * - *

- * Each repository in the WORKSPACE file is represented by a {@link SkyValue} that is computed by + *

Each repository in the WORKSPACE file is represented by a {@link SkyValue} that is computed by * this function. */ public final class RepositoryDelegatorFunction implements SkyFunction { @@ -219,18 +217,21 @@ public SkyValue compute(SkyKey skyKey, Environment env) // repository as valid even though it is in an inconsistent state. Clear the marker file and // only recreate it after fetching is done to prevent this scenario. DigestWriter.clearMarkerFile(directories, repositoryName); - RepositoryDirectoryValue.Builder builder = - fetchRepository( - skyKey, repoRoot, env, digestWriter.getRecordedInputValues(), handler, rule); - if (builder == null) { + RepositoryFunction.FetchResult result = + fetchRepository(skyKey, repoRoot, env, handler, rule); + if (result == null) { return null; } // No new Skyframe dependencies must be added between calling the repository implementation // and writing the marker file because if they aren't computed, it would cause a Skyframe // restart thus calling the possibly very slow (networking, decompression...) fetch() // operation again. So we write the marker file here immediately. - byte[] digest = digestWriter.writeMarkerFile(); - return builder.setDigest(digest).setExcludeFromVendoring(excludeRepoFromVendoring).build(); + byte[] digest = digestWriter.writeMarkerFile(result.recordedInputValues()); + return result + .repoBuilder() + .setDigest(digest) + .setExcludeFromVendoring(excludeRepoFromVendoring) + .build(); } if (!repoRoot.exists()) { @@ -420,13 +421,8 @@ private boolean isRepoExcludedFromVendoringByDefault(RepositoryFunction handler, } @Nullable - private RepositoryDirectoryValue.Builder fetchRepository( - SkyKey skyKey, - Path repoRoot, - Environment env, - Map recordedInputValues, - RepositoryFunction handler, - Rule rule) + private RepositoryFunction.FetchResult fetchRepository( + SkyKey skyKey, Path repoRoot, Environment env, RepositoryFunction handler, Rule rule) throws InterruptedException, RepositoryFunctionException { handler.setupRepoRootBeforeFetching(repoRoot); @@ -434,9 +430,9 @@ private RepositoryDirectoryValue.Builder fetchRepository( RepositoryName repoName = (RepositoryName) skyKey.argument(); env.getListener().post(RepositoryFetchProgress.ongoing(repoName, "starting")); - RepositoryDirectoryValue.Builder repoBuilder; + RepositoryFunction.FetchResult result; try { - repoBuilder = handler.fetch(rule, repoRoot, directories, env, recordedInputValues, skyKey); + result = handler.fetch(rule, repoRoot, directories, env, skyKey); } catch (RepositoryFunctionException e) { // Upon an exceptional exit, the fetching of that repository is over as well. env.getListener().post(RepositoryFetchProgress.finished(repoName)); @@ -461,7 +457,7 @@ private RepositoryDirectoryValue.Builder fetchRepository( return null; } env.getListener().post(RepositoryFetchProgress.finished(repoName)); - return Preconditions.checkNotNull(repoBuilder); + return Preconditions.checkNotNull(result); } /** @@ -596,11 +592,11 @@ static String unescape(String str) { for (int i = 0; i < str.length(); i++) { char c = str.charAt(i); if (escaped) { - if (c == 'n') { // n means new line + if (c == 'n') { // n means new line result.append("\n"); } else if (c == 's') { // s means space result.append(" "); - } else { // Any other escaped characters are just un-escaped + } else { // Any other escaped characters are just un-escaped result.append(c); } escaped = false; @@ -617,8 +613,6 @@ private static class DigestWriter { private final BlazeDirectories directories; private final Path markerPath; private final Rule rule; - // not just Map<> to signal that iteration order must be deterministic - private final TreeMap recordedInputValues; private final String ruleKey; DigestWriter( @@ -630,13 +624,14 @@ private static class DigestWriter { ruleKey = computeRuleKey(rule, starlarkSemantics); markerPath = getMarkerPath(directories, repositoryName); this.rule = rule; - recordedInputValues = Maps.newTreeMap(); } - byte[] writeMarkerFile() throws RepositoryFunctionException { + byte[] writeMarkerFile(Map recordedInputValues) + throws RepositoryFunctionException { StringBuilder builder = new StringBuilder(); builder.append(ruleKey).append("\n"); - for (Map.Entry recordedInput : recordedInputValues.entrySet()) { + for (Map.Entry recordedInput : + new TreeMap(recordedInputValues).entrySet()) { String key = recordedInput.getKey().toString(); String value = recordedInput.getValue(); builder.append(escape(key)).append(" ").append(escape(value)).append("\n"); @@ -698,10 +693,6 @@ byte[] areRepositoryAndMarkerFileConsistent( } } - Map getRecordedInputValues() { - return recordedInputValues; - } - @Nullable private static String readMarkerFile( String content, Map recordedInputValues) { diff --git a/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryFunction.java b/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryFunction.java index b6daae8d0e4041..a32eca28cf0f9c 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryFunction.java +++ b/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryFunction.java @@ -20,7 +20,6 @@ import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableMap; -import com.google.common.collect.Maps; import com.google.devtools.build.lib.analysis.BlazeDirectories; import com.google.devtools.build.lib.analysis.RuleDefinition; import com.google.devtools.build.lib.cmdline.Label; @@ -171,24 +170,28 @@ public void reportSkyframeRestart(Environment env, RepositoryName repoName) { * repository function would be restarted after that SkyFunction has been run, and it * would wipe the output directory clean. * - * - *

The {@code markerData} argument can be mutated to augment the data to write to the - * repository marker file. If any data in the {@code markerData} change between 2 execute of the - * {@link RepositoryDelegatorFunction} then this should be a reason to invalidate the repository. - * The {@link #verifyRecordedInputs} method is responsible for checking the value added to that - * map when checking the content of a marker file. */ @ThreadSafe @Nullable - public abstract RepositoryDirectoryValue.Builder fetch( - Rule rule, - Path outputDirectory, - BlazeDirectories directories, - Environment env, - Map recordedInputValues, - SkyKey key) + public abstract FetchResult fetch( + Rule rule, Path outputDirectory, BlazeDirectories directories, Environment env, SkyKey key) throws InterruptedException, RepositoryFunctionException; + /** + * The result of the {@link #fetch} method. + * + * @param repoBuilder A builder for the eventual {@link RepositoryDirectoryValue} with necessary + * fields set. + * @param recordedInputValues Any recorded inputs (and their values) encountered during the fetch + * of the repo. Changes to these inputs will result in the repo being refetched in the future. + * The {@link #verifyRecordedInputs} method is responsible for checking the value added to + * that map when checking the content of a marker file. Not an ImmutableMap, because + * regrettably the values can be null sometimes. + */ + public record FetchResult( + RepositoryDirectoryValue.Builder repoBuilder, + Map recordedInputValues) {} + protected static void ensureNativeRepoRuleEnabled(Rule rule, Environment env, String replacement) throws RepositoryFunctionException, InterruptedException { if (!isWorkspaceRepo(rule)) { @@ -244,31 +247,6 @@ public static RootedPath getRootedPathFromLabel(Label label, Environment env) return RootedPath.toRootedPath(packageRoot, label.toPathFragment()); } - /** - * A method that can be called from a implementation of {@link #fetch(Rule, Path, - * BlazeDirectories, Environment, Map, SkyKey)} to declare a list of Skyframe dependencies on - * environment variable. It also add the information to the marker file. It returns the list of - * environment variable on which the function depends, or null if the skyframe function needs to - * be restarted. - */ - @Nullable - protected Map declareEnvironmentDependencies( - Map recordedInputValues, Environment env, Set keys) - throws InterruptedException { - if (keys.isEmpty()) { - return ImmutableMap.of(); - } - - ImmutableMap> envDepOptionals = getEnvVarValues(env, keys); - if (envDepOptionals == null) { - return null; - } - // Add the dependencies to the marker file. The repository marker file supports null values. - recordedInputValues.putAll( - Maps.transformValues(RepoRecordedInput.EnvVar.wrap(envDepOptionals), v -> v.orElse(null))); - return Maps.transformValues(envDepOptionals, v -> v.orElse(null)); - } - /** * Returns the values of the environment variables in the given set as an immutable map while * registering them as dependencies.